From 39934299f92e27d2a96977d13f663df025e53095 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sun, 23 Aug 2026 15:42:20 -0700 Subject: [PATCH] Stop the snapshot retry being an unconfirmed revoke, and unbrick legacy configs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three things, all reachable from a single credential-handling round. **The "Retry snapshot cleanup" button deleted the token.** `clear_claude_token` called `secure::delete_claude_oauth_token()` unconditionally; the `mode` argument only changed the toast wording, so there was no sweep-only path on the wire. The leftover panel rendered on `leftover !== null` alone and `onAuthenticated` never cleared it, so the sequence revoke -> snapshot skipped -> re-authenticate from the button directly above -> press the retry the panel is still offering threw away the token acquired seconds earlier, announced by a message about images. The deliberate Revoke needs a confirmation modal; this needed nothing. `sweep_claude_token_snapshots` is the honest primitive: it rewrites the images and never touches the keychain. The images are the durable record, so the retry re-derives its work from Docker and needs no stored token. Re-authenticating now clears the panel, and the Authenticate button is disabled while a cleanup runs — a sweep is a per-image inspect/create/commit/rmi over the Docker socket and takes minutes. **Sweep-first left the token live for that whole window.** The keychain delete sat behind `list_images` plus the per-image loop, at bollard's 120s-per-request default, while the UI said "Revoking...". A quit or crash in there and nothing was revoked at all; worse, `has_claude_token` stayed true and `shared_claude_auth` reads the keychain at container-*create* time, so a project whose `SecretScrub` guard had already released could be started later in the same sweep and be handed a fresh copy of the credential in its env. Keychain-first now, and the comment that claimed "no window in which a scrubbed image is re-poisoned" — true of images, silent about containers — is corrected. The reorder's original justification (crash-mid-sweep recoverability) is what the sweep-only command covers. A keychain refusal no longer discards a scrub report, because nothing has been swept yet, and both remedies stay on screen: Revoke, and the image sweep, which is now offered in every state rather than only when nothing is stored. **`update_project` validating every folder list bricked existing projects.** It validated nothing until recently while `WorkspaceSection` saved `{paths}` on every blur, so `projects.json` can hold a half-filled row, a mount name with a space, a duplicate, or `/` as a host path. Any such project became entirely unsavable — every Config toggle, every permission-mode change and `useTerminal.ts`'s tab rename came back with a message about folders — and refusing the save did not unmount anything. `validate_project_paths_update` admits a row carried over verbatim from what is stored and holds a new or edited row to every rule, which keeps the escalation closed: introducing a bad value through this command is exactly what a non-carried-over row is. The `/workspace/../tmp/claude-x` chain is the one exception and runs on every row regardless, because a stored one is live data loss rather than untidy data. `ssh_key_path` and `ca_cert_path` had no check at all; a filesystem root there read-only bind-mounts the whole host at /tmp/.host-ssh. Refused on change, with the same grandfathering. Tests are mutation-checked: reverting to sweep-first fails all four new Rust ordering tests, pointing the retry back at `clear_claude_token` fails five frontend tests, and validating an update in isolation fails the legacy-data tests. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01GBq2rGum6GX7xXgsas1fDc --- .../src/commands/auth_token_commands.rs | 359 ++++++++++++--- .../src/commands/project_commands.rs | 414 ++++++++++++++++-- app/src-tauri/src/lib.rs | 1 + .../settings/SharedAuthSettings.test.tsx | 171 +++++++- .../settings/SharedAuthSettings.tsx | 121 +++-- app/src/lib/tauri-commands.ts | 10 +- 6 files changed, 910 insertions(+), 166 deletions(-) diff --git a/app/src-tauri/src/commands/auth_token_commands.rs b/app/src-tauri/src/commands/auth_token_commands.rs index 9d6df9a..c50a10c 100644 --- a/app/src-tauri/src/commands/auth_token_commands.rs +++ b/app/src-tauri/src/commands/auth_token_commands.rs @@ -1214,9 +1214,11 @@ fn is_project_busy_refusal(reason: &str) -> bool { reason.contains(PROJECT_BUSY_MARKER) } -/// What [`clear_claude_token`] managed to reach. The keychain entry is gone by -/// the time this is returned — the rest is about copies of the token that live -/// outside it. +/// What a cleanup managed to reach. Every field is about copies of the token +/// that live *outside* the keychain — snapshot images — so the same shape +/// serves [`clear_claude_token`], where the keychain entry is already gone by +/// the time this is returned, and [`sweep_claude_token_snapshots`], where the +/// keychain was never touched. /// /// Three lists rather than one, because "we rewrote it", "we could not rewrite /// it" and "we did not try" are three different things to tell somebody who @@ -1277,6 +1279,93 @@ fn summarise_scrub(report: crate::docker::container::SnapshotScrubReport) -> Cle outcome } +/// Which halves of a cleanup to run. +/// +/// The distinction has to exist **on the wire**, not in a toast string. The UI +/// offers a "Retry snapshot cleanup" button after an incomplete revocation, and +/// while [`clear_claude_token`] was the only command behind it that button was +/// a *second revoke* wearing a retry's label: it deleted the keychain entry +/// unconditionally, with no confirmation, in a panel that survived the user +/// re-authenticating from the button directly above it. Pressing it then threw +/// away the token they had just acquired and said only that some images had +/// been checked. +/// +/// [`Cleanup::ImagesOnly`] is the honest primitive the retry actually wanted: +/// the images are the durable record of what is left to do, so re-deriving the +/// work from Docker needs no keychain entry and must not consume one. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum Cleanup { + /// Delete the keychain entry, then rewrite the images. What "Revoke" does, + /// behind its confirmation modal. + KeychainThenImages, + /// Rewrite the images and leave the keychain entirely alone. What "Retry + /// snapshot cleanup" and "Check snapshot images" do. + ImagesOnly, +} + +/// The body of both cleanup commands, with its two halves injected so the +/// *order* — and the fact that [`Cleanup::ImagesOnly`] never reaches the +/// keychain at all — can be tested without a keychain or a Docker daemon. +async fn run_cleanup( + what: Cleanup, + delete_keychain: K, + sweep: S, +) -> Result +where + K: FnOnce() -> Result<(), String>, + S: FnOnce() -> F, + F: std::future::Future, +{ + let revoking = what == Cleanup::KeychainThenImages; + + if revoking { + // First, and before anything slow — see "Why the keychain goes first". + // Nothing has been touched if this fails, so the error is the whole + // answer: the caller still has its Revoke button, and the standalone + // sweep is available for the images regardless. + if let Err(e) = delete_keychain() { + log::error!( + "Could not delete the shared Claude token from the keychain; no snapshot image \ + was touched: {}", + e + ); + return Err(e); + } + log::info!("Cleared the shared Claude authentication token from the keychain"); + } + + let report = sweep().await; + let swept_clean = !report.left_something_behind(); + let outcome = summarise_scrub(report); + + let lead = if revoking { + "Revoked the shared Claude token but" + } else { + "Swept the snapshot images but" + }; + for image in &outcome.snapshots_failed { + log::warn!("{} could not clear it from {}", lead, image); + } + for image in &outcome.snapshots_skipped { + log::warn!( + "{} left it in {} — the project was busy; running the snapshot sweep again will retry", + lead, + image + ); + } + if let Some(ref reason) = outcome.docker_unavailable { + log::warn!("{} checked no snapshot image at all: {}", lead, reason); + } + if swept_clean && !outcome.needs_another_pass() { + log::info!( + "No snapshot image is still holding the shared Claude token ({} rewritten)", + outcome.snapshots_scrubbed.len() + ); + } + + Ok(outcome) +} + /// Forget the shared Claude token, and remove the copies of it that outlive the /// keychain entry. /// @@ -1296,81 +1385,79 @@ fn summarise_scrub(report: crate::docker::container::SnapshotScrubReport) -> Cle /// committed by earlier builds have to be rewritten, which is what /// [`crate::docker::container::scrub_secrets_from_snapshots`] does here. /// -/// ## Why the snapshots go first +/// ## Why the keychain goes first /// -/// They used to go second, and that ordering had no recovery path. The scrub -/// runs **once** and skips any project another operation holds; the keychain -/// entry, already deleted, made `has_claude_token` false; and the frontend only -/// rendered Revoke while a token was stored. So a project that happened to be -/// starting during a revoke kept a live ~1-year OAuth token in its snapshot's -/// `Config.Env` permanently, and the only remedy the UI still offered was Reset, -/// which destroys both volumes. +/// The sweep is not quick. It lists every `triple-c-snapshot-*` image and then +/// inspects, creates, commits and removes *per image*, over bollard's Docker +/// socket with its 120-second-per-request default. Deferring the keychain +/// delete behind all of that leaves the credential live for the whole window +/// while the UI says "Revoking…", and two separate things go wrong in it: /// -/// Scrubbing first closes the crash window in that story: the app can be killed -/// at any point during the sweep and the *next* launch still says -/// "authenticated", so the same button is still there and still does the same -/// thing. Nothing is lost by the reorder — `rewrite_image_without_secrets` -/// never reads the keychain, and `commit_container_snapshot` no longer bakes -/// the token in, so there is no window in which a scrubbed image is re-poisoned -/// by the entry we have not deleted yet. +/// * A quit, a crash or a kill mid-sweep and the entry was never deleted at +/// all. The token the user believes they revoked is still in the keychain, +/// still ~1-year valid, and still injected into every container start. +/// * [`has_claude_token`] stays true throughout, and +/// [`crate::docker::container::create_container`] reads the keychain at +/// container-**create** time rather than at app start. The per-project +/// [`crate::project_lock::ProjectOp::SecretScrub`] guard is released as soon +/// as that one project's image has been rewritten — so a project scrubbed +/// early in the sweep can be started again later in the *same* sweep and be +/// handed a fresh copy of the credential in its env. The images end up clean +/// and the running fleet does not. /// -/// ## This command is the retry +/// An earlier version ran the sweep first, on the argument that a crash +/// mid-sweep would otherwise leave the token in an image with the keychain +/// entry — and therefore the Revoke button — already gone. That argument was +/// about *recoverability*, and [`sweep_claude_token_snapshots`] answers it +/// directly: the images are the durable record, so the retry needs no keychain +/// entry to exist and no persisted to-do list. The comment that ordering +/// carried ("no window in which a scrubbed image is re-poisoned") was true of +/// images and silent about containers, which is where the leak was, and silent +/// about the minutes the token stayed live. /// -/// It is idempotent and safe to call with no token stored: `delete_entry` -/// treats a missing entry as success, and the sweep re-derives what is left -/// from Docker rather than from any record we would have to keep. So "try the -/// snapshot cleanup again" needs no second command and no persisted to-do -/// list — the images themselves are the durable record, and calling this again -/// is how the user acts on [`ClearTokenOutcome::needs_another_pass`]. -/// -/// The keychain deletion is never rolled back if the scrub fails; a partially -/// completed revocation is still better than none, and the outcome is reported -/// so the UI can be explicit about what is left. +/// The keychain deletion is never rolled back if the scrub then fails; a +/// partially completed revocation is still better than none, and the outcome is +/// reported so the UI can be explicit about what is left. #[tauri::command] pub async fn clear_claude_token() -> Result { - let report = crate::docker::container::scrub_secrets_from_snapshots().await; - let swept_clean = !report.left_something_behind(); - let outcome = summarise_scrub(report); + run_cleanup( + Cleanup::KeychainThenImages, + secure::delete_claude_oauth_token, + crate::docker::container::scrub_secrets_from_snapshots, + ) + .await +} - // The keychain second — see "Why the snapshots go first". An error here is - // the loud case (the token may still be usable), so it wins over the - // report, which is logged rather than dropped on the floor. - if let Err(e) = secure::delete_claude_oauth_token() { - log::error!( - "Could not delete the shared Claude token from the keychain; the snapshot sweep had \ - already scrubbed {} image(s), failed on {}, skipped {}", - outcome.snapshots_scrubbed.len(), - outcome.snapshots_failed.len(), - outcome.snapshots_skipped.len() - ); - return Err(e); - } - log::info!("Cleared the shared Claude authentication token"); - - for image in &outcome.snapshots_failed { - log::warn!("Revoked the shared Claude token but could not clear it from {}", image); - } - for image in &outcome.snapshots_skipped { - log::warn!( - "Revoked the shared Claude token but left it in {} — the project was busy; \ - revoking again will retry", - image - ); - } - if let Some(ref reason) = outcome.docker_unavailable { - log::warn!( - "Revoked the shared Claude token without checking any snapshot image: {}", - reason - ); - } - if swept_clean && !outcome.needs_another_pass() { - log::info!( - "No snapshot image is still holding the shared Claude token ({} rewritten)", - outcome.snapshots_scrubbed.len() - ); - } - - Ok(outcome) +/// Rewrite every snapshot image that still carries a credential, **without +/// touching the keychain**. +/// +/// This is the retry, and it is its own command because the retry is its own +/// act. `docker commit` copied the token into each project's snapshot image; +/// rewriting those images is a cleanup that has nothing to do with whether a +/// token is stored today, and folding it into [`clear_claude_token`] made every +/// press of "Retry snapshot cleanup" an unconfirmed credential deletion. +/// +/// Safe to call at any time and in any state: +/// +/// * with a token stored — a snapshot committed by an older build carries the +/// *current* token, and clearing it out of the image does not stop the +/// keychain entry being injected on the next container start; +/// * with nothing stored — images committed by earlier builds still carry +/// whatever token was live when they were committed, which is exactly the +/// case the old sweep-first ordering could strand; +/// * repeatedly — the work is re-derived from Docker each time, so an image +/// whose project was busy on the last pass is simply picked up on this one. +#[tauri::command] +pub async fn sweep_claude_token_snapshots() -> Result { + run_cleanup( + Cleanup::ImagesOnly, + // Never called; the `ImagesOnly` branch is the entire point of this + // command, and a change that made it reachable must fail loudly rather + // than delete a credential quietly. + || -> Result<(), String> { unreachable!("an images-only sweep must never touch the keychain") }, + crate::docker::container::scrub_secrets_from_snapshots, + ) + .await } #[cfg(test)] @@ -2012,4 +2099,134 @@ mod tests { assert!(object.contains_key(key), "missing {} in {:?}", key, object); } } + + // ── The order of a revocation, and what a retry may touch ───────────── + // + // `run_cleanup` takes both halves as arguments precisely so this can be + // asserted with no keychain and no Docker daemon: the recorded order *is* + // the subject. Sweep-first put a live ~1-year credential behind a + // per-image inspect/create/commit/rmi loop — minutes, at bollard's + // 120s-per-request default — during which `has_claude_token` stayed true + // and `create_container` kept handing the token to anything started. + + /// Records which half ran, in order. + type Trace = std::sync::Arc>>; + + fn trace() -> Trace { + std::sync::Arc::new(std::sync::Mutex::new(Vec::new())) + } + + fn scrubbed_one() -> SnapshotScrubReport { + SnapshotScrubReport { + scrubbed: vec!["triple-c-snapshot-a:latest".into()], + ..Default::default() + } + } + + #[tokio::test] + async fn the_keychain_entry_is_gone_before_the_first_image_is_touched() { + let t = trace(); + let (tk, ts) = (t.clone(), t.clone()); + + let outcome = run_cleanup( + Cleanup::KeychainThenImages, + move || { + tk.lock().unwrap().push("keychain"); + Ok(()) + }, + move || async move { + ts.lock().unwrap().push("sweep"); + scrubbed_one() + }, + ) + .await + .expect("a cleanup whose halves both succeed is not an error"); + + assert_eq!( + *t.lock().unwrap(), + ["keychain", "sweep"], + "the token stayed in the keychain — and therefore in every container created — \ + for the whole length of the image sweep" + ); + assert_eq!(outcome.snapshots_scrubbed, vec!["triple-c-snapshot-a:latest"]); + } + + #[tokio::test] + async fn a_keychain_failure_leaves_the_images_untouched_and_is_reported() { + let t = trace(); + let ts = t.clone(); + + let err = run_cleanup( + Cleanup::KeychainThenImages, + || Err("the keychain is locked".to_string()), + move || async move { + ts.lock().unwrap().push("sweep"); + SnapshotScrubReport::default() + }, + ) + .await + .expect_err("a keychain that refused the delete must be reported, not swallowed"); + + assert_eq!(err, "the keychain is locked"); + assert!( + t.lock().unwrap().is_empty(), + "images were rewritten for a revocation that never happened; the report is then \ + discarded with the error and the user is told nothing they can act on" + ); + } + + /// The bug the images-only primitive exists to close: the "Retry snapshot + /// cleanup" button used to run `clear_claude_token`, so pressing it after + /// re-authenticating deleted the brand-new token with no confirmation. + #[tokio::test] + async fn an_images_only_cleanup_never_reaches_the_keychain() { + let t = trace(); + let (tk, ts) = (t.clone(), t.clone()); + + let outcome = run_cleanup( + Cleanup::ImagesOnly, + move || { + tk.lock().unwrap().push("keychain"); + Ok(()) + }, + move || async move { + ts.lock().unwrap().push("sweep"); + scrubbed_one() + }, + ) + .await + .expect("a sweep-only cleanup is not an error"); + + assert_eq!( + *t.lock().unwrap(), + ["sweep"], + "the retry deleted a credential nobody confirmed deleting" + ); + assert_eq!(outcome.snapshots_scrubbed, vec!["triple-c-snapshot-a:latest"]); + } + + /// …and it still has to report what it could not finish, because "run it + /// again once that project is idle" is the whole affordance. + #[tokio::test] + async fn an_images_only_cleanup_still_reports_what_it_could_not_finish() { + let outcome = run_cleanup( + Cleanup::ImagesOnly, + || -> Result<(), String> { unreachable!("images only") }, + || async { + SnapshotScrubReport { + failed: vec![( + "triple-c-snapshot-b:latest".into(), + format!("This project is being started. {}removing a credential from its snapshot.", PROJECT_BUSY_MARKER), + )], + ..Default::default() + } + }, + ) + .await + .expect("an image left for the next pass is not a command failure"); + + assert!(outcome.needs_another_pass()); + assert_eq!(outcome.snapshots_skipped.len(), 1, "{:?}", outcome); + assert!(outcome.snapshots_failed.is_empty(), "{:?}", outcome); + } } diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 3f57c48..3ff42ae 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -187,18 +187,20 @@ pub(crate) fn load_secrets_for_project(project: &mut Project) { /// destination of `/tmp/claude-x`, i.e. the host directory is mounted straight /// on top of one of the paths the pre-commit scrub owns. The next recreate then /// runs the scrub, as root, over the user's own project directory. That is the -/// C1 data-loss chain end to end, and the character check is the half of it -/// that stops the path ever being spelled. -/// -/// `..` on its own passes a check for "alphanumeric, dash, underscore or dot", -/// which is why it is called out separately: it is the only single component -/// that walks *up*, and `/workspace/..` is `/`. +/// C1 data-loss chain end to end, and +/// [`check_mount_name_stays_under_workspace`] is the half of it that stops the +/// path ever being spelled. /// /// `host_path` is the other side of the same mount. `/` there bind-mounts the /// entire host filesystem read-write into a container whose agent has /// passwordless sudo. Anything short of a filesystem root is the user choosing /// a folder — the Browse button and the free-text field lead to the same place /// — so only the roots themselves are refused. +/// +/// This is the **whole** rule set, and it belongs to `add_project`, where every +/// row is new by definition. `update_project` runs +/// [`validate_project_paths_update`] instead: see there for why a list that is +/// already in `projects.json` cannot be held to all of it. fn validate_project_paths(paths: &[ProjectPath]) -> Result<(), String> { let mut seen_names = std::collections::HashSet::new(); for p in paths { @@ -209,30 +211,7 @@ fn validate_project_paths(paths: &[ProjectPath]) -> Result<(), String> { if p.host_path.is_empty() && p.mount_name.is_empty() { continue; } - if p.mount_name.is_empty() { - return Err("Mount name cannot be empty.".to_string()); - } - if !p.mount_name.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_' || c == '.') { - return Err(format!("Mount name '{}' contains invalid characters. Use alphanumeric, dash, underscore, or dot.", p.mount_name)); - } - if p.mount_name.chars().all(|c| c == '.') { - return Err(format!( - "Mount name '{}' is not a folder name — it names the directory the mount would sit in.", - p.mount_name - )); - } - if p.host_path.is_empty() { - return Err(format!( - "Folder mounted at '/workspace/{}' has no host path.", - p.mount_name - )); - } - if is_filesystem_root(&p.host_path) { - return Err(format!( - "'{}' is a filesystem root. Choose the project folder itself — mounting the whole drive gives the container everything on it.", - p.host_path - )); - } + validate_one_path(p)?; if !seen_names.insert(p.mount_name.clone()) { return Err(format!("Duplicate mount name '{}'.", p.mount_name)); } @@ -240,6 +219,176 @@ fn validate_project_paths(paths: &[ProjectPath]) -> Result<(), String> { Ok(()) } +/// Every rule that applies to a single folder row, duplicates aside. +fn validate_one_path(p: &ProjectPath) -> Result<(), String> { + if p.mount_name.is_empty() { + return Err("Mount name cannot be empty.".to_string()); + } + if !p.mount_name.chars().all(|c| c.is_alphanumeric() || c == '-' || c == '_' || c == '.') { + return Err(format!("Mount name '{}' contains invalid characters. Use alphanumeric, dash, underscore, or dot.", p.mount_name)); + } + check_mount_name_stays_under_workspace(&p.mount_name)?; + if p.host_path.is_empty() { + return Err(format!( + "Folder mounted at '/workspace/{}' has no host path.", + p.mount_name + )); + } + if is_filesystem_root(&p.host_path) { + return Err(format!( + "'{}' is a filesystem root. Choose the project folder itself — mounting the whole drive gives the container everything on it.", + p.host_path + )); + } + Ok(()) +} + +/// The one rule that survives every exemption: the mount has to land under +/// `/workspace`. +/// +/// A name carrying a path separator, or one that is nothing but dots, does not +/// name a folder inside `/workspace` — it moves the mount. `/workspace/..` is +/// `/`, `/workspace/../tmp/claude-x` normalises to `/tmp/claude-x`, and +/// `/workspace/.` is `/workspace` itself, shadowing every other mount. The +/// first of those is the C1 chain: a host directory mounted over a path the +/// pre-commit scrub empties as root. +/// +/// Everything else `validate_one_path` checks is hygiene — a space or an `@` in +/// a mount name is untidy, not an escape — which is why only this one is +/// applied to rows [`validate_project_paths_update`] otherwise grandfathers. +fn check_mount_name_stays_under_workspace(mount_name: &str) -> Result<(), String> { + if mount_name.contains('/') || mount_name.contains('\\') { + return Err(format!( + "Mount name '{}' contains a path separator, so the folder would be mounted somewhere \ + /workspace/{{name}} does not reach. Use a plain folder name.", + mount_name + )); + } + if !mount_name.is_empty() && mount_name.chars().all(|c| c == '.') { + return Err(format!( + "Mount name '{}' is not a folder name — it names the directory the mount would sit in.", + mount_name + )); + } + Ok(()) +} + +/// Validate the folder list of a project that **already exists**, admitting the +/// rows it is already stored with. +/// +/// ## Why this is not just [`validate_project_paths`] +/// +/// `update_project` validated nothing at all until recently, while +/// `WorkspaceSection` saved `{paths}` on every blur — so `projects.json` files +/// in the field hold rows that the full rule set refuses: a half-filled row, a +/// mount name with a space in it, two rows sharing a name, `/` as a host path. +/// +/// Running the full check on every save made every such project **entirely +/// unsavable**. Not just its folder list: `update_project` is the one command +/// behind the Config tab, so every toggle, every permission-mode change and +/// `useTerminal.ts`'s tab rename came back with a message about folders. The +/// remedy exists — fix the row in the Workspace section — but nothing about +/// "cannot save" on a sandbox switch points at it. +/// +/// And blocking the save bought nothing for the rows it was blocking. They are +/// *already stored*, and already mounted on every container start; refusing to +/// persist an unrelated field does not unmount them. +/// +/// So: a row carried over verbatim from what is stored is admitted, and a row +/// that is new or edited is held to every rule. That is enough to keep the +/// escalation closed, because escalation means *introducing* a bad value +/// through this command, and an introduced row is never a carried-over one. +/// +/// The single exception is [`check_mount_name_stays_under_workspace`], which +/// runs on every row either way. A stored `..` is a live data-loss chain rather +/// than untidy data, its remedy is one edit in the Workspace section, and the +/// message names the mount rather than talking about folders in the abstract. +fn validate_project_paths_update( + stored: &[ProjectPath], + incoming: &[ProjectPath], +) -> Result<(), String> { + let is_blank = |p: &ProjectPath| p.host_path.is_empty() && p.mount_name.is_empty(); + + // Rows carried over, counted rather than set-tested: a *second* copy of an + // existing row is a new row, and has to be checked like one. + let mut carried: std::collections::HashMap<(&str, &str), usize> = + std::collections::HashMap::new(); + for p in stored.iter().filter(|p| !is_blank(p)) { + *carried + .entry((p.host_path.as_str(), p.mount_name.as_str())) + .or_insert(0) += 1; + } + + for p in incoming.iter().filter(|p| !is_blank(p)) { + check_mount_name_stays_under_workspace(&p.mount_name)?; + + match carried.get_mut(&(p.host_path.as_str(), p.mount_name.as_str())) { + Some(remaining) if *remaining > 0 => { + *remaining -= 1; + log::debug!( + "Admitting a stored folder row unchanged: '{}' at /workspace/{}", + p.host_path, + p.mount_name + ); + } + _ => validate_one_path(p)?, + } + } + + // Duplicates get the same treatment, one level up: a name may repeat as + // many times as it already did, and no more. Counting both sides keeps the + // answer independent of the order the rows arrive in. + let count_names = |rows: &[ProjectPath]| { + let mut counts: std::collections::HashMap = + std::collections::HashMap::new(); + for p in rows.iter().filter(|p| !is_blank(p)) { + *counts.entry(p.mount_name.clone()).or_insert(0) += 1; + } + counts + }; + let stored_names = count_names(stored); + for (name, count) in count_names(incoming) { + let allowed = stored_names.get(&name).copied().unwrap_or(0).max(1); + if count > allowed { + return Err(format!("Duplicate mount name '{}'.", name)); + } + } + + Ok(()) +} + +/// Refuse a filesystem root newly set as `ssh_key_path` or `ca_cert_path`. +/// +/// Both are bind-mounted into the container by `docker::create_container` — +/// `/tmp/.host-ssh` and `/tmp/.host-ca` — and neither had any check at all, so +/// `/` handed the whole host filesystem to the agent to read. Read-only, so +/// this is disclosure rather than the read-write hole a `/` project folder is, +/// but the fix is the same one line. +/// +/// Same grandfathering as the folder list, for the same reason: a value already +/// stored is already mounted on every start, and refusing an unrelated save +/// does not unmount it. Only a *change* is held to the rule. +fn validate_mounted_host_path( + label: &str, + stored: Option<&str>, + incoming: Option<&str>, +) -> Result<(), String> { + let Some(value) = incoming.map(str::trim).filter(|v| !v.is_empty()) else { + return Ok(()); + }; + if stored.map(str::trim) == Some(value) { + return Ok(()); + } + if is_filesystem_root(value) { + return Err(format!( + "'{}' is a filesystem root, so setting it as {} would mount the whole drive into the \ + container. Choose the folder itself.", + value, label + )); + } + Ok(()) +} + /// Whether a host path is the root of a filesystem, in any spelling the three /// desktop platforms produce: `/`, a Windows drive root, or a bare UNC/share /// prefix. Trailing separators are ignored, so `C:\\` and `C:/` are the same @@ -358,14 +507,6 @@ pub async fn update_project( let mut project: Project = serde_json::from_value(project) .map_err(|e| format!("Could not read the project being saved: {}", e))?; - // **This takes a whole `Project` over IPC and used to store it verbatim.** - // `add_project` validated its folder list and this did not, so every check - // there was one edit away from being bypassed — and the Config tab's mount - // name is a free-text field on an existing project, saved on blur, calling - // exactly this command. See [`validate_project_paths`] for what a mount - // name of `../tmp/claude-x` does to the user's files. - validate_project_paths(&project.paths)?; - // Fields this command does not get to write, whoever is calling it. // // `container_id` is the one that matters: it is the handle the whole file @@ -386,6 +527,30 @@ pub async fn update_project( .projects_store .get(&project.id) .ok_or_else(|| format!("Project {} not found", project.id))?; + + // **This takes a whole `Project` over IPC and used to store it verbatim.** + // `add_project` validated its folder list and this did not, so every check + // there was one edit away from being bypassed — and the Config tab's mount + // name is a free-text field on an existing project, saved on blur, calling + // exactly this command. See [`validate_project_paths`] for what a mount + // name of `../tmp/claude-x` does to the user's files. + // + // Validated against what is *stored*, not in isolation: a rule this command + // never enforced can be violated by data already on disk, and a project + // that cannot be saved at all is a Config tab that cannot be used at all. + // See [`validate_project_paths_update`]. + validate_project_paths_update(&stored.paths, &project.paths)?; + validate_mounted_host_path( + "the SSH key folder", + stored.ssh_key_path.as_deref(), + project.ssh_key_path.as_deref(), + )?; + validate_mounted_host_path( + "the CA certificate path", + stored.ca_cert_path.as_deref(), + project.ca_cert_path.as_deref(), + )?; + project.container_id = stored.container_id; project.status = stored.status; project.created_at = stored.created_at; @@ -1065,10 +1230,177 @@ mod tests { #[test] fn add_and_update_cannot_disagree_about_what_a_folder_list_may_contain() { // `update_project` used to validate nothing at all, so every rule in - // `add_project` was one save-on-blur away from being bypassed. Both go - // through the same function now; this fails if either grows its own - // copy. + // `add_project` was one save-on-blur away from being bypassed. It now + // validates against what is stored rather than in isolation, but a row + // it has never seen before is held to exactly the same rules — this + // fails if either side grows its own copy. let bad = [path("/home/u/project", "../tmp/claude-x")]; assert!(validate_project_paths(&bad).is_err()); + assert!(validate_project_paths_update(&[], &bad).is_err()); + let good = [path("/home/u/project", "project")]; + assert!(validate_project_paths(&good).is_ok()); + assert!(validate_project_paths_update(&[], &good).is_ok()); + } + + // ── Folder lists that are already in `projects.json` ────────────────── + // + // `update_project` validated nothing while `WorkspaceSection` saved + // `{paths}` on every blur, so a stored list can break rules that only + // `add_project` ever enforced. Holding a save to all of them turned every + // such project into one that cannot be saved *at all* — not its folders: + // `update_project` is the single command behind the whole Config tab, so a + // sandbox toggle, a permission-mode change and `useTerminal.ts`'s tab + // rename all came back with a message about folders. + + /// The shapes a real `projects.json` can be holding. None of them is an + /// escape from `/workspace`; all of them used to brick the editor. + fn legacy_rows() -> Vec> { + vec![ + // Half-filled: "+ Add folder", a host path typed, no name yet, and + // an unrelated blur saved the list. + vec![path("/home/u/a", "a"), path("/home/u/b", "")], + vec![path("/home/u/a", "a"), path("", "b")], + // A mount name the character check refuses but the daemon puts + // exactly where it says: /workspace/my project. + vec![path("/home/u/a", "my project")], + vec![path("/home/u/a", "web@2")], + // Two rows sharing a name. + vec![path("/home/u/a", "same"), path("/home/u/b", "same")], + // The whole drive, from before anything refused it. + vec![path("/", "everything")], + vec![path("C:\\", "everything")], + ] + } + + #[test] + fn a_project_stored_with_a_bad_row_can_still_be_saved() { + for rows in legacy_rows() { + // The full rule set is what made these unsavable… + assert!( + validate_project_paths(&rows).is_err(), + "fixture {:?} is not actually a rule violation", + rows + ); + // …and an unrelated Config save re-sends the list it was given. + assert!( + validate_project_paths_update(&rows, &rows).is_ok(), + "saving an unrelated setting on a project stored as {:?} is refused, so every \ + toggle in the Config tab fails with a message about folders", + rows + ); + } + } + + #[test] + fn the_same_bad_row_is_refused_when_it_is_new() { + let stored = [path("/home/u/project", "project")]; + for rows in legacy_rows() { + assert!( + validate_project_paths_update(&stored, &rows).is_err(), + "{:?} was introduced through update_project, which is the escalation the \ + validation exists to stop", + rows + ); + } + } + + /// The one rule no exemption reaches. `/workspace/../tmp/claude-x` + /// normalises to `/tmp/claude-x` — a path the pre-commit scrub owns and + /// empties as root — so a stored one is a live data-loss chain rather than + /// untidy data, and the Workspace section is one edit away. + #[test] + fn a_mount_that_leaves_workspace_is_refused_however_it_got_there() { + for escape in ["..", "../tmp/claude-x", "../../etc", "/tmp/claude-x", "a/../..", ".", "..\\x"] { + let rows = [path("/home/u/project", escape)]; + assert!( + validate_project_paths_update(&rows, &rows).is_err(), + "mount name '{}' was grandfathered, so the C1 chain stays open for anyone who \ + already has it stored", + escape + ); + assert!(validate_project_paths_update(&[], &rows).is_err()); + } + } + + #[test] + fn editing_a_grandfathered_row_holds_it_to_every_rule_again() { + let stored = [path("/", "everything")]; + // Renaming the mount but keeping the root host path is a new row. + assert!( + validate_project_paths_update(&stored, &[path("/", "all")]).is_err(), + "an edited row was admitted on the strength of the row it replaced" + ); + // Fixing the host path is what the message asks for, and it saves. + assert!( + validate_project_paths_update(&stored, &[path("/home/u/a", "everything")]).is_ok() + ); + // Dropping the row entirely is always fine. + assert!(validate_project_paths_update(&stored, &[]).is_ok()); + } + + #[test] + fn a_stored_duplicate_may_be_kept_but_not_multiplied() { + let stored = [path("/home/u/a", "same"), path("/home/u/b", "same")]; + assert!(validate_project_paths_update(&stored, &stored).is_ok()); + // Order must not change the answer. + let reordered = [stored[1].clone(), stored[0].clone()]; + assert!(validate_project_paths_update(&stored, &reordered).is_ok()); + // A third row taking the same name is new, and refused. + let more = [ + stored[0].clone(), + stored[1].clone(), + path("/home/u/c", "same"), + ]; + assert!(validate_project_paths_update(&stored, &more).is_err()); + // And a second copy of a name that was unique stays refused. + let unique = [path("/home/u/a", "a")]; + assert!(validate_project_paths_update( + &unique, + &[path("/home/u/a", "a"), path("/home/u/b", "a")] + ) + .is_err()); + } + + #[test] + fn the_blank_placeholder_row_is_still_not_an_error_on_either_path() { + let stored = [path("/home/u/a", "a")]; + let with_placeholder = [path("/home/u/a", "a"), path("", "")]; + assert!(validate_project_paths(&with_placeholder).is_ok()); + assert!(validate_project_paths_update(&stored, &with_placeholder).is_ok()); + } + + // ── The two host paths that had no check at all ─────────────────────── + + #[test] + fn a_filesystem_root_cannot_be_newly_set_as_an_ssh_or_ca_path() { + for root in ["/", "//", "\\", "C:\\", "c:/", "D:"] { + assert!( + validate_mounted_host_path("the SSH key folder", None, Some(root)).is_err(), + "'{}' was accepted as an SSH key path, which read-only bind-mounts the whole \ + host filesystem at /tmp/.host-ssh", + root + ); + assert!( + validate_mounted_host_path("the CA certificate path", Some("/etc/ssl"), Some(root)) + .is_err(), + "'{}' was accepted as a CA certificate path", + root + ); + } + // A real folder, a cleared value and an absent one are all fine. + assert!(validate_mounted_host_path("x", None, Some("/home/u/.ssh")).is_ok()); + assert!(validate_mounted_host_path("x", Some("/home/u/.ssh"), None).is_ok()); + assert!(validate_mounted_host_path("x", Some("/home/u/.ssh"), Some("")).is_ok()); + } + + #[test] + fn an_ssh_path_already_stored_does_not_brick_the_editor_either() { + // Nothing ever validated this field, so it can hold a root today — and + // it is mounted on every container start whether or not an unrelated + // Config save is allowed through. + assert!(validate_mounted_host_path("x", Some("/"), Some("/")).is_ok()); + assert!(validate_mounted_host_path("x", Some("/"), Some(" / ")).is_ok()); + // Changing it to a different root is a change, and refused. + assert!(validate_mounted_host_path("x", Some("/"), Some("C:\\")).is_err()); } } diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 2e415be..f945579 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -476,6 +476,7 @@ pub fn run() { commands::auth_token_commands::cancel_claude_token, commands::auth_token_commands::has_claude_token, commands::auth_token_commands::clear_claude_token, + commands::auth_token_commands::sweep_claude_token_snapshots, // Settings commands::settings_commands::get_settings, commands::settings_commands::update_settings, diff --git a/app/src/components/settings/SharedAuthSettings.test.tsx b/app/src/components/settings/SharedAuthSettings.test.tsx index e3e867d..d221894 100644 --- a/app/src/components/settings/SharedAuthSettings.test.tsx +++ b/app/src/components/settings/SharedAuthSettings.test.tsx @@ -6,14 +6,38 @@ import type { ClearTokenOutcome, Project } from "../../lib/types"; const hasClaudeToken = vi.fn(); const clearClaudeToken = vi.fn(); +const sweepClaudeTokenSnapshots = vi.fn(); vi.mock("../../lib/tauri-commands", () => ({ hasClaudeToken: () => hasClaudeToken(), clearClaudeToken: () => clearClaudeToken(), + sweepClaudeTokenSnapshots: () => sweepClaudeTokenSnapshots(), acquireClaudeToken: vi.fn(), submitClaudeTokenCode: vi.fn(), })); +// Stood in for so a sign-in can be *completed* in a test. The panel's reaction +// to `onAuthenticated` is the subject of the H1 sequence below, and driving the +// real acquisition dialog to get there would test the dialog instead. +vi.mock("./ClaudeAuthModal", () => ({ + default: ({ + onAuthenticated, + onClose, + }: { + onAuthenticated: () => void; + onClose: () => void; + }) => ( + + ), +})); + vi.mock("@tauri-apps/api/event", () => ({ listen: vi.fn(async () => vi.fn()) })); vi.mock("@tauri-apps/plugin-opener", () => ({ openUrl: vi.fn() })); @@ -59,11 +83,22 @@ const running = (over: Partial = {}): Project => ({ ...over, }); +/** A `ClearTokenOutcome` with nothing left behind, plus any overrides. */ +const outcome = (over: Partial = {}): ClearTokenOutcome => ({ + snapshots_scrubbed: [], + snapshots_failed: [], + snapshots_skipped: [], + snapshots_superseded: [], + docker_unavailable: null, + ...over, +}); + describe("SharedAuthSettings", () => { beforeEach(() => { vi.clearAllMocks(); projects = []; hasClaudeToken.mockResolvedValue(false); + sweepClaudeTokenSnapshots.mockResolvedValue(outcome()); useAppState.setState({ toasts: [] }); }); @@ -251,19 +286,17 @@ describe("SharedAuthSettings", () => { fireEvent.click(await screen.findByRole("button", { name: "Revoke token" })); const retry = await screen.findByTestId("shared-auth-retry"); - clearClaudeToken.mockResolvedValueOnce({ - snapshots_scrubbed: ["triple-c-snapshot-p1:latest"], - snapshots_failed: [], - snapshots_skipped: [], - snapshots_superseded: [], - docker_unavailable: null, - }); + sweepClaudeTokenSnapshots.mockResolvedValueOnce( + outcome({ snapshots_scrubbed: ["triple-c-snapshot-p1:latest"] }), + ); fireEvent.click(retry); await waitFor(() => expect(screen.queryByTestId("shared-auth-leftover")).not.toBeInTheDocument(), ); - expect(clearClaudeToken).toHaveBeenCalledTimes(2); + // One revoke, one sweep — the retry must not be a second revoke. + expect(clearClaudeToken).toHaveBeenCalledTimes(1); + expect(sweepClaudeTokenSnapshots).toHaveBeenCalledTimes(1); const toast = useAppState.getState().toasts.at(-1)!; expect(toast.kind).toBe("success"); expect(toast.message).toMatch(/cleared from 1 snapshot image/i); @@ -274,20 +307,14 @@ describe("SharedAuthSettings", () => { // anything is in the keychain today, so the sweep cannot be gated on it. projects = [running()]; hasClaudeToken.mockResolvedValue(false); - clearClaudeToken.mockResolvedValue({ - snapshots_scrubbed: [], - snapshots_failed: [], - snapshots_skipped: [], - snapshots_superseded: [], - docker_unavailable: null, - }); render(); const sweep = await screen.findByTestId("shared-auth-sweep"); expect(screen.queryByRole("button", { name: "Revoke" })).not.toBeInTheDocument(); fireEvent.click(sweep); - await waitFor(() => expect(clearClaudeToken).toHaveBeenCalled()); + await waitFor(() => expect(sweepClaudeTokenSnapshots).toHaveBeenCalled()); + expect(clearClaudeToken).not.toHaveBeenCalled(); const toast = useAppState.getState().toasts.at(-1)!; expect(toast.kind).toBe("success"); expect(toast.message).toBe("No snapshot image is holding the token."); @@ -300,4 +327,116 @@ describe("SharedAuthSettings", () => { expect(toast.kind).toBe("success"); expect(screen.queryByTestId("shared-auth-leftover")).not.toBeInTheDocument(); }); + + // ── The retry must not be a revoke in disguise ─────────────────────────── + // The sequence that shipped green: revoke, get a skipped image, re-authenticate + // from the button directly above the warning, then press the retry the warning + // is still offering. One command was behind both, so that press deleted the + // token acquired seconds earlier — no confirmation, and a toast that mentioned + // only images. The deliberate Revoke needs a modal; this needed nothing. + + it("does not leave a stale cleanup panel over a freshly acquired token", async () => { + projects = [running()]; + // Stored when the panel mounts, gone after the revoke, stored again once + // the sign-in finishes. + hasClaudeToken + .mockResolvedValueOnce(true) + .mockResolvedValueOnce(false) + .mockResolvedValue(true); + clearClaudeToken.mockResolvedValue( + outcome({ snapshots_skipped: ["triple-c-snapshot-p1:latest: busy"] }), + ); + render(); + + fireEvent.click(await screen.findByRole("button", { name: "Revoke" })); + fireEvent.click(await screen.findByRole("button", { name: "Revoke token" })); + await screen.findByTestId("shared-auth-leftover"); + + // Re-authenticate from the button above the warning. + fireEvent.click(await screen.findByRole("button", { name: "Authenticate" })); + fireEvent.click(await screen.findByRole("button", { name: "finish sign-in" })); + + await screen.findByRole("button", { name: "Revoke" }); + expect(screen.queryByTestId("shared-auth-leftover")).not.toBeInTheDocument(); + expect(screen.queryByTestId("shared-auth-retry")).not.toBeInTheDocument(); + + // The images can still be cleaned up — and doing so does not spend the new + // token. + fireEvent.click(screen.getByTestId("shared-auth-sweep")); + await waitFor(() => expect(sweepClaudeTokenSnapshots).toHaveBeenCalled()); + expect(clearClaudeToken).toHaveBeenCalledTimes(1); + }); + + it("sends the retry to the sweep-only command, never to the revoke", async () => { + projects = [running()]; + hasClaudeToken.mockResolvedValueOnce(true).mockResolvedValue(false); + clearClaudeToken.mockResolvedValue( + outcome({ snapshots_skipped: ["triple-c-snapshot-p1:latest: busy"] }), + ); + sweepClaudeTokenSnapshots.mockResolvedValue( + outcome({ snapshots_skipped: ["triple-c-snapshot-p1:latest: busy"] }), + ); + render(); + + fireEvent.click(await screen.findByRole("button", { name: "Revoke" })); + fireEvent.click(await screen.findByRole("button", { name: "Revoke token" })); + const retry = await screen.findByTestId("shared-auth-retry"); + + fireEvent.click(retry); + await waitFor(() => expect(sweepClaudeTokenSnapshots).toHaveBeenCalledTimes(1)); + fireEvent.click(await screen.findByTestId("shared-auth-retry")); + await waitFor(() => expect(sweepClaudeTokenSnapshots).toHaveBeenCalledTimes(2)); + + // However many times it is pressed, the keychain is touched exactly once — + // by the revoke the user confirmed. + expect(clearClaudeToken).toHaveBeenCalledTimes(1); + // …and a retry that still finds a busy project says so without ever + // claiming a token was removed. + const toast = useAppState.getState().toasts.at(-1)!; + expect(toast.message).not.toMatch(/keychain/i); + }); + + it("will not start a sign-in while a cleanup is still running", async () => { + // A sweep is a per-image inspect/create/commit/rmi over the Docker socket + // and runs for minutes. Acquiring a token inside that window races the + // rewrite that is meant to be removing one. + projects = [running()]; + hasClaudeToken.mockResolvedValue(false); + sweepClaudeTokenSnapshots.mockReturnValue(new Promise(() => {})); + render(); + + const authenticate = await screen.findByRole("button", { name: "Authenticate" }); + expect(authenticate).toBeEnabled(); + + fireEvent.click(await screen.findByTestId("shared-auth-sweep")); + await waitFor(() => + expect(screen.getByRole("button", { name: "Authenticate" })).toBeDisabled(), + ); + }); + + // ── A keychain that refuses the delete has to leave something to press ─── + + it("keeps both remedies on screen when the keychain refuses the delete", async () => { + projects = [running()]; + hasClaudeToken.mockResolvedValue(true); + clearClaudeToken.mockRejectedValue("the keychain is locked"); + render(); + + fireEvent.click(await screen.findByRole("button", { name: "Revoke" })); + fireEvent.click(await screen.findByRole("button", { name: "Revoke token" })); + + await waitFor(() => + expect(useAppState.getState().toasts.length).toBeGreaterThan(0), + ); + const toast = useAppState.getState().toasts[0]; + expect(toast.kind).toBe("error"); + expect(toast.detail).toMatch(/still stored/i); + + // The token is still there, so Revoke is still the retry for it — and the + // images, which the failed delete says nothing about, have their own sweep. + await screen.findByRole("button", { name: "Revoke" }); + const sweep = screen.getByTestId("shared-auth-sweep"); + fireEvent.click(sweep); + await waitFor(() => expect(sweepClaudeTokenSnapshots).toHaveBeenCalled()); + }); }); diff --git a/app/src/components/settings/SharedAuthSettings.tsx b/app/src/components/settings/SharedAuthSettings.tsx index 2551073..23a1d1c 100644 --- a/app/src/components/settings/SharedAuthSettings.tsx +++ b/app/src/components/settings/SharedAuthSettings.tsx @@ -4,7 +4,10 @@ import Modal from "../ui/Modal"; import StatusIndicator, { type StatusTone } from "../ui/StatusIndicator"; import { selectClass } from "../ui/Field"; import ClaudeAuthModal from "./ClaudeAuthModal"; -import { clearClaudeToken } from "../../lib/tauri-commands"; +import { + clearClaudeToken, + sweepClaudeTokenSnapshots, +} from "../../lib/tauri-commands"; import type { ClearTokenOutcome } from "../../lib/types"; import { useProjects } from "../../hooks/useProjects"; import { useAppState } from "../../store/appState"; @@ -96,18 +99,30 @@ export default function SharedAuthSettings() { const display = STATUS_DISPLAY[status]; /** - * Run `clear_claude_token`. It is deliberately the same command for both the - * first revoke and every retry: it sweeps the snapshot images first, deletes - * the keychain entry second, and treats a missing entry as success — so - * calling it again with nothing stored is a pure snapshot sweep, and the - * images themselves are the durable record of what is left to do. + * Run one of the two cleanups. **They are different commands**, and that is + * the point. + * + * `"revoke"` deletes the keychain entry and then rewrites the images. It is + * destructive, and it is only ever reached through the confirmation modal. + * + * `"sweep"` rewrites the images and never touches the keychain. Both the + * standalone check and the retry offered after an incomplete revocation use + * it, because neither is a request to delete a credential — the images are + * the durable record of what is left to do, so the work is re-derived from + * Docker rather than from any token that happens to be stored. + * + * While there was one command behind both, the retry button was an + * unconfirmed revoke: re-authenticate from the button above the panel, press + * "Retry snapshot cleanup", and the token acquired seconds earlier was gone, + * announced by a toast that mentioned only images. */ - const runSweep = async (mode: "revoke" | "sweep") => { + const runCleanup = async (mode: "revoke" | "sweep") => { setSweeping(true); try { - const outcome = (await clearClaudeToken()) as RevokeOutcome; + const outcome = (await (mode === "revoke" + ? clearClaudeToken() + : sweepClaudeTokenSnapshots())) as RevokeOutcome; setConfirmRevoke(false); - await refresh(); const failed = list(outcome.snapshots_failed); const skipped = list(outcome.snapshots_skipped); @@ -116,12 +131,14 @@ export default function SharedAuthSettings() { setLeftover(needsAnotherPass(outcome) ? outcome : null); - // The keychain entry is gone either way. What matters here is the copy of - // the token that `docker commit` baked into each project's snapshot - // image: that one outlives every container, and `docker image inspect` - // will keep printing it until the image is rewritten. If that could not - // be done, the revocation is incomplete and saying "removed" would be a - // lie. + // Whatever the mode, what is being reported here is the copy of the + // token that `docker commit` baked into each project's snapshot image: + // that one outlives every container, and `docker image inspect` will + // keep printing it until the image is rewritten. On a revoke the + // keychain entry is already gone by this point — the command deletes it + // before it touches an image — so if the rewrite could not be done the + // revocation is incomplete and saying "removed" would be a lie. On a + // sweep nothing was deleted at all, and the wording must not imply it. if (outcome.docker_unavailable) { pushToast({ kind: "error", @@ -137,7 +154,10 @@ export default function SharedAuthSettings() { } else if (failed.length > 0) { pushToast({ kind: "error", - message: "Token removed from the keychain, but it is still in some images.", + message: + mode === "revoke" + ? "Token removed from the keychain, but it is still in some images." + : "A token is still readable in some snapshot images.", detail: `${failed.length} snapshot image(s) could not be rewritten and ` + "still contain the token, readable via `docker image inspect`. Reset those " + @@ -187,12 +207,24 @@ export default function SharedAuthSettings() { mode === "revoke" ? "Could not remove the shared Claude token." : "Could not clear the token from snapshot images.", - detail: authErrorMessage( - e, - "The OS keychain rejected the delete. The token may still be stored.", - ), + detail: + mode === "revoke" + ? // The keychain delete runs first and nothing else runs until it + // succeeds, so the consequence is knowable and worth stating + // rather than leaving the user with a raw keyring error: the + // token is still stored, and no image was touched. Revoke stays + // on screen because `status` is still "stored", and the snapshot + // sweep beside it does not need this delete to have worked. + `${authErrorMessage(e, "The OS keychain rejected the delete.")} ` + + "The token is still stored and no snapshot image was changed — try again, " + + "or use “Check snapshot images” to clean the images on their own." + : authErrorMessage(e, "The snapshot images could not be checked."), }); } finally { + // In `finally` rather than in the success path: a failed revoke leaves + // the token stored, and the panel has to say so rather than keeping + // whatever it believed before the attempt. + await refresh(); setSweeping(false); } }; @@ -255,7 +287,11 @@ export default function SharedAuthSettings() { )} - {status === "absent" && ( - // Not gated on a stored token, on purpose. A revoke that could not - // finish leaves the token in a snapshot image while the keychain - // entry — and therefore the Revoke button — is already gone, and a - // snapshot committed by an older build carries it whether or not - // anything is stored today. With nothing in the keychain the same - // command is a pure image sweep. + {status !== "checking" && ( + // Offered in every state, not just "absent". Three reasons, and the + // command behind it deletes nothing, so none of them costs anything: + // a snapshot committed by an older build carries a token whether or + // not one is stored today; a revoke that could not finish leaves one + // in an image while the keychain entry — and the Revoke button — is + // already gone; and when the keychain refuses the delete, `status` + // stays "stored", which used to leave the images with no affordance + // at all. @@ -327,7 +365,9 @@ export default function SharedAuthSettings() { variant="secondary" disabled={sweeping} data-testid="shared-auth-retry" - onClick={() => void runSweep("sweep")} + // `runCleanup("sweep")` is `sweep_claude_token_snapshots`, which + // has no keychain delete on the wire at all — see `runCleanup`. + onClick={() => void runCleanup("sweep")} > {sweeping ? "Retrying…" : "Retry snapshot cleanup"} @@ -360,6 +400,13 @@ export default function SharedAuthSettings() { projectName={host.name} onClose={() => setAuthOpen(false)} onAuthenticated={() => { + // The panel is about a *previous* credential and the button that + // finishes it is a sweep, so leaving it up after a fresh sign-in + // is at best confusing and was at worst fatal: while the retry ran + // `clear_claude_token`, the obvious next click deleted the token + // just acquired. "Check snapshot images" stays available above, so + // nothing is lost by clearing this. + setLeftover(null); void refresh(); }} /> @@ -384,7 +431,7 @@ export default function SharedAuthSettings() { size="md" variant="danger" disabled={sweeping} - onClick={() => void runSweep("revoke")} + onClick={() => void runCleanup("revoke")} > {sweeping ? "Revoking…" : "Revoke token"} @@ -399,11 +446,13 @@ export default function SharedAuthSettings() { restarted.

- Each project’s snapshot image is rewritten first, because{" "} - docker commit copies the token into it - and an image outlives every container built from it. A project that is - busy right now is skipped rather than rewritten unsafely — you will - be told which, and the cleanup can be run again from here afterwards. + The keychain entry goes first, then each project’s snapshot image is + rewritten — docker commit copies + the token into it, and an image outlives every container built from it. + That second half takes a while, and a project that is busy right now is + skipped rather than rewritten unsafely: you will be told which, and + “Check snapshot images” runs the cleanup again afterwards + without touching the keychain.

)} diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index 5212ce5..1c1b28f 100644 --- a/app/src/lib/tauri-commands.ts +++ b/app/src/lib/tauri-commands.ts @@ -317,10 +317,16 @@ export const submitClaudeTokenCode = (code: string) => /** Abort an in-flight acquisition and release the single-flight guard. No-op if nothing is running. */ export const cancelClaudeToken = () => invoke("cancel_claude_token"); export const hasClaudeToken = () => invoke("has_claude_token"); -/** Revoke the shared token. Also rewrites any snapshot image that still has it - * baked into its env — see `ClearTokenOutcome` for what may be left behind. */ +/** Revoke the shared token: delete the keychain entry **first**, then rewrite + * any snapshot image that still has it baked into its env — see + * `ClearTokenOutcome` for what may be left behind. Destructive; confirm it. */ export const clearClaudeToken = () => invoke("clear_claude_token"); +/** Rewrite snapshot images that still carry a credential, **without touching + * the keychain**. This is the retry behind an incomplete revocation, and the + * standalone "check my images" sweep; it never deletes a token. */ +export const sweepClaudeTokenSnapshots = () => + invoke("sweep_claude_token_snapshots"); // Container base-image migration — move a project onto the current base image // without deleting its volumes. Reset is the destructive alternative: it wipes