From eead7482222bcbb07a7eb2fa3972bceb961af0c6 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Tue, 25 Aug 2026 10:42:26 -0700 Subject: [PATCH] Close what two reviews found in the Files tab transfers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent reviews of 2c9482a, one for correctness and one against the threat model. Between them they found two ways to lose a file, one way for a container to choose a Windows save destination, and a rule that made every dotfile unsavable. Every finding below was demonstrated against a real container before being fixed, and the fixes are demonstrated the same way. ## The download was accepting truncated reads and refusing whole ones The `[ -f ]` bracket around the read was wrong in both directions. It missed the case that loses data. Truncation *in place* — `> file`, log rotation, `tar -x`, most build tools — leaves a regular file behind, so `dd` stopped at the new EOF and exited 0. Measured: a 600 MB source truncated mid-read delivered 34 MB, which was then renamed over the user's own earlier copy and toasted as `Saved (34.1 MB)`. Truncate-to-zero did the same and needs no adversary at all. And it failed *good* downloads. `dd` already holds the fd, and neither `rm` nor `mv` can touch an open one — the bytes are complete. But `rm` makes `[ -f ]` false, so a finished 600 MB transfer of a file a bundler happened to unlink was deleted, reporting "nothing was saved". The comment claiming the bracket caught a "mix of two files" was simply wrong; an open fd cannot be a mix. So the script now measures the file before reading it and puts the answer on stderr, and Rust checks that at least that many bytes arrived. One rule, subsuming everything the bracket was for. Verified against a real container: truncation mid-read and the FIFO race are refused; deletion, rename-over, a growing file, an empty file and an untouched 600 MB read all pass. ## On Windows the container, not the user, was naming the save destination `suggested_save_name` split on `/` only, and it feeds the save dialog's pre-filled name. Backslash is a legal Linux filename character and `validate_container_path` has no reason to object — `..\..\Users\…` is one POSIX segment. The Windows common file dialog parses its name box as a path on Save, so a container-created file called `..\..\..\Users\vic\AppData\Roaming\Microsoft\Word\STARTUP\x.dotm` put its own bytes in an auto-loading Office directory on one un-read click. `resolve_host_path` did not stop it: Word's `STARTUP` and Excel's `XLSTART` are not in the autorun denylist, which this file's own docs already concede is "losing by construction" and was never meant to be the boundary here. The name is now sanitized of every separator, the drive colon and the rest of what NTFS refuses, so it cannot be a path on any platform this ships to. ## No dotfile could be saved, and the app pre-filled the name that guaranteed it The write policy judged the leaf for hiddenness, so `/workspace/.env` was refused *after* the modal and the overwrite prompt — quoting the name the app itself had suggested. `.gitignore`, `.dockerignore`, `.eslintrc.json`, `.nvmrc`: all unsavable, while uploading them worked, so a dotfile could go in and never come out. `HostPathUse` gains a third mode. `WriteChosenName` drops the leaf check and keeps every directory rule, and only `download_container_file` uses it — the dialog is a real boundary for that caller and only that caller. `download_container_backup` still takes its path over IPC as a string and keeps the strict rule. ## Smaller, all found by the reviews * A download had no ceiling. `dd` resolves through the container's `PATH`, which its agent owns with passwordless sudo; a replacement writing forever was measured at ~6 GB/s, so one click on a file listed as 2 KB filled the host disk with no progress shown and no cancel. Bounded now by what the file measured, with slack that is absolute for small files and proportional for large ones, so an honest growing log is unaffected. * "Framed rather than verbatim" did not stop container text reaching the toast headline: `readableRefusal` matches with `includes`, and it has to, because the app's own refusals carry those markers mid-sentence. Anchoring would break them. The fix is at the injection point — container text is clipped to one 200-character line with control characters stripped — plus a `max-h-40` on the toast message, which the `detail` block always had and this half did not. 8 KB of prose in a `z-[60]` card pushed its own dismiss button off-screen. * `savingPath` was a scalar while the design deliberately allows concurrent saves. Starting a second freed the first's row mid-transfer, and whichever finished first cleared both; dismissing the second dialog was enough. It is a `Set` now. * `setUploading(false)` fired when the command settled, not when the refresh finished, so a second click landed mid-relisting. * `upload_files_to_container` checked the container directory before the picker and then used the unresolved path. A modal has no time limit. It re-checks after, which is what `docker::exec`'s doc comment already claimed. * `wait_for_exec_exit` flattened a missing exit code to `Some(0)`, which made the new `!= Some(0)` check unreachable by construction. * `normalize_host_path` did not collapse repeated separators, so the lexical system-root rule was silently absent for `C:\\Windows\…`. Not exploitable — the resolved pass catches it — but a documented layer that does nothing is a trap for the next caller. * README still carried the "no host path crosses IPC in either direction" claim the previous commit narrowed everywhere else, and HOW-TO-USE described the hidden rule without saying it applies to folders only. 480 Rust tests, 602 frontend, no new clippy warnings. Eleven mutations against the new tests, all killed — two of the first round survived and were rewritten: one because `split_whitespace` already handled the case I thought I was testing, one because I had deleted a comment rather than the behaviour. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01LHL9ty7arp8FHwvE77ne7y --- HOW-TO-USE.md | 6 +- README.md | 12 +- app/src-tauri/src/commands/file_commands.rs | 534 ++++++++++++++++-- app/src-tauri/src/docker/exec.rs | 13 +- app/src/components/projects/home/FilesTab.tsx | 6 +- app/src/components/ui/ToastHost.tsx | 11 +- app/src/hooks/useFileManager.test.ts | 100 +++- app/src/hooks/useFileManager.ts | 70 ++- 8 files changed, 624 insertions(+), 128 deletions(-) diff --git a/HOW-TO-USE.md b/HOW-TO-USE.md index c67d239..9e30741 100644 --- a/HOW-TO-USE.md +++ b/HOW-TO-USE.md @@ -1212,13 +1212,17 @@ cannot name a place on your machine — it can only ask for a dialog — and not until you pick somewhere in it. Closing a dialog without choosing is not an error: nothing happens, and nothing is said about it. -Every one of these routes refuses a location whose path passes through a hidden folder — anything +Every one of these routes refuses a location whose path passes through a hidden *folder* — anything with a component beginning with `.`, such as `~/.ssh`, `~/.cache` or `~/.local/share` — or a system location, and it checks both the path as written and where it points after any symbolic links. That rule catches more than it strictly needs to, so now and then it will refuse a place you genuinely meant, `~/.config` among them. The refusal is a plain sentence saying so; choose a visible location such as `~/Documents` or `~/Downloads`. +The *file's own name* is a different matter, and dotfiles are fine: `.env`, `.gitignore` and the +rest save normally, since you chose the name in the save dialog yourself. Only the folders on the +way are judged. + If you already keep the project in a folder mounted into the container, the simplest answer is usually none of the above: edit the file on your host and it is already inside. diff --git a/README.md b/README.md index 5dae25b..a4c17f5 100644 --- a/README.md +++ b/README.md @@ -451,10 +451,16 @@ policy in `commands/file_commands.rs`. - **The OS dialogs are opened by Rust, not by the webview.** `upload_files_to_container` and `download_container_file` drive `tauri-plugin-dialog` themselves and take nothing but a project id and a container-side path; `FilesTab.tsx` imports no dialog plugin and `useFileManager`'s - `uploadFiles` takes no argument at all. No host path crosses IPC in either direction — the web UI - can ask for a dialog, and that is the whole of its influence over where a file comes from or goes. + `uploadFiles` takes no argument at all. The web UI can ask for a dialog, and that is the whole of + its influence over where a file comes from or goes — it cannot name a host path as an *input*. This is a boundary rather than a convention: a dialog the page itself opens is only as trustworthy - as the page. + as the page. Be precise about the limit, though — host paths still travel *outward* in error text, + canonical ones included, so this closes the inbound direction and not both. +- **The dialog's pre-filled name is sanitized, because a container authored it.** On Windows the + save dialog parses its name box as a path, and a container can name a file + `..\..\Users\you\…\Word\STARTUP\x.dotm` — one POSIX segment, so nothing upstream objects. + `suggested_save_name` replaces every separator and every character NTFS refuses, so the string + cannot be a path on any platform this ships to. - **One policy for every host path.** A source or destination whose path passes through a hidden folder (`~/.ssh`, `~/.cache`, `~/.local/share`, anything dot-prefixed) or a system location is refused, and the check is applied both to the path as written and to what it resolves to after diff --git a/app/src-tauri/src/commands/file_commands.rs b/app/src-tauri/src/commands/file_commands.rs index d9d8353..779c3cf 100644 --- a/app/src-tauri/src/commands/file_commands.rs +++ b/app/src-tauri/src/commands/file_commands.rs @@ -484,14 +484,39 @@ fn is_under_root(path: &str, root: &str) -> bool { path == root || path.strip_prefix(root).is_some_and(|rest| rest.starts_with('/')) } -/// What a host path is about to be used for. The two directions differ over -/// hidden names — see [`validate_host_path`]. +/// What a host path is about to be used for. The three modes differ over which +/// components may be hidden — see [`validate_host_path`] and the variants. #[derive(Clone, Copy, Debug, PartialEq)] enum HostPathUse { /// Host bytes are about to be read *into* the container. Read, - /// Container bytes are about to be written *onto* the host. + /// Container bytes are about to be written *onto* the host, at a path that + /// arrived **over IPC as a string** — `download_container_backup`. + /// + /// The strictest of the three, and the leaf is judged along with every + /// directory above it, because creating `~/.bashrc` is escape all by + /// itself and nothing here can tell a path a person picked from one a + /// compromised webview invented. Write, + /// Container bytes are about to be written onto the host, at a name a + /// person typed or accepted in an **OS save dialog opened by Rust** — + /// `download_container_file`. + /// + /// Identical to [`HostPathUse::Write`] except that the final component is + /// not judged for hiddenness, which is the difference between a rule and a + /// bug. Under `Write`, saving `/workspace/.env` was refused *after* the + /// modal and the overwrite prompt, with the message "\".env\" is a hidden + /// file — Triple-C will not save there" — and the app had pre-filled that + /// exact name itself. `.gitignore`, `.dockerignore`, `.eslintrc.json`, + /// `.nvmrc` and the rest of an ordinary workspace were all unsavable, while + /// uploading them worked, so a dotfile could go in and never come out. + /// + /// What justifies dropping it *here* and nowhere else is that the dialog is + /// a real boundary for this caller and only this caller: the name is on + /// screen, the user chose the directory, and the OS asked before + /// overwriting anything. Every *directory* rule still applies, so `~/.ssh` + /// and `~/.config` are as refused as they ever were. + WriteChosenName, } /// Host directories nothing in this app has any business reading a file out of @@ -583,7 +608,35 @@ fn normalize_host_path(path: &str) -> String { break; } } - s + // Collapse runs of separators, keeping any leading pair (a UNC root is + // `//server/share` and means something). + // + // Without this the *lexical* system-root rule was quietly absent for + // Windows paths: `C:\\Windows\System32\x.dll` normalises to + // `c://windows/...`, which `is_under_root` does not match, while the + // single-separator form is refused. Nothing was exploitable — `resolve_host_path` + // runs the same policy again over the canonical form and `canonicalize` + // collapses the run — but a documented layer that silently does nothing is + // a trap for the next caller who reaches for it without the resolved pass. + let lead = if s.starts_with("//") { "//" } else { "" }; + let body: String = { + let rest = &s[lead.len()..]; + let mut out = String::with_capacity(rest.len()); + let mut prev_sep = false; + for c in rest.chars() { + if c == '/' { + if !prev_sep { + out.push(c); + } + prev_sep = true; + } else { + out.push(c); + prev_sep = false; + } + } + out + }; + format!("{}{}", lead, body) } /// The named components of a host path, with the drive letter, the separators @@ -763,7 +816,9 @@ fn validate_host_path(path: &str, use_for: HostPathUse) -> Result names.len(), - HostPathUse::Read => names.len().saturating_sub(1), + // The leaf is the user's own choice in both of these — dropped from a + // file manager, or typed into a save dialog. See the enum. + HostPathUse::Read | HostPathUse::WriteChosenName => names.len().saturating_sub(1), }; if let Some(hidden) = names[..hidden_limit].iter().find(|n| n.starts_with('.')) { let verb = if use_for == HostPathUse::Write { "save" } else { "read" }; @@ -861,7 +916,10 @@ async fn resolve_host_path(path: &str, use_for: HostPathUse) -> Result tokio::fs::canonicalize(&candidate) .await .map_err(|e| format!("Cannot access {}: {}", candidate.display(), e))?, - HostPathUse::Write => { + // Both write modes resolve the *parent* and keep the caller's leaf; they + // differ only in whether that leaf may be hidden, which + // `validate_host_path` has already decided by this point. + HostPathUse::Write | HostPathUse::WriteChosenName => { let parent = candidate .parent() .ok_or_else(|| format!("{} does not name a file", candidate.display()))?; @@ -878,13 +936,15 @@ async fn resolve_host_path(path: &str, use_for: HostPathUse) -> Result