From f2a84c18f9d8696b6455731446efa1b86035f217 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 13:13:35 -0700 Subject: [PATCH] Judge a host path by where it leads, not by how it is spelled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `validate_host_path` was a string test. Nothing in the module called `canonicalize`, `read_link` or `O_NOFOLLOW`, so a path whose components are all visible could still land somewhere hidden: with `~/Downloads/pub` a symlink to `~/.ssh`, a `host_path` of `~/Downloads/pub/authorized_keys` has no hidden component, no `..` and no system root — and writes into `~/.ssh`. The container end is not hypothetical: `/proc/self/mountinfo` inside a Triple-C container spells the host's project paths out verbatim, so code in there knows both where to plant the link and what host path to ask for. The same bypass read host files back the other way. So the policy now runs twice: once on the string, and once on what the OS says the string resolves to. A write resolves the parent and keeps the caller's leaf, because the leaf is never followed — the partial file is created with `O_EXCL` and the download finishes with a rename, which replaces a link rather than writing through it. A read resolves the whole path, because the whole path is opened. On Linux the descriptor is then checked against the path that was validated (`/proc/self/fd`), which is what closes the window between resolving and opening; elsewhere that window stays open and the comment says so. Also here: * The upload's overwrite guard is a guard again. `noOverwriteDirNonDir` refuses only dir-over-non-dir and the reverse — file-over-file extraction proceeds, which is exactly the `.credentials.json` case (verified against a live daemon). The probe and the write are now one `set -C` exclusive create, with the path travelling as `$0` rather than as script. The `FILE_EXISTS: already exists` contract with the frontend is unchanged, and now pinned by a test — as is the claim the old comment made. * Windows normalisation stopped being a string swap: `\\?\`, `\\?\UNC\` and administrative shares all reach the same places and are compared as such, and the rules are pure functions over a string, so the Windows entries are exercised on any platform. The old test passed on Linux only because `Path::is_absolute` was false for a Windows path. * Container write roots are resolved inside the container too, and the comment no longer claims more than the check does. * A failed download can no longer delete a pre-existing file that happened to collide with the partial's name. * One-shot exec output is buffered as bytes and decoded once, so a filename split across two Docker frames survives; stdout and stderr are tellable apart, so `find`'s diagnostics stay out of the listing parser; and a directory too big to buffer is described as one. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc --- app/src-tauri/src/commands/file_commands.rs | 1310 +++++++++++++++-- .../src/commands/terminal_commands.rs | 14 +- app/src-tauri/src/docker/exec.rs | 234 ++- 3 files changed, 1406 insertions(+), 152 deletions(-) diff --git a/app/src-tauri/src/commands/file_commands.rs b/app/src-tauri/src/commands/file_commands.rs index e4c14bf..48cdbff 100644 --- a/app/src-tauri/src/commands/file_commands.rs +++ b/app/src-tauri/src/commands/file_commands.rs @@ -1,5 +1,6 @@ use std::path::{Path, PathBuf}; -use std::sync::OnceLock; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, OnceLock}; use std::time::{Duration, SystemTime}; use base64::engine::general_purpose::STANDARD as BASE64; @@ -12,7 +13,8 @@ use tauri::{AppHandle, Manager, State}; use crate::docker::client::get_docker; use crate::docker::exec::{ - build_single_file_tar, container_user_ids, exec_oneshot_as, now_epoch_secs, + build_single_file_tar, container_user_ids, exec_oneshot_as, exec_oneshot_streams_as, + now_epoch_secs, OUTPUT_LIMIT_MARKER, }; use crate::AppState; @@ -79,14 +81,20 @@ pub async fn list_container_files( // only decides what an *empty* result means, though: `find` also exits // non-zero when a single child vanished mid-scan, and the rows it did // print are still the right answer. - let (output, code) = - exec_oneshot_as(container_id, "claude", list_argv(&path), Vec::new()).await?; + // + // The two streams are taken apart rather than merged: `find`'s diagnostics + // are the error message, its `-printf` records are the listing, and the + // parser should never be handed the former. + let (records, diagnostics, code) = + exec_oneshot_streams_as(container_id, "claude", list_argv(&path), Vec::new()) + .await + .map_err(|e| describe_listing_failure(&path, e))?; - let entries = parse_find_output(&path, &output); + let entries = parse_find_output(&path, &records); if code != 0 && entries.is_empty() { // `find`'s own words — "Permission denied", "No such file or directory" - // — are the whole diagnosis, and stderr is merged into `output`. - let detail = output.trim(); + // — are the whole diagnosis. + let detail = diagnostics.trim(); return Err(if detail.is_empty() { format!("Could not list {} (exit {})", path, code) } else { @@ -136,6 +144,24 @@ fn list_argv(path: &str) -> Vec { ] } +/// Turn a listing exec's failure into something the person looking at the +/// folder can act on. +/// +/// One case is worth naming: a directory with more entries than +/// [`crate::docker::exec::MAX_ONESHOT_OUTPUT`] will hold. Roughly 100k names is +/// the point where a `find` record set passes 8 MiB, and what the panel showed +/// was "Command output exceeded 8388608 bytes and was abandoned" — a true +/// statement about a buffer, and no help at all about a directory. +fn describe_listing_failure(path: &str, error: String) -> String { + if error.starts_with(OUTPUT_LIMIT_MARKER) { + return format!( + "{} holds too many entries for this panel to list. Open it in a terminal, or look at a subfolder.", + path + ); + } + error +} + /// Turn `find -printf '%y\t%Y\t%s\t%T@\t%m\t%f\0'` output into sorted entries. /// /// Split out from the command so it can be tested without a container: it is @@ -323,6 +349,18 @@ fn validate_container_path(what: &str, path: &str) -> Result<(), String> { /// [`validate_container_path`] plus containment in [`CONTAINER_WRITE_ROOTS`], /// for every path this module is about to change something at. +/// +/// **Lexical, and only lexical.** `/workspace/link/x` is "under `/workspace`" +/// as a string no matter what `/workspace/link` points at, so this on its own +/// does not keep an operation inside the write roots — [`resolve_container_dir`] +/// is what asks the container where the path actually goes. +/// +/// Worth being clear about what that resolution is and is not for. It is not a +/// containment boundary: the container user has a shell, and anything this +/// panel could be tricked into writing through a symlink it could write +/// directly. What it buys is that the *panel* keeps its promise — the roots +/// named in the refusal are the roots it writes to — and that a mis-aimed drop +/// cannot quietly land outside them. fn validate_container_write_path(what: &str, path: &str) -> Result<(), String> { validate_container_path(what, path)?; if CONTAINER_WRITE_ROOTS @@ -339,6 +377,60 @@ fn validate_container_write_path(what: &str, path: &str) -> Result<(), String> { )) } +/// Resolve a container *directory* and check where it really lands. +/// +/// `realpath -m` because the path is being written into rather than read: `-m` +/// wants no component to exist, which is what makes it usable for the parent of +/// a `mkdir`. The resolved answer goes back through +/// [`validate_container_write_path`], so a symlink out of `/workspace` is +/// refused by the same sentence a literal `/etc` would be. +/// +/// The caller keeps operating on the path the *user* typed rather than on the +/// resolved one: they name the same directory, and the unresolved form is the +/// one the listing shows and the UI navigates back to. What is validated and +/// what is operated on can therefore drift if a link is swapped in between — +/// this is a container-side TOCTOU with the same shape as H4's, and unlike H4's +/// it costs nothing, because both sides of the window are already inside the +/// container's own trust boundary. +/// +/// A `realpath` that cannot run at all (an image without coreutils) is logged +/// and the lexical answer stands: failing every write closed would break the +/// panel outright for a risk the container user does not need this code path to +/// take. +async fn resolve_container_dir(container_id: &str, what: &str, dir: &str) -> Result<(), String> { + validate_container_write_path(what, dir)?; + + let (output, code) = exec_oneshot_as( + container_id, + "claude", + vec![ + "realpath".to_string(), + "-m".to_string(), + "--".to_string(), + dir.to_string(), + ], + Vec::new(), + ) + .await?; + + let resolved = output.trim(); + if code != 0 || resolved.is_empty() { + log::warn!( + "Could not resolve {} in the container (exit {}); using the literal path", + dir, + code + ); + return Ok(()); + } + if resolved == dir { + return Ok(()); + } + + validate_container_write_path(what, resolved).map_err(|e| { + format!("{} leads to {} — {}", dir, resolved, e) + }) +} + /// Whether `path` is `root` itself or something beneath it. /// /// Compared by whole segments, so `/workspace-backup` is not "under" @@ -366,70 +458,217 @@ enum HostPathUse { /// write would fail anyway. They are listed so that a build running with more /// privilege than usual still cannot be talked into replacing a system file, /// and so the refusal is a sentence rather than an errno. Compared after -/// lowercasing and mapping `\` to `/`, which is what makes the Windows entries -/// work. +/// [`normalize_host_path`] and lowercasing, which is what makes the Windows +/// entries work. const HOST_SYSTEM_ROOTS: &[&str] = &[ - "/bin", "/boot", "/dev", "/etc", "/lib", "/lib32", "/lib64", "/libx32", "/proc", "/root", - "/sbin", "/sys", "/usr", "/var", - // macOS keeps its own copies of the same idea. - "/system", "/library", + "/bin", "/boot", "/dev", "/etc", "/lib", "/lib32", "/lib64", "/libx32", "/opt", "/proc", + "/root", "/sbin", "/snap", "/srv", "/sys", "/usr", "/var", + // macOS keeps its own copies of the same idea. Its `/etc` and `/var` are + // symlinks into `/private`, and the check now runs on the *resolved* path + // (see [`resolve_host_path`]), so the resolved spellings have to be here + // too. `/private/tmp` deliberately is not: that is what an entirely + // ordinary `/tmp/report.pdf` resolves to on a Mac. + "/system", "/library", "/applications", "/private/etc", "/private/var", // Windows. "c:/windows", "c:/program files", "c:/program files (x86)", "c:/programdata", ]; -/// Validate a host path that arrived over IPC, returning it as a [`PathBuf`]. +/// Places that sit *under* a [`HOST_SYSTEM_ROOTS`] entry and are nonetheless +/// entirely ordinary, because a real system puts real user data there. +/// +/// Both of these only started to matter once the check ran on the *resolved* +/// path: `/home` is a symlink to `/var/home` on rpm-ostree systems (Fedora +/// Silverblue and friends), and a Mac's per-user temp directory resolves into +/// `/private/var/folders`. Without these, saving a download to your own home +/// directory on Silverblue is "that is a system location". +const HOST_SYSTEM_ROOT_EXCEPTIONS: &[&str] = &["/var/home", "/var/folders", "/private/var/folders"]; + +/// Directory *tails* whose contents the OS runs on the user's behalf at login. +/// +/// The same defence-in-depth footing as [`HOST_SYSTEM_ROOTS`], and the same +/// caveat in stronger form: this is a list of places that happen to be known, +/// not a description of the ones that exist. See [`validate_host_path`] for why +/// the write policy cannot be finished here. +const HOST_AUTORUN_DIRS: &[&[&str]] = &[ + &["library", "launchagents"], + &["library", "launchdaemons"], + &["library", "startupitems"], + &["start menu", "programs", "startup"], +]; + +/// Length of a `C:` drive prefix at the head of `path`, or 0. +fn drive_prefix_len(path: &str) -> usize { + let b = path.as_bytes(); + if b.len() >= 2 && b[0].is_ascii_alphabetic() && b[1] == b':' { + 2 + } else { + 0 + } +} + +/// Whether `path` is written in Windows form, and so whether `\` separates its +/// components. On Linux a backslash is an ordinary filename character, which is +/// why this is a question rather than an unconditional substitution. +fn is_windows_style_path(path: &str) -> bool { + cfg!(windows) || path.starts_with("\\\\") || drive_prefix_len(path) > 0 +} + +/// `path` with its separators unified and any Win32 verbatim/device prefix +/// removed — the form every rule below is expressed against. +/// +/// `\\?\C:\Windows` and `\\?\UNC\server\share` name the *same locations* as +/// `C:\Windows` and `\\server\share`; the prefix only turns off Win32 path +/// parsing. Stripping it is what stops four characters being a bypass of +/// [`HOST_SYSTEM_ROOTS`] — and it has to run on our own output as well, because +/// `std::fs::canonicalize` hands back exactly that spelling on Windows. +fn normalize_host_path(path: &str) -> String { + let mut s = if is_windows_style_path(path) { + path.replace('\\', "/") + } else { + path.to_string() + }; + // Slicing by byte index is safe here only because a prefix matched + // case-insensitively as ASCII is ASCII, so its end is a char boundary. + for prefix in ["//?/unc/", "//./unc/"] { + if s.len() >= prefix.len() && s.as_bytes()[..prefix.len()].eq_ignore_ascii_case(prefix.as_bytes()) { + return format!("//{}", &s[prefix.len()..]); + } + } + for prefix in ["//?/", "//./"] { + if s.len() >= prefix.len() && s.as_bytes()[..prefix.len()].eq_ignore_ascii_case(prefix.as_bytes()) { + s = s[prefix.len()..].to_string(); + break; + } + } + s +} + +/// The named components of a host path, with the drive letter, the separators +/// and any `.` dropped. +fn host_path_names(path: &str) -> Vec { + let norm = normalize_host_path(path); + norm[drive_prefix_len(&norm)..] + .split('/') + .filter(|s| !s.is_empty() && *s != ".") + .map(|s| s.to_string()) + .collect() +} + +/// Whether `path` names a location at all, on whichever platform wrote it. +/// +/// Deliberately not [`Path::is_absolute`], which answers for the *host* +/// platform: under it a Windows path on Linux is simply "not absolute", every +/// Windows rule below goes unreached, and the tests that thought they were +/// exercising them were only ever exercising this line. +fn is_absolute_host_path(path: &str) -> bool { + let norm = normalize_host_path(path); + norm.starts_with('/') || norm[drive_prefix_len(&norm)..].starts_with('/') +} + +/// A UNC path rewritten as the local path it actually reaches, when the share +/// is an administrative one: `\\host\C$\Windows` *is* `C:\Windows`, and +/// `\\host\ADMIN$` is the Windows directory itself. An ordinary file share has +/// no local equivalent and gets `None` — [`HOST_SYSTEM_ROOTS`] cannot reason +/// about someone else's server, and says so rather than guessing. +fn admin_share_target(norm_lower: &str) -> Option { + let mut parts = norm_lower.strip_prefix("//")?.splitn(3, '/'); + let _server = parts.next()?; + let share = parts.next()?; + let tail = parts.next().unwrap_or(""); + let b = share.as_bytes(); + if b.len() == 2 && b[0].is_ascii_alphabetic() && b[1] == b'$' { + Some(format!("{}:/{}", b[0] as char, tail)) + } else if share == "admin$" { + Some(format!("c:/windows/{}", tail)) + } else { + None + } +} + +/// The [`HOST_SYSTEM_ROOTS`] entry `path` falls under, if any. +/// +/// Pure, and platform-independent on purpose: this is the whole of the Windows +/// policy, so it is also the whole of what the tests have to be able to drive +/// from a Linux CI box. +fn host_system_root_for(path: &str) -> Option<&'static str> { + let norm = normalize_host_path(path).to_lowercase(); + if HOST_SYSTEM_ROOT_EXCEPTIONS + .iter() + .any(|allowed| is_under_root(&norm, allowed)) + { + return None; + } + let admin = admin_share_target(&norm); + HOST_SYSTEM_ROOTS.iter().copied().find(|root| { + is_under_root(&norm, root) || admin.as_deref().is_some_and(|p| is_under_root(p, root)) + }) +} + +/// Whether these directory components end in one of [`HOST_AUTORUN_DIRS`]. +fn is_autorun_dir(names: &[String]) -> bool { + HOST_AUTORUN_DIRS.iter().any(|tail| { + names.len() >= tail.len() + && names[names.len() - tail.len()..] + .iter() + .zip(tail.iter()) + .all(|(have, want)| have.eq_ignore_ascii_case(want)) + }) +} + +/// Structural and policy checks on a host path *as written*, returning it as a +/// [`PathBuf`]. /// /// The `save()`/`open()` dialog the Files pane puts in front of these commands /// is a UI convention, not a boundary — every one of them is a single `invoke` /// away from any code running in the webview, with a container-controlled -/// payload on one side. So the backend has its own policy, and it is deliberately -/// blunt: +/// payload on one side. So the backend has its own policy: /// -/// * absolute, no `..`, no NUL — the same structural rules as a container -/// path, using [`Path::components`] so a Windows path is judged as one; -/// * nothing under [`HOST_SYSTEM_ROOTS`]; -/// * no *hidden* path components. This is the rule that matters. The -/// interesting targets for "write a container-controlled file to an -/// arbitrary host path" are all dot directories — `~/.ssh/authorized_keys`, -/// `~/.config/autostart/`, `~/.claude/` — and the interesting targets for -/// the reverse, reading a host file into the container, are the same ones -/// plus `~/.aws/credentials`. A download is refused a hidden *name* too -/// (creating `~/.bashrc` is escape all by itself); an upload only cares -/// about hidden *directories*, because dragging a project's own `.env` into -/// the container is an ordinary thing to do and its parent is not hidden. +/// * absolute, no `..`, no NUL — judged on the path's own components, so a +/// Windows path is judged as one wherever this runs; +/// * nothing under [`HOST_SYSTEM_ROOTS`] or in a login-item directory; +/// * no *hidden* path components. The interesting targets for "write a +/// container-controlled file to an arbitrary host path" are mostly dot +/// directories — `~/.ssh/authorized_keys`, `~/.config/autostart/`, +/// `~/.claude/` — and the interesting targets for the reverse, reading a +/// host file into the container, are the same ones plus `~/.aws/credentials`. +/// A download is refused a hidden *name* too (creating `~/.bashrc` is escape +/// all by itself); an upload only cares about hidden *directories*, because +/// dragging a project's own `.env` into the container is an ordinary thing +/// to do and its parent is not hidden. /// -/// What it costs: saving a container file to a hidden host location now has to -/// go somewhere visible first. That is a small, explainable price for closing a -/// container→host write primitive. +/// **This is a lexical check on a string, and lexical is not enough on its own.** +/// A path whose components are all visible can still lead somewhere hidden, so +/// nothing calls this directly any more: [`resolve_host_path`] resolves the +/// symlinks first and then applies this to the answer. Keeping the two apart is +/// what lets the policy stay pure and testable while the thing it judges is the +/// path that will really be opened. +/// +/// **And the policy itself is a denylist, which is losing by construction.** +/// `~/Library/LaunchAgents`, `%AppData%\…\Startup`, `~/bin` and `/opt` are only +/// refused because someone thought of them; the next persistence directory is +/// not. The honest fix is not a longer list — it is for the *backend* to own the +/// file dialog (`tauri-plugin-dialog` can be driven from Rust) so that the only +/// host paths these commands accept are ones the user just pointed at, and no +/// path arrives over IPC at all. That is a frontend change as well as this one. +/// Until then: this list is defence in depth, and the dialog is the boundary. fn validate_host_path(path: &str, use_for: HostPathUse) -> Result { - use std::path::Component; - if path.trim().is_empty() { return Err("No host path was given".to_string()); } if path.contains('\0') { return Err("Host path cannot contain a null byte".to_string()); } - - let candidate = PathBuf::from(path); - if !candidate.is_absolute() { + if !is_absolute_host_path(path) { return Err(format!("Host path must be absolute: {}", path)); } - let components: Vec = candidate.components().collect(); - if components.iter().any(|c| matches!(c, Component::ParentDir)) { + let names = host_path_names(path); + if names.iter().any(|n| n == "..") { return Err(format!("Host path cannot contain \"..\": {}", path)); } // The final component is the file itself; everything before it is a // directory the path passes *through*. - let names: Vec = components - .iter() - .filter_map(|c| match c { - Component::Normal(s) => Some(s.to_string_lossy().to_string()), - _ => None, - }) - .collect(); let hidden_limit = match use_for { HostPathUse::Write => names.len(), HostPathUse::Read => names.len().saturating_sub(1), @@ -443,11 +682,15 @@ fn validate_host_path(path: &str, use_for: HostPathUse) -> Result Result