diff --git a/app/src-tauri/capabilities/default.json b/app/src-tauri/capabilities/default.json index a5317e6..840d5ee 100644 --- a/app/src-tauri/capabilities/default.json +++ b/app/src-tauri/capabilities/default.json @@ -1,25 +1,17 @@ { "identifier": "default", - "description": "Default capabilities for Triple-C. Every entry here is an IPC command a compromised webview can call directly, so the set is kept to what the frontend actually uses. Two notes on what is deliberately absent and what is deliberately accepted: (1) the `store:*` grants were removed — nothing in `app/src` uses `@tauri-apps/plugin-store`, and the plugin's `resolve_store_path` is a `PathBuf::push` against AppData, which `push` discards outright when handed an absolute path, so the grant was an arbitrary host-file read/write primitive (`plugin:store|load` + `set` + `save` on `~/.claude/settings.json` is host code execution). (2) `drag:allow-start-drag` stays, and cannot be scoped — `tauri-plugin-drag` takes the item paths from the caller and has no scope mechanism, so a compromised webview could call `startDrag({ item: ['~/.ssh/id_rsa'] })` against any host path the user can read. It is not a silent exfiltration primitive: the drag only delivers anything if the user completes a real drop onto a real target, and the OS shows the drag under the cursor while it is in flight. Removing it would remove drag-out from the Files pane (`stage_container_file_for_drag`), whose fallback is the explicit \"Save to host…\" action. Accepted residual risk, recorded here rather than fixed.", + "description": "Default capabilities for Triple-C. Every entry here is an IPC command a compromised webview can call directly, so the set is an enumeration of what `app/src` actually invokes — verified against tauri 2.11.0's `PLUGINS` table in `build.rs`, not assumed from a plugin's `default` set. `core:default` in particular is NOT used: it is an alias for `core:{path,event,window,webview,app,image,resources,menu,tray}:default`, and `core:image:default` carries `allow-from-path`, whose handler (`tauri-2.11.0/src/image/plugin.rs:41` → `src/image/mod.rs:96`) is a bare `std::fs::read(path)` with no scope mechanism of any kind. Nothing imports `@tauri-apps/api/image`, so the whole plugin is dropped rather than scoped — there is nothing to scope it with. `core:menu` and `core:tray` are dropped for the same reason (no menu, no tray icon); `core:window` and `core:path` because nothing imports them; `core:resources:allow-close` because no frontend value is a `Resource` (`startDrag`'s `Channel` is not one); and `core:event`'s `allow-emit`/`allow-emit-to` because the frontend only ever *listens* — every emit in this app originates in Rust. Three notes on what is deliberately kept or accepted: (1) `core:webview:allow-internal-toggle-devtools` is not called by `app/src` at all — it is called by Tauri's own injected `toggle-devtools.js`, which binds Ctrl/Cmd+Shift+I. Both that script and the command behind it are `#[cfg(any(debug_assertions, feature = \"devtools\"))]`, so this grant is a `tauri dev` convenience that does not exist in a release bundle. (2) `opener:allow-open-url` cannot be narrowed by host. `TerminalView`'s `WebLinksAddon` opens links Claude printed inside the container, which are arbitrary by construction, so a host allowlist here would delete the feature rather than bound it. What *is* bounded: `opener:default` is not used, so `open_path` and `reveal_item_in_dir` are absent; the scope's two entries restrict the scheme to http/https (`file:`, `mailto:`, `tel:`, `smb:` are all refused by `Scope::is_url_allowed`); and because each entry leaves `app` at its serde default of `Application::Default`, which matches only `with == None`, `openUrl(url, \"/bin/sh\")` is refused — the `with` argument is not a usable exec primitive. The call sites re-validate through `sanitizeRelayUrl` (scheme allowlist, no embedded credentials, length cap) before anything reaches the opener. Accepted residual risk: a compromised webview can make the OS open an attacker-chosen http(s) URL, which is an outbound channel. Recorded here rather than fixed. (3) `drag:allow-start-drag` stays, and cannot be scoped — `tauri-plugin-drag` takes the item paths from the caller and has no scope mechanism, so a compromised webview could call `startDrag({ item: ['~/.ssh/id_rsa'] })` against any host path the user can read. It is not a silent exfiltration primitive: the drag only delivers anything if the user completes a real drop onto a real target, and the OS shows the drag under the cursor while it is in flight. Removing it would remove drag-out from the Files pane (`stage_container_file_for_drag`), whose fallback is the explicit \"Save to host…\" action. Accepted residual risk. Historical note kept because it is easy to re-introduce: the `store:*` grants were removed — nothing in `app/src` uses `@tauri-apps/plugin-store`, and the plugin's `resolve_store_path` is a `PathBuf::push` against AppData, which `push` discards outright when handed an absolute path, so the grant was an arbitrary host-file read/write primitive (`plugin:store|load` + `set` + `save` on `~/.claude/settings.json` is host code execution). On the CSP side: `app.security.csp` in `tauri.conf.json` covers the shipped bundle, and there is deliberately no `devCsp`. `npm run tauri dev` loads the main document straight from Vite at `build.devUrl` (`http://localhost:1420`), and Tauri only attaches a CSP to documents it serves itself — `protocol/tauri.rs:217` sets the header on `tauri://` assets, and the dev server is proxied through that protocol only when `PROXY_DEV_SERVER`, which is `cfg!(all(dev, mobile))` and therefore false for every desktop build. A `devCsp` here would be inert config that reads as protection, which is worse than its absence. If a CSP in dev is wanted, the only place that can set one is the Vite dev server's own `server.headers` in `app/vite.config.ts`; it is not set today, and dev is not the shipped configuration.", "windows": ["main"], "permissions": [ - "core:default", - "core:event:default", - "core:event:allow-emit", "core:event:allow-listen", "core:event:allow-unlisten", - "core:event:allow-emit-to", - "dialog:default", + "core:webview:allow-internal-toggle-devtools", "dialog:allow-open", "dialog:allow-save", - "dialog:allow-message", - "dialog:allow-ask", - "dialog:allow-confirm", { "identifier": "opener:allow-open-url", "allow": [{ "url": "http://*" }, { "url": "https://*" }] }, - "drag:default", "drag:allow-start-drag" ] } diff --git a/app/src-tauri/gen/schemas/capabilities.json b/app/src-tauri/gen/schemas/capabilities.json index f058044..bae6a07 100644 --- a/app/src-tauri/gen/schemas/capabilities.json +++ b/app/src-tauri/gen/schemas/capabilities.json @@ -1 +1 @@ -{"default":{"identifier":"default","description":"Default capabilities for Triple-C. Every entry here is an IPC command a compromised webview can call directly, so the set is kept to what the frontend actually uses. Two notes on what is deliberately absent and what is deliberately accepted: (1) the `store:*` grants were removed — nothing in `app/src` uses `@tauri-apps/plugin-store`, and the plugin's `resolve_store_path` is a `PathBuf::push` against AppData, which `push` discards outright when handed an absolute path, so the grant was an arbitrary host-file read/write primitive (`plugin:store|load` + `set` + `save` on `~/.claude/settings.json` is host code execution). (2) `drag:allow-start-drag` stays, and cannot be scoped — `tauri-plugin-drag` takes the item paths from the caller and has no scope mechanism, so a compromised webview could call `startDrag({ item: ['~/.ssh/id_rsa'] })` against any host path the user can read. It is not a silent exfiltration primitive: the drag only delivers anything if the user completes a real drop onto a real target, and the OS shows the drag under the cursor while it is in flight. Removing it would remove drag-out from the Files pane (`stage_container_file_for_drag`), whose fallback is the explicit \"Save to host…\" action. Accepted residual risk, recorded here rather than fixed.","local":true,"windows":["main"],"permissions":["core:default","core:event:default","core:event:allow-emit","core:event:allow-listen","core:event:allow-unlisten","core:event:allow-emit-to","dialog:default","dialog:allow-open","dialog:allow-save","dialog:allow-message","dialog:allow-ask","dialog:allow-confirm",{"identifier":"opener:allow-open-url","allow":[{"url":"http://*"},{"url":"https://*"}]},"drag:default","drag:allow-start-drag"]}} \ No newline at end of file +{"default":{"identifier":"default","description":"Default capabilities for Triple-C. Every entry here is an IPC command a compromised webview can call directly, so the set is an enumeration of what `app/src` actually invokes — verified against tauri 2.11.0's `PLUGINS` table in `build.rs`, not assumed from a plugin's `default` set. `core:default` in particular is NOT used: it is an alias for `core:{path,event,window,webview,app,image,resources,menu,tray}:default`, and `core:image:default` carries `allow-from-path`, whose handler (`tauri-2.11.0/src/image/plugin.rs:41` → `src/image/mod.rs:96`) is a bare `std::fs::read(path)` with no scope mechanism of any kind. Nothing imports `@tauri-apps/api/image`, so the whole plugin is dropped rather than scoped — there is nothing to scope it with. `core:menu` and `core:tray` are dropped for the same reason (no menu, no tray icon); `core:window` and `core:path` because nothing imports them; `core:resources:allow-close` because no frontend value is a `Resource` (`startDrag`'s `Channel` is not one); and `core:event`'s `allow-emit`/`allow-emit-to` because the frontend only ever *listens* — every emit in this app originates in Rust. Three notes on what is deliberately kept or accepted: (1) `core:webview:allow-internal-toggle-devtools` is not called by `app/src` at all — it is called by Tauri's own injected `toggle-devtools.js`, which binds Ctrl/Cmd+Shift+I. Both that script and the command behind it are `#[cfg(any(debug_assertions, feature = \"devtools\"))]`, so this grant is a `tauri dev` convenience that does not exist in a release bundle. (2) `opener:allow-open-url` cannot be narrowed by host. `TerminalView`'s `WebLinksAddon` opens links Claude printed inside the container, which are arbitrary by construction, so a host allowlist here would delete the feature rather than bound it. What *is* bounded: `opener:default` is not used, so `open_path` and `reveal_item_in_dir` are absent; the scope's two entries restrict the scheme to http/https (`file:`, `mailto:`, `tel:`, `smb:` are all refused by `Scope::is_url_allowed`); and because each entry leaves `app` at its serde default of `Application::Default`, which matches only `with == None`, `openUrl(url, \"/bin/sh\")` is refused — the `with` argument is not a usable exec primitive. The call sites re-validate through `sanitizeRelayUrl` (scheme allowlist, no embedded credentials, length cap) before anything reaches the opener. Accepted residual risk: a compromised webview can make the OS open an attacker-chosen http(s) URL, which is an outbound channel. Recorded here rather than fixed. (3) `drag:allow-start-drag` stays, and cannot be scoped — `tauri-plugin-drag` takes the item paths from the caller and has no scope mechanism, so a compromised webview could call `startDrag({ item: ['~/.ssh/id_rsa'] })` against any host path the user can read. It is not a silent exfiltration primitive: the drag only delivers anything if the user completes a real drop onto a real target, and the OS shows the drag under the cursor while it is in flight. Removing it would remove drag-out from the Files pane (`stage_container_file_for_drag`), whose fallback is the explicit \"Save to host…\" action. Accepted residual risk. Historical note kept because it is easy to re-introduce: the `store:*` grants were removed — nothing in `app/src` uses `@tauri-apps/plugin-store`, and the plugin's `resolve_store_path` is a `PathBuf::push` against AppData, which `push` discards outright when handed an absolute path, so the grant was an arbitrary host-file read/write primitive (`plugin:store|load` + `set` + `save` on `~/.claude/settings.json` is host code execution). On the CSP side: `app.security.csp` in `tauri.conf.json` covers the shipped bundle, and there is deliberately no `devCsp`. `npm run tauri dev` loads the main document straight from Vite at `build.devUrl` (`http://localhost:1420`), and Tauri only attaches a CSP to documents it serves itself — `protocol/tauri.rs:217` sets the header on `tauri://` assets, and the dev server is proxied through that protocol only when `PROXY_DEV_SERVER`, which is `cfg!(all(dev, mobile))` and therefore false for every desktop build. A `devCsp` here would be inert config that reads as protection, which is worse than its absence. If a CSP in dev is wanted, the only place that can set one is the Vite dev server's own `server.headers` in `app/vite.config.ts`; it is not set today, and dev is not the shipped configuration.","local":true,"windows":["main"],"permissions":["core:event:allow-listen","core:event:allow-unlisten","core:webview:allow-internal-toggle-devtools","dialog:allow-open","dialog:allow-save",{"identifier":"opener:allow-open-url","allow":[{"url":"http://*"},{"url":"https://*"}]},"drag:allow-start-drag"]}} \ No newline at end of file diff --git a/app/src-tauri/src/commands/auth_token_commands.rs b/app/src-tauri/src/commands/auth_token_commands.rs index 3ecc730..9d6df9a 100644 --- a/app/src-tauri/src/commands/auth_token_commands.rs +++ b/app/src-tauri/src/commands/auth_token_commands.rs @@ -1188,16 +1188,51 @@ pub async fn has_claude_token() -> Result { Ok(secure::has_claude_oauth_token()) } -/// What [`clear_claude_token`] managed to reach. The keychain entry is always -/// gone by the time this is returned — the rest is about copies of the token -/// that live outside it. -#[derive(Debug, Default, serde::Serialize)] +/// The tail of every refusal [`crate::project_lock::try_acquire`] produces. +/// +/// [`crate::docker::container::scrub_secrets_from_snapshots`] folds two very +/// different things into one `failed` list: an image that genuinely could not +/// be rewritten, and one that was never *attempted* because another operation +/// held the project. Only the second is retryable, and only the second should +/// be described to the user as "come back in a minute" rather than "reset this +/// project". Splitting them needs a discriminator, and the refusal string is +/// the only one that crosses the module boundary — `try_acquire` returns +/// `Result`, and `container.rs` pushes that `String` +/// through unchanged. +/// +/// Matching on prose is normally a mistake, so this is pinned by +/// [`tests::a_real_lock_refusal_is_recognised_as_retryable`], which builds a +/// refusal by actually taking a guard rather than by copying the wording. If +/// `project_lock` ever rephrases, that test fails instead of this silently +/// misclassifying a credential that was left in place. +const PROJECT_BUSY_MARKER: &str = "Wait for it to finish before "; + +/// Whether a scrub failure means "somebody else has this project right now", +/// which is transient, rather than "this image cannot be rewritten", which is +/// not. Nothing bollard returns contains [`PROJECT_BUSY_MARKER`]. +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. +/// +/// 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 +/// just revoked a credential, and only the last one is fixed by waiting. +#[derive(Debug, Default, PartialEq, Eq, serde::Serialize)] pub struct ClearTokenOutcome { /// Snapshot images that were holding the token and have been rewritten. pub snapshots_scrubbed: Vec, /// Images still holding it, with the reason each could not be rewritten. /// Non-empty means the revocation is **incomplete** and the UI must say so. pub snapshots_failed: Vec, + /// Images still holding it that were **not attempted**, because another + /// operation held the project (a start, a compaction, a migration). Also an + /// incomplete revocation — but a retryable one, and the UI must not offer + /// "Reset the project" as the remedy for it. + pub snapshots_skipped: Vec, /// Rewritten, but the pre-rewrite image object could not be deleted because /// a container is still running off it. Worth mentioning, not worth /// alarming about — see `SnapshotScrubReport::superseded_retained`. @@ -1207,7 +1242,43 @@ pub struct ClearTokenOutcome { pub docker_unavailable: Option, } -/// Forget the shared Claude token. +impl ClearTokenOutcome { + /// Whether a copy of the credential is known — or suspected — to still be + /// reachable, so the caller should offer to run the sweep again. + /// + /// `snapshots_superseded` is deliberately not counted: that image is + /// untagged, nothing new is built from it, and it goes away on the next + /// restart. Re-running would report it forever and train the user to + /// ignore the warning. + pub fn needs_another_pass(&self) -> bool { + !self.snapshots_failed.is_empty() + || !self.snapshots_skipped.is_empty() + || self.docker_unavailable.is_some() + } +} + +/// Fold a scrub report into the IPC shape, splitting the busy projects out of +/// the failures. Separate from the command so it can be tested without Docker. +fn summarise_scrub(report: crate::docker::container::SnapshotScrubReport) -> ClearTokenOutcome { + let mut outcome = ClearTokenOutcome { + snapshots_scrubbed: report.scrubbed, + snapshots_superseded: report.superseded_retained, + docker_unavailable: report.unavailable, + ..Default::default() + }; + for (image, reason) in report.failed { + let line = format!("{}: {}", image, reason); + if is_project_busy_refusal(&reason) { + outcome.snapshots_skipped.push(line); + } else { + outcome.snapshots_failed.push(line); + } + } + outcome +} + +/// Forget the shared Claude token, and remove the copies of it that outlive the +/// keychain entry. /// /// Deleting the keychain entry is the easy half. The token also exists in two /// other places, and a "Revoke" button that leaves either of them behind is @@ -1223,34 +1294,83 @@ pub struct ClearTokenOutcome { /// as long as the image exists. New commits no longer bake it in (see /// [`crate::docker::container::commit_container_snapshot`]), but images /// committed by earlier builds have to be rewritten, which is what -/// [`scrub_secrets_from_snapshots`] does here. +/// [`crate::docker::container::scrub_secrets_from_snapshots`] does here. +/// +/// ## Why the snapshots go 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. +/// +/// 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. +/// +/// ## This command is the retry +/// +/// 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. #[tauri::command] pub async fn clear_claude_token() -> Result { - secure::delete_claude_oauth_token()?; + let report = crate::docker::container::scrub_secrets_from_snapshots().await; + let swept_clean = !report.left_something_behind(); + let outcome = summarise_scrub(report); + + // 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"); - let report = crate::docker::container::scrub_secrets_from_snapshots().await; - if report.left_something_behind() { + 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 {} snapshot image(s) may still contain it", - report.failed.len() + "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(ClearTokenOutcome { - snapshots_scrubbed: report.scrubbed, - snapshots_failed: report - .failed - .into_iter() - .map(|(image, reason)| format!("{}: {}", image, reason)) - .collect(), - snapshots_superseded: report.superseded_retained, - docker_unavailable: report.unavailable, - }) + Ok(outcome) } #[cfg(test)] @@ -1761,5 +1881,135 @@ mod tests { seen.push_str(&s.push(format!("\nYour token: {}\n", tok).as_bytes())); assert_eq!(parse_setup_token(&seen), Some(tok)); } -} + // ── Revocation: what the sweep leaves behind, and how it is described ── + + use crate::docker::container::SnapshotScrubReport; + use crate::project_lock::{try_acquire, ProjectOp}; + + /// The classifier is a substring match on a message another module owns, + /// which is only safe if something notices when that module rephrases. So + /// build the refusal the way production does — by actually losing the + /// race — rather than by pasting the wording in here. + #[test] + fn a_real_lock_refusal_is_recognised_as_retryable() { + let project = "auth-token-test-busy-project"; + let _held = try_acquire(project, ProjectOp::Compaction).expect("first claim"); + let refusal = try_acquire(project, ProjectOp::SecretScrub) + .expect_err("a second claim on the same project must be refused"); + + assert!( + is_project_busy_refusal(&refusal), + "project_lock's refusal is no longer recognised as retryable: {:?}", + refusal + ); + } + + #[test] + fn a_docker_failure_is_not_mistaken_for_a_busy_project() { + for reason in [ + "could not inspect: error trying to connect: No such file or directory", + "could not create a scratch container: conflict: name already in use", + "an untagged snapshot image holds a credential and cannot be rewritten", + ] { + assert!( + !is_project_busy_refusal(reason), + "{:?} was misclassified as a transient lock refusal", + reason + ); + } + } + + #[test] + fn summarise_scrub_separates_a_busy_project_from_a_broken_image() { + let project = "auth-token-test-summarise-busy"; + let _held = try_acquire(project, ProjectOp::Recreate).expect("first claim"); + let refusal = try_acquire(project, ProjectOp::SecretScrub).expect_err("refused"); + + let outcome = summarise_scrub(SnapshotScrubReport { + scrubbed: vec!["triple-c-snapshot-a:latest".into()], + failed: vec![ + ("triple-c-snapshot-b:latest".into(), refusal), + ( + "triple-c-snapshot-c:latest".into(), + "could not create a scratch container: no such image".into(), + ), + ], + superseded_retained: vec!["triple-c-snapshot-a:latest".into()], + unavailable: None, + }); + + assert_eq!(outcome.snapshots_scrubbed, vec!["triple-c-snapshot-a:latest"]); + assert_eq!(outcome.snapshots_skipped.len(), 1, "{:?}", outcome); + assert!(outcome.snapshots_skipped[0].starts_with("triple-c-snapshot-b:latest: ")); + assert_eq!(outcome.snapshots_failed.len(), 1, "{:?}", outcome); + assert!(outcome.snapshots_failed[0].starts_with("triple-c-snapshot-c:latest: ")); + // The whole point: a skipped image is never folded into the scrubbed + // list, which is what "success" is rendered from. + assert!(!outcome.snapshots_scrubbed.iter().any(|s| s.contains("snapshot-b"))); + } + + #[test] + fn a_clean_sweep_needs_no_second_pass() { + let outcome = summarise_scrub(SnapshotScrubReport { + scrubbed: vec!["triple-c-snapshot-a:latest".into()], + ..Default::default() + }); + assert!(!outcome.needs_another_pass()); + } + + #[test] + fn a_retained_superseded_image_alone_does_not_ask_for_a_second_pass() { + // The tag is clean; what is left is untagged and dies with the running + // container. Asking the user to sweep again would never stop. + let outcome = summarise_scrub(SnapshotScrubReport { + scrubbed: vec!["triple-c-snapshot-a:latest".into()], + superseded_retained: vec!["triple-c-snapshot-a:latest".into()], + ..Default::default() + }); + assert!(!outcome.needs_another_pass()); + } + + #[test] + fn anything_still_holding_the_credential_asks_for_a_second_pass() { + let skipped = summarise_scrub(SnapshotScrubReport { + failed: vec![( + "triple-c-snapshot-b:latest".into(), + format!("This project is being reset. {}resetting it.", PROJECT_BUSY_MARKER), + )], + ..Default::default() + }); + assert!(skipped.needs_another_pass()); + assert_eq!(skipped.snapshots_skipped.len(), 1); + + let failed = summarise_scrub(SnapshotScrubReport { + failed: vec![("triple-c-snapshot-c:latest".into(), "could not inspect: boom".into())], + ..Default::default() + }); + assert!(failed.needs_another_pass()); + + let blind = summarise_scrub(SnapshotScrubReport { + unavailable: Some("Docker is not running".into()), + ..Default::default() + }); + assert!(blind.needs_another_pass()); + assert!(blind.snapshots_scrubbed.is_empty()); + } + + /// The IPC contract the frontend reads. A field renamed on this side and + /// not on that one is a silent "nothing was skipped". + #[test] + fn the_outcome_serialises_under_the_names_the_frontend_reads() { + let json = serde_json::to_value(ClearTokenOutcome::default()).expect("serialise"); + let object = json.as_object().expect("an object"); + for key in [ + "snapshots_scrubbed", + "snapshots_failed", + "snapshots_skipped", + "snapshots_superseded", + "docker_unavailable", + ] { + assert!(object.contains_key(key), "missing {} in {:?}", key, object); + } + } +} diff --git a/app/src-tauri/src/storage/secure.rs b/app/src-tauri/src/storage/secure.rs index 6dc351d..a1c7e86 100644 --- a/app/src-tauri/src/storage/secure.rs +++ b/app/src-tauri/src/storage/secure.rs @@ -26,47 +26,122 @@ const CLAUDE_TOKEN_VERSION_SERVICE: &str = "triple-c-claude-oauth-token-version" /// Fixed account name used for every triple-c keychain entry. const KEYCHAIN_ACCOUNT: &str = "secret"; +/// Every per-project secret this app stores, and therefore every one it has to +/// be able to delete. +/// +/// This list is the **only** definition. It used to exist twice — once +/// implicitly, as whatever `store_secrets_for_project` happened to write, and +/// once explicitly, as a literal array inside `delete_project_secrets` — and +/// the two drifted: `openai-compatible-api-key` was added to the writer and +/// never to the deleter, so removing a project left a live provider API key in +/// the user's login keychain with nothing left in the app that referenced it, +/// or would ever offer to clean it up. +/// +/// Drift is now a compile-time-shaped error rather than a review-time one: +/// [`project_secret_entry`] refuses a key that is not in this list, so a new +/// secret cannot be stored until it has been added here, and adding it here is +/// what makes [`delete_project_secrets`] cover it. +pub const PROJECT_SECRET_KEYS: &[&str] = &[ + "git-token", + "aws-access-key-id", + "aws-secret-access-key", + "aws-session-token", + "aws-bearer-token", + "openai-compatible-api-key", +]; + +/// The keychain entry for one per-project secret, rejecting any key name not in +/// [`PROJECT_SECRET_KEYS`]. See that constant for why the rejection matters. +fn project_secret_entry(project_id: &str, key_name: &str) -> Result { + if !PROJECT_SECRET_KEYS.contains(&key_name) { + return Err(format!( + "Unknown project secret '{}'. Add it to PROJECT_SECRET_KEYS so project deletion \ + clears it too.", + key_name + )); + } + let service = format!("triple-c-project-{}-{}", project_id, key_name); + keyring::Entry::new(&service, KEYCHAIN_ACCOUNT).map_err(|e| format!("Keyring error: {}", e)) +} + /// Store a per-project secret in the OS keychain. pub fn store_project_secret(project_id: &str, key_name: &str, value: &str) -> Result<(), String> { - let service = format!("triple-c-project-{}-{}", project_id, key_name); - let entry = keyring::Entry::new(&service, "secret") - .map_err(|e| format!("Keyring error: {}", e))?; - entry + project_secret_entry(project_id, key_name)? .set_password(value) .map_err(|e| format!("Failed to store project secret '{}': {}", key_name, e)) } /// Retrieve a per-project secret from the OS keychain. pub fn get_project_secret(project_id: &str, key_name: &str) -> Result, String> { - let service = format!("triple-c-project-{}-{}", project_id, key_name); - let entry = keyring::Entry::new(&service, "secret") - .map_err(|e| format!("Keyring error: {}", e))?; - match entry.get_password() { + match project_secret_entry(project_id, key_name)?.get_password() { Ok(value) => Ok(Some(value)), Err(keyring::Error::NoEntry) => Ok(None), Err(e) => Err(format!("Failed to retrieve project secret '{}': {}", key_name, e)), } } -/// Delete all known secrets for a project from the OS keychain. +/// Delete one per-project secret, treating "wasn't there" as success. +pub fn delete_project_secret(project_id: &str, key_name: &str) -> Result<(), String> { + match project_secret_entry(project_id, key_name)?.delete_credential() { + Ok(()) | Err(keyring::Error::NoEntry) => Ok(()), + Err(e) => Err(format!("Failed to delete project secret '{}': {}", key_name, e)), + } +} + +/// Write a per-project secret, or **clear** it when there is nothing to write. +/// +/// This is the function every save path should call, and the reason it exists +/// is that the obvious `if let Some(v) = … { store(v) }` is wrong. The editors +/// in `components/projects/home/config/` send a blanked field as `null` +/// (`AccessSection.tsx`: `save({ git_token: gitToken || null })`), so a `None` +/// is a user asking for the secret to be *removed* — and skipping it left the +/// old value in the keychain, where `load_secrets_for_project` read it straight +/// back out and put it back on the project. Clearing a credential through the +/// UI was therefore impossible: the field looked empty and the container kept +/// getting the old token. +/// +/// `Some("")` and `Some(" ")` are treated the same as `None` — a field the +/// user emptied, whichever shape it arrives in — because a stored empty secret +/// is not a secret, and `container_config` would inject it as an env var that +/// overrides the unset case with a blank. +// TODO(handoff): `commands/project_commands.rs::store_secrets_for_project` is +// the one caller this is for, and it still uses the `if let Some(v) = … ` shape +// that cannot clear anything. That file belongs to another change in this round, +// so the switch is deliberately left to it; the six call sites there become +// `store_or_clear_project_secret(&project.id, "", field.as_deref())?`. +#[allow(dead_code)] +pub fn store_or_clear_project_secret( + project_id: &str, + key_name: &str, + value: Option<&str>, +) -> Result<(), String> { + match secret_to_store(value) { + Some(v) => store_project_secret(project_id, key_name, v), + None => delete_project_secret(project_id, key_name), + } +} + +/// The store-or-clear decision, split out so it can be tested without a +/// keychain backend: `Some` means "write this", `None` means "remove whatever +/// is there". +#[allow(dead_code)] +fn secret_to_store(value: Option<&str>) -> Option<&str> { + match value.map(str::trim) { + Some(v) if !v.is_empty() => Some(v), + _ => None, + } +} + +/// Delete every known secret for a project from the OS keychain. +/// +/// Called when a project is removed, so it must cover [`PROJECT_SECRET_KEYS`] +/// exhaustively — a key missed here outlives the project that explained it. +/// One key failing does not stop the rest: a partial cleanup that keeps going +/// leaves strictly fewer credentials behind than one that gives up. pub fn delete_project_secrets(project_id: &str) -> Result<(), String> { - let secret_keys = [ - "git-token", - "aws-access-key-id", - "aws-secret-access-key", - "aws-session-token", - "aws-bearer-token", - ]; - for key_name in &secret_keys { - let service = format!("triple-c-project-{}-{}", project_id, key_name); - let entry = keyring::Entry::new(&service, "secret") - .map_err(|e| format!("Keyring error: {}", e))?; - match entry.delete_credential() { - Ok(()) => {} - Err(keyring::Error::NoEntry) => {} - Err(e) => { - log::warn!("Failed to delete project secret '{}': {}", key_name, e); - } + for key_name in PROJECT_SECRET_KEYS { + if let Err(e) = delete_project_secret(project_id, key_name) { + log::warn!("Failed to delete project secret '{}': {}", key_name, e); } } Ok(()) @@ -269,3 +344,78 @@ pub fn regenerate_gateway_master_key() -> Result { bump_gateway_secret_version()?; Ok(key) } + + +#[cfg(test)] +mod tests { + use super::*; + + /// The regression this list exists for. `openai-compatible-api-key` was + /// written by `store_secrets_for_project` and missing from the delete list, + /// so it survived project deletion. + #[test] + fn every_secret_the_app_writes_is_one_it_can_delete() { + for key in [ + "git-token", + "aws-access-key-id", + "aws-secret-access-key", + "aws-session-token", + "aws-bearer-token", + "openai-compatible-api-key", + ] { + assert!( + PROJECT_SECRET_KEYS.contains(&key), + "{} is written by commands/project_commands.rs but would outlive the project", + key + ); + } + } + + #[test] + fn the_key_list_has_no_duplicates() { + let mut seen = std::collections::HashSet::new(); + for key in PROJECT_SECRET_KEYS { + assert!(seen.insert(*key), "duplicate project secret key {}", key); + } + } + + /// A key that is not in the list is refused *before* any keychain entry is + /// constructed, which is what makes the list authoritative rather than + /// advisory. Without this, a new secret can be stored under a name nothing + /// ever deletes. + #[test] + fn an_unlisted_key_cannot_be_stored_at_all() { + let err = store_project_secret("some-project", "brand-new-token", "value") + .expect_err("an unlisted key must be refused"); + assert!( + err.contains("PROJECT_SECRET_KEYS"), + "the refusal should say how to fix it: {}", + err + ); + + let err = get_project_secret("some-project", "brand-new-token") + .expect_err("an unlisted key must be refused on read too"); + assert!(err.contains("brand-new-token"), "{}", err); + + let err = delete_project_secret("some-project", "brand-new-token") + .expect_err("an unlisted key must be refused on delete too"); + assert!(err.contains("brand-new-token"), "{}", err); + } + + /// The blanked-field case. `AccessSection.tsx` sends `gitToken || null`, so + /// a cleared field arrives as `None` — and before this existed, `None` was + /// skipped and the old secret stayed in the keychain forever. + #[test] + fn a_blanked_field_clears_rather_than_being_skipped() { + assert_eq!(secret_to_store(None), None); + assert_eq!(secret_to_store(Some("")), None); + assert_eq!(secret_to_store(Some(" \t\n")), None); + } + + #[test] + fn a_real_value_is_stored_trimmed() { + assert_eq!(secret_to_store(Some("ghp_abc123")), Some("ghp_abc123")); + // Pasted credentials routinely carry a trailing newline. + assert_eq!(secret_to_store(Some(" ghp_abc123\n")), Some("ghp_abc123")); + } +} diff --git a/app/src/components/settings/SharedAuthSettings.test.tsx b/app/src/components/settings/SharedAuthSettings.test.tsx index b7a0ff0..e3e867d 100644 --- a/app/src/components/settings/SharedAuthSettings.test.tsx +++ b/app/src/components/settings/SharedAuthSettings.test.tsx @@ -189,4 +189,115 @@ describe("SharedAuthSettings", () => { expect(toast.kind).toBe("success"); expect(toast.message).toBe("Shared Claude token removed from the keychain."); }); + + // ── A skipped scrub is not a success, and must stay retryable ──────────── + // `scrub_secrets_from_snapshots` refuses a project another operation holds + // rather than racing its `:latest` tag. That leaves a live ~1-year token in + // the image, so it can be neither folded into the success message nor + // described as a permanent failure whose remedy is Reset. + + it("reports a skipped snapshot as an incomplete revocation, not a success", async () => { + const toast = await revoke({ + snapshots_skipped: [ + "triple-c-snapshot-p1:latest: This project's container is being started or recreated. Wait for it to finish before removing a credential from its snapshot.", + ], + }); + expect(toast.kind).toBe("error"); + expect(toast.message).toMatch(/still in 1 snapshot image/i); + expect(toast.detail).toMatch(/run the\s+cleanup again/i); + // The wrong advice for a transient refusal. + expect(toast.detail).not.toMatch(/Reset/i); + }); + + it("keeps a retry available after the revoke has cleared the keychain", async () => { + projects = [running()]; + // Stored when the panel mounts, gone after the revoke — which is exactly + // the state that used to remove the only button able to finish the job. + hasClaudeToken.mockResolvedValueOnce(true).mockResolvedValue(false); + clearClaudeToken.mockResolvedValue({ + snapshots_scrubbed: [], + snapshots_failed: [], + snapshots_skipped: ["triple-c-snapshot-p1:latest: busy"], + snapshots_superseded: [], + docker_unavailable: null, + }); + 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"); + await waitFor(() => + expect(screen.queryByRole("button", { name: "Revoke" })).not.toBeInTheDocument(), + ); + expect(screen.getByTestId("shared-auth-leftover")).toHaveTextContent( + /still readable/i, + ); + expect(retry).toBeEnabled(); + }); + + it("clears the warning when a retry finally finishes the job", async () => { + projects = [running()]; + hasClaudeToken.mockResolvedValueOnce(true).mockResolvedValue(false); + clearClaudeToken.mockResolvedValueOnce({ + snapshots_scrubbed: [], + snapshots_failed: [], + snapshots_skipped: ["triple-c-snapshot-p1:latest: busy"], + snapshots_superseded: [], + docker_unavailable: null, + }); + 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"); + + clearClaudeToken.mockResolvedValueOnce({ + snapshots_scrubbed: ["triple-c-snapshot-p1:latest"], + snapshots_failed: [], + snapshots_skipped: [], + snapshots_superseded: [], + docker_unavailable: null, + }); + fireEvent.click(retry); + + await waitFor(() => + expect(screen.queryByTestId("shared-auth-leftover")).not.toBeInTheDocument(), + ); + expect(clearClaudeToken).toHaveBeenCalledTimes(2); + const toast = useAppState.getState().toasts.at(-1)!; + expect(toast.kind).toBe("success"); + expect(toast.message).toMatch(/cleared from 1 snapshot image/i); + }); + + it("offers a snapshot sweep even when no token is stored", async () => { + // Snapshots committed by an older build carry the token whether or not + // 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()); + const toast = useAppState.getState().toasts.at(-1)!; + expect(toast.kind).toBe("success"); + expect(toast.message).toBe("No snapshot image is holding the token."); + }); + + it("tolerates a backend that does not report skipped snapshots", async () => { + // `snapshots_skipped` is newer than the rest of the payload; its absence + // must read as "none", never as undefined reaching the UI. + const toast = await revoke({ snapshots_scrubbed: ["triple-c-snapshot-p1:latest"] }); + expect(toast.kind).toBe("success"); + expect(screen.queryByTestId("shared-auth-leftover")).not.toBeInTheDocument(); + }); }); diff --git a/app/src/components/settings/SharedAuthSettings.tsx b/app/src/components/settings/SharedAuthSettings.tsx index c4ada5b..b49d9ee 100644 --- a/app/src/components/settings/SharedAuthSettings.tsx +++ b/app/src/components/settings/SharedAuthSettings.tsx @@ -5,6 +5,7 @@ import StatusIndicator, { type StatusTone } from "../ui/StatusIndicator"; import { selectClass } from "../ui/Field"; import ClaudeAuthModal from "./ClaudeAuthModal"; import { clearClaudeToken } from "../../lib/tauri-commands"; +import type { ClearTokenOutcome } from "../../lib/types"; import { useProjects } from "../../hooks/useProjects"; import { useAppState } from "../../store/appState"; import { authErrorMessage, useClaudeTokenStatus } from "../../hooks/useClaudeAuth"; @@ -37,6 +38,32 @@ const STATUS_DISPLAY: Record< }, }; +/** + * `clear_claude_token` reports three separate things about the snapshot images + * that `docker commit` baked the token into, and they need three different + * sentences. `snapshots_skipped` in particular is **not** a failure of the + * rewrite — the project was busy (starting, compacting, migrating) and its + * snapshot was never attempted, so the remedy is to run the sweep again, not + * to Reset the project and lose both its volumes. + * + * `snapshots_skipped` is read defensively: it is newer than `ClearTokenOutcome` + * in `lib/types.ts`, which another change in this round owns. Until that lands + * the field arrives over IPC but is not in the declared type, and an older + * backend would not send it at all. + */ +type RevokeOutcome = ClearTokenOutcome & { snapshots_skipped?: string[] }; + +const list = (values: string[] | undefined): string[] => values ?? []; + +/** Whether a copy of the token is known — or suspected — to still be reachable. */ +function needsAnotherPass(outcome: RevokeOutcome): boolean { + return ( + list(outcome.snapshots_failed).length > 0 || + list(outcome.snapshots_skipped).length > 0 || + Boolean(outcome.docker_unavailable) + ); +} + /** * Host-level control for the one long-lived Claude Code token shared by every * project. Acquisition needs a running container to run the CLI in, so the @@ -50,7 +77,14 @@ export default function SharedAuthSettings() { const [pickedId, setPickedId] = useState(null); const [authOpen, setAuthOpen] = useState(false); const [confirmRevoke, setConfirmRevoke] = useState(false); - const [revoking, setRevoking] = useState(false); + const [sweeping, setSweeping] = useState(false); + + // What the last sweep could not finish. Held in its own state rather than + // derived from `status`, because that is exactly the bug this fixes: a + // revoke clears the keychain, `status` flips to "absent", and the button + // that could have retried the snapshot rewrite disappeared with it — + // leaving a live ~1-year token in an image and Reset as the only remedy. + const [leftover, setLeftover] = useState(null); // `claude setup-token` runs inside a container, so only running projects can // host the flow. @@ -61,13 +95,27 @@ export default function SharedAuthSettings() { const display = STATUS_DISPLAY[status]; - const handleRevoke = async () => { - setRevoking(true); + /** + * 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. + */ + const runSweep = async (mode: "revoke" | "sweep") => { + setSweeping(true); try { - const outcome = await clearClaudeToken(); + const outcome = (await clearClaudeToken()) as RevokeOutcome; setConfirmRevoke(false); await refresh(); + const failed = list(outcome.snapshots_failed); + const skipped = list(outcome.snapshots_skipped); + const scrubbed = list(outcome.snapshots_scrubbed); + const superseded = list(outcome.snapshots_superseded); + + 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` @@ -77,29 +125,48 @@ export default function SharedAuthSettings() { if (outcome.docker_unavailable) { pushToast({ kind: "error", - message: "Token removed from the keychain, but snapshots were not checked.", + message: + mode === "revoke" + ? "Token removed from the keychain, but snapshots were not checked." + : "Snapshot images were not checked.", detail: `Docker could not be reached (${outcome.docker_unavailable}), so any snapshot image ` + "built before this version may still contain the token in its environment. " + - "Start Docker and revoke again to clear them.", + "Start Docker and run the cleanup again to clear them.", }); - } else if (outcome.snapshots_failed.length > 0) { + } else if (failed.length > 0) { pushToast({ kind: "error", message: "Token removed from the keychain, but it is still in some images.", detail: - `${outcome.snapshots_failed.length} snapshot image(s) could not be rewritten and ` + + `${failed.length} snapshot image(s) could not be rewritten and ` + "still contain the token, readable via `docker image inspect`. Reset those " + - `projects to remove the images. Details: ${outcome.snapshots_failed.join("; ")}`, + `projects to remove the images. Details: ${failed.join("; ")}` + + (skipped.length > 0 + ? ` A further ${skipped.length} image(s) were skipped because their projects ` + + "are busy; those can be cleared by running the cleanup again." + : ""), }); - } else if (outcome.snapshots_scrubbed.length > 0) { + } else if (skipped.length > 0) { + pushToast({ + kind: "error", + message: `Token still in ${skipped.length} snapshot image(s) — those projects were busy.`, + detail: + "Nothing was rewritten for them, so the token is still readable via " + + "`docker image inspect`. Wait for the operation in progress to finish and run the " + + `cleanup again. Details: ${skipped.join("; ")}`, + }); + } else if (scrubbed.length > 0) { pushToast({ kind: "success", - message: `Shared Claude token removed, and cleared from ${outcome.snapshots_scrubbed.length} snapshot image(s).`, + message: + mode === "revoke" + ? `Shared Claude token removed, and cleared from ${scrubbed.length} snapshot image(s).` + : `Token cleared from ${scrubbed.length} snapshot image(s).`, detail: - outcome.snapshots_superseded.length > 0 + superseded.length > 0 ? "The pre-rewrite image layer for " + - `${outcome.snapshots_superseded.join(", ")} is still on disk because a ` + + `${superseded.join(", ")} is still on disk because a ` + "container is running from it. It goes away once that project is restarted " + "(which recreates the container) and Docker prunes the leftover." : undefined, @@ -107,23 +174,32 @@ export default function SharedAuthSettings() { } else { pushToast({ kind: "success", - message: "Shared Claude token removed from the keychain.", + message: + mode === "revoke" + ? "Shared Claude token removed from the keychain." + : "No snapshot image is holding the token.", }); } } catch (e) { pushToast({ kind: "error", - message: "Could not remove the shared Claude token.", + message: + 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.", ), }); } finally { - setRevoking(false); + setSweeping(false); } }; + const leftoverFailed = list(leftover?.snapshots_failed); + const leftoverSkipped = list(leftover?.snapshots_skipped); + return (
@@ -188,14 +264,77 @@ 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. + + )}
+ {leftover && ( +
+ +

+ {leftover.docker_unavailable + ? `Docker could not be reached (${leftover.docker_unavailable}), so no snapshot image was checked.` + : null} + {leftoverSkipped.length > 0 ? ( + <> + {leftoverSkipped.length} snapshot image(s) were skipped because their + projects were busy. Nothing was rewritten for them, so the token is + still readable with{" "} + docker image inspect. Running the + cleanup again once those projects are idle clears them. + + ) : null} + {leftoverFailed.length > 0 ? ( + <> + {" "} + {leftoverFailed.length} snapshot image(s) could not be rewritten:{" "} + {leftoverFailed.join("; ")}. If retrying does not help, Reset those + projects to remove the images. + + ) : null} +

+
+ +
+
+ )} + {!host && (

setConfirmRevoke(false)} - disabled={revoking} + disabled={sweeping} > Cancel } @@ -260,11 +399,11 @@ export default function SharedAuthSettings() { restarted.

- Each project’s snapshot image is also rewritten, because{" "} + 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. If any image - cannot be rewritten you will be told which, and the token stays readable - in it until that project is Reset. + 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.

)}