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