From 913aa858057647da9d19b189c7c0bc58e276e1fb Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 17:09:47 -0700 Subject: [PATCH] Stop the pre-commit scrub running with its guards silently absent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two fail-open gaps, both latent on the shipped image and both live on an ImageSource::Custom one. `--one-file-system` is the only bound on the scrub's `rm -rf`, and it was probed into `$rmopt` with `rm --one-file-system --help`. BusyBox answers that with `unrecognized option` and exits 1, so on any busybox- or toybox-derived image the variable was empty and the delete ran as root across mount boundaries — reported as a completed `Reclaimed(n)`. Measured in `busybox:latest` with a named volume one level below the match at /tmp/claude-x/inner: emptied, `###TRIPLE-C-SCRUBBED 102423`. It is a prerequisite now, probed by `rm --one-file-system -f -- ''` — the code path that matters, with the one operand no filesystem can name — and an image without it prints `###TRIPLE-C-SCRUB-UNAVAILABLE` and deletes nothing. That costs Alpine and busybox images their scrub, which is the cheaper half of the trade: declining costs disk, proceeding costs the mount. The second is that resetting `PATH` was never the whole of command lookup. `bash` builds functions out of `BASH_FUNC_%%` environment variables, function lookup precedes `PATH` entirely, and `command -v` reports a function as found — so a planted `stat` passed the prerequisite probe and then answered the containment checks. Every in-shell answer is itself importable: `unset`, `command`, and — the point that settles it — `[`, `pwd` and `cd`, which are checks 0 to 2 rather than merely the tools. So the script is no longer run by the shell that read the environment. The exec's argv is a bootstrap using only reserved words, parameter expansions and command words containing a `/` (which `bash` refuses to import a function for), and it hands the script as `$1` to a second `/bin/sh` started by `env -i`. Measured on `/bin/sh -> bash` with `BASH_FUNC_stat%%` set on the container and a volume mounted at the match: the old invocation emptied it and printed 306565, the bootstrap left it intact and printed 65536. `/usr/bin/env` then `/bin/env`, because H3's lesson about hardcoded coreutils locations applies to `env` too. The comment claiming the `PATH` reset "defeats it just as completely as spelling /usr/bin/stat out" is corrected, as is the one claiming a sudo-written /usr/bin/stat does not survive a container restart — it lands in the writable layer, which is exactly what the commit this runs in front of captures. Also here: a stored project path row with an empty host_path or mount_name no longer becomes a mount. The first sends `field Source must not be empty` back for the whole create, so the project cannot start at all until the row is gone, and no amount of save-time validation reaches a record already on disk; the second mounts over /workspace itself and the daemon then creates the other rows' mount points inside the user's real folder. And in migration_commands, the deferred-reconcile claim is RAII rather than a trailing statement — a panic inside `reconcile_migration_now` stranded it for the rest of the process — and `await_release` looks before it sleeps, so a project released a moment later no longer costs a full twenty seconds. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc --- .../src/commands/migration_commands.rs | 199 ++++- app/src-tauri/src/docker/container.rs | 816 +++++++++++++++++- 2 files changed, 947 insertions(+), 68 deletions(-) diff --git a/app/src-tauri/src/commands/migration_commands.rs b/app/src-tauri/src/commands/migration_commands.rs index 33c534f..6be50da 100644 --- a/app/src-tauri/src/commands/migration_commands.rs +++ b/app/src-tauri/src/commands/migration_commands.rs @@ -1071,21 +1071,54 @@ fn reconcile_retries() -> &'static std::sync::Mutex bool { +/// `None` means somebody else already is. +fn claim_reconcile_retry(project_id: &str) -> Option { reconcile_retries() .lock() .unwrap_or_else(|e| e.into_inner()) .insert(project_id.to_string()) -} - -/// Give the claim back, so a later `reconcile_project_statuses` can defer again. -fn release_reconcile_retry(project_id: &str) { - reconcile_retries() - .lock() - .unwrap_or_else(|e| e.into_inner()) - .remove(project_id); + .then(|| ReconcileRetryClaim { + project_id: project_id.to_string(), + }) } /// Come back to a project that was held when [`reconcile_migration`] reached it. @@ -1108,13 +1141,19 @@ fn defer_migration_reconcile(project: &Project, app_handle: &tauri::AppHandle) { if !migration_store::has_record(&project.id).unwrap_or(true) { return; } - if !claim_reconcile_retry(&project.id) { + let Some(claim) = claim_reconcile_retry(&project.id) else { return; - } + }; let project = project.clone(); let app_handle = app_handle.clone(); tauri::async_runtime::spawn(async move { + // Moved in and bound for the whole body, rather than released by a + // statement at the bottom: everything below this line can panic or be + // dropped mid-await, and a claim that only comes back on the happy path + // is a claim that eventually does not come back at all. See + // [`ReconcileRetryClaim`]. + let _claim = claim; let released = await_release(&project.id, RECONCILE_RETRY_INTERVAL, RECONCILE_RETRY_ATTEMPTS).await; if released { @@ -1133,28 +1172,42 @@ fn defer_migration_reconcile(project: &Project, app_handle: &tauri::AppHandle) { RECONCILE_RETRY_INTERVAL.as_secs() as usize * RECONCILE_RETRY_ATTEMPTS / 60 ); } - release_reconcile_retry(&project.id); }); } -/// Wait for `project_id` to stop being held, up to `attempts` looks -/// `interval` apart. `true` means it was released, `false` that the budget ran -/// out with it still held. +/// Wait for `project_id` to stop being held: `attempts` looks, the first +/// immediate and the rest `interval` apart. `true` means it was released, +/// `false` that the budget ran out with it still held. /// /// Split out of [`defer_migration_reconcile`] so the waiting can be tested -/// against a real [`crate::project_lock`] guard on a paused clock — the part -/// that is easy to get wrong is "gives up while still holding the claim" and -/// "never looks again", neither of which is visible from the constants. +/// against a real [`crate::project_lock`] guard on a paused clock — the parts +/// that are easy to get wrong are "gives up while still holding the claim", +/// "never looks again", and the ordering of the look against the sleep, none of +/// which is visible from the constants. async fn await_release( project_id: &str, interval: std::time::Duration, attempts: usize, ) -> bool { - for _ in 0..attempts { - tokio::time::sleep(interval).await; + for attempt in 0..attempts { + // Look first, sleep second. Sleeping first charged every deferral a + // full interval before anyone read the map even once, and the common + // case is a holder that has already let go: `held()` is sampled in + // `reconcile_migration`, a task is spawned, and by the time it is first + // polled the Reset that was on its last step is frequently finished. + // That bought nothing and cost twenty seconds of a startup pass waiting + // on a lock nobody holds, in front of a check that is one `HashMap` + // lookup. if crate::project_lock::held(project_id).is_none() { return true; } + // And no sleep after the final look: nothing reads the map again + // afterwards, so it is twenty seconds of delay in front of a `false` + // that has already been decided. The budget is still `attempts` looks, + // which is what the constants above are chosen against. + if attempt + 1 < attempts { + tokio::time::sleep(interval).await; + } } false } @@ -2217,6 +2270,34 @@ mod tests { assert!(budget >= std::time::Duration::from_secs(15 * 60), "{:?}", budget); } + /// MEDIUM: an unheld project is reconciled now, not in twenty seconds. + /// + /// The wait slept before its first look, so a holder that let go between + /// `reconcile_migration` sampling `held()` and this task being polled — the + /// *common* case, since a deferral is only taken when something was on its + /// way out — still cost a full `RECONCILE_RETRY_INTERVAL` of a startup pass + /// waiting on a lock nobody held. On a paused clock the assertion is exact: + /// the fixed shape returns without the clock moving at all, the sleep-first + /// shape cannot return before it has advanced one interval. + #[tokio::test(start_paused = true)] + async fn an_unheld_project_is_seen_without_waiting_out_an_interval() { + let id = format!("await-release-{}", uuid::Uuid::new_v4().simple()); + assert!( + crate::project_lock::held(&id).is_none(), + "a fresh uuid is not held" + ); + + let before = tokio::time::Instant::now(); + assert!(await_release(&id, RECONCILE_RETRY_INTERVAL, RECONCILE_RETRY_ATTEMPTS).await); + let waited = tokio::time::Instant::now() - before; + assert_eq!( + waited, + std::time::Duration::ZERO, + "an already-released project cost {:?} before anyone looked", + waited + ); + } + #[test] fn only_one_deferred_reconcile_waits_per_project() { // Every "Docker became available" walks every project, so without the @@ -2224,14 +2305,72 @@ mod tests { // per call — all of which then reconcile the same record in a row. let id = format!("retry-claim-{}", uuid::Uuid::new_v4().simple()); let other = format!("retry-claim-{}", uuid::Uuid::new_v4().simple()); - assert!(claim_reconcile_retry(&id)); - assert!(!claim_reconcile_retry(&id), "a second waiter was allowed in"); - assert!(claim_reconcile_retry(&other), "the claim is not per-project"); - release_reconcile_retry(&id); - assert!(claim_reconcile_retry(&id), "the claim was never handed back"); - release_reconcile_retry(&id); - release_reconcile_retry(&other); - // Releasing something that was never claimed is not an error. - release_reconcile_retry(&id); + let first = claim_reconcile_retry(&id).expect("a fresh project id is unclaimed"); + assert!( + claim_reconcile_retry(&id).is_none(), + "a second waiter was allowed in" + ); + let other_claim = + claim_reconcile_retry(&other).expect("the claim is not per-project"); + drop(first); + // Bound rather than discarded: the guard releases on drop, so + // `claim_reconcile_retry(&id);` as a bare statement would test nothing + // — which is what `#[must_use]` is there to catch in real callers. + let retaken = claim_reconcile_retry(&id).expect("the claim was never handed back"); + drop(retaken); + drop(other_claim); + // And the other project's claim was never the same claim. + drop(claim_reconcile_retry(&other).expect("released independently")); + } + + /// MEDIUM: the claim survives the task that holds it dying badly. + /// + /// The release used to be a trailing statement after + /// `reconcile_migration_now(...).await` at the bottom of the spawned task, + /// so a panic anywhere in that call — or the future being dropped at + /// shutdown — skipped it and left the id in the set with no task behind it. + /// Nothing removes it afterwards, so that project could never be deferred + /// again for the rest of the process: exactly the state deferring was added + /// to prevent, now permanent instead of one pass long. Fails against the + /// trailing-statement shape, which is the point. + #[tokio::test] + async fn a_panicking_deferred_reconcile_hands_its_claim_back() { + let id = format!("retry-claim-{}", uuid::Uuid::new_v4().simple()); + let claimed = claim_reconcile_retry(&id).expect("a fresh project id is unclaimed"); + + // Spawned, not just called: the real claim is held across an await + // inside a `tauri::async_runtime::spawn`, and a task panic is caught by + // the runtime rather than unwinding the caller. + let task = { + let id = id.clone(); + tokio::spawn(async move { + let _claim = claimed; + tokio::task::yield_now().await; + panic!("reconcile_migration_now blew up on '{}'", id); + }) + }; + assert!(task.await.is_err(), "the task was supposed to panic"); + + let after = claim_reconcile_retry(&id); + assert!( + after.is_some(), + "a panicking reconcile stranded the claim — this project can never be \ + deferred again for the rest of the process" + ); + drop(after); + + // The other half of the same failure: a task that is simply dropped + // mid-flight, which is every in-flight task at shutdown. + let claimed = claim_reconcile_retry(&id).expect("released above"); + let never_finishes = tokio::spawn(async move { + let _claim = claimed; + std::future::pending::<()>().await; + }); + never_finishes.abort(); + let _ = never_finishes.await; + assert!( + claim_reconcile_retry(&id).is_some(), + "a dropped task stranded the claim" + ); } } diff --git a/app/src-tauri/src/docker/container.rs b/app/src-tauri/src/docker/container.rs index 7dc2d54..58b84c8 100644 --- a/app/src-tauri/src/docker/container.rs +++ b/app/src-tauri/src/docker/container.rs @@ -1118,6 +1118,49 @@ fn explain_container_failure(action: &str, err: &str) -> String { format!("Failed to {} container: {}", action, err) } +/// One bind mount per stored project path, skipping the rows that cannot +/// produce a usable one. +/// +/// Both halves of the filter are about data that is **already on disk**, which +/// is why this is a silent skip and not a validation error. `project_commands` +/// now refuses an empty `mount_name` or `host_path` on save, and the Workspace +/// pane can no longer send a half-filled row — but neither of those reaches a +/// record written before they existed, and the two failures such a record +/// causes are not equally visible: +/// +/// * An empty `host_path` becomes `{"Target":"/workspace/x","Source":""}`, and +/// the daemon answers `invalid mount config for type "bind": field Source +/// must not be empty` for the whole create. The project cannot be started or +/// recreated at all — it is bricked, and it stays bricked however carefully +/// the next save is validated. Skipping the row lets it start again, minus a +/// mount that was never going to work. +/// * An empty `mount_name` targets `/workspace/` itself, mounting the row's +/// host directory *over* the workspace volume. The daemon then creates the +/// other rows' mount points inside the user's real folder, which is a +/// directory tree appearing in their project from nowhere. +/// +/// Failing louder is the wrong instinct here: the loud version is the one that +/// already happened, and it took the project down with it. See +/// `project_commands.rs`'s `check_mount_name_stays_under_workspace` for the +/// save-time half — deliberately still tolerant of an empty name there, since +/// refusing it would re-brick every project holding a legacy row. +fn project_path_mounts(paths: &[crate::models::project::ProjectPath]) -> Vec { + paths + .iter() + // Trimmed, because a name of `" "` targets `/workspace/ ` and a source + // of `" "` is a path the daemon will happily create at the filesystem + // root — neither is what anyone typed on purpose. + .filter(|pp| !pp.mount_name.trim().is_empty() && !pp.host_path.trim().is_empty()) + .map(|pp| Mount { + target: Some(format!("/workspace/{}", pp.mount_name)), + source: Some(pp.host_path.clone()), + typ: Some(MountTypeEnum::BIND), + read_only: Some(false), + ..Default::default() + }) + .collect() +} + pub async fn create_container( project: &Project, docker_socket_path: &str, @@ -1539,15 +1582,7 @@ pub async fn create_container( let mut mounts: Vec = Vec::new(); // Project directories -> /workspace/{mount_name} - for pp in &project.paths { - mounts.push(Mount { - target: Some(format!("/workspace/{}", pp.mount_name)), - source: Some(pp.host_path.clone()), - typ: Some(MountTypeEnum::BIND), - read_only: Some(false), - ..Default::default() - }); - } + mounts.extend(project_path_mounts(&project.paths)); // Named volume for the entire home directory — preserves ~/.claude.json, // ~/.local (pip/npm globals), and any other user-level state across @@ -2163,6 +2198,16 @@ const SCRUB_MARKER: &str = "###TRIPLE-C-SCRUBBED "; /// `parse_scrub_total` can never mistake one for the other. const SCRUB_UNAVAILABLE_MARKER: &str = "###TRIPLE-C-SCRUB-UNAVAILABLE "; +/// The external tools [`snapshot_scrub_script`] probes for before it will +/// delete anything, in the order the probe names them. +/// +/// Named once rather than spelled into the script text, because +/// [`parse_scrub_unavailable`] has to be able to tell a missing *tool* from the +/// other two prerequisites — the root device id, and an `rm` that takes +/// `--one-file-system` — so that the log line reads as a sentence rather than +/// as a list with `root-device-id` wedged into it. +const SCRUB_PREREQ_TOOLS: &[&str] = &["stat", "rm", "du", "cut", "find"]; + /// Environment the scrub exec sets explicitly, rather than inheriting. /// /// The exec inherits the container's configured env, and a project's *custom* @@ -2183,8 +2228,93 @@ const SCRUB_UNAVAILABLE_MARKER: &str = "###TRIPLE-C-SCRUB-UNAVAILABLE "; /// `LD_LIBRARY_PATH` is here for the same reason as `LD_PRELOAD`: it is /// searched ahead of the cache for every `DT_NEEDED` library, so a planted /// `libselinux.so.1` is as good as a preload. +/// +/// M2: [`SCRUB_BOOTSTRAP`] now starts the shell that runs the script under +/// `env -i`, so none of these three reaches the script's own tools whatever +/// this list says. They are kept because they still reach the *two processes +/// in front of it* — the `/bin/sh` that runs the bootstrap and the `env` it +/// calls — and an `env` with a preload in it is an `env` that can hand its +/// child any environment it likes. Blanking here and emptying there are the +/// same defence applied at the two points it has to hold at. const SCRUB_EXEC_ENV: &[&str] = &["LD_PRELOAD=", "LD_AUDIT=", "LD_LIBRARY_PATH="]; +/// The `/bin/sh -c` program that starts the real scrub, with +/// [`snapshot_scrub_script`] passed to it as `$1` rather than spliced into it. +/// +/// ## Why the script is not simply the exec's command (M2) +/// +/// `docker exec` cannot *replace* a container's environment, only add to and +/// override it — which is why [`SCRUB_EXEC_ENV`] works by naming keys. The env +/// a project carries is therefore inherited by whatever shell the exec starts, +/// and a shell reads more out of its environment than variables. +/// +/// `bash` imports **functions** from the environment: an env var named +/// `BASH_FUNC_stat%%` becomes a shell function called `stat`. Function lookup +/// happens *before* `PATH` is ever consulted, so resetting `PATH` does not +/// touch it, and `command -v` reports a function as found — so a planted +/// function passes the prerequisite probe and then answers the containment +/// checks. Measured against `bash` 5.2 on `ubuntu:24.04`, an environment +/// carrying `BASH_FUNC_stat%%` had `stat -c %d` return a constant of its +/// choosing while `command -v stat` said the tool was present. +/// +/// Every in-shell answer to that was tried and measured, and each one is +/// itself importable: `unset -f stat` is defeated by `BASH_FUNC_unset%%`, +/// `command stat` by `BASH_FUNC_command%%`, and — this is what settles it — +/// `[`, `test`, `pwd` and `cd` import just as readily, which is checks 0, 1 +/// and 2 of the containment guarantee, not merely the tools. There is no +/// subset of the script that can be written in shell and still be trusted +/// inside a shell that has already imported the attacker's functions. +/// +/// So the script is not run by that shell. This bootstrap is, and everything +/// it uses is either a reserved word (`case`, `esac`), a parameter expansion, +/// or a command word containing a `/` — and `bash` refuses to import a +/// function whose name contains a `/`, verified on the same image. It hands +/// the script to a **second** `/bin/sh` started by `env -i`, which has no +/// environment at all: no `BASH_FUNC_*`, no `LD_*`, no `ENV`/`BASH_ENV`, and +/// no `PATH` but the one named here. The inner shell's builtins are its own +/// again, and the script does not have to care what `/bin/sh` is. +/// +/// Measured on an `ubuntu:24.04` with `/bin/sh -> bash`, a project env var of +/// `BASH_FUNC_stat%%=() { echo 1; }` set on the container the way a custom env +/// var is, and a named volume mounted **at** the match `/tmp/claude-x` — the +/// position checks 3 and 4 exist for: +/// +/// * `/bin/sh -c