diff --git a/.gitea/workflows/build-app.yml b/.gitea/workflows/build-app.yml index 968f5ab..25a0b3b 100644 --- a/.gitea/workflows/build-app.yml +++ b/.gitea/workflows/build-app.yml @@ -357,6 +357,38 @@ jobs: (Get-Content app/src-tauri/Cargo.toml) -replace '^version = ".*?"', "version = `"$version`"" | Set-Content app/src-tauri/Cargo.toml Write-Host "Patched version to $version" + - name: Install MSVC C++ build tools + shell: cmd + run: | + rem Tauri links with MSVC, so rustc needs link.exe and the Windows SDK. + rem This job previously assumed a hand-provisioned runner; a runner + rem without them registers fine, accepts windows-latest jobs, and then + rem fails at link time with "linker `link.exe` not found" after having + rem already downloaded the whole crate graph. + rem + rem rustc locates MSVC through vswhere/the registry rather than PATH, + rem so installing is sufficient — no dev-shell activation needed here. + set "VSWHERE=%ProgramFiles(x86)%\Microsoft Visual Studio\Installer\vswhere.exe" + if exist "%VSWHERE%" ( + "%VSWHERE%" -latest -products * -requires Microsoft.VisualStudio.Component.VC.Tools.x86.x64 -property installationPath > "%TEMP%\vcpath.txt" 2>nul + for /f "usebackq delims=" %%i in ("%TEMP%\vcpath.txt") do set "VCPATH=%%i" + ) + if defined VCPATH ( + echo MSVC build tools already present at %VCPATH% + ) else ( + echo MSVC build tools not found - installing Visual Studio Build Tools + curl -fSL -o "%TEMP%\vs_BuildTools.exe" https://aka.ms/vs/17/release/vs_BuildTools.exe || exit /b 1 + rem 3010 means "installed, reboot pending" and is a success for our purposes. + "%TEMP%\vs_BuildTools.exe" --quiet --wait --norestart --nocache ^ + --add Microsoft.VisualStudio.Workload.VCTools --includeRecommended + set "VSEXIT=%ERRORLEVEL%" + del "%TEMP%\vs_BuildTools.exe" 2>nul + if not "%VSEXIT%"=="0" if not "%VSEXIT%"=="3010" ( + echo Visual Studio Build Tools installer failed with %VSEXIT% + exit /b %VSEXIT% + ) + echo Visual Studio Build Tools installed + ) - name: Install Rust stable run: | where rustup >nul 2>&1 && ( diff --git a/CLAUDE.md b/CLAUDE.md index 02fb523..1c3dea7 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -112,9 +112,13 @@ docker exec stdout → tokio task → emit("terminal-output-{sessionId}") → li - `image.rs` — Image build/pull with progress streaming - `gateway.rs` — Optional LiteLLM sibling container giving Claude Code an Anthropic-format front end for providers that only speak OpenAI (see `gateway-container/`). Mirrors `stt.rs`. - Binds `0.0.0.0` — unlike STT — because *project containers*, not the host process, consume - it; it therefore **always** sets a LiteLLM `master_key`, since LiteLLM without one accepts - any key. + Its bind address is **detected, never `0.0.0.0`** — unlike STT, *project containers* consume + it, so loopback alone is not always enough: Docker Desktop gets `127.0.0.1` (containers reach + it via `host.docker.internal`), native Linux gets the default bridge gateway (`172.17.0.1`). + `GatewayBinding` derives the bind address and the advertised `base_url` together so they + cannot drift. A wildcard bind would be LAN-reachable — Docker's rules precede host firewalls — + in front of a container config holding a billed provider key. It also **always** sets a + LiteLLM `master_key`, since LiteLLM without one accepts any key. - `migration.rs` — Base-image migration: manifest capture via throwaway containers, the pure delta computation (dpkg-ownership filter, bind-mount exclusion, verbatim-copy set), and the crash-recovery state machine. See "Base-image migration" below. @@ -180,6 +184,25 @@ security update. Migration is the non-destructive way out; Reset is the destruct label plus the persisted state file let `reconcile_project_statuses` offer resume or rollback. - **Rollback restores the system layer only.** The volumes are never touched at any point, so work done in `$HOME` during a migrated session survives a rollback. Say so in any UI copy. +- **`/var` is never copied either, and that is the one way migration is *more* destructive than + the ordinary recreate.** A recreate builds from the project's snapshot, so `/var/lib/postgresql` + rides along; a migration builds from the base and the apt replay hands back an empty cluster. + Copying a live database's files onto a different base's version of the same package is a + corruption risk, not a fix — so the answer is disclosure. `unpreserved_data()` reports + first-level directories under `/var/lib` and `/var/www` that the base does not ship *and* that + hold non-dpkg-owned files (which is what keeps `/var/lib/apt` and `/var/lib/dpkg` out of it), + and the pre-flight, the banner and the finished report all name them. Do not make this silent. +- **The rollback pin is not best-effort.** After `commit_container_snapshot` the commit is the only + copy of the old system layer, so a `docker tag` that fails — or succeeds without the reference + resolving — aborts the migration before `remove_container`. Same rule in reverse for + `rollback_migration`: the image is confirmed to exist before the container is destroyed. +- **`resume` must check the container's `triple-c.migration-state` label**, exactly as + `reconcile_migration` does. Without it a record left behind by a failed commit "resumes" into + the *old, unmigrated* container and commits it as migrated. +- **Anything that stops, removes or recreates a project's container consults + `migration_commands::is_migrating`.** The window between `remove_container` and the create that + follows looks exactly like "no container" to Start, and Reset would delete the volumes out from + under a live run. - **`/etc` is never copied**, only reported: the snapshot lineage has `/etc/apt/sources.list.d/nodesource.sources` where the current base has `nodesource.list`, and having both breaks every `apt-get update` on a duplicate source. Verified, not theoretical. diff --git a/app/src-tauri/Cargo.toml b/app/src-tauri/Cargo.toml index f4a2031..52cff6f 100644 --- a/app/src-tauri/Cargo.toml +++ b/app/src-tauri/Cargo.toml @@ -38,6 +38,11 @@ base64 = "0.22" rand = "0.9" local-ip-address = "0.6" +[dev-dependencies] +# `test-util` (not part of tokio's `full`) lets the auto-start retry tests run +# their backoff schedule under a paused clock instead of in real seconds. +tokio = { version = "1", features = ["full", "test-util"] } + [build-dependencies] tauri-build = { version = "2", features = [] } diff --git a/app/src-tauri/src/auth_bridge/mod.rs b/app/src-tauri/src/auth_bridge/mod.rs index 8739157..3f89ba3 100644 --- a/app/src-tauri/src/auth_bridge/mod.rs +++ b/app/src-tauri/src/auth_bridge/mod.rs @@ -56,7 +56,7 @@ use tokio::sync::{watch, Mutex}; use tokio::task::JoinHandle; use crate::docker::container::is_container_running; -use crate::docker::exec::exec_oneshot; +use crate::docker::exec::{exec_oneshot_limited, PROC_NET_OUTPUT_LIMIT}; use crate::storage::projects_store::ProjectsStore; use proc_net::PortFamily; @@ -248,9 +248,14 @@ impl AuthBridgeManager { /// project whose bridge is on but whose container is stopped still reports /// `enabled: true` with no active ports. pub async fn status(&self, project_id: &str, enabled: bool) -> AuthBridgeStatus { - let map = self.bridges.lock().await; - match map.get(project_id) { - Some(bridge) => bridge.state.lock().await.snapshot(enabled), + // Clone the per-project handle out and drop the map lock before taking + // the state lock. Holding both across the nested await is not a + // deadlock — the order is consistently bridges→state — but it puts a + // cheap UI status call behind whatever the poller is doing under + // `state`, and behind every other project's status call too. + let state = self.bridges.lock().await.get(project_id).map(|b| b.state.clone()); + match state { + Some(state) => state.lock().await.snapshot(enabled), None => AuthBridgeStatus { enabled, ..AuthBridgeStatus::disabled() @@ -300,8 +305,16 @@ async fn poll_loop( } // One exec per tick reads both procfs files. + // + // Absolute path, deliberately: the image's `ENV PATH` puts a + // container-writable directory first, so a bare `cat` is a name the + // container can rebind to a shim that prints whatever it likes. It + // still could not make us bind a *non-loopback* port, but it decides + // how much output this loop ingests and how many host ports it is asked + // for, which is why the call is also length-capped and the result + // count is capped in `reconcile`. let cmd = vec![ - "cat".to_string(), + "/usr/bin/cat".to_string(), "/proc/net/tcp".to_string(), "/proc/net/tcp6".to_string(), ]; @@ -309,7 +322,7 @@ async fn poll_loop( // bridge or stopping the container doesn't wait out an in-flight poll. let discovery = tokio::select! { _ = cancel.changed() => break, - res = exec_oneshot(&container_id, cmd) => res, + res = exec_oneshot_limited(&container_id, cmd, PROC_NET_OUTPUT_LIMIT) => res, }; match discovery { @@ -373,6 +386,7 @@ fn skipped_ports(project: &crate::models::Project) -> HashSet { .flat_map(|m| [m.container_port, m.host_port]) .collect(); skip.extend(RESERVED_CONTAINER_PORTS.clone()); + skip.extend(RESERVED_HOST_PORTS.clone()); skip } @@ -399,6 +413,33 @@ fn skipped_ports(project: &crate::models::Project) -> HashSet { /// `browser_view::VIEWER_PORTS`, which asserts on it. pub const RESERVED_CONTAINER_PORTS: std::ops::RangeInclusive = 39321..=39328; +/// Host ports another feature binds on demand, which the bridge must not take +/// first. +/// +/// These are the browser-view proxy's host ports. The bridge binds *host* ports +/// named by the container, so a container listening on 47820 would have the +/// bridge take the host side of that number — and then the browser-view pane, +/// which only binds when the user opens it, finds its port gone. The two ranges +/// are separate constants because they guard opposite ends of the same +/// mechanism: [`RESERVED_CONTAINER_PORTS`] is about not *publishing* something, +/// this one is about not *stealing* something. +pub const RESERVED_HOST_PORTS: std::ops::RangeInclusive = + crate::browser_view::proxy::PROXY_PORTS; + +/// Most host ports the bridge will hold for one project at a time. +/// +/// The discovery input is entirely container-controlled, and each +/// [`PortForward`] costs two listeners plus a task, so without a cap a +/// container that reports tens of thousands of fake listeners exhausts the +/// app's file descriptors and the host's ephemeral ports in a single tick. A +/// real login flow uses one or two ports at a time; anything past a couple of +/// dozen is not a login. +const MAX_FORWARDS: usize = 24; + +/// Most conflicts recorded at once, so a flood of unbindable ports can't grow +/// the status payload (and the UI list) without bound either. +const MAX_CONFLICTS: usize = 32; + /// Bring the set of host listeners in line with what the container is currently /// listening on. Returns whether anything the UI cares about changed. async fn reconcile( @@ -441,6 +482,20 @@ async fn reconcile( if skip.contains(&port) || st.forwards.contains_key(&port) { continue; } + if st.forwards.len() >= MAX_FORWARDS { + // Don't even attempt the bind: the point of the cap is to stop the + // container dictating how many host resources we take. + changed |= note_conflict( + &mut st, + port, + format!( + "The auth bridge is already holding {} ports for this project; \ + {} was not bridged.", + MAX_FORWARDS, port + ), + ); + continue; + } match PortForward::bind(container_id.to_string(), port, family).await { Ok(forward) => { if st.conflicts.remove(&port).is_some() { @@ -467,9 +522,8 @@ async fn reconcile( ); if st.conflicts.get(&port) != Some(&reason) { log::warn!("Auth bridge: {}", reason); - st.conflicts.insert(port, reason); - changed = true; } + changed |= note_conflict(&mut st, port, reason); } } } @@ -477,6 +531,23 @@ async fn reconcile( changed } +/// Record why a port wasn't bridged, up to [`MAX_CONFLICTS`]. Returns whether +/// the recorded set changed. +fn note_conflict(state: &mut BridgeState, port: u16, reason: String) -> bool { + match state.conflicts.get(&port) { + Some(existing) if *existing == reason => false, + Some(_) => { + state.conflicts.insert(port, reason); + true + } + None if state.conflicts.len() < MAX_CONFLICTS => { + state.conflicts.insert(port, reason); + true + } + None => false, + } +} + /// Release every host port held for this project. Awaits each shutdown, so on /// return nothing is bound. async fn teardown(project_id: &str, state: &Arc>) { @@ -548,9 +619,68 @@ mod tests { } #[test] - fn no_mappings_means_nothing_but_the_reserved_range_is_skipped() { + fn no_mappings_means_nothing_but_the_reserved_ranges_are_skipped() { let skip = skipped_ports(&project_with_mappings(vec![])); - assert_eq!(skip.len(), RESERVED_CONTAINER_PORTS.clone().count()); + assert_eq!( + skip.len(), + RESERVED_CONTAINER_PORTS.clone().count() + RESERVED_HOST_PORTS.clone().count() + ); + } + + #[test] + fn the_browser_views_host_ports_are_never_taken() { + // The bridge binds *host* ports chosen by the container, so without + // this it can take the port the browser-view proxy will want later — + // that pane binds on demand, so first-come would win. + let skip = skipped_ports(&project_with_mappings(vec![])); + for port in RESERVED_HOST_PORTS { + assert!(skip.contains(&port), "host port {} should be reserved", port); + } + assert!(!skip.contains(&(RESERVED_HOST_PORTS.end() + 1))); + } + + #[test] + fn conflicts_stop_being_recorded_past_the_cap() { + let mut st = BridgeState::default(); + for port in 1000u16..1000 + MAX_CONFLICTS as u16 { + assert!(note_conflict(&mut st, port, "busy".to_string())); + } + // Past the cap: new ports are dropped rather than growing the status + // payload the UI renders. + assert!(!note_conflict(&mut st, 9999, "busy".to_string())); + assert_eq!(st.conflicts.len(), MAX_CONFLICTS); + // A changed reason for a port already tracked still updates. + assert!(!note_conflict(&mut st, 1000, "busy".to_string())); + assert!(note_conflict(&mut st, 1000, "different".to_string())); + assert_eq!(st.conflicts.len(), MAX_CONFLICTS); + } + + #[tokio::test] + async fn the_host_ports_one_container_can_demand_are_capped() { + // The container fully controls the discovery input (it can shim the + // probe command), and each forward costs two listeners plus a task — + // uncapped, one tick could exhaust the app's fds and the host's + // ephemeral ports. + let discovered: BTreeMap = + (45000u16..45200).map(|p| (p, PortFamily::V4)).collect(); + let state = Arc::new(Mutex::new(BridgeState::default())); + + reconcile("no-such-container", &discovered, &HashSet::new(), &state).await; + + let mut st = state.lock().await; + assert!( + st.forwards.len() <= MAX_FORWARDS, + "bridged {} ports, cap is {}", + st.forwards.len(), + MAX_FORWARDS + ); + assert!(st.conflicts.len() <= MAX_CONFLICTS); + // Nowhere near the 200 the "container" asked for. + assert!(st.forwards.len() + st.conflicts.len() < discovered.len()); + + for (_, mut forward) in std::mem::take(&mut st.forwards) { + forward.shutdown().await; + } } #[test] diff --git a/app/src-tauri/src/commands/auth_token_commands.rs b/app/src-tauri/src/commands/auth_token_commands.rs index eb42463..337162f 100644 --- a/app/src-tauri/src/commands/auth_token_commands.rs +++ b/app/src-tauri/src/commands/auth_token_commands.rs @@ -23,11 +23,20 @@ //! * The flow needs a way to deliver the pasted code, hence //! [`submit_claude_token_code`] and the stdin channel below. Without it the //! command would simply sit at the prompt until it timed out. -//! * [`crate::auth_bridge`] is *not* required for this particular command, -//! since there is no container-local callback to reach. It is still enabled -//! for the duration (and restored afterwards) as designed: it costs nothing -//! here and keeps the flow working if a future CLI version, or the plain -//! `claude login` path, goes back to a loopback redirect. +//! * [`crate::auth_bridge`] is **not involved**. There is no container-local +//! listener to reach, so there is nothing for it to bridge. +//! +//! An earlier version turned the bridge on for the duration "in case a +//! future CLI version goes back to a loopback redirect", and turned it off +//! again afterwards. That was wrong twice over. The bridge flag is +//! *persisted* to `projects.json`, and the restore only ran if the future +//! completed — so a force-quit, kill or panic inside the 15-minute +//! [`SETUP_TIMEOUT`] left it latched on, and the app re-armed an +//! unauthenticated loopback port mirror on every subsequent launch, for a +//! project whose owner had never opted in. And it bought nothing: the +//! speculative future it defended against would need code changes here +//! anyway. The bridge remains available as the per-project setting it always +//! was; this command does not touch it. //! //! ## Handling of the token itself //! @@ -66,10 +75,27 @@ const SETUP_TIMEOUT: Duration = Duration::from_secs(15 * 60); const TOKEN_PREFIX: &str = "sk-ant-oat01-"; /// Minimum number of body characters after [`TOKEN_PREFIX`] for a match to be -/// believed. Real tokens run to ~90 characters; this is set well below that but -/// far above anything prose would produce, so documentation-style decoys like -/// `sk-ant-oat01-...` or `sk-ant-oat01-` are rejected. -const MIN_TOKEN_BODY: usize = 32; +/// believed. Real `setup-token` credentials run to ~90 body characters. +/// +/// This must be close to that real length, not merely "longer than prose". +/// The earlier value of 32 accepted a *fragment* of a token — which is what a +/// line wrap produces if `stty cols 200` fails and the pty falls back to 80 +/// columns, splitting the value across two lines. Two bad things follow from +/// accepting one: +/// +/// * the fragment gets stored as if it were the credential, so every +/// container authenticates with a token that cannot work, and the failure +/// surfaces far from its cause; +/// * [`SecretRedactor`] masks the leading fragment because it carries the +/// `sk-ant-` marker, but the *tail* on the next line carries no marker and +/// is emitted to the UI in clear. +/// +/// Set below the real length by enough margin to survive a modest change in +/// token format, and far above any fragment an 80-column wrap can produce (the +/// prefix alone eats 13 columns, so the longest possible first line fragment is +/// ~67). Rejecting a real-but-shorter token is a loud, recoverable failure — +/// "printed no recognisable token" — whereas accepting a fragment is silent. +const MIN_TOKEN_BODY: usize = 80; /// Redaction is deliberately broader than extraction: anything shaped like an /// Anthropic credential is masked on its way to the UI, not just `oat01` ones. @@ -361,6 +387,20 @@ fn strip_ansi_prefix(bytes: &[u8]) -> (String, usize) { (out, i) } +/// Cap on the bytes [`AnsiStripper`] will hold waiting for a control sequence +/// to terminate. +/// +/// [`strip_ansi_prefix`] stops at the first *incomplete* sequence and the +/// remainder is carried to the next chunk — which is correct while the +/// sequence really is going to end, and unbounded when it never does. A single +/// `ESC ]` with no BEL and no ST swallows everything the container prints for +/// the rest of the 15-minute [`SETUP_TIMEOUT`], and the buffer grows with it; +/// `transcript` and [`SecretRedactor::pending`] are both already capped, so +/// this was the one remaining way to make the flow eat memory. Generous enough +/// that no legitimate sequence comes close (OSC 8 hyperlinks are the longest +/// thing Claude Code emits, in the low hundreds of bytes). +const MAX_ANSI_CARRY: usize = 64 * 1024; + /// Stateful wrapper around [`strip_ansi_prefix`] that carries an incomplete /// trailing sequence over to the next chunk. #[derive(Default)] @@ -371,8 +411,29 @@ struct AnsiStripper { impl AnsiStripper { fn push(&mut self, chunk: &[u8]) -> String { self.carry.extend_from_slice(chunk); - let (out, consumed) = strip_ansi_prefix(&self.carry); + let (mut out, consumed) = strip_ansi_prefix(&self.carry); self.carry.drain(..consumed); + + // Past the cap the leading sequence is not going to terminate. Drop + // its introducer and re-strip: the bytes behind it are then treated as + // ordinary text rather than discarded, so a token hiding inside a + // runaway OSC still reaches the parser. Dropping one byte per + // overflowing chunk is enough — the cap can only be re-crossed by a + // fresh chunk, which re-enters here. + if self.carry.len() > MAX_ANSI_CARRY { + log::warn!( + "`claude setup-token` emitted an unterminated control sequence \ + longer than {} bytes — treating it as text", + MAX_ANSI_CARRY + ); + } + while self.carry.len() > MAX_ANSI_CARRY { + self.carry.drain(..1); + let (more, consumed) = strip_ansi_prefix(&self.carry); + self.carry.drain(..consumed); + out.push_str(&more); + } + out } } @@ -606,65 +667,23 @@ pub async fn acquire_claude_token( *cancel_slot().lock().await = Some(cancel_tx); } - let bridge_was_enabled = project.auth_bridge_enabled; + // No auth-bridge elevation here — see the module docs. `setup-token` has no + // container-local callback to bridge, and the flag that would enable one is + // *persisted*, so a crash anywhere inside the 15-minute timeout used to + // leave an unauthenticated port mirror armed for good. + emit_progress( + &app_handle, + &project_id, + "Running `claude setup-token` — sign in at the URL below, then submit the code it gives you.", + ); - let result = async { - // See the module docs: 2.1.226's `setup-token` redirects to an - // Anthropic-hosted callback, so no container-local listener needs - // bridging. Enabled anyway, per design, to cover CLI versions and login - // paths that do use a loopback redirect. Temporary elevation — the - // prior setting is restored below whatever happens. - if !bridge_was_enabled { - state - .projects_store - .set_auth_bridge_enabled(&project_id, true)?; - emit_progress( - &app_handle, - &project_id, - "Auth bridge enabled for the duration of login.", - ); - } - // Called unconditionally, and idempotent: the flag may already have - // been on while the poller was not running (e.g. enabled before start). - state - .auth_bridge - .start( - project_id.clone(), - container_id.clone(), - app_handle.clone(), - state.projects_store.clone(), - ) - .await; + let result = + run_setup_token(&app_handle, &project_id, &container_id, input_rx, cancel_rx).await; - emit_progress( - &app_handle, - &project_id, - "Running `claude setup-token` — sign in at the URL below, then submit the code it gives you.", - ); - - run_setup_token(&app_handle, &project_id, &container_id, input_rx, cancel_rx).await - } - .await; - - // Release the flow, then restore the bridge — both unconditionally, so a - // failed or cancelled login leaves nothing latched on. + // Release the flow. Nothing else needs unwinding: this command changes no + // persisted state until the token itself is stored, which is the point. *pending_input().lock().await = None; *cancel_slot().lock().await = None; - if !bridge_was_enabled { - // Stop the poller first: it awaits teardown, so host ports are provably - // released before the flag goes back. - state.auth_bridge.stop(&project_id).await; - if let Err(e) = state - .projects_store - .set_auth_bridge_enabled(&project_id, false) - { - log::warn!( - "Failed to restore the auth bridge setting for project {}: {}", - project_id, - e - ); - } - } let token = result?; secure::store_claude_oauth_token(&token)?; @@ -736,14 +755,69 @@ pub async fn has_claude_token() -> Result { Ok(secure::has_claude_oauth_token()) } -/// Forget the shared Claude token. Containers keep the injected value until -/// each is next started, at which point the rotation-id label mismatch forces a -/// recreation that blanks the env var. +/// 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)] +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, + /// 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`. + pub snapshots_superseded: Vec, + /// Set when Docker could not be reached at all, so nothing is known about + /// what is still on disk. + pub docker_unavailable: Option, +} + +/// Forget the shared Claude token. +/// +/// 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 +/// telling the user something untrue: +/// +/// * **Running containers** hold it in their environment. That resolves +/// itself: the rotation id in `triple-c.claude-token-version` no longer +/// matches, so the next start recreates the container and +/// `MANAGED_AUTH_KEYS` blanks the variable. +/// * **Snapshot images** hold it in `Config.Env`, and nothing resolves that on +/// its own — the image outlives every container built from it, and +/// `docker image inspect` will keep printing a live ~1-year credential for +/// 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. +/// +/// 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<(), String> { +pub async fn clear_claude_token() -> Result { secure::delete_claude_oauth_token()?; log::info!("Cleared the shared Claude authentication token"); - Ok(()) + + let report = crate::docker::container::scrub_secrets_from_snapshots().await; + if report.left_something_behind() { + log::warn!( + "Revoked the shared Claude token but {} snapshot image(s) may still contain it", + report.failed.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, + }) } #[cfg(test)] @@ -938,5 +1012,72 @@ mod tests { visible.push_str(&s.push(b"1mb")); assert_eq!(visible, "ab"); } -} + // ── Truncated credentials ──────────────────────────────────────────── + // `stty cols 200` runs before Claude Code starts precisely so the ~103 + // character token never wraps. If that fails, the pty falls back to 80 + // columns and the token arrives split across two lines. The old floor of + // 32 body characters believed the first half. + #[test] + fn a_token_shorter_than_a_real_one_is_rejected() { + let fragment = format!("{}{}", TOKEN_PREFIX, "M".repeat(MIN_TOKEN_BODY - 1)); + assert_eq!( + parse_setup_token(&format!("Your token: {}\n", fragment)), + None + ); + } + + #[test] + fn a_line_wrapped_token_yields_nothing_rather_than_half_a_credential() { + let tok = token('N'); + // Column 12 is where `Your token: ` ends, so an 80-column pty breaks + // the value 68 characters in. + let wrapped = format!("Your token: {}\n{}\n", &tok[..68], &tok[68..]); + assert_eq!( + parse_setup_token(&wrapped), + None, + "a wrapped token must fail loudly, not be stored truncated" + ); + } + + #[test] + fn a_real_length_token_is_still_accepted() { + // Guards the floor from being raised past what Anthropic actually mints. + let tok = token('O'); + assert_eq!(tok.len() - TOKEN_PREFIX.len(), 90); + assert!(90 > MIN_TOKEN_BODY); + assert_eq!(parse_setup_token(&format!("{}\n", tok)), Some(tok)); + } + + // ── Bounded buffering ──────────────────────────────────────────────── + + #[test] + fn an_unterminated_control_sequence_cannot_grow_the_carry_without_bound() { + let mut s = AnsiStripper::default(); + // An OSC introducer with no BEL and no ST. `strip_ansi_prefix` cannot + // know it has ended, so every byte after it is carried — for the whole + // 15-minute timeout, if nothing caps it. + let mut seen = s.push(b"\x1b]8;id=1;"); + for _ in 0..40 { + seen.push_str(&s.push(&vec![b'A'; 4096])); + } + assert!( + s.carry.len() <= MAX_ANSI_CARRY, + "carry grew to {} bytes", + s.carry.len() + ); + assert!(!seen.is_empty(), "the withheld text must be released, not dropped"); + } + + #[test] + fn a_token_printed_after_a_runaway_sequence_is_still_found() { + let tok = token('Q'); + let mut s = AnsiStripper::default(); + let mut seen = s.push(b"\x1b]8;id=1;"); + for _ in 0..40 { + seen.push_str(&s.push(&vec![b'A'; 4096])); + } + seen.push_str(&s.push(format!("\nYour token: {}\n", tok).as_bytes())); + assert_eq!(parse_setup_token(&seen), Some(tok)); + } +} diff --git a/app/src-tauri/src/commands/migration_commands.rs b/app/src-tauri/src/commands/migration_commands.rs index 695f1bf..2b2ef2f 100644 --- a/app/src-tauri/src/commands/migration_commands.rs +++ b/app/src-tauri/src/commands/migration_commands.rs @@ -60,8 +60,8 @@ use crate::docker; use crate::docker::migration::{self as mig, Recovery}; use crate::models::{ ContainerStaleness, MigrationOptions, MigrationPhase, MigrationPlan, MigrationReport, - MigrationState, PackageFailure, Project, ProjectStatus, MIGRATION_PHASE_AWAITING, - MIGRATION_PHASE_INTERRUPTED, + MigrationState, PackageFailure, Project, ProjectStatus, UnpreservedData, + MIGRATION_PHASE_AWAITING, MIGRATION_PHASE_INTERRUPTED, }; use crate::storage::migration_store; use crate::AppState; @@ -159,6 +159,20 @@ pub async fn get_container_staleness( &mig::bind_mount_exclusions(&project.paths), ); out.outdated_package_count = mig::outdated_package_count(&from_manifest, &base_manifest); + // Reported, never copied, and never silent — see `unpreserved_data`. + out.unpreserved_data = mig::unpreserved_data(&from_manifest, &base_manifest); + if !out.unpreserved_data.is_empty() { + log::warn!( + "Project {}: {} data-bearing subtree(s) under /var would not survive a migration: {}", + project_id, + out.unpreserved_data.len(), + out.unpreserved_data + .iter() + .map(|d| d.path.as_str()) + .collect::>() + .join(", ") + ); + } let (etc_only_container, etc_only_base) = mig::etc_deltas(&from_manifest, &base_manifest); if !etc_only_container.is_empty() || !etc_only_base.is_empty() { @@ -207,7 +221,12 @@ fn active_migrations() -> &'static std::sync::Mutex bool { +/// Whether a migration for this project is running **in this process right +/// now**. Every command that stops, removes or recreates the project's +/// container has to consult it: the window between `remove_container` and the +/// create that follows looks exactly like "no container", and an ordinary +/// Start landing in it creates a second container under the same name. +pub(crate) fn is_migrating(project_id: &str) -> bool { active_migrations() .lock() .unwrap_or_else(|e| e.into_inner()) @@ -335,6 +354,22 @@ async fn fresh_migration( let npm_delta = mig::set_delta(&from_manifest.npm_global, &base_manifest.npm_global); let (missing_paths, _) = mig::missing_features(&from_manifest, &base_manifest); let payload_bytes = mig::verbatim_payload_bytes(&from_manifest, &verbatim); + // Measured here as well as in the pre-flight probe, because this is the one + // reading taken against the container that is about to be destroyed. It is + // frozen into the plan so the finished report can still name it. + let unpreserved = mig::unpreserved_data(&from_manifest, &base_manifest); + if !unpreserved.is_empty() { + log::warn!( + "Project {}: migrating destroys {} data-bearing subtree(s) under /var: {}", + project_id, + unpreserved.len(), + unpreserved + .iter() + .map(|d| format!("{} ({} files)", d.path, d.file_count)) + .collect::>() + .join(", ") + ); + } emit_progress(&app_handle, &project_id, "Checking network and disk..."); let env = mig::preflight_environment(&base_image).await?; @@ -367,6 +402,7 @@ async fn fresh_migration( npm_packages: npm_delta.clone(), verbatim_paths: verbatim.clone(), missing_paths, + unpreserved_data: unpreserved, }); migration_store::save(&project_id, &mstate)?; @@ -408,22 +444,63 @@ async fn fresh_migration( let _ = docker::stop_container(&container_id).await; emit_progress(&app_handle, &project_id, "Saving the current image..."); - docker::commit_container_snapshot(&container_id, &project).await?; + if let Err(e) = docker::commit_container_snapshot(&container_id, &project).await { + // The container is stopped but intact and `:latest` is untouched, so + // the only repair needed is to stop claiming a migration is in flight. + abandon_before_swap(&project_id, &state, None).await; + return Ok(MigrationReport::failed_preflight(format!( + "Could not save the container's current image: {}. Nothing was changed — the container is still on its previous image and can be started as usual.", + e + ))); + } // The rollback pin. `docker tag` of a 5.49 GB image was measured at 0.036 s // and 0 bytes, so this is free to take and only costs disk if it is kept. + // + // It is *not* best-effort. The commit above is now the only copy of the old + // system layer, the next step removes the container, and `finish_migration` + // repoints `:latest` at the new lineage — so a migration that carries on + // without a usable pin has quietly made itself irreversible while the UI + // goes on offering "Roll back". Tag, read it back, and abort while aborting + // still costs nothing. + emit_progress(&app_handle, &project_id, "Pinning a rollback image..."); let (repo, _) = mig::split_image_ref(&snapshot_image); let tag = mig::rollback_tag(&chrono::Utc::now()); let rollback_ref = format!("{}:{}", repo, tag); - if let Err(e) = mig::tag_image(&snapshot_image, &repo, &tag).await { - log::warn!("Could not create the rollback tag: {} — continuing", e); + let tag_result = mig::tag_image(&snapshot_image, &repo, &tag).await; + let resolved = if tag_result.is_ok() { + mig::image_id(&rollback_ref).await.unwrap_or(None) } else { - mstate.rollback_image = Some(rollback_ref); - migration_store::save(&project_id, &mstate)?; + None + }; + if let Err(why) = rollback_pin_verdict( + tag_result.as_ref().err().map(|e| e.as_str()), + resolved.as_deref(), + ) { + abandon_before_swap(&project_id, &state, Some(&rollback_ref)).await; + return Ok(MigrationReport::failed_preflight(why)); + } + mstate.rollback_image = Some(rollback_ref.clone()); + if let Err(e) = migration_store::save(&project_id, &mstate) { + // A pin nothing has recorded is a pin no recovery path can find, which + // is the same hole as not having one. Still pre-swap, so stop here. + abandon_before_swap(&project_id, &state, Some(&rollback_ref)).await; + return Ok(MigrationReport::failed_preflight(format!( + "Could not record the rollback image before replacing the container: {}. Nothing was changed — the container is still on its previous image.", + e + ))); } emit_progress(&app_handle, &project_id, "Recreating on the new base image..."); - docker::remove_container(&container_id).await?; + if let Err(e) = docker::remove_container(&container_id).await { + // Still pre-swap: the old container is the one in place and `:latest` + // still holds its lineage. + abandon_before_swap(&project_id, &state, Some(&rollback_ref)).await; + return Ok(MigrationReport::failed_preflight(format!( + "Could not remove the old container: {}. Nothing was changed — the container is still on its previous image and can be started as usual.", + e + ))); + } let docker_socket = settings .docker_socket_path @@ -487,9 +564,63 @@ async fn resume_migration( )); } }; + // The container being *there* is not evidence that the swap happened. If + // `commit_container_snapshot` failed, the old, unmigrated container is + // still in place — and resuming into it would replay the deltas onto the + // container that already had them, commit it as the migrated image and + // report success. `reconcile_migration` has always checked this label; the + // path the Migrate button reaches did not. + let labelled = container_label(&container_id, mig::LABEL_MIGRATION_STATE) + .await + .as_deref() + == Some(mig::MIGRATION_LABEL_IN_PROGRESS); + match resume_verdict(&mstate.phase, labelled) { + ResumeVerdict::Proceed => {} + ResumeVerdict::RefuseUnswapped => { + // Exactly the `SelfHeal` case `reconcile_migration` handles, so + // handle it the same way: the record describes work that never + // landed, `:latest` still holds the old lineage, and clearing it + // turns a dead end into "just run the update again". + if let Some(ref reference) = mstate.rollback_image { + let _ = mig::untag_image(reference).await; + } + let _ = migration_store::clear_staging(&project_id); + let _ = migration_store::clear(&project_id); + return Ok(MigrationReport::failed_preflight( + "This project's container is still the original one — the interrupted update never got as far as replacing it, so there was nothing to resume. Nothing was changed and the record has been cleared: start the project as usual, or run the update again from the beginning.", + )); + } + ResumeVerdict::RefuseNotResumable => { + return Ok(MigrationReport::failed_preflight(format!( + "This project's migration record is in the '{}' state, which cannot be resumed. Confirm it or roll it back first.", + mstate.phase + ))); + } + } + if !docker::is_container_running(&container_id).await.unwrap_or(false) { emit_progress(&app_handle, &project_id, "Starting the migrated container..."); - docker::start_container(&container_id).await?; + if let Err(e) = docker::start_container(&container_id).await { + // Leave the record alone — the container is still mid-swap and the + // user's choices are unchanged — but do not leave the project + // parked at a transitional status nothing will ever revisit. + let _ = state + .projects_store + .update_status(&project_id, ProjectStatus::Stopped); + return Ok(MigrationReport { + phase: MigrationPhase::Failed, + packages_requested: Vec::new(), + packages_installed: Vec::new(), + packages_failed: Vec::new(), + paths_copied: Vec::new(), + features_restored: Vec::new(), + rollback_available: mstate.rollback_image.is_some(), + message: format!( + "The half-migrated container would not start ({}), so the update could not be resumed. Nothing was lost — your home directory and Claude config live in volumes that were never touched. Try again, or roll back.", + e + ), + }); + } } emit_progress(&app_handle, &project_id, "Resuming the interrupted migration..."); finish_migration(project, mstate, container_id, app_handle, state).await @@ -592,6 +723,12 @@ async fn finish_migration( }; mstate.report = Some(report.clone()); let _ = migration_store::save(&project_id, &mstate); + // The container really is up, so say so. Left at `Stopping` this + // project would never be re-examined: `reconcile_project_statuses` + // only looks at `Running` and `Error`. + let _ = state + .projects_store + .update_status(&project_id, ProjectStatus::Running); return Ok(report); } @@ -630,6 +767,7 @@ async fn finish_migration( &features_restored, ¬es, rollback_available, + &plan.unpreserved_data, ), phase, packages_requested, @@ -642,11 +780,13 @@ async fn finish_migration( mstate.phase = MIGRATION_PHASE_AWAITING.to_string(); mstate.report = Some(report.clone()); - migration_store::save(&project_id, &mstate)?; - + // Before the save, not after: a failed save must not leave the project + // parked at `Stopping` with a container that is plainly running. let _ = state .projects_store .update_status(&project_id, ProjectStatus::Running); + migration_store::save(&project_id, &mstate)?; + emit_progress(&app_handle, &project_id, "Migration finished."); Ok(report) } @@ -663,9 +803,28 @@ pub async fn confirm_migration( state: State<'_, AppState>, ) -> Result<(), String> { let _ = &state; + // Confirming drops the only way back. Doing that underneath a running + // migration would delete the pin it is relying on mid-flight. + let Some(_guard) = ActiveGuard::acquire(&project_id) else { + return Err( + "A container base update is running for this project right now. Wait for it to finish." + .to_string(), + ); + }; let Some(mstate) = migration_store::load(&project_id)? else { return Ok(()); }; + // An unfinished migration is not a thing that can be accepted: `:latest` + // still points at the old lineage, so dropping the pin and the record here + // would strand a container the app can no longer reason about. + if mstate.phase == MIGRATION_PHASE_INTERRUPTED + || mstate.phase == crate::models::MIGRATION_PHASE_IN_PROGRESS + { + return Err( + "This project's container base update never finished, so there is nothing to accept yet. Resume it or roll it back." + .to_string(), + ); + } if let Some(ref reference) = mstate.rollback_image { if let Err(e) = mig::untag_image(reference).await { log::warn!("Could not drop the rollback tag {}: {}", reference, e); @@ -688,6 +847,13 @@ pub async fn rollback_migration( app_handle: tauri::AppHandle, state: State<'_, AppState>, ) -> Result<(), String> { + let Some(_guard) = ActiveGuard::acquire(&project_id) else { + return Err( + "A container base update is running for this project right now. Wait for it to finish." + .to_string(), + ); + }; + let mut project = state .projects_store .get(&project_id) @@ -704,6 +870,16 @@ pub async fn rollback_migration( .to_string(), ); }; + // Check the image is really there *before* removing the container. The + // record only says a tag was created; a prune, a `docker rmi` or a failed + // tag from an older build can all leave the reference dangling, and by the + // time the container is gone there is nothing left to put back. + if mig::image_id(&rollback_ref).await?.is_none() { + return Err(format!( + "The rollback image '{}' no longer exists, so this update cannot be undone — it may have been pruned. Nothing was changed: the container is still running on the new base.", + rollback_ref + )); + } emit_progress(&app_handle, &project_id, "Rolling back..."); state.auth_bridge.stop(&project_id).await; @@ -842,6 +1018,112 @@ pub async fn reconcile_migration(project: &Project, app_handle: &tauri::AppHandl // Internals // ───────────────────────────────────────────────────────────────────────────── +/// Whether the rollback pin can be relied on for the rest of the migration. +/// +/// Pure so the decision is testable without Docker. Both failure shapes matter: +/// `docker tag` can fail outright, and it can also "succeed" against an image +/// that is not there any more (a concurrent prune), in which case reading the +/// new reference back is the only thing that catches it. Either way the answer +/// is to stop *before* `remove_container`, which is the last moment at which +/// stopping is free. +fn rollback_pin_verdict( + tag_error: Option<&str>, + resolved_image_id: Option<&str>, +) -> Result<(), String> { + if let Some(e) = tag_error { + return Err(format!( + "Could not pin a rollback image, so the update was stopped before it could become irreversible: {}. Nothing was changed — the container is still on its previous image. Free some disk space or check the Docker daemon, then try again.", + e + )); + } + match resolved_image_id { + Some(id) if !id.is_empty() => Ok(()), + _ => Err( + "The rollback image was tagged but could not be read back, so rolling this update back could not be guaranteed. The update was stopped before it could become irreversible — nothing was changed, and the container is still on its previous image." + .to_string(), + ), + } +} + +/// Undo the *bookkeeping* of a migration that gave up before the container +/// swap. Everything real is still in place: the container exists, `:latest` +/// still holds the old lineage, and the volumes were never involved. +/// +/// The status reset is the load-bearing part. `reconcile_project_statuses` +/// only ever re-examines projects it finds in `Running` or `Error`, so a +/// project abandoned at `Stopping` stays there for good — the Start button +/// stays disabled and nothing in the app ever puts it right. +async fn abandon_before_swap( + project_id: &str, + state: &State<'_, AppState>, + rollback_ref: Option<&str>, +) { + if let Some(reference) = rollback_ref { + let _ = mig::untag_image(reference).await; + } + let _ = migration_store::clear_staging(project_id); + let _ = migration_store::clear(project_id); + let _ = state + .projects_store + .update_status(project_id, ProjectStatus::Stopped); +} + +/// What a user-initiated resume is allowed to do. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum ResumeVerdict { + /// The swap landed — the container in place is the migrated one. + Proceed, + /// The container in place is the **original**. The swap never happened + /// (typically `commit_container_snapshot` failed), so replaying into it and + /// committing the result would silently stamp the unmigrated container as + /// migrated and report success. + RefuseUnswapped, + /// The record is not one a resume can act on at all. + RefuseNotResumable, +} + +/// Gate `resume_migration` on the same two signals `reconcile_migration` uses. +/// +/// `reconcile_migration` has always checked the container's +/// `triple-c.migration-state` label; the frontend-callable resume path did not, +/// and that asymmetry is the bug: clicking Migrate on a record left behind by a +/// failed commit "resumed" into the old container. +fn resume_verdict(phase: &str, container_has_in_progress_label: bool) -> ResumeVerdict { + match mig::decide_recovery(Some(phase), container_has_in_progress_label) { + Recovery::OfferResumeOrRollback => ResumeVerdict::Proceed, + Recovery::SelfHeal => ResumeVerdict::RefuseUnswapped, + Recovery::None | Recovery::OfferConfirmOrRollback => ResumeVerdict::RefuseNotResumable, + } +} + +/// Drop every host- and Docker-side trace of a migration for a project that is +/// being destroyed or reset. +/// +/// Reset and Remove both delete the snapshot image and the volumes, so a +/// surviving migration record can only describe things that no longer exist — +/// and its `:pre-migration-` tag would hold an entire snapshot image (3.8 +/// to 12.3 GB measured) alive with nothing left that could ever use it. The +/// staged payload tar is the same story at a smaller scale. +pub(crate) async fn purge_migration_artifacts(project_id: &str) { + match migration_store::load(project_id) { + Ok(Some(state)) => { + if let Some(ref reference) = state.rollback_image { + if let Err(e) = mig::untag_image(reference).await { + log::warn!("Could not drop the rollback tag {}: {}", reference, e); + } + } + } + Ok(None) => return, + Err(e) => log::warn!( + "Could not read the migration record for {} while cleaning up: {}", + project_id, + e + ), + } + let _ = migration_store::clear_staging(project_id); + let _ = migration_store::clear(project_id); +} + fn default_docker_socket() -> String { if cfg!(target_os = "windows") { "//./pipe/docker_engine".to_string() @@ -865,6 +1147,23 @@ fn human_bytes(n: u64) -> String { } } +/// Create (or truncate) a file that only the current user can read. +/// +/// On Unix the mode goes on at `open(2)` time so there is no window in which +/// the file exists world-readable. Windows has no equivalent bit and inherits +/// the directory ACL, which is already per-user under `%APPDATA%`. +async fn create_private_file(path: &std::path::Path) -> std::io::Result { + let mut opts = tokio::fs::OpenOptions::new(); + opts.write(true).create(true).truncate(true); + #[cfg(unix)] + { + // `tokio::fs::OpenOptions::mode` is the inherent unix method — no + // `OpenOptionsExt` import needed, and it is applied at `open(2)` time. + opts.mode(0o600); + } + opts.open(path).await +} + /// Tar the verbatim set out of the running container into a host file. /// /// Mirrors `download_container_backup`'s stream-an-exec's-stdout-to-a-file @@ -913,7 +1212,12 @@ async fn stage_payload( let exec = docker::exec::create_attached_exec_as(container_id, cmd, false, "root", "/").await?; let mut output = exec.output; - let file = tokio::fs::File::create(&host_path) + // 0600, not the default umask. This tar holds the whole of `/usr/local`, + // `/opt`, `/srv` and every loose `/workspace` file — private keys, tokens + // in scripts, whatever the user put there — sitting in a predictable path + // under the data directory, possibly for the length of a long migration. + // World-readable is the wrong default for that. + let file = create_private_file(&host_path) .await .map_err(|e| format!("Failed to create the staging file: {}", e))?; let mut writer = tokio::io::BufWriter::new(file); @@ -1168,9 +1472,23 @@ fn shell_quote(s: &str) -> String { format!("'{}'", s.replace('\'', r#"'\''"#)) } +/// The last ~400 bytes of a package manager's output, for a failure reason. +/// +/// The offset is a **byte** offset into arbitrary apt-get/npm output, which +/// routinely contains multi-byte UTF-8 (mirror names, `‘quoted’` package names, +/// progress glyphs). Slicing straight at `len - 400` panics the moment that +/// lands inside a character — turning a reportable per-package failure into a +/// crash of the whole migration. Walk forward to the next boundary instead. fn tail(s: &str) -> String { + const LIMIT: usize = 400; let t = s.trim(); - let start = t.len().saturating_sub(400); + if t.len() <= LIMIT { + return t.to_string(); + } + let mut start = t.len() - LIMIT; + while start < t.len() && !t.is_char_boundary(start) { + start += 1; + } t[start..].to_string() } @@ -1221,11 +1539,27 @@ async fn auto_rollback( let _ = state .projects_store .set_container_id(&project.id, Some(id)); + // Report what is actually true: a container that was recreated but + // would not start is Stopped, not Running. + let _ = state.projects_store.update_status( + &project.id, + if restored { + ProjectStatus::Running + } else { + ProjectStatus::Stopped + }, + ); + } + Err(e) => { + log::error!("Could not recreate the previous container: {}", e); + // The status is `Stopping` at this point and nothing else will + // revisit it — `reconcile_project_statuses` only re-examines + // `Running` and `Error`. There is no container, so `Stopped` is + // both true and the state the Start button needs. let _ = state .projects_store - .update_status(&project.id, ProjectStatus::Running); + .update_status(&project.id, ProjectStatus::Stopped); } - Err(e) => log::error!("Could not recreate the previous container: {}", e), } if let Some(ref reference) = mstate.rollback_image { @@ -1264,6 +1598,7 @@ fn summarize( features: &[String], notes: &[String], rollback_available: bool, + unpreserved: &[UnpreservedData], ) -> String { let mut parts: Vec = Vec::new(); parts.push(match phase { @@ -1291,6 +1626,25 @@ fn summarize( for note in notes { parts.push(note.clone()); } + // Stated in the outcome as well as the pre-flight. A user who clicked + // through the warning still has to be told, in the record that persists, + // which directories are now empty — silence here is how someone discovers + // an empty database a week later. + if !unpreserved.is_empty() { + parts.push(format!( + "Service data under {} was not carried across and cannot be restored by reinstalling the package{}.", + unpreserved + .iter() + .map(|d| d.path.as_str()) + .collect::>() + .join(", "), + if rollback_available { + " — roll back if you needed it" + } else { + "" + } + )); + } parts.push( "Your home directory and Claude config live in volumes that were never touched, so the login, skills, transcripts and scheduled tasks are exactly as they were." .to_string(), @@ -1330,6 +1684,7 @@ mod tests { &["Auth bridge tunnel (socat)".to_string()], &[], true, + &[], ); assert!(msg.contains("never touched")); assert!(msg.contains("Auth bridge tunnel (socat)")); @@ -1346,12 +1701,115 @@ mod tests { &[], &[], false, + &[], ); assert!(msg.contains("obsolete-pkg")); assert!(msg.contains("cannot be undone")); assert!(msg.contains("never touched")); } + #[test] + fn the_summary_names_the_var_data_the_migration_destroyed() { + // The pre-flight warned about it; the record that persists has to as + // well, or the only trace of an emptied database is a dialog the user + // clicked through minutes ago. + let msg = summarize( + MigrationPhase::Succeeded, + &["postgresql".to_string()], + &[], + &[], + &[], + &[], + true, + &[UnpreservedData { + path: "/var/lib/postgresql".to_string(), + bytes: 41_000_000, + file_count: 912, + }], + ); + assert!(msg.contains("/var/lib/postgresql")); + assert!(msg.contains("cannot be restored by reinstalling the package")); + } + + // ── The rollback pin is not best-effort ───────────────────────────────── + + #[test] + fn a_migration_refuses_to_continue_without_a_verified_rollback_pin() { + // By this point the commit is the only copy of the old system layer and + // `remove_container` is next, so anything short of a tag that reads + // back has to stop the migration rather than warn and carry on. + assert!(rollback_pin_verdict(None, Some("sha256:abc")).is_ok()); + + let err = rollback_pin_verdict(Some("no space left on device"), None).unwrap_err(); + assert!(err.contains("no space left on device")); + assert!(err.contains("Nothing was changed")); + + // Tagged, but the reference does not resolve — a concurrent prune, or a + // daemon that accepted the call and did nothing. + let err = rollback_pin_verdict(None, None).unwrap_err(); + assert!(err.contains("could not be read back")); + assert!(err.contains("nothing was changed")); + assert!(rollback_pin_verdict(None, Some("")).is_err()); + } + + // ── Resume must prove the swap actually happened ──────────────────────── + + #[test] + fn resuming_refuses_when_the_container_is_still_the_unmigrated_one() { + use crate::models::MIGRATION_PHASE_IN_PROGRESS; + + // The label is the only evidence the swap landed. Without it the + // container in place is the original: replaying into it and committing + // would stamp an unmigrated container as migrated and report success. + assert_eq!( + resume_verdict(MIGRATION_PHASE_INTERRUPTED, false), + ResumeVerdict::RefuseUnswapped + ); + assert_eq!( + resume_verdict(MIGRATION_PHASE_IN_PROGRESS, false), + ResumeVerdict::RefuseUnswapped + ); + assert_eq!( + resume_verdict(MIGRATION_PHASE_INTERRUPTED, true), + ResumeVerdict::Proceed + ); + assert_eq!( + resume_verdict(MIGRATION_PHASE_IN_PROGRESS, true), + ResumeVerdict::Proceed + ); + // A finished migration is a confirm/rollback decision, never a resume. + assert_eq!( + resume_verdict(MIGRATION_PHASE_AWAITING, true), + ResumeVerdict::RefuseNotResumable + ); + assert_eq!( + resume_verdict("who-knows", true), + ResumeVerdict::RefuseNotResumable + ); + } + + // ── tail() ────────────────────────────────────────────────────────────── + + #[test] + fn the_failure_tail_never_splits_a_multibyte_character() { + // apt-get quotes package names with ‘…’ and mirrors have non-ASCII + // names, so the 400-byte cut lands mid-character routinely. The old + // slice panicked, turning one reportable package failure into a crash + // of the whole migration. + for pad in 0..8 { + let s = format!("{}{}", "x".repeat(pad), "é".repeat(400)); + let t = tail(&s); + assert!(s.ends_with(&t), "tail must be a suffix of the input"); + assert!(t.len() <= 400 + 1); + } + // Short input is returned whole, trimmed. + assert_eq!(tail(" apt-get exited 100 "), "apt-get exited 100"); + assert_eq!(tail(""), ""); + // A single character wider than the window is still returned intact. + let wide = "🐳".repeat(200); + assert!(wide.ends_with(&tail(&wide))); + } + #[test] fn a_live_migration_is_distinguishable_from_a_crashed_one() { // The whole point: reconcile cannot tell them apart from the outside, diff --git a/app/src-tauri/src/commands/project_commands.rs b/app/src-tauri/src/commands/project_commands.rs index 4d41728..4d5d3f3 100644 --- a/app/src-tauri/src/commands/project_commands.rs +++ b/app/src-tauri/src/commands/project_commands.rs @@ -145,6 +145,11 @@ pub async fn remove_project( // before the container (and the project record) go away. state.auth_bridge.stop(&project_id).await; + // A migration record outliving its project leaks a state file, a staged + // payload tar that can run to several GB, and a `:pre-migration-` tag + // holding an entire snapshot image that nothing will ever reference again. + crate::commands::migration_commands::purge_migration_artifacts(&project_id).await; + // Stop and remove container if it exists if let Some(ref project) = state.projects_store.get(&project_id) { if let Some(ref container_id) = project.container_id { @@ -215,6 +220,20 @@ pub async fn start_project_container( app_handle: tauri::AppHandle, state: State<'_, AppState>, ) -> Result { + // A migration removes the container and creates its replacement moments + // later. Starting in that window finds no container, creates a second one + // under the same name, and the migration's own create then fails on the + // name conflict — which sends it into an auto-rollback that also cannot + // create. The UI already refuses (`canMigrate` gates on the container being + // stopped and no run being in flight); this is the same gate on the side + // that actually owns the invariant. + if crate::commands::migration_commands::is_migrating(&project_id) { + return Err( + "A container base update is running for this project. Wait for it to finish, then start the project." + .to_string(), + ); + } + let mut project = state .projects_store .get(&project_id) @@ -536,11 +555,28 @@ pub async fn rebuild_project_container( app_handle: tauri::AppHandle, state: State<'_, AppState>, ) -> Result { + // Reset deletes both volumes and the snapshot image. Doing that while a + // migration is mid-flight pulls the ground out from under it and leaves an + // orphan migration record pointing at images that no longer exist. + if crate::commands::migration_commands::is_migrating(&project_id) { + return Err( + "A container base update is running for this project. Wait for it to finish before resetting." + .to_string(), + ); + } + let project = state .projects_store .get(&project_id) .ok_or_else(|| format!("Project {} not found", project_id))?; + // Reset supersedes any migration decision that was still pending: the + // snapshot image and both volumes are about to go, so a surviving record + // could only describe things that no longer exist — while its + // `:pre-migration-` tag held a whole snapshot image (multiple GB) alive + // with nothing left that could ever use it. + crate::commands::migration_commands::purge_migration_artifacts(&project_id).await; + // The bridge is bound to the container that is about to be destroyed; // `start_project_container` below re-arms it against the new one. state.auth_bridge.stop(&project_id).await; @@ -588,7 +624,27 @@ pub async fn reconcile_project_statuses( } for project in &projects { - if project.status != ProjectStatus::Running && project.status != ProjectStatus::Error { + // `Starting` and `Stopping` are in here as a backstop, not because + // anything is expected to leave a project in one. They are transitional + // states owned by an in-flight command, so a project still wearing one + // is a project whose command died — a crash mid-start, or a migration + // that bailed out between the stop and the swap. Skipping them, as this + // loop used to, meant nothing in the app ever put such a project right: + // it sat at "Stopping" with the Start button disabled, permanently. + // Docker is the authority either way, so the check below is correct for + // all four. + if !matches!( + project.status, + ProjectStatus::Running + | ProjectStatus::Error + | ProjectStatus::Starting + | ProjectStatus::Stopping + ) { + continue; + } + // ...but never for a project this process is actively migrating: the + // container is legitimately absent for part of that run. + if crate::commands::migration_commands::is_migrating(&project.id) { continue; } diff --git a/app/src-tauri/src/commands/settings_commands.rs b/app/src-tauri/src/commands/settings_commands.rs index e83bd6c..8a9a7fe 100644 --- a/app/src-tauri/src/commands/settings_commands.rs +++ b/app/src-tauri/src/commands/settings_commands.rs @@ -1,6 +1,7 @@ use tauri::State; use crate::docker; +use crate::models::gateway_settings::GatewaySettings; use crate::models::AppSettings; use crate::AppState; @@ -14,7 +15,94 @@ pub async fn update_settings( settings: AppSettings, state: State<'_, AppState>, ) -> Result { - state.settings_store.update(settings) + let before = state.settings_store.get(); + let saved = state.settings_store.update(settings)?; + + // Persisting a setting is not the same as applying it. The gateway is the + // one settings block that owns a *container*, so a saved change that the + // running container doesn't reflect is a live desync, not a preference. + reconcile_gateway(&before.gateway, &saved.gateway).await; + + Ok(saved) +} + +/// What a settings save has to do to the gateway container to stay honest. +/// +/// Kept separate from the IPC command and expressed over plain settings so the +/// decision is testable without Docker. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum GatewayAction { + /// Nothing to do. + None, + /// The gateway is off — a container left running must be stopped. + StopIfRunning, + /// The published shape moved. A *running* container is now serving on the + /// old binding while status reports the new one, so it has to be recreated. + RestartIfRunning, +} + +/// Whether the container's published shape (as opposed to a purely cosmetic +/// field) changed. Provider, models and base URL all change the rendered +/// LiteLLM config, which is only read at boot. +fn gateway_shape_changed(before: &GatewaySettings, after: &GatewaySettings) -> bool { + before.port != after.port + || before.provider.trim() != after.provider.trim() + || before.api_base.as_deref().unwrap_or("").trim() + != after.api_base.as_deref().unwrap_or("").trim() + || before.valid_models() != after.valid_models() +} + +fn gateway_action(before: &GatewaySettings, after: &GatewaySettings) -> GatewayAction { + if !after.enabled { + // Includes the case where it was already disabled: a container found + // running while the feature is off should not stay up. + return GatewayAction::StopIfRunning; + } + if gateway_shape_changed(before, after) { + return GatewayAction::RestartIfRunning; + } + GatewayAction::None +} + +/// Apply [`gateway_action`]. Never fails the settings save: the settings *are* +/// saved by this point, and a Docker hiccup must not make the UI think they +/// weren't. Both paths are no-ops when no container exists, so this stays cheap +/// on the overwhelmingly common "gateway not in use" save. +async fn reconcile_gateway(before: &GatewaySettings, after: &GatewaySettings) { + let action = gateway_action(before, after); + if action == GatewayAction::None { + return; + } + + let (exists, running) = match docker::gateway::gateway_container_presence().await { + Ok(presence) => presence, + // Docker down: there is nothing running to desync from. + Err(e) => { + log::debug!("Gateway reconcile skipped ({})", e); + return; + } + }; + if !exists || !running { + return; + } + + match action { + GatewayAction::StopIfRunning => { + log::info!("Model gateway disabled in settings — stopping the container"); + if let Err(e) = docker::gateway::stop_gateway_container().await { + log::error!("Failed to stop the model gateway after it was disabled: {}", e); + } + } + GatewayAction::RestartIfRunning => { + log::info!("Model gateway settings changed — recreating the container"); + // The fingerprint no longer matches, so this stops, removes and + // recreates with the new port/config in one step. + if let Err(e) = docker::gateway::ensure_gateway_running(after).await { + log::error!("Failed to apply the new model gateway settings: {}", e); + } + } + GatewayAction::None => unreachable!(), + } } #[tauri::command] @@ -115,3 +203,96 @@ pub async fn list_aws_profiles() -> Result, String> { Ok(profiles) } + +#[cfg(test)] +mod tests { + use super::*; + use crate::models::gateway_settings::GatewayModel; + + fn enabled_gateway() -> GatewaySettings { + GatewaySettings { + enabled: true, + port: 4000, + provider: "openai".to_string(), + api_base: None, + models: vec![GatewayModel { + name: "gpt-5.1".to_string(), + model_id: "gpt-5.1".to_string(), + }], + } + } + + #[test] + fn disabling_the_gateway_stops_it() { + // The bug: turning the toggle off only persisted `enabled: false` and + // hid the Stop button, leaving a container serving with no way to stop + // it. + let before = enabled_gateway(); + let mut after = before.clone(); + after.enabled = false; + assert_eq!(gateway_action(&before, &after), GatewayAction::StopIfRunning); + // Still true when it was already off — a stray running container is + // still a container that shouldn't be up. + assert_eq!(gateway_action(&after, &after), GatewayAction::StopIfRunning); + } + + #[test] + fn changing_the_port_reconciles_the_container() { + // Otherwise status reports the new port while the container keeps the + // old binding, and every project gets a broken ANTHROPIC_BASE_URL. + let before = enabled_gateway(); + let mut after = before.clone(); + after.port = 4100; + assert_eq!( + gateway_action(&before, &after), + GatewayAction::RestartIfRunning + ); + } + + #[test] + fn config_changes_that_only_take_effect_at_boot_reconcile_too() { + let before = enabled_gateway(); + + let mut provider = before.clone(); + provider.provider = "groq".to_string(); + assert_eq!( + gateway_action(&before, &provider), + GatewayAction::RestartIfRunning + ); + + let mut api_base = before.clone(); + api_base.api_base = Some("https://example.test/v1".to_string()); + assert_eq!( + gateway_action(&before, &api_base), + GatewayAction::RestartIfRunning + ); + + let mut models = before.clone(); + models.models[0].model_id = "gpt-4.1".to_string(); + assert_eq!( + gateway_action(&before, &models), + GatewayAction::RestartIfRunning + ); + } + + #[test] + fn saving_an_unchanged_or_half_typed_gateway_touches_nothing() { + let before = enabled_gateway(); + assert_eq!(gateway_action(&before, &before), GatewayAction::None); + + // Whitespace-only edits don't reach the rendered config. + let mut trimmed = before.clone(); + trimmed.provider = " openai ".to_string(); + trimmed.api_base = Some(" ".to_string()); + assert_eq!(gateway_action(&before, &trimmed), GatewayAction::None); + + // A half-filled model row is skipped when rendering, so it must not + // bounce a live container either. + let mut half_typed = before.clone(); + half_typed.models.push(GatewayModel { + name: "gpt".to_string(), + model_id: String::new(), + }); + assert_eq!(gateway_action(&before, &half_typed), GatewayAction::None); + } +} diff --git a/app/src-tauri/src/docker/container.rs b/app/src-tauri/src/docker/container.rs index c444f2e..8e8c558 100644 --- a/app/src-tauri/src/docker/container.rs +++ b/app/src-tauri/src/docker/container.rs @@ -176,6 +176,39 @@ fn build_claude_instructions( /// stale-value neutralization pass can never disagree about the spelling. pub const CLAUDE_OAUTH_TOKEN_ENV: &str = "CLAUDE_CODE_OAUTH_TOKEN"; +/// Every managed env var whose *value* is a credential. +/// +/// These are the names that must never survive into a snapshot image. A +/// container's env is visible to `docker inspect`, which is bad but bounded — +/// the container is recreated whenever the credential rotates, and removed +/// with the project. An **image**'s env is neither: `docker commit` copies the +/// container's full environment into `triple-c-snapshot-{id}:latest`, that tag +/// outlives every container built from it, and nothing about deleting a +/// keychain entry touches it. A ~1-year OAuth token baked in that way is +/// readable by `docker image inspect` for as long as the image exists, long +/// after the user has clicked Revoke. +/// +/// [`commit_container_snapshot`] therefore blanks all of them at commit time, +/// and [`scrub_secrets_from_snapshots`] rewrites images committed before that +/// was true. +/// +/// Blanked rather than omitted, because Docker's commit endpoint *merges* the +/// supplied config over the container's rather than replacing it: a key left +/// out of the list is inherited with its original value, so `KEY=` is the only +/// way to clear one. That matches how `MANAGED_AUTH_KEYS` already works at +/// create time, and Claude Code, the AWS SDK and git all treat an empty value +/// as unset. +pub const SECRET_ENV_KEYS: &[&str] = &[ + CLAUDE_OAUTH_TOKEN_ENV, + "ANTHROPIC_API_KEY", + "ANTHROPIC_AUTH_TOKEN", + "AWS_ACCESS_KEY_ID", + "AWS_SECRET_ACCESS_KEY", + "AWS_SESSION_TOKEN", + "AWS_BEARER_TOKEN_BEDROCK", + "GIT_TOKEN", +]; + /// Env var name prefixes Triple-C manages itself; users cannot set these by hand. const RESERVED_ENV_PREFIXES: &[&str] = &["ANTHROPIC_", "AWS_", "GIT_", "HOST_", "TRIPLE_C_"]; @@ -998,11 +1031,16 @@ pub async fn create_container( // ── Neutralize stale backend auth env vars ────────────────────────────── // When a project switches backends (e.g. Bedrock → Anthropic) the container - // is recreated *from a snapshot image* committed off the previous container. - // `docker commit` always bakes the previous container's full ENV into that - // image, and the commit API cannot strip it. So any auth var set under the - // old backend (e.g. CLAUDE_CODE_USE_BEDROCK=1, AWS_*) survives in the image - // ENV and stays active unless we explicitly override it at create time. + // is recreated *from a snapshot image* committed off the previous container, + // and `docker commit` copies that container's full ENV into the image. So + // any auth var set under the old backend (e.g. CLAUDE_CODE_USE_BEDROCK=1, + // AWS_PROFILE, a model alias) survives in the image ENV and stays active + // unless we explicitly override it at create time. + // + // This pass is about *staleness*, not secrecy. It fixes the container it is + // building and does nothing to the image, so it is not — and never was — + // a defence against a credential baked into a snapshot. That is + // `commit_container_snapshot`'s job, via SECRET_ENV_KEYS. // Create-time env takes precedence over image ENV, so we set every managed // auth key the *current* backend did NOT set to an empty value, clearing the // stale baked-in one. @@ -1514,14 +1552,29 @@ chmod 600 "$HOME/.aws/credentials""#; /// changes (apt/pip/npm installs, ~/.claude.json, etc.) survive container /// removal. /// -/// NOTE: `docker commit` always bakes the *running container's* full ENV into -/// the resulting image — passing an empty Config here does NOT strip it, and -/// the commit API gives no way to remove env vars. As a result auth vars (e.g. -/// CLAUDE_CODE_USE_BEDROCK, AWS_*, CLAUDE_CODE_OAUTH_TOKEN) are present in this -/// snapshot image's ENV — this image is local and per-project, never pushed. -/// `create_container` defends against that by explicitly overriding every -/// managed auth key for the active backend (see MANAGED_AUTH_KEYS), so a -/// backend switch does not inherit the previous backend's stale credentials. +/// ## Why this passes a Config instead of `Default::default()` +/// +/// `docker commit` bakes the *running container's* full ENV into the resulting +/// image. An earlier version of this function passed an empty `Config` and a +/// comment asserting that the API "gives no way to remove env vars", with +/// `MANAGED_AUTH_KEYS` cited as the defence. That was wrong on both counts. +/// +/// `MANAGED_AUTH_KEYS` defends the *next container* — it overrides the image's +/// stale value at create time — and does nothing whatsoever about the value +/// sitting in the image. So `docker image inspect triple-c-snapshot-:latest +/// --format '{{json .Config.Env}}'` returned the shared ~1-year OAuth token, +/// and kept returning it after the user revoked the token, because +/// `clear_claude_token` only deletes a keychain entry. +/// +/// And the API does allow it. Verified against Engine 29.6: the config in the +/// commit body is **merged over** the container's config, key by key for `Env`, +/// with unmentioned fields (`Cmd`, `WorkingDir`, `Labels`, …) inherited +/// untouched. A key cannot be *dropped*, but it can be set — so every name in +/// [`SECRET_ENV_KEYS`] is committed as `KEY=`, which is exactly the "empty +/// means unset" convention the rest of the auth plumbing already uses. +/// +/// Non-secret env (PATH, TZ, model aliases, instructions) is inherited as +/// before, so nothing about the snapshot's behaviour changes. pub async fn commit_container_snapshot(container_id: &str, project: &Project) -> Result<(), String> { let docker = get_docker()?; let image_name = get_snapshot_image_name(project); @@ -1540,8 +1593,8 @@ pub async fn commit_container_snapshot(container_id: &str, project: &Project) -> ..Default::default() }; - // Empty config — no env vars / cmd baked in let config = Config:: { + env: Some(blanked_secret_env()), ..Default::default() }; @@ -1554,6 +1607,258 @@ pub async fn commit_container_snapshot(container_id: &str, project: &Project) -> Ok(()) } +/// `KEY=` for every name in [`SECRET_ENV_KEYS`] — the env override handed to +/// `docker commit` so no credential value reaches an image. +fn blanked_secret_env() -> Vec { + SECRET_ENV_KEYS + .iter() + .map(|key| format!("{}=", key)) + .collect() +} + +/// Whether `env` (an image's `Config.Env`) holds a non-empty value for any +/// name in [`SECRET_ENV_KEYS`]. +fn env_holds_a_secret(env: &[String]) -> bool { + env.iter().any(|entry| { + let Some((key, value)) = entry.split_once('=') else { + return false; + }; + !value.is_empty() && SECRET_ENV_KEYS.contains(&key) + }) +} + +/// Outcome of [`scrub_secrets_from_snapshots`], so callers can tell the user +/// what actually happened rather than guessing. +#[derive(Debug, Default, Clone, serde::Serialize)] +pub struct SnapshotScrubReport { + /// Snapshot images that were found to hold a credential and were rewritten. + pub scrubbed: Vec, + /// Snapshot images that hold a credential and could **not** be rewritten, + /// each with the reason. A non-empty list means the tag every future + /// container is built from still carries the credential. + pub failed: Vec<(String, String)>, + /// Tags that *were* rewritten, but whose superseded image object could not + /// be deleted — almost always because a container is still running off it. + /// + /// Much weaker than `failed`, and normal rather than exceptional. The tag + /// is clean, so nothing new is built with the credential; what remains is + /// an untagged image whose config is still readable by id, for exactly as + /// long as the container using it survives — and that container already + /// holds the same value in its own env, so it is not a new exposure. The + /// rotation-id label mismatch recreates it on the next start, after which + /// an image prune collects the leftover. + pub superseded_retained: Vec, + /// Set when the image list itself could not be read (Docker not running, + /// no permission). Nothing was scrubbed and nothing is known. + pub unavailable: Option, +} + +impl SnapshotScrubReport { + /// True when a credential is known to still be reachable through something + /// that will keep being used — a tag, or an unknown state. + pub fn left_something_behind(&self) -> bool { + !self.failed.is_empty() || self.unavailable.is_some() + } +} + +/// Rewrite every `triple-c-snapshot-*` image whose ENV still carries a +/// credential, blanking the values in place. +/// +/// Revoking a shared token has to mean something. Deleting the keychain entry +/// stops *new* containers getting it, but images committed before +/// [`commit_container_snapshot`] learned to strip secrets still have the token +/// in their config, and those images are the ones every future container of +/// that project is built from. This is the cleanup for them. +/// +/// Mechanics: create (do not start) a throwaway container from the image, then +/// commit it straight back over the same tag with the secret keys blanked. The +/// new image shares every layer with the old one, so this costs no meaningful +/// disk and preserves the project's installed packages exactly. The superseded +/// image is then removed by id; if Docker refuses (some storage drivers will +/// not delete an image that is a parent of another), the report says so rather +/// than pretending the secret is gone. +/// +/// Never fails the caller: an unreachable Docker engine is reported in the +/// return value, because the keychain deletion that precedes it must still +/// stand. +pub async fn scrub_secrets_from_snapshots() -> SnapshotScrubReport { + use bollard::image::ListImagesOptions; + + let mut report = SnapshotScrubReport::default(); + + let docker = match get_docker() { + Ok(d) => d, + Err(e) => { + report.unavailable = Some(e); + return report; + } + }; + + let filters: HashMap> = HashMap::from([( + "reference".to_string(), + vec!["triple-c-snapshot-*".to_string()], + )]); + let images = match docker + .list_images(Some(ListImagesOptions { + filters, + ..Default::default() + })) + .await + { + Ok(images) => images, + Err(e) => { + report.unavailable = Some(format!("Could not list snapshot images: {}", e)); + return report; + } + }; + + for summary in images { + // `list_images` does not return Config, so inspect each candidate. + let details = match docker.inspect_image(&summary.id).await { + Ok(d) => d, + Err(e) => { + report + .failed + .push((summary.id.clone(), format!("could not inspect: {}", e))); + continue; + } + }; + let env = details + .config + .as_ref() + .and_then(|c| c.env.clone()) + .unwrap_or_default(); + if !env_holds_a_secret(&env) { + continue; + } + + if summary.repo_tags.is_empty() { + // The reference filter should make this impossible; if it happens, + // say so rather than silently leaving a credential in place. + report.failed.push(( + summary.id.clone(), + "an untagged snapshot image holds a credential and cannot be rewritten" + .to_string(), + )); + continue; + } + + // Rewrite every tag this image answers to, so an old tag cannot keep + // serving the un-scrubbed config. + let mut all_tags_rewritten = true; + for tag in summary.repo_tags.iter() { + let (repo, tag_part) = match tag.rsplit_once(':') { + Some((r, t)) => (r.to_string(), t.to_string()), + None => (tag.clone(), "latest".to_string()), + }; + if let Err(e) = rewrite_image_without_secrets(&docker, tag, &repo, &tag_part).await { + all_tags_rewritten = false; + report.failed.push((tag.clone(), e)); + } else { + report.scrubbed.push(tag.clone()); + } + } + + // Drop the superseded image so its config stops being inspectable. + // Best effort by design — see the doc comment. + if all_tags_rewritten { + if let Err(e) = docker + .remove_image( + &summary.id, + Some(RemoveImageOptions { + force: false, + noprune: false, + }), + None, + ) + .await + { + // Expected whenever the project's container is still around: + // Docker will not delete an image a container was created + // from. Not a failure of the scrub — see `superseded_retained`. + log::info!( + "Scrubbed snapshot {} but kept the superseded image {}: {}", + summary.repo_tags.join(", "), + summary.id, + e + ); + report + .superseded_retained + .push(summary.repo_tags.join(", ")); + } + } + } + + report +} + +/// Commit `source_image` back over `repo:tag` with [`SECRET_ENV_KEYS`] blanked. +async fn rewrite_image_without_secrets( + docker: &bollard::Docker, + source_image: &str, + repo: &str, + tag: &str, +) -> Result<(), String> { + let scratch_name = format!("triple-c-scrub-{}", uuid::Uuid::new_v4().simple()); + + let created = docker + .create_container( + Some(CreateContainerOptions { + name: scratch_name.clone(), + ..Default::default() + }), + Config:: { + image: Some(source_image.to_string()), + // Deliberately nothing else. The container is never started; + // its only job is to be a config to commit from, and every + // field left unset here is inherited from the image and + // inherited back out by the commit. Setting `cmd` to a + // placeholder — the obvious way to satisfy an image with no + // CMD — would write that placeholder into the rewritten + // snapshot. The base image has an ENTRYPOINT, so `create` + // needs no command; an image with neither fails here and is + // reported rather than silently mangled. + ..Default::default() + }, + ) + .await + .map_err(|e| format!("could not create a scratch container: {}", e))?; + + let commit = docker + .commit_container( + CommitContainerOptions { + container: created.id.clone(), + repo: repo.to_string(), + tag: tag.to_string(), + // Nothing is running; pausing a created container is an error. + pause: false, + ..Default::default() + }, + Config:: { + env: Some(blanked_secret_env()), + ..Default::default() + }, + ) + .await + .map_err(|e| format!("could not re-commit without the credential: {}", e)); + + // Remove the scratch container whatever happened to the commit. + if let Err(e) = docker + .remove_container( + &created.id, + Some(RemoveContainerOptions { + force: true, + ..Default::default() + }), + ) + .await + { + log::warn!("Could not remove scratch container {}: {}", scratch_name, e); + } + + commit.map(|_| ()) +} + /// Remove the snapshot image for a project (used on Reset / project removal). pub async fn remove_snapshot_image(project: &Project) -> Result<(), String> { let docker = get_docker()?; @@ -1806,8 +2111,10 @@ pub async fn container_needs_recreation( // re-acquired (rotated), revoked, or opted out of. Both "" means no token // is in play, which is also what a container predating this feature reports // — so existing installs are not recreated until a token actually exists. - // Recreation is the only way to change container env, and it is also what - // makes MANAGED_AUTH_KEYS blank a revoked token out of the snapshot image. + // Recreation is the only way to change a container's env, so it is the only + // way a revoked token stops being live in one. It does *not* clean the + // snapshot image — `clear_claude_token` calls + // `scrub_secrets_from_snapshots` for that. let expected_claude_token = claude_token_label(project); let container_claude_token = get_label("triple-c.claude-token-version").unwrap_or_default(); if container_claude_token != expected_claude_token { @@ -2195,4 +2502,84 @@ mod tests { // The new per-backend haiku override also defaults cleanly. assert!(p.ollama_config.unwrap().haiku_model_id.is_none()); } + // ── Snapshot secret stripping ──────────────────────────────────────── + // The bug these cover: `docker commit` copies the container's whole + // environment into `triple-c-snapshot-{id}:latest`, that image outlives + // every container built from it, and revoking the shared token used to + // touch only the keychain — so `docker image inspect` kept returning a + // live ~1-year OAuth credential indefinitely. + + #[test] + fn the_commit_override_blanks_every_credential_bearing_key() { + let blanked = blanked_secret_env(); + assert_eq!(blanked.len(), SECRET_ENV_KEYS.len()); + for key in SECRET_ENV_KEYS { + assert!( + blanked.contains(&format!("{}=", key)), + "{} is not blanked at commit time", + key + ); + } + // Blanked, never omitted: the commit endpoint merges this over the + // container's env key by key, so a name left out is inherited with its + // original value. + assert!(blanked.iter().all(|e| e.ends_with('='))); + } + + #[test] + fn the_shared_claude_token_is_one_of_the_stripped_keys() { + assert!(SECRET_ENV_KEYS.contains(&CLAUDE_OAUTH_TOKEN_ENV)); + } + + #[test] + fn an_image_holding_a_credential_is_detected() { + let env = vec![ + "PATH=/usr/bin".to_string(), + format!("{}=sk-ant-oat01-{}", CLAUDE_OAUTH_TOKEN_ENV, "x".repeat(90)), + ]; + assert!(env_holds_a_secret(&env)); + } + + #[test] + fn a_blanked_or_secret_free_image_is_left_alone() { + // Already scrubbed. + assert!(!env_holds_a_secret(&blanked_secret_env())); + // Never had one. + assert!(!env_holds_a_secret(&[ + "PATH=/usr/bin".to_string(), + "TZ=UTC".to_string(), + format!("{}=claude-sonnet-4-5", ANTHROPIC_DEFAULT_SONNET_MODEL), + ])); + // A non-secret var whose *name* merely contains a secret name. + assert!(!env_holds_a_secret(&[ + format!("MY_{}=not-a-secret", CLAUDE_OAUTH_TOKEN_ENV) + ])); + } + + #[test] + fn a_value_containing_an_equals_sign_is_still_recognised() { + let env = vec!["ANTHROPIC_AUTH_TOKEN=abc=def==".to_string()]; + assert!(env_holds_a_secret(&env)); + } + + #[test] + fn the_scrub_report_only_claims_success_when_nothing_is_left() { + let clean = SnapshotScrubReport { + scrubbed: vec!["triple-c-snapshot-a:latest".to_string()], + ..Default::default() + }; + assert!(!clean.left_something_behind()); + + let partial = SnapshotScrubReport { + failed: vec![("triple-c-snapshot-b:latest".to_string(), "nope".to_string())], + ..Default::default() + }; + assert!(partial.left_something_behind()); + + let blind = SnapshotScrubReport { + unavailable: Some("Docker is not running".to_string()), + ..Default::default() + }; + assert!(blind.left_something_behind()); + } } diff --git a/app/src-tauri/src/docker/exec.rs b/app/src-tauri/src/docker/exec.rs index 309b3dd..9095150 100644 --- a/app/src-tauri/src/docker/exec.rs +++ b/app/src-tauri/src/docker/exec.rs @@ -432,11 +432,51 @@ pub async fn upload_bytes_to_container( Ok(format!("{}/{}", dest_dir.trim_end_matches('/'), file_name)) } +/// Ceiling on how much container output a one-shot exec will buffer into the +/// host process. +/// +/// Every `exec_oneshot*` call reads the whole stream into a `String` before any +/// caller sees a byte, and what it is reading is *container-controlled* — the +/// scheduler notifications reader `cat`s up to 50 files with no size cap, and +/// the auth bridge reads `/proc/net/tcp` every two seconds. Neither has an +/// upstream bound, so this is where the bound goes. Generous enough that no +/// legitimate reader (the largest is a package manifest of a full image) comes +/// close. +pub const MAX_ONESHOT_OUTPUT: usize = 8 * 1024 * 1024; + +/// The auth bridge's per-tick budget. It reads two procfs files whose rows are +/// ~150 bytes; a real container has tens of listeners, and the parser only ever +/// yields at most one entry per port number. 1 MiB is thousands of rows — far +/// past anything genuine, far short of a problem. +pub const PROC_NET_OUTPUT_LIMIT: usize = 1024 * 1024; + +/// Append to `buf` while it stays inside `limit`. Returns `false` once the +/// limit is exceeded, at which point the caller must stop reading. +fn push_capped(buf: &mut String, chunk: &str, limit: usize) -> bool { + if buf.len() + chunk.len() > limit { + return false; + } + buf.push_str(chunk); + true +} + /// Run a one-shot (non-interactive) exec command in a container and collect stdout. pub async fn exec_oneshot(container_id: &str, cmd: Vec) -> Result { exec_oneshot_env(container_id, cmd, Vec::new()).await } +/// [`exec_oneshot`] with a caller-chosen output ceiling, for readers whose +/// input is fully container-controlled and whose legitimate output is small. +pub async fn exec_oneshot_limited( + container_id: &str, + cmd: Vec, + limit: usize, +) -> Result { + exec_oneshot_inner(container_id, "claude", cmd, Vec::new(), limit) + .await + .map(|(output, _)| output) +} + /// Like `exec_oneshot`, but passes additional environment variables to the exec /// process. Secrets passed this way live only in `/proc//environ` (readable /// by the same user / root) rather than in the process argv, so they are not @@ -477,6 +517,16 @@ pub async fn exec_oneshot_as( user: &str, cmd: Vec, env: Vec, +) -> Result<(String, i64), String> { + exec_oneshot_inner(container_id, user, cmd, env, MAX_ONESHOT_OUTPUT).await +} + +async fn exec_oneshot_inner( + container_id: &str, + user: &str, + cmd: Vec, + env: Vec, + limit: usize, ) -> Result<(String, i64), String> { let docker = get_docker()?; @@ -505,7 +555,19 @@ pub async fn exec_oneshot_as( StartExecResults::Attached { mut output, .. } => { while let Some(msg) = output.next().await { match msg { - Ok(data) => combined.push_str(&String::from_utf8_lossy(&data.into_bytes())), + Ok(data) => { + let chunk = String::from_utf8_lossy(&data.into_bytes()).into_owned(); + if !push_capped(&mut combined, &chunk, limit) { + // Stop reading rather than truncate silently: every + // caller parses this output, and a half-read + // manifest or JSON array is worse than an error. + // Dropping `output` kills the exec's stream. + return Err(format!( + "Command output exceeded {} bytes and was abandoned", + limit + )); + } + } Err(e) => return Err(format!("Exec output error: {}", e)), } } @@ -540,3 +602,42 @@ pub async fn wait_for_exec_exit(exec_id: &str) -> Option { } None } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn output_under_the_limit_is_buffered_whole() { + let mut buf = String::new(); + assert!(push_capped(&mut buf, "hello ", 16)); + assert!(push_capped(&mut buf, "world", 16)); + assert_eq!(buf, "hello world"); + } + + #[test] + fn output_over_the_limit_is_refused_rather_than_truncated() { + // The abandoned chunk must not land in the buffer either: a caller that + // ignored the error would otherwise parse a half-read document. + let mut buf = String::new(); + assert!(push_capped(&mut buf, "0123456789", 12)); + assert!(!push_capped(&mut buf, "0123456789", 12)); + assert_eq!(buf, "0123456789"); + } + + #[test] + fn a_single_oversized_chunk_is_refused() { + let mut buf = String::new(); + assert!(!push_capped(&mut buf, "0123456789", 4)); + assert!(buf.is_empty()); + } + + #[test] + fn the_bridge_budget_is_far_smaller_than_the_general_one() { + // The auth bridge re-reads container-controlled procfs every 2s, so it + // gets a tighter ceiling than one-shot readers that run on demand. + assert!(PROC_NET_OUTPUT_LIMIT < MAX_ONESHOT_OUTPUT); + // …but still comfortably above a genuine /proc/net/tcp{,6} pair. + assert!(PROC_NET_OUTPUT_LIMIT > 100 * 150); + } +} diff --git a/app/src-tauri/src/docker/gateway.rs b/app/src-tauri/src/docker/gateway.rs index 02b9217..c32d60a 100644 --- a/app/src-tauri/src/docker/gateway.rs +++ b/app/src-tauri/src/docker/gateway.rs @@ -7,11 +7,14 @@ //! //! Two things differ from STT, both deliberate: //! -//! * **The port is published on `0.0.0.0`, not `127.0.0.1`.** STT is consumed -//! by the Tauri host process, so loopback is enough. The gateway is consumed -//! by *project containers*, which sit on Docker's default bridge and reach -//! the host through the bridge gateway — a loopback-only bind is invisible to -//! them. See [`gateway_base_url`]. +//! * **The published host address is *detected*, not fixed.** STT is consumed +//! by the Tauri host process, so loopback is always enough. The gateway is +//! consumed by *project containers*, and how a container reaches the host +//! depends on the engine — so the bind address does too. See +//! [`GatewayBinding`]. It is never `0.0.0.0`: the config behind this port +//! holds a billed provider key, and Docker's published-port rules land in the +//! `DOCKER` iptables chain *ahead* of a host firewall, so a wildcard bind is +//! genuinely LAN-reachable even with `ufw` enabled. //! * **The rendered config is uploaded into the container over the Docker //! API** rather than passed as env. It holds the provider API key, and both //! env vars and labels are readable by anything on the host via @@ -23,10 +26,14 @@ use bollard::container::{ }; use bollard::image::BuildImageOptions; use bollard::models::{HostConfig, Mount, MountTypeEnum, PortBinding}; +use bollard::network::InspectNetworkOptions; +use bollard::Docker; use futures_util::StreamExt; use sha2::{Digest, Sha256}; use std::collections::HashMap; use std::io::Write; +use std::sync::OnceLock; +use tokio::sync::{Mutex, OnceCell}; use super::client::get_docker; use crate::models::gateway_settings::{GatewaySettings, GatewayStatus}; @@ -58,21 +65,139 @@ const GATEWAY_INTERNAL_PORT: u16 = 4000; const CONFIG_FINGERPRINT_LABEL: &str = "triple-c.gateway.config-fingerprint"; -/// The value a project should use as its base URL (`ANTHROPIC_BASE_URL`). +/// The default bridge gateway address on a stock native-Linux engine. Only a +/// fallback: the real value is read from the `bridge` network's IPAM config. +const DEFAULT_BRIDGE_GATEWAY: &str = "172.17.0.1"; + +/// Where the gateway's published port is bound on the host, and the address a +/// *project container* uses to reach it. /// /// Project containers run on Docker's default bridge with no user-defined -/// network and no `--add-host`, so the only address they share with the -/// gateway is the host itself. Publishing the gateway on `0.0.0.0:` -/// makes it reachable from every container network on the machine: +/// network and no `--add-host`, so the only address they share with the gateway +/// is the host itself — but *which* host address works is engine-specific, and +/// the whole point of this type is that the two answers are derived together so +/// they cannot drift apart: /// -/// * Docker Desktop (macOS / Windows / WSL2) resolves `host.docker.internal` -/// from inside containers automatically — that is the portable value and the -/// one already suggested by the existing OpenAI-compatible placeholder text. -/// * On native Linux Docker `host.docker.internal` is not injected, and the -/// equivalent address is the default bridge gateway, normally -/// `http://172.17.0.1:`. -pub fn gateway_base_url(port: u16) -> String { - format!("http://host.docker.internal:{}", port) +/// * **Docker Desktop** (macOS / Windows / WSL2) resolves `host.docker.internal` +/// from inside containers automatically, and its port forwarder reaches the +/// host's *loopback*. So: bind `127.0.0.1`, hand out `host.docker.internal`. +/// * **Native Linux Docker** injects no `host.docker.internal`, and the address +/// containers share with the host is the default bridge gateway (normally +/// `172.17.0.1`). So: bind that address, and hand out the same literal. +/// +/// Neither case binds `0.0.0.0`. The bridge-gateway bind is reachable from +/// every container on the default bridge — which is the requirement — without +/// publishing a key-bearing proxy to the LAN. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct GatewayBinding { + /// Host address the published port is bound to (`HostIp`). + pub host_ip: String, + /// Host address a project container should dial. + pub container_host: String, +} + +impl GatewayBinding { + fn desktop() -> Self { + Self { + host_ip: "127.0.0.1".to_string(), + container_host: "host.docker.internal".to_string(), + } + } + + fn bridge(gateway_ip: &str) -> Self { + Self { + host_ip: gateway_ip.to_string(), + container_host: gateway_ip.to_string(), + } + } + + /// The value a project should use as its base URL (`ANTHROPIC_BASE_URL`). + pub fn base_url(&self, port: u16) -> String { + format!("http://{}:{}", self.container_host, port) + } + + /// The address the *host* process (health checks) should dial. + fn host_url(&self, port: u16) -> String { + format!("http://{}:{}", self.host_ip, port) + } +} + +/// Decide the binding from what the daemon reports. Pure, so the engine-shape +/// matrix is testable without a daemon. +fn binding_for(operating_system: &str, bridge_gateway: Option<&str>) -> GatewayBinding { + // Docker Desktop reports exactly "Docker Desktop" here on every platform it + // ships for; matched loosely so a future suffix doesn't silently flip us + // onto the bridge path. + if operating_system.to_ascii_lowercase().contains("docker desktop") { + return GatewayBinding::desktop(); + } + GatewayBinding::bridge( + bridge_gateway + .map(str::trim) + .filter(|g| !g.is_empty()) + .unwrap_or(DEFAULT_BRIDGE_GATEWAY), + ) +} + +/// Detection is one `info` + one `inspect_network` per process; the answer +/// cannot change without the engine being replaced under us. +static GATEWAY_BINDING: OnceCell = OnceCell::const_new(); + +/// The gateway's host binding, detected once and cached. +/// +/// When Docker is unreachable the *loopback* answer is returned without being +/// cached: it is the conservative one (nothing is published anywhere yet, and +/// the only caller in that state is status reporting), and the next call +/// re-detects once the daemon is up. +pub async fn gateway_binding() -> GatewayBinding { + if let Some(binding) = GATEWAY_BINDING.get() { + return binding.clone(); + } + match detect_binding().await { + Ok(binding) => { + let _ = GATEWAY_BINDING.set(binding.clone()); + binding + } + Err(e) => { + log::debug!("Gateway bind detection deferred ({}), assuming loopback", e); + GatewayBinding::desktop() + } + } +} + +async fn detect_binding() -> Result { + let docker = get_docker()?; + let info = docker + .info() + .await + .map_err(|e| format!("Failed to query the Docker daemon: {}", e))?; + let operating_system = info.operating_system.unwrap_or_default(); + let gateway_ip = bridge_gateway_ip(&docker).await; + let binding = binding_for(&operating_system, gateway_ip.as_deref()); + log::info!( + "Model gateway will publish on {} (engine OS: {})", + binding.host_ip, + if operating_system.is_empty() { + "unknown" + } else { + &operating_system + } + ); + Ok(binding) +} + +/// The default bridge's gateway address, straight from its IPAM config, so a +/// host whose bridge subnet was customised still gets a reachable bind. +async fn bridge_gateway_ip(docker: &Docker) -> Option { + let network = docker + .inspect_network("bridge", None::>) + .await + .ok()?; + network + .ipam? + .config? + .into_iter() + .find_map(|c| c.gateway.filter(|g| !g.trim().is_empty())) } fn sha256_hex(input: &str) -> String { @@ -101,10 +226,31 @@ pub async fn get_gateway_status(settings: &GatewaySettings) -> Result Result<(bool, bool), String> { + Ok(match find_gateway_container().await? { + Some((_, state, _)) => (true, state == "running"), + None => (false, false), + }) +} + +/// Whether a container summary's names contain *exactly* our container. +/// +/// Docker's `name` filter is an unanchored regex, so listing with it also +/// returns `triple-c-gateway-backup`, `my-triple-c-gateway`, and anything else +/// containing the string. Taking `.first()` of that would let this module +/// adopt — and then force-remove — a container it does not own. +/// `container::find_existing_container` matches exactly for the same reason. +fn is_gateway_container(names: Option<&Vec>) -> bool { + let expected = format!("/{}", GATEWAY_CONTAINER_NAME); + names.is_some_and(|names| names.iter().any(|n| n == &expected)) +} + /// `(id, state, config fingerprint label)` for the gateway container, if any. async fn find_gateway_container() -> Result, String> { let docker = get_docker()?; @@ -123,7 +269,11 @@ async fn find_gateway_container() -> Result, St .await .map_err(|e| format!("Failed to list containers: {}", e))?; - if let Some(container) = containers.first() { + // The filter is a prefilter only — the exact-name check is what decides. + for container in &containers { + if !is_gateway_container(container.names.as_ref()) { + continue; + } let id = container.id.clone().unwrap_or_default(); let state = container.state.clone().unwrap_or_default(); let fingerprint = container @@ -169,17 +319,21 @@ fn yaml_str(value: &str) -> String { /// The parts of the config that are safe to hash into a Docker label — i.e. /// everything except the two secrets, whose changes are tracked by the /// keychain rotation id instead. -fn config_shape(settings: &GatewaySettings) -> String { +fn config_shape(settings: &GatewaySettings, binding: &GatewayBinding) -> String { let models: Vec = settings .valid_models() .iter() .map(|m| format!("{}={}", m.name.trim(), m.model_id.trim())) .collect(); + // `bind` is part of the shape so that moving between engines (or a bridge + // subnet change) recreates the container instead of leaving it published on + // an address the new environment doesn't use. format!( - "provider={};api_base={};port={};models={}", + "provider={};api_base={};port={};bind={};models={}", settings.provider.trim(), settings.api_base.as_deref().unwrap_or("").trim(), settings.port, + binding.host_ip, models.join(",") ) } @@ -273,6 +427,7 @@ async fn upload_config(container_id: &str, config: &str) -> Result<(), String> { async fn create_gateway_container( settings: &GatewaySettings, + binding: &GatewayBinding, fingerprint: &str, ) -> Result { let docker = get_docker()?; @@ -299,9 +454,9 @@ async fn create_gateway_container( port_bindings.insert( format!("{}/tcp", GATEWAY_INTERNAL_PORT), Some(vec![PortBinding { - // Not loopback — project containers reach this through the host. - // See `gateway_base_url`. - host_ip: Some("0.0.0.0".to_string()), + // Never `0.0.0.0`: the narrowest host address project containers + // can still reach. See `GatewayBinding`. + host_ip: Some(binding.host_ip.clone()), host_port: Some(settings.port.to_string()), }]), ); @@ -328,6 +483,7 @@ async fn create_gateway_container( "triple-c.gateway.port".to_string(), settings.port.to_string(), ); + labels.insert("triple-c.gateway.bind".to_string(), binding.host_ip.clone()); labels.insert( "triple-c.gateway.provider".to_string(), settings.provider.trim().to_string(), @@ -365,7 +521,29 @@ async fn create_gateway_container( Ok(response.id) } +/// Serialises every mutation of the single fixed-name gateway container. +/// +/// `ensure_gateway_running` is check-then-act over one container name, so two +/// concurrent callers — the setup auto-start and the user's Start button is the +/// realistic pair — would both see `None` and both try to create it, and the +/// loser would surface a raw Docker 409. Migration guards the same shape with +/// `ActiveGuard`; here the right behaviour is to *serialise* rather than +/// refuse, because the second caller then observes the first's container, finds +/// a matching fingerprint, and returns its status — which is exactly what it +/// asked for. +fn gateway_lock() -> &'static Mutex<()> { + static LOCK: OnceLock> = OnceLock::new(); + LOCK.get_or_init(|| Mutex::new(())) +} + pub async fn ensure_gateway_running(settings: &GatewaySettings) -> Result { + let _guard = gateway_lock().lock().await; + ensure_gateway_running_locked(settings).await +} + +async fn ensure_gateway_running_locked( + settings: &GatewaySettings, +) -> Result { let docker = get_docker()?; if settings.valid_models().is_empty() { @@ -382,11 +560,13 @@ pub async fn ensure_gateway_running(settings: &GatewaySettings) -> Result Result Result Result Result<(), String> { + // Same lock as `ensure_gateway_running`, so a stop can't interleave with a + // create/start and leave a container running behind a "stopped" return. + let _guard = gateway_lock().lock().await; let docker = get_docker()?; if let Some((id, state, _)) = find_gateway_container().await? { if state == "running" { docker - .stop_container(&id, None::) + .stop_container( + &id, + Some(StopContainerOptions { + t: GATEWAY_STOP_GRACE_SECS, + }), + ) .await .map_err(|e| format!("Failed to stop gateway container: {}", e))?; } @@ -477,8 +670,12 @@ pub async fn check_gateway_health(port: u16) -> Result { .build() .map_err(|e| format!("Failed to build HTTP client: {}", e))?; + // Dial whatever the container is actually published on — with a + // bridge-gateway bind, the host's loopback answers nothing. + let base = gateway_binding().await.host_url(port); + match client - .get(format!("http://127.0.0.1:{}/health/liveliness", port)) + .get(format!("{}/health/liveliness", base)) .send() .await { @@ -627,18 +824,126 @@ mod tests { #[test] fn config_shape_excludes_secrets_and_tracks_changes() { - let a = config_shape(&settings()); + let binding = GatewayBinding::desktop(); + let a = config_shape(&settings(), &binding); let mut s = settings(); s.models[0].model_id = "gpt-4.1".to_string(); - assert_ne!(a, config_shape(&s)); + assert_ne!(a, config_shape(&s, &binding)); assert!(!a.contains("sk-")); } #[test] - fn base_url_points_at_the_host_not_loopback() { - // A project container cannot reach the host's loopback interface. - let url = gateway_base_url(4000); - assert_eq!(url, "http://host.docker.internal:4000"); - assert!(!url.contains("127.0.0.1")); + fn config_shape_tracks_the_bind_address() { + // Moving between engines must recreate the container rather than leave + // it published on an address the new environment doesn't use. + let s = settings(); + assert_ne!( + config_shape(&s, &GatewayBinding::desktop()), + config_shape(&s, &GatewayBinding::bridge("172.17.0.1")) + ); + } + + #[test] + fn docker_desktop_binds_loopback_and_hands_out_host_docker_internal() { + let binding = binding_for("Docker Desktop", None); + assert_eq!(binding.host_ip, "127.0.0.1"); + assert_eq!(binding.base_url(4000), "http://host.docker.internal:4000"); + // Detection must not depend on the bridge answer on this engine. + assert_eq!(binding, binding_for("Docker Desktop", Some("172.17.0.1"))); + } + + #[test] + fn native_linux_binds_the_bridge_gateway_it_reports() { + // A project container can't reach the host's loopback here, but it can + // reach the bridge gateway — and so can nothing on the LAN. + let binding = binding_for("Ubuntu 24.04.1 LTS", Some("172.19.0.1")); + assert_eq!(binding.host_ip, "172.19.0.1"); + assert_eq!(binding.base_url(4000), "http://172.19.0.1:4000"); + assert_eq!(binding.host_url(4000), "http://172.19.0.1:4000"); + } + + #[test] + fn a_missing_bridge_answer_falls_back_to_the_documented_default() { + for reported in [None, Some(""), Some(" ")] { + assert_eq!( + binding_for("Ubuntu 24.04.1 LTS", reported).host_ip, + "172.17.0.1" + ); + } + } + + #[test] + fn no_engine_shape_ever_binds_a_wildcard_address() { + // The regression this guards: the published port fronts a container + // config holding a billed provider key, and Docker's rules sit ahead of + // the host firewall. + for os in ["Docker Desktop", "Ubuntu 24.04.1 LTS", "", "Rancher Desktop"] { + for gw in [None, Some("172.17.0.1"), Some("10.0.0.1")] { + let host_ip = binding_for(os, gw).host_ip; + assert_ne!(host_ip, "0.0.0.0", "os={:?} gw={:?}", os, gw); + assert_ne!(host_ip, "::", "os={:?} gw={:?}", os, gw); + assert!(!host_ip.is_empty()); + } + } + } + + #[test] + fn only_the_exact_container_name_is_adopted() { + // Docker's `name` filter is an unanchored regex: all of these come back + // from a filtered list. Adopting one would force-remove a user's + // container. + assert!(is_gateway_container(Some(&vec![ + "/triple-c-gateway".to_string() + ]))); + assert!(is_gateway_container(Some(&vec![ + "/something-else".to_string(), + "/triple-c-gateway".to_string(), + ]))); + for impostor in [ + "/triple-c-gateway-backup", + "/my-triple-c-gateway", + "/triple-c-gateway2", + "triple-c-gateway", + ] { + assert!( + !is_gateway_container(Some(&vec![impostor.to_string()])), + "{} must not be adopted", + impostor + ); + } + assert!(!is_gateway_container(None)); + assert!(!is_gateway_container(Some(&vec![]))); + } + + #[tokio::test] + async fn the_gateway_lock_serialises_concurrent_callers() { + // The auto-start racing the Start button: both would otherwise see no + // container and both create one, and the loser gets a Docker 409. + use std::sync::atomic::{AtomicUsize, Ordering}; + use std::sync::Arc; + + let inside = Arc::new(AtomicUsize::new(0)); + let overlaps = Arc::new(AtomicUsize::new(0)); + + let mut tasks = Vec::new(); + for _ in 0..8 { + let inside = inside.clone(); + let overlaps = overlaps.clone(); + tasks.push(tokio::spawn(async move { + let _guard = gateway_lock().lock().await; + if inside.fetch_add(1, Ordering::SeqCst) != 0 { + overlaps.fetch_add(1, Ordering::SeqCst); + } + tokio::task::yield_now().await; + tokio::time::sleep(std::time::Duration::from_millis(1)).await; + inside.fetch_sub(1, Ordering::SeqCst); + })); + } + for t in tasks { + t.await.unwrap(); + } + + assert_eq!(overlaps.load(Ordering::SeqCst), 0); + assert_eq!(inside.load(Ordering::SeqCst), 0); } } diff --git a/app/src-tauri/src/docker/migration.rs b/app/src-tauri/src/docker/migration.rs index c9d8688..6e9fecf 100644 --- a/app/src-tauri/src/docker/migration.rs +++ b/app/src-tauri/src/docker/migration.rs @@ -45,7 +45,7 @@ use bollard::models::HostConfig; use futures_util::StreamExt; use super::client::get_docker; -use crate::models::ProjectPath; +use crate::models::{ProjectPath, UnpreservedData}; // ───────────────────────────────────────────────────────────────────────────── // Policy constants @@ -81,7 +81,35 @@ pub const COPY_EXCLUSIONS: &[&str] = &["/usr/local/aws-cli", "/opt/mission-contr /// Roots the filesystem manifest walks. Wider than [`COPY_ROOTS`] so the /// manifest stays useful for debugging; [`compute_verbatim_paths`] applies the /// narrower policy. -pub const MANIFEST_ROOTS: &[&str] = &["/usr/local", "/opt", "/srv", "/workspace"]; +/// +/// [`DATA_ROOTS`] are in here for a different reason: they are never copied, +/// but they *are* destroyed by the container swap, so the walk has to see them +/// in order to warn about them. +pub const MANIFEST_ROOTS: &[&str] = &[ + "/usr/local", + "/opt", + "/srv", + "/workspace", + "/var/lib", + "/var/www", +]; + +/// Roots holding **state a base-image swap destroys and no replay can put +/// back**. Reported by [`unpreserved_data`], never copied. +/// +/// A container running Postgres, MySQL, Redis or nginx keeps its actual data in +/// `/var/lib/` or `/var/www`. Replaying the apt delta reinstalls the +/// *package* onto the new base and gets an empty data directory back — the +/// database is gone. That is worse than the ordinary recreate path, which +/// creates from the project's snapshot and therefore keeps `/var` intact. +/// +/// These are deliberately **not** in [`COPY_ROOTS`]. A live database's on-disk +/// files cannot be tarred out from under a running server and restored into a +/// different base's version of the same package with any confidence — a copy +/// that half-works is worse than a warning that lets the user take a proper +/// dump first. So migration's answer is disclosure, loudly, before anything is +/// touched. +pub const DATA_ROOTS: &[&str] = &["/var/lib", "/var/www"]; /// Base-image capabilities worth telling the user they are missing, as /// `(path, human label)`. @@ -125,6 +153,16 @@ pub const LABEL_CREATE_IMAGE: &str = "triple-c.create-image"; pub const LABEL_MIGRATION_STATE: &str = "triple-c.migration-state"; /// Value of [`LABEL_MIGRATION_STATE`] while a migration is unfinished. pub const MIGRATION_LABEL_IN_PROGRESS: &str = "in-progress"; +/// Label stamped on the short-lived probe containers [`run_throwaway`] creates. +/// +/// They are removed on every path including failure, but a hard crash of the +/// app (or of Docker) between create and remove would otherwise leave a +/// container that carries no `triple-c.*` marking at all — invisible to every +/// cleanup this app has, and unattributable by hand. The label makes +/// `docker ps -a --filter label=triple-c.probe=migration` find them. +pub const LABEL_PROBE: &str = "triple-c.probe"; +/// Value of [`LABEL_PROBE`] on a migration manifest/pre-flight probe container. +pub const PROBE_LABEL_MIGRATION: &str = "migration"; // ───────────────────────────────────────────────────────────────────────────── // Manifests @@ -437,6 +475,72 @@ pub fn verbatim_payload_bytes(from: &Manifest, verbatim: &[String]) -> u64 { .sum() } +/// The reporting unit under a [`DATA_ROOTS`] entry: the first path component +/// below the root, e.g. `/var/lib/postgresql`. Directory-level, because that is +/// the granularity a user can actually act on ("dump this database"), and +/// because a per-file list of a Postgres cluster would be thousands of lines. +fn data_unit(path: &str) -> Option { + for root in DATA_ROOTS { + let prefix = format!("{}/", root); + if let Some(rest) = path.strip_prefix(&prefix) { + let first = rest.split('/').next()?; + if first.is_empty() { + return None; + } + return Some(format!("{}/{}", root, first)); + } + } + None +} + +/// Data-bearing subtrees under [`DATA_ROOTS`] that the migration will destroy +/// and cannot restore, with the size and file count of what is at risk. +/// +/// A subtree qualifies when **all** of: +/// * it is a first-level directory under a [`DATA_ROOTS`] entry, +/// * the current base image does not have that directory **at all** — if the +/// base ships it, it is the base's own machinery (`/var/lib/apt`, +/// `/var/lib/dpkg`, `/var/lib/systemd`, …) and the base's copy is the right +/// one, exactly as for `/etc`, +/// * it contains at least one regular file that neither image's dpkg database +/// owns — a package's own scaffolding is recreated by the apt replay, the +/// data written into it is not. +/// +/// That pair of filters is what keeps this quiet on an ordinary container and +/// loud on one running a database: `/var/lib/postgresql` is absent from the +/// base and full of unowned files, while `/var/lib/apt/lists` is present in the +/// base and never reported. +pub fn unpreserved_data(from: &Manifest, base: &Manifest) -> Vec { + let base_paths = base.path_set(); + let mut acc: BTreeMap = BTreeMap::new(); + + for entry in &from.paths { + let Some(unit) = data_unit(&entry.path) else { + continue; + }; + if base_paths.contains(unit.as_str()) { + continue; + } + if entry.is_dir() { + continue; + } + if from.dpkg_owned.contains(&entry.path) || base.dpkg_owned.contains(&entry.path) { + continue; + } + let slot = acc.entry(unit).or_insert((0, 0)); + slot.0 = slot.0.saturating_add(entry.size); + slot.1 += 1; + } + + acc.into_iter() + .map(|(path, (bytes, file_count))| UnpreservedData { + path, + bytes, + file_count, + }) + .collect() +} + /// Base-image capabilities the container does not have, as /// `(concrete paths, human labels)`. /// @@ -582,6 +686,13 @@ pub async fn run_throwaway(image: &str, script: &str) -> Result>(), + vec!["/var/lib/postgresql", "/var/www/site"] + ); + assert_eq!(got[0].bytes, 9216); + assert_eq!(got[0].file_count, 2); + // And it is emphatically not in the copy set — reporting is the whole + // answer here, not a half-working copy of a live database. + assert!(!COPY_ROOTS.iter().any(|r| is_under("/var/lib/postgresql", r))); + } + + #[test] + fn package_machinery_under_var_is_never_reported_as_data_at_risk() { + // /var/lib/apt exists in the base too, so it is the base's to own — + // the same rule /etc gets. Reporting apt's lists would bury the one + // line that matters under noise on every single migration. + let from = manifest( + &[ + ('d', 4096, "/var/lib/apt"), + ('f', 900_000, "/var/lib/apt/lists/some.mirror_InRelease"), + ('d', 4096, "/var/lib/dpkg"), + ('f', 4096, "/var/lib/dpkg/status"), + ], + &[], + &[], + ); + let base = manifest( + &[ + ('d', 4096, "/var/lib/apt"), + ('d', 4096, "/var/lib/dpkg"), + ('f', 4096, "/var/lib/dpkg/status"), + ], + &[], + &[], + ); + assert!(unpreserved_data(&from, &base).is_empty()); + } + + #[test] + fn a_packages_own_scaffolding_under_var_is_not_data() { + // nginx-common ships /var/www/html/index.nginx-debian.html. The apt + // replay puts that back; only what the user wrote is at risk. + let from = manifest( + &[ + ('d', 4096, "/var/www/html"), + ('f', 612, "/var/www/html/index.nginx-debian.html"), + ], + &["/var/www/html/index.nginx-debian.html"], + &[], + ); + assert!(unpreserved_data(&from, &Manifest::default()).is_empty()); + } + + #[test] + fn data_is_reported_per_directory_not_per_file() { + assert_eq!( + data_unit("/var/lib/mysql/ibdata1").as_deref(), + Some("/var/lib/mysql") + ); + // A first-level directory is its own unit. + assert_eq!( + data_unit("/var/lib/mysql").as_deref(), + Some("/var/lib/mysql") + ); + // The root itself is not: it exists in every image. + assert_eq!(data_unit("/var/lib").as_deref(), None); + assert_eq!(data_unit("/var/www").as_deref(), None); + assert_eq!(data_unit("/usr/local/bin/tool"), None); + } + // ── Bind-mount exclusion ──────────────────────────────────────────────── fn pp(mount: &str) -> ProjectPath { diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index ef204af..b5f65ed 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -8,13 +8,17 @@ mod models; mod storage; pub mod web_terminal; -use std::sync::Arc; +use std::sync::atomic::{AtomicBool, Ordering}; +use std::sync::{Arc, Mutex}; +use std::time::Duration; use auth_bridge::AuthBridgeManager; use docker::exec::ExecSessionManager; use storage::projects_store::ProjectsStore; use storage::settings_store::SettingsStore; -use tauri::Manager; +use tauri::async_runtime::JoinHandle; +use tauri::{Emitter, Manager}; +use tokio::sync::watch; use web_terminal::WebTerminalServer; pub struct AppState { @@ -23,6 +27,161 @@ pub struct AppState { pub exec_manager: Arc, pub auth_bridge: Arc, pub web_terminal_server: Arc>>, + pub lifecycle: Arc, +} + +// ───────────────────────────────────────────────────────────────────────────── +// Startup / shutdown coordination +// ───────────────────────────────────────────────────────────────────────────── + +/// Total wall-clock budget for teardown before the process exits regardless. +/// +/// Six teardown steps used to run *serially* inside a `block_on` on the +/// window-event thread with no timeout: two container stops at Docker's default +/// 10s grace, a `docker exec` per browser-view project, and every bollard call +/// inheriting a 120s client timeout. Quitting after Docker Desktop had already +/// gone away froze the window for minutes. Nothing here is worth more than a +/// few seconds of a user's exit. +const SHUTDOWN_BUDGET: Duration = Duration::from_secs(8); + +/// How long the in-flight auto-start tasks get to notice cancellation before +/// they are aborted. They only have to reach their next await point. +const STARTUP_CANCEL_BUDGET: Duration = Duration::from_secs(3); + +/// Backoff (seconds) between auto-start attempts. Docker Desktop routinely +/// takes 30-60s to accept API calls after login, which is exactly the window in +/// which Triple-C used to be launched, fail once, and stay broken for the whole +/// session. +const AUTOSTART_DELAYS: [u64; 8] = [0, 2, 4, 8, 15, 15, 30, 30]; + +/// Owns the "is the app going away?" signal and the handles of the background +/// tasks started during `setup`. +/// +/// Both auto-starts are fire-and-forget, and quitting quickly used to race +/// them: `CloseRequested` stopped a gateway container that did not exist yet, +/// and the detached task then created and started it *after* the app was gone — +/// leaving an orphan proxy holding a provider key. The same shape orphaned the +/// web terminal, whose task wrote its server into the state slot that +/// `CloseRequested` had already `take()`-n. Shutdown therefore cancels and +/// waits for these tasks *before* running teardown, so teardown always sees the +/// final state of the world. +pub struct Lifecycle { + cancel: watch::Sender, + tasks: Mutex>>, + shutting_down: AtomicBool, +} + +impl Lifecycle { + fn new() -> Self { + let (cancel, _) = watch::channel(false); + Self { + cancel, + tasks: Mutex::new(Vec::new()), + shutting_down: AtomicBool::new(false), + } + } + + /// A receiver that flips to `true` when the app starts shutting down. + pub fn cancellation(&self) -> watch::Receiver { + self.cancel.subscribe() + } + + pub fn is_shutting_down(&self) -> bool { + *self.cancel.borrow() + } + + /// Register a startup task so shutdown can wait for it. + fn track(&self, handle: JoinHandle<()>) { + self.tasks + .lock() + .unwrap_or_else(|e| e.into_inner()) + .push(handle); + } + + /// `true` the first time only — the window can emit `CloseRequested` again + /// once we ask the app to exit, and teardown must not restart. + fn begin_shutdown(&self) -> bool { + if self.shutting_down.swap(true, Ordering::SeqCst) { + return false; + } + // `send_replace`, not `send`: `send` reports an error *and leaves the + // value untouched* when nothing is subscribed, which is exactly the + // case when neither auto-start is enabled — and `is_shutting_down` (the + // web terminal's check) reads that stored value. + self.cancel.send_replace(true); + true + } + + /// Let the tracked startup tasks unwind, then abort whatever is left. + async fn settle_startup_tasks(&self) { + let mut handles: Vec> = std::mem::take( + &mut *self.tasks.lock().unwrap_or_else(|e| e.into_inner()), + ); + if handles.is_empty() { + return; + } + let settle = async { + for handle in &mut handles { + let _ = handle.await; + } + }; + if tokio::time::timeout(STARTUP_CANCEL_BUDGET, settle).await.is_err() { + log::warn!("Startup tasks did not settle in time — aborting them"); + for handle in &handles { + handle.abort(); + } + } + } +} + +/// Run an auto-start until it succeeds, the app quits, or the retries run out. +/// +/// Without this a launch that beats the Docker daemon (or Docker Desktop) to +/// readiness left the gateway and STT down for the entire session, with no +/// path back: nothing re-attempts them. +async fn autostart_with_retry(label: &str, mut cancel: watch::Receiver, mut attempt: F) +where + F: FnMut() -> Fut, + Fut: std::future::Future>, +{ + for (index, delay) in AUTOSTART_DELAYS.iter().enumerate() { + if *delay > 0 { + tokio::select! { + _ = cancel.changed() => return, + _ = tokio::time::sleep(Duration::from_secs(*delay)) => {} + } + } + if *cancel.borrow() { + return; + } + + // Cancellation races the attempt itself, not just the backoff, so a + // quick quit isn't held up by an in-flight Docker call — and, more + // importantly, so the attempt cannot complete after teardown has run. + let result = tokio::select! { + _ = cancel.changed() => return, + r = attempt() => r, + }; + + match result { + Ok(()) => { + if index > 0 { + log::info!("{} auto-start succeeded on attempt {}", label, index + 1); + } + return; + } + Err(e) => { + let last = index + 1 == AUTOSTART_DELAYS.len(); + if index == 0 { + log::warn!("{} auto-start failed ({}) — will retry", label, e); + } else if last { + log::error!("{} auto-start gave up after {} attempts: {}", label, index + 1, e); + } else { + log::debug!("{} auto-start attempt {} failed: {}", label, index + 1, e); + } + } + } + } } pub fn run() { @@ -44,11 +203,13 @@ pub fn run() { }); let exec_manager = Arc::new(ExecSessionManager::new()); let auth_bridge = Arc::new(AuthBridgeManager::new()); + let lifecycle = Arc::new(Lifecycle::new()); // Clone Arcs for the setup closure (web terminal auto-start) let projects_store_setup = projects_store.clone(); let settings_store_setup = settings_store.clone(); let exec_manager_setup = exec_manager.clone(); + let lifecycle_setup = lifecycle.clone(); tauri::Builder::default() .plugin(tauri_plugin_store::Builder::default().build()) @@ -60,6 +221,7 @@ pub fn run() { exec_manager, auth_bridge, web_terminal_server: Arc::new(tokio::sync::Mutex::new(None)), + lifecycle, }) .setup(move |app| { match tauri::image::Image::from_bytes(include_bytes!("../icons/icon.png")) { @@ -84,8 +246,9 @@ pub fn run() { let set_store = settings_store_setup.clone(); let state = app.state::(); let web_server_mutex = state.web_terminal_server.clone(); + let lifecycle = lifecycle_setup.clone(); - tauri::async_runtime::spawn(async move { + let handle = tauri::async_runtime::spawn(async move { match WebTerminalServer::start( port, token, @@ -96,6 +259,16 @@ pub fn run() { .await { Ok(server) => { + // The app may have been asked to quit while the + // server was coming up, in which case teardown + // has already emptied this slot and would never + // look at it again. Stop it here instead of + // storing an orphan. + if lifecycle.is_shutting_down() { + server.stop(); + log::info!("Web terminal stopped immediately: app is exiting"); + return; + } let mut guard = web_server_mutex.lock().await; *guard = Some(server); log::info!("Web terminal auto-started on port {}", port); @@ -105,68 +278,122 @@ pub fn run() { } } }); + lifecycle_setup.track(handle); } } // Auto-start STT container if enabled in settings if settings.stt.enabled { let stt_settings = settings.stt.clone(); - tauri::async_runtime::spawn(async move { - match docker::stt::ensure_stt_running(&stt_settings).await { - Ok(status) => { - if status.running { - log::info!("STT container auto-started on port {}", stt_settings.port); - } else { - log::warn!("STT auto-start: container not running after ensure_stt_running"); - } + let cancel = lifecycle_setup.cancellation(); + let handle = tauri::async_runtime::spawn(async move { + autostart_with_retry("STT container", cancel, || async { + let status = docker::stt::ensure_stt_running(&stt_settings).await?; + if status.running { + log::info!("STT container auto-started on port {}", stt_settings.port); + Ok(()) + } else { + Err("container not running after ensure_stt_running".to_string()) } - Err(e) => { - log::error!("Failed to auto-start STT container: {}", e); - } - } + }) + .await; }); + lifecycle_setup.track(handle); } // Auto-start model gateway container if enabled in settings if settings.gateway.enabled { let gateway_settings = settings.gateway.clone(); - tauri::async_runtime::spawn(async move { - match docker::gateway::ensure_gateway_running(&gateway_settings).await { - Ok(status) => { - if status.running { - log::info!("Model gateway auto-started on port {}", gateway_settings.port); - } else { - log::warn!("Model gateway auto-start: container not running after ensure_gateway_running"); - } + let cancel = lifecycle_setup.cancellation(); + let handle = tauri::async_runtime::spawn(async move { + autostart_with_retry("Model gateway", cancel, || async { + let status = + docker::gateway::ensure_gateway_running(&gateway_settings).await?; + if status.running { + log::info!( + "Model gateway auto-started on port {}", + gateway_settings.port + ); + Ok(()) + } else { + Err("container not running after ensure_gateway_running".to_string()) } - Err(e) => { - log::error!("Failed to auto-start model gateway container: {}", e); - } - } + }) + .await; }); + lifecycle_setup.track(handle); } Ok(()) }) .on_window_event(|window, event| { - if let tauri::WindowEvent::CloseRequested { .. } = event { + if let tauri::WindowEvent::CloseRequested { api, .. } = event { let state = window.state::(); - tauri::async_runtime::block_on(async { - // Stop web terminal server - let mut server_guard = state.web_terminal_server.lock().await; - if let Some(server) = server_guard.take() { - server.stop(); + let lifecycle = state.lifecycle.clone(); + + // Already shutting down: let the window close. That covers our + // own `exit` unwinding it, and it deliberately leaves a second + // click on the X as a force-quit — teardown is a courtesy, not + // a hostage situation. + if !lifecycle.begin_shutdown() { + return; + } + + let exec_manager = state.exec_manager.clone(); + let auth_bridge = state.auth_bridge.clone(); + let web_terminal_server = state.web_terminal_server.clone(); + drop(state); + + // Teardown talks to Docker, so it cannot be instant. Keep the + // window alive and tell the UI what is happening rather than + // blocking the event thread on it and looking hung. + api.prevent_close(); + let _ = window.emit("app-shutting-down", ()); + + let app_handle = window.app_handle().clone(); + tauri::async_runtime::spawn(async move { + let teardown = async { + // First: let the auto-starts unwind. Anything they are + // midway through creating has to exist before the stops + // below run, or it outlives the app. + lifecycle.settle_startup_tasks().await; + + // Then everything else, concurrently — these touch + // different subsystems and nothing here depends on + // another's result. Serially, the two container stops + // alone were 20s of Docker's default grace period. + let web_terminal = async { + if let Some(server) = web_terminal_server.lock().await.take() { + server.stop(); + } + }; + let stop_stt = async { + if let Err(e) = docker::stt::stop_stt_container().await { + log::warn!("Failed to stop the STT container on exit: {}", e); + } + }; + let stop_gateway = async { + if let Err(e) = docker::gateway::stop_gateway_container().await { + log::warn!("Failed to stop the model gateway on exit: {}", e); + } + }; + tokio::join!( + web_terminal, + stop_stt, + stop_gateway, + exec_manager.close_all_sessions(), + auth_bridge.stop_all(), + browser_view::manager().stop_all(), + ); + }; + + if tokio::time::timeout(SHUTDOWN_BUDGET, teardown).await.is_err() { + log::warn!( + "Shutdown exceeded {}s — exiting with teardown incomplete", + SHUTDOWN_BUDGET.as_secs() + ); } - // Stop STT container - let _ = docker::stt::stop_stt_container().await; - // Stop model gateway container - let _ = docker::gateway::stop_gateway_container().await; - // Close all exec sessions - state.exec_manager.close_all_sessions().await; - // Release every host loopback port held by the auth bridge - state.auth_bridge.stop_all().await; - // Stop any browser-view proxies and in-container dashboards - browser_view::manager().stop_all().await; + app_handle.exit(0); }); } }) @@ -278,3 +505,130 @@ pub fn run() { .run(tauri::generate_context!()) .expect("error while running tauri application"); } + +#[cfg(test)] +mod tests { + use super::*; + use std::sync::atomic::AtomicUsize; + + /// Drives the retry loop under a paused clock, so the real backoff schedule + /// is exercised without waiting for it. + async fn run_autostart( + cancel: watch::Receiver, + outcomes: Vec>, + ) -> usize { + let calls = Arc::new(AtomicUsize::new(0)); + let counter = calls.clone(); + let outcomes = Arc::new(Mutex::new(outcomes.into_iter())); + autostart_with_retry("test", cancel, move || { + let counter = counter.clone(); + let outcomes = outcomes.clone(); + async move { + counter.fetch_add(1, Ordering::SeqCst); + outcomes + .lock() + .unwrap() + .next() + .unwrap_or(Err("still down".to_string())) + } + }) + .await; + calls.load(Ordering::SeqCst) + } + + #[tokio::test(start_paused = true)] + async fn a_working_autostart_runs_exactly_once() { + let (_tx, rx) = watch::channel(false); + assert_eq!(run_autostart(rx, vec![Ok(())]).await, 1); + } + + #[tokio::test(start_paused = true)] + async fn an_autostart_that_beat_docker_to_readiness_recovers() { + // The regression: Docker not being up yet used to cost the whole + // session — gateway down, STT down, and nothing ever retried. + let (_tx, rx) = watch::channel(false); + let calls = run_autostart( + rx, + vec![ + Err("daemon not running".to_string()), + Err("daemon not running".to_string()), + Ok(()), + ], + ) + .await; + assert_eq!(calls, 3); + } + + #[tokio::test(start_paused = true)] + async fn a_permanently_failing_autostart_gives_up_rather_than_looping_forever() { + let (_tx, rx) = watch::channel(false); + assert_eq!( + run_autostart(rx, vec![]).await, + AUTOSTART_DELAYS.len(), + "should attempt once per backoff step and then stop" + ); + } + + #[tokio::test(start_paused = true)] + async fn a_quick_quit_stops_the_retries_before_they_start() { + // Quitting before the first attempt must not leave a task that creates + // and starts a container after teardown has already run. + let (tx, rx) = watch::channel(false); + tx.send(true).unwrap(); + assert_eq!(run_autostart(rx, vec![Ok(())]).await, 0); + } + + #[tokio::test(start_paused = true)] + async fn cancelling_between_attempts_stops_the_retries() { + let (tx, rx) = watch::channel(false); + let calls = Arc::new(AtomicUsize::new(0)); + let counter = calls.clone(); + autostart_with_retry("test", rx, move || { + let counter = counter.clone(); + let tx = tx.clone(); + async move { + counter.fetch_add(1, Ordering::SeqCst); + // The app starts quitting while this attempt is in flight. + let _ = tx.send(true); + Err("daemon not running".to_string()) + } + }) + .await; + assert_eq!(calls.load(Ordering::SeqCst), 1); + } + + #[test] + fn shutdown_begins_exactly_once() { + // `CloseRequested` fires again when our own `exit(0)` unwinds the + // window; teardown must not start a second time. + let lifecycle = Lifecycle::new(); + assert!(!lifecycle.is_shutting_down()); + assert!(lifecycle.begin_shutdown()); + assert!(lifecycle.is_shutting_down()); + assert!(!lifecycle.begin_shutdown()); + } + + #[tokio::test] + async fn beginning_shutdown_notifies_already_running_startup_tasks() { + let lifecycle = Lifecycle::new(); + let mut cancel = lifecycle.cancellation(); + assert!(!*cancel.borrow()); + lifecycle.begin_shutdown(); + assert!(cancel.changed().await.is_ok()); + assert!(*cancel.borrow()); + } + + #[tokio::test(start_paused = true)] + async fn a_startup_task_that_ignores_cancellation_is_abandoned_not_awaited() { + // The budget is what keeps a wedged auto-start from turning quit into a + // multi-minute freeze. + let lifecycle = Lifecycle::new(); + lifecycle.track(tauri::async_runtime::spawn(async { + tokio::time::sleep(Duration::from_secs(600)).await; + })); + lifecycle.begin_shutdown(); + let started = tokio::time::Instant::now(); + lifecycle.settle_startup_tasks().await; + assert!(started.elapsed() <= STARTUP_CANCEL_BUDGET + Duration::from_secs(1)); + } +} diff --git a/app/src-tauri/src/models/migration.rs b/app/src-tauri/src/models/migration.rs index 14043cc..5068df7 100644 --- a/app/src-tauri/src/models/migration.rs +++ b/app/src-tauri/src/models/migration.rs @@ -28,9 +28,26 @@ //! //! What is genuinely lost is confined to the container's writable layer: //! root-level `apt` installs, `npm -g` packages (npm's prefix is `/usr`), -//! `/usr/local`, `/opt`, `/srv`, and anything under `/workspace` that is not on -//! a bind mount. Those four categories are exactly what -//! [`MigrationOptions`] can replay. +//! `/usr/local`, `/opt`, `/srv`, anything under `/workspace` that is not on a +//! bind mount — and **`/var`**. The first four are what [`MigrationOptions`] +//! can replay. `/var` is not, and that gap is deliberate rather than an +//! oversight, so it is stated here rather than glossed over: +//! +//! Service state lives in `/var/lib/` and `/var/www`. Replaying the +//! apt delta reinstalls `postgresql` onto the new base and hands back an +//! **empty** cluster; the old one is gone with the writable layer. The +//! ordinary recreate path does not have this problem, because it creates from +//! the project's own snapshot and `/var` rides along — so a silent migration +//! would be *more* destructive than the thing it is sold as a safer +//! alternative to. +//! +//! Copying a live database's files out with `tar` and unpacking them onto a +//! different base's version of the same package is not a fix; it is a +//! corruption risk wearing a fix's clothes. So the answer is disclosure: +//! [`crate::docker::migration::unpreserved_data`] finds the data-bearing +//! subtrees under `/var` that the base does not ship, and +//! [`ContainerStaleness::unpreserved_data`] carries them into the pre-flight, +//! where the user is told to back them up before anything is touched. //! //! ## Serde //! @@ -67,6 +84,20 @@ pub struct PackageFailure { pub reason: String, } +/// A data-bearing subtree the migration will destroy and cannot put back. +/// +/// See [`crate::docker::migration::unpreserved_data`]. Surfaced in the +/// pre-flight so the user can take a backup first; never copied. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct UnpreservedData { + /// Absolute path of the directory, e.g. `/var/lib/postgresql`. + pub path: String, + /// Total size of the non-package files beneath it. + pub bytes: u64, + /// How many non-package files it holds. + pub file_count: u32, +} + /// Everything the UI needs to decide whether a project is worth migrating, and /// to explain to the user what migrating would actually change. /// @@ -102,6 +133,12 @@ pub struct ContainerStaleness { /// Non-dpkg-owned paths under the verbatim-copy roots that would be carried /// across. Empty when nothing user-authored was found. pub verbatim_paths: Vec, + /// Data-bearing subtrees under `/var` that a migration **destroys and + /// cannot restore** — a database's files, a served site. Empty on an + /// ordinary container; when it is not, the pre-flight has to say so before + /// anything is touched. See [`UnpreservedData`]. + #[serde(default)] + pub unpreserved_data: Vec, /// dpkg packages the current base carries at a different version than this /// container does. A rough "how much security drift" number, not a promise /// that every one of them is newer. @@ -178,6 +215,11 @@ pub struct MigrationPlan { /// Base-image paths the old container lacked, so the finished migration can /// report which of them it actually gained. pub missing_paths: Vec, + /// What the pre-flight found under `/var` that the migration would destroy. + /// Frozen here so the finished report can name it even though the container + /// it was measured on no longer exists. + #[serde(default)] + pub unpreserved_data: Vec, } /// Persisted, host-side migration state. Written **before** anything diff --git a/app/src-tauri/src/web_terminal/terminal.html b/app/src-tauri/src/web_terminal/terminal.html index cffa01a..cd06179 100644 --- a/app/src-tauri/src/web_terminal/terminal.html +++ b/app/src-tauri/src/web_terminal/terminal.html @@ -389,13 +389,30 @@ let relayLastAt = 0; let relayHideTimer = null; + // ─── shared-url-sanitizer ───────────────────────────────────── + // THIS IS A COPY OF `sanitizeRelayUrl` IN app/src/lib/urlRelay.ts. + // It exists only because this file is embedded standalone via include_str!() + // and cannot import a module. Change one, change the other — and note that + // app/src/lib/urlRelay.embedded.test.ts reads this file, extracts the block + // between these two markers and runs it against the same table of cases as + // the TypeScript original, so a divergence fails the suite instead of + // silently shipping. Keep the markers, the function name and the arity + // intact: that test finds the code by them. function sanitizeRelayUrl(raw) { if (typeof raw !== 'string') return null; const s = raw.trim(); if (!s || s.length > RELAY_MAX_URL) return null; // Control characters and whitespace first: new URL() strips tabs/newlines, - // so "java\nscript:" would otherwise slip through as javascript:. - if (/[\s\u0000-\u0020\u007f]/.test(s)) return null; + // so "java\nscript:" would otherwise slip through as javascript:. Quotes + // and backticks go with them — all three are illegal in a URL, and this + // string ends up as an argument to something that may treat them as syntax. + for (const ch of s) { + const code = ch.codePointAt(0); + if (code <= 0x20 || code === 0x7f) return null; + if (code >= 0x80 && code <= 0x9f) return null; + if (ch === '"' || ch === "'" || ch === '`') return null; + if (ch.trim() === '') return null; + } let u; try { u = new URL(s); } catch (e) { return null; } if (u.protocol !== 'http:' && u.protocol !== 'https:') return null; @@ -403,6 +420,7 @@ if (u.username || u.password) return null; // origin spoofing return u.toString(); } + // ─── end shared-url-sanitizer ──────────────────────────────── function parseRelayOsc(data) { if (typeof data !== 'string') return null; diff --git a/app/src/App.tsx b/app/src/App.tsx index f408a6f..7245f4f 100644 --- a/app/src/App.tsx +++ b/app/src/App.tsx @@ -1,5 +1,6 @@ -import { useEffect, useState } from "react"; +import { useCallback, useEffect, useState } from "react"; import { useShallow } from "zustand/react/shallow"; +import { listen } from "@tauri-apps/api/event"; import Sidebar from "./components/layout/Sidebar"; import TopBar from "./components/layout/TopBar"; import StatusBar from "./components/layout/StatusBar"; @@ -38,6 +39,25 @@ export default function App() { })) ); const [showInstallDialog, setShowInstallDialog] = useState(false); + const [shuttingDown, setShuttingDown] = useState(false); + + /** + * Everything that can only be done once Docker answers. Called from the + * startup check *and* from the poller when the daemon shows up later — a + * session that launched before Docker was ready otherwise never reconciles + * container state or recovers an interrupted migration. + */ + const onDockerReady = useCallback(async () => { + checkImage(); + // Reconcile project statuses against actual Docker container state, + // then refresh the project list so the UI reflects reality. + try { + setProjects(await reconcileProjectStatuses()); + } catch { + // If reconciliation fails (e.g. Docker hiccup), just load from store + refresh(); + } + }, [checkImage, setProjects, refresh]); // Single STT instance bound to the active session. The mic lives in the // StatusBar; the terminal's Ctrl+Shift+M shortcut calls stt.toggle via the @@ -57,18 +77,10 @@ export default function App() { let stopPolling: (() => void) | undefined; checkDocker().then((available) => { if (available) { - checkImage(); - // Reconcile project statuses against actual Docker container state, - // then refresh the project list so the UI reflects reality. - reconcileProjectStatuses().then((projects) => { - setProjects(projects); - }).catch(() => { - // If reconciliation fails (e.g. Docker hiccup), just load from store - refresh(); - }); + onDockerReady(); } else { setShowInstallDialog(true); - stopPolling = startDockerPolling(); + stopPolling = startDockerPolling(onDockerReady); } }); refresh(); @@ -87,6 +99,23 @@ export default function App() { }; }, []); // eslint-disable-line react-hooks/exhaustive-deps + // The backend prevents the window closing so it can stop containers first, + // which freezes the UI for several seconds. This says why. + useEffect(() => { + let unlisten: (() => void) | undefined; + let cancelled = false; + listen("app-shutting-down", () => setShuttingDown(true)) + .then((fn) => { + if (cancelled) fn(); + else unlisten = fn; + }) + .catch((e) => console.error("Failed to listen for shutdown:", e)); + return () => { + cancelled = true; + unlisten?.(); + }; + }, []); + const homeProjectIds = tabOrder.filter(isHomeTab).map(tabKeyId); return ( @@ -122,6 +151,21 @@ export default function App() { {showInstallDialog && ( setShowInstallDialog(false)} /> )} + {shuttingDown && ( +
+
+ +

+ Stopping containers before quitting. This window will close on its own. +

+
+
+ )} ); } diff --git a/app/src/components/projects/MigrateContainerModal.test.tsx b/app/src/components/projects/MigrateContainerModal.test.tsx index 4da145f..852d246 100644 --- a/app/src/components/projects/MigrateContainerModal.test.tsx +++ b/app/src/components/projects/MigrateContainerModal.test.tsx @@ -22,6 +22,7 @@ const STALE: ContainerStaleness = { apt_delta: ["socat", "bubblewrap"], npm_global_delta: [], verbatim_paths: [], + unpreserved_data: [], outdated_package_count: 61, probe_error: null, }; @@ -30,6 +31,7 @@ function migration(overrides: Partial = {}): ContainerMigrat return { staleness: STALE, probing: false, + probeSettled: true, running: false, recovered: false, interrupted: null, @@ -41,7 +43,7 @@ function migration(overrides: Partial = {}): ContainerMigrat resume: vi.fn(async () => {}), keep: vi.fn(async () => {}), rollback: vi.fn(async () => {}), - dismiss: vi.fn(), + dismiss: vi.fn(async () => {}), refresh: vi.fn(async () => {}), ...overrides, }; @@ -153,18 +155,95 @@ describe("MigrateContainerModal", () => { }); }); - it("does not ask to copy paths when there are none to copy", async () => { + it("never derives copy_paths from a delta the probe may not have read", async () => { + // The regression: `copy_paths: copyPaths && verbatim.length > 0` read the + // toggle's meaning off `staleness`, which is null while the ~6 s probe + // runs. That sent `copy_paths: false` to a backend that recomputes the + // real set but honours the flag — files silently not copied, while this + // dialog said there was nothing to copy. The toggle's own value is the + // only thing that may be sent; the backend skips the step when *its* set + // comes out empty, which is the only place that knows. const { m } = await renderModal({ ...STALE, verbatim_paths: [] }); fireEvent.click( screen.getByRole("button", { name: "Update container base" }), ); expect(m.start).toHaveBeenCalledWith({ replay_packages: true, - copy_paths: false, + copy_paths: true, keep_rollback: true, }); }); + it("cannot be started until the probe has settled, and says so", async () => { + await renderModal(null, { probeSettled: false, probing: true }); + expect( + screen.getByRole("button", { name: "Update container base" }), + ).toBeDisabled(); + expect( + screen.getByText(/lists below are not complete until it finishes/i), + ).toBeInTheDocument(); + // "None found" and "not checked yet" must not be the same sentence. + expect( + screen.getByText(/Still checking which apt packages/i), + ).toBeInTheDocument(); + expect(screen.getByText("Not checked yet.")).toBeInTheDocument(); + expect( + screen.queryByText(/No extra apt packages were found/i), + ).not.toBeInTheDocument(); + }); + + it("names the data under /var that the update destroys and cannot restore", async () => { + await renderModal({ + ...STALE, + unpreserved_data: [ + { path: "/var/lib/postgresql", bytes: 41_000_000, file_count: 912 }, + ], + }); + const panel = screen.getByTestId("migration-unpreserved"); + expect(panel.textContent).toMatch(/\/var\/lib\/postgresql/); + expect(panel.textContent).toMatch(/41\.0 MB in 912 files/); + expect(panel.textContent).toMatch(/reinstalling the package does not bring it back/i); + }); + + it("says plainly that /var is not carried across even when nothing is at risk", async () => { + await renderModal(); + const panel = screen.getByTestId("migration-unpreserved"); + expect(panel.textContent).toMatch(/nothing here to lose/i); + expect(panel.textContent).toMatch(/Data written under \/var is not carried across/i); + }); + + it("offers Resume rather than Keep on a container that is mid-swap", async () => { + // Keep drops the rollback image, and on an unfinished migration + // `:latest` still points at the old lineage — so Keep here deletes the + // only way back from a container the app can no longer reason about. + const { m } = await renderModal(STALE, { + interrupted: { + phase: "interrupted", + from_image_id: "sha256:aaa", + to_base_id: "sha256:bbb", + started_at: "2026-08-09T10:00:00Z", + report: null, + rollback_image: "triple-c-snapshot-p1:pre-migration-20260809-100000", + staging_path: null, + options: { replay_packages: true, copy_paths: true, keep_rollback: true }, + plan: null, + }, + report: { + phase: "failed", + packages_requested: [], + packages_installed: [], + packages_failed: [], + paths_copied: [], + features_restored: [], + rollback_available: true, + message: "saving it failed", + }, + }); + expect(screen.queryByRole("button", { name: "Keep" })).not.toBeInTheDocument(); + fireEvent.click(screen.getByRole("button", { name: "Resume update" })); + expect(m.resume).toHaveBeenCalledTimes(1); + }); + it("does not start anything on cancel", async () => { const { m, onClose } = await renderModal(); fireEvent.click(screen.getByRole("button", { name: "Cancel" })); diff --git a/app/src/components/projects/MigrateContainerModal.tsx b/app/src/components/projects/MigrateContainerModal.tsx index 807293e..baf876d 100644 --- a/app/src/components/projects/MigrateContainerModal.tsx +++ b/app/src/components/projects/MigrateContainerModal.tsx @@ -5,8 +5,10 @@ import Button from "../ui/Button"; import Toggle from "../ui/Toggle"; import { SwitchRow } from "../ui/Field"; import MigrationReportCard from "./MigrationReportCard"; +import MigrationInterruptedCard from "./MigrationInterruptedCard"; import type { ContainerMigration } from "../../hooks/useContainerMigration"; import { + DATA_NOT_CARRIED, KEPT_AUTOMATICALLY, KEPT_WHY, LOST_WITHOUT_REPLAY, @@ -14,6 +16,7 @@ import { REPLAY_COST, ROLLBACK_DISK_COST, ROLLBACK_SCOPE, + formatDataSize, formatSnapshotDate, } from "./migrationCopy"; @@ -85,10 +88,12 @@ export default function MigrateContainerModal({ const [keepRollback, setKeepRollback] = useState(true); const logRef = useRef(null); - const { running, report, log, phaseMessage, busy } = migration; + const { running, report, interrupted, log, phaseMessage, busy, probeSettled } = + migration; const aptDelta = staleness?.apt_delta ?? []; const npmDelta = staleness?.npm_global_delta ?? []; const verbatim = staleness?.verbatim_paths ?? []; + const atRisk = staleness?.unpreserved_data ?? []; const gains = staleness?.missing_features ?? []; const snapshot = formatSnapshotDate(staleness?.snapshot_created_at ?? null); @@ -100,13 +105,44 @@ export default function MigrateContainerModal({ const start = () => { const options: MigrationOptions = { + // Deliberately *not* `&& verbatim.length > 0`. That looked like a + // harmless optimisation but read the toggle's meaning off a probe that + // may not have landed, so a null `staleness` sent `copy_paths: false` + // and the backend — which recomputes the real set but honours the flag — + // skipped files that did exist. The backend already skips the step when + // its own set comes out empty; that is the only place that knows. replay_packages: replayPackages, - copy_paths: copyPaths && verbatim.length > 0, + copy_paths: copyPaths, keep_rollback: keepRollback, }; void migration.start(options); }; + // ---- Unfinished --------------------------------------------------------- + // Ahead of the report, for the reason spelled out in MigrationInterruptedCard: + // Keep is not a legitimate action on a container that is mid-swap. + if (interrupted) { + return ( + + Close + + } + > + void migration.resume()} + onRollback={() => void migration.rollback().then(onClose)} + /> + + ); + } + // ---- Outcome ------------------------------------------------------------ if (report) { return ( @@ -125,10 +161,7 @@ export default function MigrateContainerModal({ busy={busy} onKeep={() => void migration.keep().then(onClose)} onRollback={() => void migration.rollback().then(onClose)} - onDismiss={() => { - migration.dismiss(); - onClose(); - }} + onDismiss={() => void migration.dismiss().then(onClose)} /> ); @@ -187,13 +220,35 @@ export default function MigrateContainerModal({ - } >
+ {/* 0. Until the probe lands, every list below is "not known" wearing + "empty"'s clothes. Say which one it is, and do not let the run + start on an unread delta. */} + {!probeSettled && ( +
+

+ Still working out what this container has that the current base + does not. The lists below are not complete until it finishes, so + the update cannot start yet. +

+
+ )} + {/* 1. Reassurance first. Not a choice — a statement of fact. */}
@@ -203,6 +258,51 @@ export default function MigrateContainerModal({

+ {/* 1b. The one thing that is genuinely destroyed. Directly under the + reassurance, because a user who reads only the top of this dialog + must not come away thinking nothing is at stake. */} +
+

+ {atRisk.length > 0 + ? `Destroyed, and not restored by this update (${atRisk.length})` + : "Not carried across"} +

+

+ {DATA_NOT_CARRIED} +

+ {probeSettled ? ( + atRisk.length > 0 ? ( +
    + {atRisk.map((d) => ( +
  • + {d.path} + + {" "} + — {formatDataSize(d.bytes)} in {d.file_count} file + {d.file_count === 1 ? "" : "s"} + +
    • + ))} +
+ ) : ( +

+ Nothing was found under /var{" "} + on this container, so there is nothing here to lose. +

+ ) + ) : ( +

+ Not checked yet. +

+ )} +
+ {/* 2. The apt replay. */}
{aptDelta.length === 0 ? (

- No extra apt packages were found on this container. + {/* "None found" and "not looked yet" are different sentences. + Printing the first while the probe is still running is how a + user ends up believing a delta was empty when it was unread. */} + {probeSettled + ? "No extra apt packages were found on this container." + : "Still checking which apt packages this container added."}

) : ( @@ -232,10 +337,16 @@ export default function MigrateContainerModal({

{REPLAY_COST}

- {/* 3. Verbatim copies — usually nothing, so usually not shown at all. */} - {verbatim.length > 0 && ( + {/* 3. Verbatim copies — usually nothing once the probe has settled, so + usually not shown at all. Shown while it has not, because a hidden + section reads as "there is nothing here". */} + {(verbatim.length > 0 || !probeSettled) && (
/workspace that belongs to no package, so it cannot be reinstalled from a repository.

- + {probeSettled ? ( + + ) : ( +

+ Still checking what is there. +

+ )}
)} diff --git a/app/src/components/projects/MigrationInterruptedCard.tsx b/app/src/components/projects/MigrationInterruptedCard.tsx new file mode 100644 index 0000000..60caa61 --- /dev/null +++ b/app/src/components/projects/MigrationInterruptedCard.tsx @@ -0,0 +1,74 @@ +import type { MigrationState } from "../../lib/types"; +import Button from "../ui/Button"; +import StatusIndicator from "../ui/StatusIndicator"; +import { ROLLBACK_SCOPE, formatSnapshotDate } from "./migrationCopy"; + +interface Props { + record: MigrationState; + /** Disables the action row while resume/rollback is in flight. */ + busy?: boolean; + onResume: () => void; + onRollback: () => void; +} + +/** + * A migration that got past the container swap and stopped there. + * + * This is deliberately **not** [`MigrationReportCard`]. That card's primary + * action is Keep, which means "accept this and drop the rollback image" — and + * on an unfinished migration `triple-c-snapshot-:latest` still points at + * the *old* lineage, so Keep would delete the only way back while leaving a + * container the app can no longer reason about. The backend's own message on + * this record says to resume; offering Keep beside it was the UI contradicting + * the backend and losing. + * + * So the two actions here are Resume and Roll back, and nothing else. It is + * shown ahead of any report, whether the record was found on mount or produced + * by a run that just failed — those are the same situation. + */ +export default function MigrationInterruptedCard({ + record, + busy = false, + onResume, + onRollback, +}: Props) { + const started = formatSnapshotDate(record.started_at); + + return ( +
+ + +

+ This container is part-way onto the new base: it was replaced, but the + result was never saved + {started ? `. The update started ${started}` : ""}. Resuming replays the + same plan it was given — it is the only way to finish it. +

+ + {record.report?.message && ( +

+ {record.report.message} +

+ )} + +

+ {ROLLBACK_SCOPE} +

+ +
+ + {record.rollback_image && ( + + )} +
+
+ ); +} diff --git a/app/src/components/projects/home/ContainerMigrationBanner.test.tsx b/app/src/components/projects/home/ContainerMigrationBanner.test.tsx index f9f6cdf..9b328a5 100644 --- a/app/src/components/projects/home/ContainerMigrationBanner.test.tsx +++ b/app/src/components/projects/home/ContainerMigrationBanner.test.tsx @@ -18,6 +18,7 @@ const FRESH: ContainerStaleness = { apt_delta: [], npm_global_delta: [], verbatim_paths: [], + unpreserved_data: [], outdated_package_count: 0, probe_error: null, }; @@ -40,6 +41,7 @@ function migration(overrides: Partial = {}): ContainerMigrat return { staleness: null, probing: false, + probeSettled: true, running: false, recovered: false, interrupted: null, @@ -51,7 +53,7 @@ function migration(overrides: Partial = {}): ContainerMigrat resume: vi.fn(async () => {}), keep: vi.fn(async () => {}), rollback: vi.fn(async () => {}), - dismiss: vi.fn(), + dismiss: vi.fn(async () => {}), refresh: vi.fn(async () => {}), ...overrides, }; @@ -173,7 +175,7 @@ describe("ContainerMigrationBanner", () => { }); renderBanner(m); expect( - screen.getByText(/A container base update was interrupted/i), + screen.getByText(/The container base update did not finish/i), ).toBeInTheDocument(); expect(screen.getByText(/part-way onto the new base/i)).toBeInTheDocument(); // The plain "Update container base…" call to action must not be what is @@ -207,6 +209,36 @@ describe("ContainerMigrationBanner", () => { expect(screen.getByRole("button", { name: "Resume update" })).toBeInTheDocument(); }); + it("distinguishes an unsettled probe from a running container", () => { + // "Stop the container to update its base" on a container that is already + // stopped — because the probe has not landed — reads as a bug. + renderBanner( + migration({ staleness: STALE, probing: true, probeSettled: false }), + false, + ); + expect( + screen.getByText(/Checking what this container has/i), + ).toBeInTheDocument(); + expect( + screen.queryByText(/Stop the container to update its base/i), + ).not.toBeInTheDocument(); + }); + + it("names the /var data that updating would destroy", () => { + renderBanner( + migration({ + staleness: { + ...STALE, + unpreserved_data: [ + { path: "/var/lib/postgresql", bytes: 41_000_000, file_count: 912 }, + ], + }, + }), + ); + expect(screen.getByText("/var/lib/postgresql")).toBeInTheDocument(); + expect(screen.getByText(/back this up before updating/i)).toBeInTheDocument(); + }); + describe("the report", () => { const CLEAN: MigrationReport = { phase: "succeeded", @@ -295,6 +327,39 @@ describe("ContainerMigrationBanner", () => { expect(screen.getByRole("button", { name: "Dismiss" })).toBeInTheDocument(); }); + it("never offers Keep over a container that is still mid-swap", () => { + // The failing-commit path returns a report *and* leaves the record + // interrupted. Keep would untag the rollback image and delete the record + // while `triple-c-snapshot-:latest` still points at the old lineage — + // and the backend's own message on that record says to resume. + const m = migration({ + staleness: STALE, + interrupted: { + phase: "interrupted", + from_image_id: "sha256:aaa", + to_base_id: "sha256:bbb", + started_at: "2026-08-09T10:00:00Z", + report: null, + rollback_image: "triple-c-snapshot-p1:pre-migration-20260809-100000", + staging_path: null, + options: { replay_packages: true, copy_paths: true, keep_rollback: true }, + plan: null, + }, + report: { + ...CLEAN, + phase: "failed", + message: "saving it failed. Resume it, or roll back.", + }, + }); + renderBanner(m); + expect(screen.queryByRole("button", { name: "Keep" })).not.toBeInTheDocument(); + expect( + screen.getByRole("button", { name: "Resume update" }), + ).toBeInTheDocument(); + fireEvent.click(screen.getByRole("button", { name: "Roll back" })); + expect(m.rollback).toHaveBeenCalledTimes(1); + }); + it("does not describe rollback as a time machine", () => { renderBanner(migration({ staleness: FRESH, report: CLEAN })); expect( diff --git a/app/src/components/projects/home/ContainerMigrationBanner.tsx b/app/src/components/projects/home/ContainerMigrationBanner.tsx index 25cb3fb..e8dbae5 100644 --- a/app/src/components/projects/home/ContainerMigrationBanner.tsx +++ b/app/src/components/projects/home/ContainerMigrationBanner.tsx @@ -2,7 +2,8 @@ import type { ContainerMigration } from "../../../hooks/useContainerMigration"; import Button from "../../ui/Button"; import StatusIndicator from "../../ui/StatusIndicator"; import MigrationReportCard from "../MigrationReportCard"; -import { ROLLBACK_SCOPE, formatSnapshotDate, joinFeatures } from "../migrationCopy"; +import MigrationInterruptedCard from "../MigrationInterruptedCard"; +import { formatSnapshotDate, joinFeatures } from "../migrationCopy"; interface Props { migration: ContainerMigration; @@ -31,8 +32,38 @@ export default function ContainerMigrationBanner({ canMigrate, onOpen, }: Props) { - const { staleness, running, recovered, interrupted, report, phaseMessage, busy } = - migration; + const { + staleness, + probing, + probeSettled, + running, + recovered, + interrupted, + report, + phaseMessage, + busy, + } = migration; + + // An unfinished migration outranks its own report. The report's action row + // offers Keep, and Keep on a mid-swap container drops the rollback image + // while `:latest` still points at the old lineage — the backend's message on + // the very same record says to resume. Resume is the only honest primary + // action here, so the report card is not rendered at all. + if (interrupted) { + return ( +
+ void migration.resume()} + onRollback={() => void migration.rollback()} + /> +
+ ); + } // The report outranks staleness: after a run, the outcome is the news. if (report) { @@ -50,7 +81,7 @@ export default function ContainerMigrationBanner({ busy={busy} onKeep={() => void migration.keep()} onRollback={() => void migration.rollback()} - onDismiss={migration.dismiss} + onDismiss={() => void migration.dismiss()} /> ); @@ -90,53 +121,6 @@ export default function ContainerMigrationBanner({ ); } - // Nothing is driving this one. It outranks staleness because the container is - // sitting mid-swap, and the one thing it must never do is look like a normal - // out-of-date container that the user can take or leave. - if (interrupted) { - return ( -
- -

- It started{" "} - {formatSnapshotDate(interrupted.started_at) ?? "earlier"} and the app - closed before it finished, so this container is part-way onto the new - base. Resuming replays the same plan it was given. -

-

- {ROLLBACK_SCOPE} -

-
- - {interrupted.rollback_image && ( - - )} -
-
- ); - } - if (!staleness) return null; // `stale` is deliberately false whenever `known` is false — an unestablished @@ -210,9 +194,32 @@ export default function ContainerMigrationBanner({

)} + {/* An out-of-date container that also has data under /var is the one + case where updating can cost something, so it is said here and not + only behind the button. */} + {staleness.unpreserved_data.length > 0 && ( +

+ Not carried across:{" "} + + {staleness.unpreserved_data.map((d) => d.path).join(", ")} + + + {" "} + — back this up before updating. + +

+ )} + {!canMigrate && (

- Stop the container to update its base. + {/* Distinguishing these matters: "stop the container" on a + container that is already stopped, because the probe has not + landed, reads as a bug. */} + {!probeSettled + ? probing + ? "Checking what this container has that the current base does not…" + : "That check did not complete, so what would be carried across is not known. Updating stays disabled until it does — try again once the container can be inspected." + : "Stop the container to update its base."}

)}
diff --git a/app/src/components/projects/home/ProjectHome.tsx b/app/src/components/projects/home/ProjectHome.tsx index c946be2..3b60987 100644 --- a/app/src/components/projects/home/ProjectHome.tsx +++ b/app/src/components/projects/home/ProjectHome.tsx @@ -93,11 +93,17 @@ export default function ProjectHome({ projectId, active }: Props) { // Reset does — with the extra condition that there is a container to migrate. // An interrupted migration is excluded too: its action is Resume, on the // Overview banner, not a fresh pre-flight. + // + // `probeSettled` is the fourth condition and it is not cosmetic. The probe + // takes ~6 s, and until it lands every delta the pre-flight renders reads as + // empty — so the dialog would tell the user there was nothing to copy while + // the backend was told not to copy anything. const canMigrate = isStopped && !actions.busy && !migration.running && !migration.interrupted && + migration.probeSettled && !!project.container_id; return ( diff --git a/app/src/components/projects/migrationCopy.ts b/app/src/components/projects/migrationCopy.ts index f1fb9da..026d541 100644 --- a/app/src/components/projects/migrationCopy.ts +++ b/app/src/components/projects/migrationCopy.ts @@ -24,8 +24,23 @@ export const KEPT_AUTOMATICALLY = [ export const KEPT_WHY = "/home/claude and ~/.claude are Docker volumes. They detach from the old container and re-attach to the new one unchanged."; +/** + * The honest list of what the writable layer holds, because the modal's own + * sections name more than one thing and copy that says "the only thing" while + * the section below it offers to copy files is copy the user cannot trust. + */ export const LOST_WITHOUT_REPLAY = - "What a new base does not carry over is the root-level system packages you installed with apt. Those are the only thing this update has to put back."; + "What a new base does not carry over is what lives in the container itself: system packages you installed with apt, global npm packages, and files under /usr/local, /opt, /srv or loose in /workspace. This update puts those back."; + +/** + * The exception, and it is not a small one — so it gets its own line wherever + * the update is offered. Reinstalling `postgresql` gets the package back and an + * empty cluster with it; the ordinary Reset-free recreate keeps /var because it + * builds from the project's own saved image, so this is the one way in which + * updating the base is more destructive than leaving it alone. + */ +export const DATA_NOT_CARRIED = + "Data written under /var is not carried across and reinstalling the package does not bring it back — a database in /var/lib, a site in /var/www. Back it up from inside the container before you update."; /** * Said plainly everywhere rollback is offered. Rollback is not a time machine: @@ -45,6 +60,18 @@ export const MID_RUN_SAFETY = export const REPLAY_COST = "Needs network access and usually takes 1–2 minutes."; +/** `41.0 MB`. Sizes here are informational, so the friendlier decimal unit. */ +export function formatDataSize(bytes: number): string { + const units = ["B", "KB", "MB", "GB", "TB"]; + let value = bytes; + let unit = 0; + while (value >= 1000 && unit < units.length - 1) { + value /= 1000; + unit += 1; + } + return unit === 0 ? `${bytes} B` : `${value.toFixed(1)} ${units[unit]}`; +} + /** `1 Mar` — short enough to sit inline in the banner sentence. */ export function formatSnapshotDate(iso: string | null): string | null { if (!iso) return null; diff --git a/app/src/components/settings/ClaudeAuthModal.tsx b/app/src/components/settings/ClaudeAuthModal.tsx index 13f5be1..2845eb6 100644 --- a/app/src/components/settings/ClaudeAuthModal.tsx +++ b/app/src/components/settings/ClaudeAuthModal.tsx @@ -9,6 +9,11 @@ import { authErrorMessage, useClaudeTokenAcquisition, } from "../../hooks/useClaudeAuth"; +import { + ANTHROPIC_SIGN_IN_HOSTS, + sanitizeRelayUrl, + urlOrigin, +} from "../../lib/urlRelay"; interface Props { /** Project whose running container is borrowed to run the CLI. */ @@ -85,11 +90,30 @@ export default function ClaudeAuthModal({ ? PHASE_STATUS.finishing : PHASE_STATUS.waiting; + // Split for display only. `flow.signInUrl` has already passed the host + // allowlist; this decides which half of it an ellipsis is allowed to eat. + const signInOrigin = flow.signInUrl ? (urlOrigin(flow.signInUrl) ?? "") : ""; + const signInPath = flow.signInUrl + ? flow.signInUrl.slice(signInOrigin.length) + : ""; + const handleOpen = async () => { if (!flow.signInUrl) return; setLinkError(null); + // Re-validated at the sink. `extractSignInUrl` already applies the host + // allowlist, so a failure here means that invariant broke — which is the + // one moment it matters that the last step before the OS opener checks. + const target = sanitizeRelayUrl(flow.signInUrl, { + allowHosts: ANTHROPIC_SIGN_IN_HOSTS, + }); + if (!target) { + setLinkError( + "That link is not an Anthropic sign-in address and was not opened. Start authentication again.", + ); + return; + } try { - await openUrl(flow.signInUrl); + await openUrl(target); } catch (e) { setLinkError( authErrorMessage( @@ -103,8 +127,19 @@ export default function ClaudeAuthModal({ const handleCopy = async () => { if (!flow.signInUrl) return; setLinkError(null); + // Copying is the manual route to the same browser, so it gets the same + // check — a link too dangerous to open is too dangerous to hand over. + const target = sanitizeRelayUrl(flow.signInUrl, { + allowHosts: ANTHROPIC_SIGN_IN_HOSTS, + }); + if (!target) { + setLinkError( + "That link is not an Anthropic sign-in address and was not copied. Start authentication again.", + ); + return; + } try { - await navigator.clipboard.writeText(flow.signInUrl); + await navigator.clipboard.writeText(target); setCopied(true); } catch (e) { setLinkError( @@ -185,16 +220,32 @@ export default function ClaudeAuthModal({ {flow.signInUrl ? (
+ {/* The origin is rendered at full length and the path is the + only part allowed to truncate. A single `truncate` element + showing the whole URL is a spoofing primitive: pad the + front and the ellipsis eats the half that decides where the + user's Anthropic password goes. */} { e.preventDefault(); void handleOpen(); }} - className="min-w-0 flex-1 truncate px-2.5 py-1.5 font-mono text-xs text-[var(--accent)] hover:text-[var(--accent-hover)] bg-[var(--bg-primary)] border border-[var(--border-color)] rounded-[var(--radius-control)] transition-colors" + className="flex min-w-0 flex-1 items-baseline px-2.5 py-1.5 font-mono text-xs text-[var(--accent)] hover:text-[var(--accent-hover)] bg-[var(--bg-primary)] border border-[var(--border-color)] rounded-[var(--radius-control)] transition-colors" title={flow.signInUrl} > - {flow.signInUrl} + + {signInOrigin} + + + {signInPath} + +
+

+ The gateway container is still present. Stop it here if it is still running; it + will not be started again while the gateway is off. +

+ {errorLine} +
+ )} + {gateway.enabled && ( <> {/* ── Container ─────────────────────────────────────────────── */} @@ -247,11 +283,7 @@ export default function GatewaySettings() { )} - {error && ( -

- {error} -

- )} + {errorLine} {/* ── Provider ──────────────────────────────────────────────── */}

Set a project's backend to OpenAI Compatible and use these - values. On native Linux Docker, where{" "} - host.docker.internal is not injected into - containers, use http://172.17.0.1:{gateway.port}{" "} - instead. + values. The base URL below is the one your Docker engine actually needs —{" "} + host.docker.internal on Docker Desktop, the + bridge gateway address on native Linux, where that name is not injected into + containers.

diff --git a/app/src/components/settings/SharedAuthSettings.test.tsx b/app/src/components/settings/SharedAuthSettings.test.tsx index 83dc956..b7a0ff0 100644 --- a/app/src/components/settings/SharedAuthSettings.test.tsx +++ b/app/src/components/settings/SharedAuthSettings.test.tsx @@ -1,7 +1,8 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { render, screen, waitFor } from "@testing-library/react"; +import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import SharedAuthSettings from "./SharedAuthSettings"; -import type { Project } from "../../lib/types"; +import { useAppState } from "../../store/appState"; +import type { ClearTokenOutcome, Project } from "../../lib/types"; const hasClaudeToken = vi.fn(); const clearClaudeToken = vi.fn(); @@ -63,8 +64,29 @@ describe("SharedAuthSettings", () => { vi.clearAllMocks(); projects = []; hasClaudeToken.mockResolvedValue(false); + useAppState.setState({ toasts: [] }); }); + /** Open the confirmation and go through with it. */ + async function revoke(outcome: Partial) { + projects = [running()]; + hasClaudeToken.mockResolvedValue(true); + clearClaudeToken.mockResolvedValue({ + snapshots_scrubbed: [], + snapshots_failed: [], + snapshots_superseded: [], + docker_unavailable: null, + ...outcome, + }); + 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), + ); + return useAppState.getState().toasts[0]; + } + it("disables Authenticate and says why when nothing is running", async () => { projects = [baseProject]; render(); @@ -123,4 +145,48 @@ describe("SharedAuthSettings", () => { await screen.findByText("keyring backend unavailable"); expect(screen.queryByRole("button", { name: "Revoke" })).not.toBeInTheDocument(); }); + + // ── Revoking has to tell the truth ────────────────────────────────────── + // Deleting the keychain entry is only part of it. `docker commit` copies the + // token into each project's snapshot image, and an image outlives every + // container built from it — so a "removed" message while a snapshot still + // holds a live ~1-year credential is the wrong thing to say. + + it("says so plainly when snapshot images were cleared too", async () => { + const toast = await revoke({ + snapshots_scrubbed: ["triple-c-snapshot-p1:latest"], + }); + expect(toast.kind).toBe("success"); + expect(toast.message).toMatch(/1 snapshot image/); + }); + + it("reports an error, not success, when an image still holds the token", async () => { + const toast = await revoke({ + snapshots_failed: ["triple-c-snapshot-p1:latest: image has child images"], + }); + expect(toast.kind).toBe("error"); + expect(toast.message).toMatch(/still in some images/i); + expect(toast.detail).toMatch(/triple-c-snapshot-p1/); + }); + + it("does not claim the images are clean when Docker could not be reached", async () => { + const toast = await revoke({ docker_unavailable: "Docker is not running" }); + expect(toast.kind).toBe("error"); + expect(toast.detail).toMatch(/Docker could not be reached/); + }); + + it("mentions a retained image layer without calling the revoke a failure", async () => { + const toast = await revoke({ + snapshots_scrubbed: ["triple-c-snapshot-p1:latest"], + snapshots_superseded: ["triple-c-snapshot-p1:latest"], + }); + expect(toast.kind).toBe("success"); + expect(toast.detail).toMatch(/still on disk because a container is running/); + }); + + it("still succeeds plainly when there was nothing to scrub", async () => { + const toast = await revoke({}); + expect(toast.kind).toBe("success"); + expect(toast.message).toBe("Shared Claude token removed from the keychain."); + }); }); diff --git a/app/src/components/settings/SharedAuthSettings.tsx b/app/src/components/settings/SharedAuthSettings.tsx index 99d33c3..c4ada5b 100644 --- a/app/src/components/settings/SharedAuthSettings.tsx +++ b/app/src/components/settings/SharedAuthSettings.tsx @@ -64,13 +64,52 @@ export default function SharedAuthSettings() { const handleRevoke = async () => { setRevoking(true); try { - await clearClaudeToken(); + const outcome = await clearClaudeToken(); setConfirmRevoke(false); await refresh(); - pushToast({ - kind: "success", - message: "Shared Claude token removed from the keychain.", - }); + + // 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. + if (outcome.docker_unavailable) { + pushToast({ + kind: "error", + message: "Token removed from the keychain, but snapshots 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.", + }); + } else if (outcome.snapshots_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 ` + + "still contain the token, readable via `docker image inspect`. Reset those " + + `projects to remove the images. Details: ${outcome.snapshots_failed.join("; ")}`, + }); + } else if (outcome.snapshots_scrubbed.length > 0) { + pushToast({ + kind: "success", + message: `Shared Claude token removed, and cleared from ${outcome.snapshots_scrubbed.length} snapshot image(s).`, + detail: + outcome.snapshots_superseded.length > 0 + ? "The pre-rewrite image layer for " + + `${outcome.snapshots_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, + }); + } else { + pushToast({ + kind: "success", + message: "Shared Claude token removed from the keychain.", + }); + } } catch (e) { pushToast({ kind: "error", @@ -220,6 +259,13 @@ export default function SharedAuthSettings() { container starts. Existing running containers keep working until they are restarted.

+

+ Each project’s snapshot image is also rewritten, 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. +

)} diff --git a/app/src/components/terminal/TerminalView.tsx b/app/src/components/terminal/TerminalView.tsx index 4f5886e..dfebdc8 100644 --- a/app/src/components/terminal/TerminalView.tsx +++ b/app/src/components/terminal/TerminalView.tsx @@ -14,6 +14,7 @@ import { RelayRateLimiter, URL_RELAY_OSC, parseUrlRelayOsc, + sanitizeRelayUrl, } from "../../lib/urlRelay"; import UrlToast from "./UrlToast"; import { trimSelection } from "./trimSelection"; @@ -45,10 +46,38 @@ export default function TerminalView({ sessionId, active }: Props) { // One toast slot, two producers: the heuristic long-URL detector and the // container's explicit "open this in the host browser" relay (OSC 7777). // Sharing the slot keeps them from stacking on top of each other. - const [urlPrompt, setUrlPrompt] = useState<{ url: string; label: string } | null>( - null, - ); + // + // Both producers read the container's PTY output, so both are untrusted, and + // both must go through `sanitizeRelayUrl` before anything is stored here — + // see `promptUrl` below, which is the only writer. + // + // `seq` exists because the slot is shared and long-lived: a second prompt + // replacing a first would otherwise mutate the toast in place, swapping the + // text under a user who is mid-read and mid-click. Keying the toast on it + // remounts the component, so a new URL is unmistakably a new prompt. + const [urlPrompt, setUrlPrompt] = useState<{ + url: string; + label: string; + seq: number; + } | null>(null); + const promptSeqRef = useRef(0); const relayLimiterRef = useRef(new RelayRateLimiter()); + + /** + * The only writer of the prompt slot. Re-validates whatever the caller + * found: the OSC relay branch has already been through `parseUrlRelayOsc`, + * but the heuristic detector branch has been through nothing at all, and a + * raw regex match is exactly the input `sanitizeRelayUrl` exists to refuse. + */ + const promptUrl = useCallback((raw: string, label: string) => { + const url = sanitizeRelayUrl(raw); + if (!url) { + console.warn("Refusing to prompt for a URL that failed validation"); + return; + } + promptSeqRef.current += 1; + setUrlPrompt({ url, label, seq: promptSeqRef.current }); + }, []); const [imagePasteMsg, setImagePasteMsg] = useState(null); const [isAtBottom, setIsAtBottom] = useState(true); const [isAutoFollow, setIsAutoFollow] = useState(true); @@ -162,9 +191,19 @@ export default function TerminalView({ sessionId, active }: Props) { // Web links addon — opens URLs in host browser via Tauri, with a permissive regex // that matches URLs even if they lack trailing path segments (the default regex // misses OAuth URLs that end mid-line). - const urlRegex = /https?:\/\/[^\s'"\x07]+/; + // eslint-disable-next-line no-control-regex + const urlRegex = /https?:\/\/[^\s'"`<>\x00-\x20\x7f]+/; const webLinksAddon = new WebLinksAddon((_event, uri) => { - openUrl(uri).catch((e) => console.error("Failed to open URL:", e)); + // Same sink, same rule: what xterm matched came off the container's + // output, so it is validated before it reaches the OS opener. A click + // here is a deliberate act on visible text, but "visible" is exactly + // what a userinfo-spoofed URL subverts. + const safe = sanitizeRelayUrl(uri); + if (!safe) { + console.warn("Refusing to open a link that failed validation"); + return; + } + openUrl(safe).catch((e) => console.error("Failed to open URL:", e)); }, { urlRegex }); term.loadAddon(webLinksAddon); @@ -244,7 +283,7 @@ export default function TerminalView({ sessionId, active }: Props) { console.warn("URL relay: rate-limited", url); return true; } - setUrlPrompt({ url, label: "Container asked to open a URL" }); + promptUrl(url, "Container asked to open a URL"); return true; }); @@ -332,7 +371,7 @@ export default function TerminalView({ sessionId, active }: Props) { let aborted = false; const detector = new UrlDetector((url) => - setUrlPrompt({ url, label: "Long URL detected" }), + promptUrl(url, "Long URL detected"), ); detectorRef.current = detector; @@ -477,12 +516,17 @@ export default function TerminalView({ sessionId, active }: Props) { }, [imagePasteMsg]); const handleOpenUrl = useCallback(() => { - if (urlPrompt) { - openUrl(urlPrompt.url).catch((e) => - console.error("Failed to open URL:", e), - ); - setUrlPrompt(null); + if (!urlPrompt) return; + // Validated again at the sink. `promptUrl` is the only writer and already + // sanitizes, so this can only fail if that invariant is broken — which is + // precisely when it matters that the last thing before `openUrl` checks. + const safe = sanitizeRelayUrl(urlPrompt.url); + setUrlPrompt(null); + if (!safe) { + console.warn("Refusing to open a URL that failed validation"); + return; } + openUrl(safe).catch((e) => console.error("Failed to open URL:", e)); }, [urlPrompt]); const handleScrollToBottom = useCallback(() => { @@ -557,6 +601,8 @@ export default function TerminalView({ sessionId, active }: Props) { > {urlPrompt && ( { + const noop = () => {}; + + it("shows the origin separately from the truncatable remainder", () => { + render( + , + ); + expect(screen.getByTestId("url-toast-origin")).toHaveTextContent( + "https://github.com", + ); + expect(screen.getByTestId("url-toast-rest")).toHaveTextContent( + "/login/device?code=ABCD-EFGH", + ); + }); + + it("keeps the origin intact when the path is long enough to push it out", () => { + const url = `https://evil.tld/${"padding/".repeat(200)}end`; + render(); + // The registrable domain must be present in its own element, whole. A + // single ellipsised line would render this and show only the padding. + expect(screen.getByTestId("url-toast-origin")).toHaveTextContent( + "https://evil.tld", + ); + }); + + it("exposes the whole URL as a tooltip", () => { + const url = "https://example.com/a/b?c=d"; + render(); + expect(screen.getByTestId("url-toast-url")).toHaveAttribute("title", url); + }); + + it("announces itself, so a replacement prompt is not silent", () => { + render( + , + ); + expect(screen.getByRole("status")).toBeInTheDocument(); + }); + + it("opens only via the button, never on its own", () => { + const onOpen = vi.fn(); + render( + , + ); + expect(onOpen).not.toHaveBeenCalled(); + screen.getByRole("button", { name: "Open" }).click(); + expect(onOpen).toHaveBeenCalledTimes(1); + }); +}); diff --git a/app/src/components/terminal/UrlToast.tsx b/app/src/components/terminal/UrlToast.tsx index d745715..dcacbc4 100644 --- a/app/src/components/terminal/UrlToast.tsx +++ b/app/src/components/terminal/UrlToast.tsx @@ -1,4 +1,7 @@ +import { urlOrigin } from "../../lib/urlRelay"; + interface Props { + /** Already validated by `sanitizeRelayUrl` — this component never opens it. */ url: string; /** Heading above the URL. Says why the toast appeared. */ label?: string; @@ -6,15 +9,36 @@ interface Props { onDismiss: () => void; } +/** + * Confirmation prompt for a URL something inside the container wants opened in + * the host browser. + * + * The origin is rendered separately from the rest of the URL and is never + * truncated. A single `nowrap`/`ellipsis` line looks tidy but is a spoofing + * primitive: `https://accounts.example.com/....(600 chars)....@evil.tld/` shows + * the reassuring half and hides the half that decides where the request goes. + * `sanitizeRelayUrl` already rejects the userinfo form; showing the origin in + * full is the belt to that braces, and it also covers the plainer case of a + * long path pushing the host out of view. + * + * Render this with a `key` that changes whenever the URL does. The prompt slot + * is shared and long-lived, so without one React mutates the node in place: the + * text swaps with no animation, and a user reading URL A can click Open on URL + * B that arrived a second later. + */ export default function UrlToast({ url, label = "Long URL detected", onOpen, onDismiss, }: Props) { + const origin = urlOrigin(url); + const rest = origin && url.startsWith(origin) ? url.slice(origin.length) : url; + return (
- {url} + {origin && ( + + {origin} + + )} + + {rest} +
diff --git a/app/src/hooks/useClaudeAuth.test.ts b/app/src/hooks/useClaudeAuth.test.ts index c8cfe8f..d9e9894 100644 --- a/app/src/hooks/useClaudeAuth.test.ts +++ b/app/src/hooks/useClaudeAuth.test.ts @@ -35,6 +35,50 @@ describe("extractSignInUrl", () => { const text = `https://claude.ai/oauth/authorize?code=tr\n${full}\n`; expect(extractSignInUrl(text)).toBe(full); }); + + // ── The spoof this function exists to refuse ────────────────────────────── + // The transcript is container output. Everything below is a URL a misbehaving + // sandboxed agent can print at will, and the modal renders whatever comes + // back under a heading that says "Sign in with Anthropic". + + it("rejects userinfo that makes an attacker's host read as Anthropic's", () => { + // Displays as `https://claude.ai...` in anything that truncates; navigates + // to evil.tld and harvests the real credential. + const spoof = + "https://claude.ai@evil.tld/oauth/authorize?" + "padding=".repeat(40); + expect(extractSignInUrl(`Use this url to sign in:\n${spoof}\n`)).toBeNull(); + }); + + it("does not let a longer hostile URL displace the real one", () => { + const real = "https://claude.ai/oauth/authorize?code=true&client_id=abc"; + const longer = + "https://evil.tld/oauth/authorize?" + "x".repeat(real.length * 2); + expect(extractSignInUrl(`${real}\n${longer}\n`)).toBe(real); + // ...and the same when the hostile one is printed first. + expect(extractSignInUrl(`${longer}\n${real}\n`)).toBe(real); + }); + + it("rejects a host that merely contains an Anthropic domain", () => { + expect( + extractSignInUrl("Sign in: https://claude.ai.evil.tld/oauth/authorize\n"), + ).toBeNull(); + expect( + extractSignInUrl("Sign in: https://evil.tld/claude.ai/oauth/authorize\n"), + ).toBeNull(); + }); + + it("rejects non-http schemes and control characters smuggled into the link", () => { + expect(extractSignInUrl("Open javascript:alert(1) to continue\n")).toBeNull(); + expect( + extractSignInUrl("https://claude.ai/oauth\u0000/authorize\n"), + ).toBe("https://claude.ai/oauth"); + }); + + it("takes the first legitimate link, not the longest", () => { + const first = "https://claude.ai/oauth/authorize?code=true"; + const second = "https://platform.claude.com/oauth/authorize?code=true&more=1"; + expect(extractSignInUrl(`${first}\n${second}\n`)).toBe(first); + }); }); describe("authErrorMessage", () => { diff --git a/app/src/hooks/useClaudeAuth.ts b/app/src/hooks/useClaudeAuth.ts index 238dab5..c42d70f 100644 --- a/app/src/hooks/useClaudeAuth.ts +++ b/app/src/hooks/useClaudeAuth.ts @@ -1,6 +1,7 @@ import { useCallback, useEffect, useMemo, useRef, useState } from "react"; import { listen, type UnlistenFn } from "@tauri-apps/api/event"; import * as commands from "../lib/tauri-commands"; +import { ANTHROPIC_SIGN_IN_HOSTS, sanitizeRelayUrl } from "../lib/urlRelay"; import type { ClaudeTokenOutputEvent, ClaudeTokenProgressEvent, @@ -39,25 +40,43 @@ export function authErrorMessage(e: unknown, fallback: string): string { /** * Pick the sign-in URL out of `claude setup-token`'s transcript. * - * Prefers an OAuth-looking URL, and among candidates prefers the longest: a - * TUI repaints, and a repaint can land a truncated copy of the same URL in the - * transcript. Longest-wins means a partial frame never replaces the full link. + * **The transcript is container output, so every candidate here is + * attacker-controlled if the sandboxed agent misbehaves.** It is then rendered + * under a heading that says "Sign in with Anthropic" and handed to the host + * browser, which makes this the highest-value URL in the app to spoof: a user + * who follows it types their real Anthropic credentials into whatever it + * resolves to. Three rules follow, and none of them are optional: + * + * - Every candidate goes through the shared {@link sanitizeRelayUrl}, with a + * host allowlist. Only Anthropic's own domains can be a sign-in link; + * userinfo (`https://claude.ai@evil.tld/...`) and control characters are + * rejected there. + * - The **first** surviving candidate wins. The previous rule was + * longest-wins, which handed the choice to the attacker: pad a hostile URL + * and it displaces the real one that came before it. + * - The one exception is a candidate that *extends* the current pick, i.e. + * starts with it. That is the case longest-wins existed for — a repainting + * TUI can land a truncated copy of the same link in the transcript before + * the complete one — and it cannot swap the origin, because a longer string + * with the same prefix has the same host. */ export function extractSignInUrl(text: string): string | null { - const matches = text.match(/https?:\/\/[^\s"'<>`]+/g); + // eslint-disable-next-line no-control-regex + const matches = text.match(/https?:\/\/[^\s"'`<>\x00-\x20\x7f]+/g); if (!matches) return null; const cleaned = matches // Trailing punctuation belongs to the prose, not the URL. .map((url) => url.replace(/[.,;:!?)\]}>'"]+$/, "")) - .filter((url) => url.length > "https://".length); + .map((url) => sanitizeRelayUrl(url, { allowHosts: ANTHROPIC_SIGN_IN_HOSTS })) + .filter((url): url is string => url !== null); const oauth = cleaned.filter((url) => /oauth|authorize|login/i.test(url)); const pool = oauth.length > 0 ? oauth : cleaned; let best: string | null = null; for (const url of pool) { - if (best === null || url.length >= best.length) best = url; + if (best === null || url.startsWith(best)) best = url; } return best; } diff --git a/app/src/hooks/useContainerMigration.test.tsx b/app/src/hooks/useContainerMigration.test.tsx index 7449668..22e07f9 100644 --- a/app/src/hooks/useContainerMigration.test.tsx +++ b/app/src/hooks/useContainerMigration.test.tsx @@ -45,6 +45,7 @@ const STALE: ContainerStaleness = { apt_delta: ["socat"], npm_global_delta: [], verbatim_paths: [], + unpreserved_data: [], outdated_package_count: 61, probe_error: null, }; @@ -199,6 +200,61 @@ describe("useContainerMigration", () => { ); }); + it("resolves the record when a report is dismissed, not just the local state", async () => { + // Dismiss is the *only* action offered when `rollback_available` is false. + // As local state it left an `awaiting-confirmation` record on disk that + // came back on the next mount and made every future migration refuse with + // "already has a finished migration waiting for a decision" — unrecoverable + // without deleting JSON by hand. + confirmMigration.mockResolvedValue(undefined); + migrateProjectToBase.mockResolvedValue({ ...CLEAN, rollback_available: false }); + const { result } = renderHook(() => useContainerMigration(project)); + await act(async () => { + await result.current.start(OPTIONS); + }); + expect(result.current.report).not.toBeNull(); + + await act(async () => { + await result.current.dismiss(); + }); + expect(confirmMigration).toHaveBeenCalledWith("p1"); + expect(result.current.report).toBeNull(); + }); + + it("keeps the report on screen when dismissing it could not be recorded", async () => { + confirmMigration.mockRejectedValue(new Error("disk is read-only")); + migrateProjectToBase.mockResolvedValue({ ...CLEAN, rollback_available: false }); + const { result } = renderHook(() => useContainerMigration(project)); + await act(async () => { + await result.current.start(OPTIONS); + }); + await act(async () => { + await result.current.dismiss(); + }); + expect(result.current.report).not.toBeNull(); + expect(pushToast).toHaveBeenCalledWith( + expect.objectContaining({ kind: "error" }), + ); + }); + + it("reports the probe as settled only once it has actually landed", async () => { + // Everything downstream reads an unlanded probe's empty arrays as "nothing + // found", so "settled" has to be a distinct signal from "not probing". + getContainerStaleness.mockResolvedValue({ + ...STALE, + probe_error: "could not exec in the container", + }); + const { result } = renderHook(() => useContainerMigration(project)); + await waitFor(() => expect(result.current.probing).toBe(false)); + expect(result.current.probeSettled).toBe(false); + + getContainerStaleness.mockResolvedValue(STALE); + await act(async () => { + await result.current.refresh(); + }); + expect(result.current.probeSettled).toBe(true); + }); + describe("crash recovery", () => { it("adopts a run that was still in progress, and polls it to a report", async () => { getMigrationState.mockResolvedValue(state()); @@ -240,6 +296,8 @@ describe("useContainerMigration", () => { const { result } = renderHook(() => useContainerMigration(project)); await waitFor(() => expect(result.current.interrupted).not.toBeNull()); + // The resume worked, so the backend cleared the record. + getMigrationState.mockResolvedValue(state({ phase: "awaiting-confirmation" })); await act(async () => { await result.current.resume(); }); @@ -250,6 +308,42 @@ describe("useContainerMigration", () => { expect(result.current.report).toEqual(CLEAN); }); + it("keeps a mid-swap container visible when the resume itself fails", async () => { + // The old behaviour nulled `interrupted` at the top of `start` and never + // looked again, so a failed resume hid a half-migrated container for the + // rest of the session — leaving Keep as the only offered action over it. + getMigrationState.mockResolvedValue(state({ phase: "interrupted" })); + migrateProjectToBase.mockRejectedValue(new Error("docker daemon went away")); + const { result } = renderHook(() => useContainerMigration(project)); + await waitFor(() => expect(result.current.interrupted).not.toBeNull()); + + await act(async () => { + await result.current.resume(); + }); + expect(result.current.report?.phase).toBe("failed"); + expect(result.current.interrupted?.phase).toBe("interrupted"); + }); + + it("adopts the interrupted record a failed fresh run leaves behind", async () => { + // `commit_container_snapshot` failing after the swap returns a report and + // writes `interrupted`. Both have to reach the UI, or Keep is offered + // over a container the app can no longer reason about. + getMigrationState.mockResolvedValue(null); + const { result } = renderHook(() => useContainerMigration(project)); + await waitFor(() => expect(result.current.staleness).toEqual(STALE)); + + migrateProjectToBase.mockResolvedValue({ + ...CLEAN, + phase: "failed", + message: "saving it failed. Resume it, or roll back.", + }); + getMigrationState.mockResolvedValue(state({ phase: "interrupted" })); + await act(async () => { + await result.current.start(OPTIONS); + }); + expect(result.current.interrupted?.phase).toBe("interrupted"); + }); + it("ignores an unrecognised phase from a future build rather than crashing", async () => { getMigrationState.mockResolvedValue(state({ phase: "quantum-tunnelling" })); const { result } = renderHook(() => useContainerMigration(project)); diff --git a/app/src/hooks/useContainerMigration.ts b/app/src/hooks/useContainerMigration.ts index b66b97f..9f8e8df 100644 --- a/app/src/hooks/useContainerMigration.ts +++ b/app/src/hooks/useContainerMigration.ts @@ -29,6 +29,17 @@ export interface ContainerMigration { /** Null until the first probe returns, or when the container has never been created. */ staleness: ContainerStaleness | null; probing: boolean; + /** + * The probe has landed with a complete answer. + * + * Until it does, `apt_delta`, `verbatim_paths` and `unpreserved_data` are all + * "not known", which is indistinguishable from "empty" at every call site + * that reads them. Starting a migration in that state means the modal telling + * the user there was nothing to copy while the backend quietly skips copying + * — so the action is gated on this, not on the probe merely having been + * kicked off. + */ + probeSettled: boolean; /** True while a migration is running — whether we started it or found it. */ running: boolean; /** True when the run in progress was recovered from disk, not started here. */ @@ -51,8 +62,15 @@ export interface ContainerMigration { start: (options: MigrationOptions) => Promise; keep: () => Promise; rollback: () => Promise; - /** Clear a report we cannot act on (failed / rolled back). Local only. */ - dismiss: () => void; + /** + * Acknowledge a report there is nothing to keep or roll back. + * + * It has to reach the backend, not just clear local state: an + * `awaiting-confirmation` record that is never resolved comes back on the + * next mount *and* makes every future migration refuse with "already has a + * finished migration waiting for a decision". + */ + dismiss: () => Promise; refresh: () => Promise; } @@ -186,6 +204,25 @@ export function useContainerMigration(project: Project): ContainerMigration { ); }, [progress, running]); + /** + * Re-read the persisted record after a run settles. + * + * A migration that got past the container swap and then failed leaves the + * record at `interrupted` — the container is mid-swap and the only correct + * next actions are Resume and Roll back. Without this the hook would show the + * failure report's Keep button over a half-migrated container, and a *failed + * resume* would clear `interrupted` and never look again, hiding the mid-swap + * container for the rest of the session. + */ + const adoptRecordAfterRun = useCallback(async () => { + try { + const state = await commands.getMigrationState(projectId); + setInterrupted(state?.phase === INTERRUPTED ? state : null); + } catch { + /* Leave whatever we had; a transient IPC failure is not an outcome. */ + } + }, [projectId]); + const start = useCallback( async (options: MigrationOptions) => { setLog([]); @@ -213,10 +250,11 @@ export function useContainerMigration(project: Project): ContainerMigration { } finally { setRunning(false); useAppState.getState().setContainerProgress(projectId, null); + await adoptRecordAfterRun(); void refresh(); } }, - [projectId, refresh], + [projectId, refresh, adoptRecordAfterRun], ); /** @@ -271,11 +309,35 @@ export function useContainerMigration(project: Project): ContainerMigration { } }, [projectId, project.name, refresh, pushToast]); - const dismiss = useCallback(() => setReport(null), []); + /** + * Dismiss resolves the record; it is not a local hide. + * + * `confirm_migration` is the backend's "this decision is made": it drops the + * rollback tag (there is none in this case), deletes the staged payload and + * removes the state file. Skipping it left an `awaiting-confirmation` record + * on disk that reappeared on every mount and made `migrate_project_to_base` + * refuse forever — recoverable only by deleting JSON by hand. + */ + const dismiss = useCallback(async () => { + setBusy(true); + try { + await commands.confirmMigration(projectId); + setReport(null); + } catch (e) { + pushToast({ + kind: "error", + message: `Could not clear the update record for “${project.name}”`, + detail: String(e), + }); + } finally { + setBusy(false); + } + }, [projectId, project.name, pushToast]); return { staleness, probing, + probeSettled: !probing && staleness !== null && !staleness.probe_error, running, recovered, interrupted, diff --git a/app/src/hooks/useDocker.test.tsx b/app/src/hooks/useDocker.test.tsx new file mode 100644 index 0000000..dbdbb68 --- /dev/null +++ b/app/src/hooks/useDocker.test.tsx @@ -0,0 +1,87 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { act, renderHook } from "@testing-library/react"; +import { useDocker } from "./useDocker"; + +const checkDocker = vi.fn(); +const checkImageExists = vi.fn(); + +vi.mock("../lib/tauri-commands", () => ({ + checkDocker: () => checkDocker(), + checkImageExists: () => checkImageExists(), + buildImage: vi.fn(), + pullImage: vi.fn(), +})); + +vi.mock("@tauri-apps/api/event", () => ({ listen: vi.fn(async () => vi.fn()) })); + +const setDockerAvailable = vi.fn(); +const setImageExists = vi.fn(); + +vi.mock("../store/appState", () => ({ + useAppState: (selector: (s: unknown) => unknown) => + selector({ + dockerAvailable: false, + setDockerAvailable, + imageExists: false, + setImageExists, + }), +})); + +/** Let the interval fire and its awaited body settle. */ +const tick = async () => { + await act(async () => { + vi.advanceTimersByTime(5000); + await Promise.resolve(); + await Promise.resolve(); + await Promise.resolve(); + }); +}; + +describe("useDocker.startDockerPolling", () => { + beforeEach(() => { + vi.clearAllMocks(); + vi.useFakeTimers(); + checkImageExists.mockResolvedValue(true); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it("runs onAvailable once, after Docker is marked available and the image re-checked", async () => { + checkDocker.mockResolvedValueOnce(false).mockResolvedValue(true); + const onAvailable = vi.fn(); + + const { result } = renderHook(() => useDocker()); + act(() => { + result.current.startDockerPolling(onAvailable); + }); + + await tick(); + expect(onAvailable).not.toHaveBeenCalled(); + + await tick(); + expect(setDockerAvailable).toHaveBeenCalledWith(true); + expect(setImageExists).toHaveBeenCalledWith(true); + expect(onAvailable).toHaveBeenCalledTimes(1); + + // Polling stopped, so no second invocation. + await tick(); + expect(onAvailable).toHaveBeenCalledTimes(1); + }); + + it("still works without a callback and can be cancelled by its cleanup", async () => { + checkDocker.mockResolvedValue(true); + + const { result } = renderHook(() => useDocker()); + let stop: () => void = () => {}; + act(() => { + stop = result.current.startDockerPolling(); + }); + act(() => stop()); + + await tick(); + expect(checkDocker).not.toHaveBeenCalled(); + expect(setDockerAvailable).not.toHaveBeenCalled(); + }); +}); diff --git a/app/src/hooks/useDocker.ts b/app/src/hooks/useDocker.ts index bb948fb..f195708 100644 --- a/app/src/hooks/useDocker.ts +++ b/app/src/hooks/useDocker.ts @@ -61,7 +61,15 @@ export function useDocker() { const pollingRef = useRef | null>(null); - const startDockerPolling = useCallback(() => { + /** + * Poll until Docker appears, then stop. + * + * `onAvailable` runs exactly once, after `dockerAvailable` is set and the + * image has been re-checked. It exists because a session that started before + * the daemon was up otherwise never does the "Docker is up" work — status + * reconciliation, interrupted-migration recovery, loading the project list. + */ + const startDockerPolling = useCallback((onAvailable?: () => void | Promise) => { // Don't start if already polling if (pollingRef.current) return () => {}; @@ -79,6 +87,11 @@ export function useDocker() { } catch { setImageExists(false); } + try { + await onAvailable?.(); + } catch (e) { + console.error("Docker-available callback failed:", e); + } } } catch { // Still not available, keep polling diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index 6766e62..8e69678 100644 --- a/app/src/lib/tauri-commands.ts +++ b/app/src/lib/tauri-commands.ts @@ -1,5 +1,5 @@ import { invoke } from "@tauri-apps/api/core"; -import type { Project, ProjectPath, ContainerInfo, SiblingContainer, AppSettings, UpdateInfo, ImageUpdateInfo, FileEntry, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, PlaywrightDetection, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState } from "./types"; +import type { Project, ProjectPath, ContainerInfo, SiblingContainer, AppSettings, UpdateInfo, ImageUpdateInfo, FileEntry, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, PlaywrightDetection, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState, ClearTokenOutcome } from "./types"; // Docker export const checkDocker = () => invoke("check_docker"); @@ -199,7 +199,10 @@ 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"); -export const clearClaudeToken = () => invoke("clear_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. */ +export const clearClaudeToken = () => + invoke("clear_claude_token"); // Container base-image migration — move a project onto the current base image // without deleting its volumes. Reset is the destructive alternative: it wipes diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index 5ed8040..427f10c 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -466,6 +466,29 @@ export interface BrowserViewChangedEvent { /** Payload of the `claude-token-progress` event: milestones during * `acquire_claude_token`. Never contains the token. */ +/** + * Result of `clear_claude_token`. + * + * Revoking is not one action but three: delete the keychain entry (always + * succeeds or throws), let container recreation clear the env var, and rewrite + * any snapshot image that still has the token baked into its `Config.Env`. + * Only the last one can partly fail, and when it does the user has to be told + * — a token sitting in an image is readable by `docker image inspect` for as + * long as the image exists. + */ +export interface ClearTokenOutcome { + /** Snapshot images that were holding the token and have been rewritten. */ + snapshots_scrubbed: string[]; + /** Images still holding it, each with the reason. Non-empty = incomplete. */ + snapshots_failed: string[]; + /** Rewritten, but the pre-rewrite image object could not be deleted because a + * container still runs off it. Clears itself when that container is + * recreated — worth mentioning, not worth alarming about. */ + snapshots_superseded: string[]; + /** Set when Docker could not be reached, so nothing is known. */ + docker_unavailable: string | null; +} + export interface ClaudeTokenProgressEvent { project_id: string; message: string; @@ -507,6 +530,23 @@ export interface PackageFailure { reason: string; } +/** A data-bearing directory a migration destroys and cannot put back. + * + * Service state lives under /var — a database's files in /var/lib/, + * a site in /var/www — and none of it is carried across: replaying the apt + * delta reinstalls the *package* onto the new base and hands back an empty + * data directory. The ordinary recreate path does not have this problem + * because it creates from the project's own snapshot, so migration has to say + * so out loud before anything is touched. */ +export interface UnpreservedData { + /** Absolute path, e.g. `/var/lib/postgresql`. */ + path: string; + /** Total size of the non-package files beneath it. */ + bytes: number; + /** How many non-package files it holds. */ + file_count: number; +} + /** Why a project is worth migrating, and what migrating would carry across. * * An empty array always means "nothing found", never "not checked" — @@ -535,6 +575,9 @@ export interface ContainerStaleness { * be carried across. Empty when nothing user-authored was found — which is * the common case. */ verbatim_paths: string[]; + /** Data under /var that the migration destroys and cannot restore. Empty on + * an ordinary container; when it is not, the pre-flight has to lead with it. */ + unpreserved_data: UnpreservedData[]; /** dpkg packages the base carries at a different version. A drift measure, * not a promise that every one is newer. */ outdated_package_count: number; @@ -599,6 +642,9 @@ export interface MigrationPlan { npm_packages: string[]; verbatim_paths: string[]; missing_paths: string[]; + /** What the pre-flight found under /var that the migration would destroy, + * frozen so the finished report can still name it. */ + unpreserved_data: UnpreservedData[]; } /** Persisted host-side migration record. Present only while a migration is in diff --git a/app/src/lib/urlDetector.ts b/app/src/lib/urlDetector.ts index 44f781c..d9fd103 100644 --- a/app/src/lib/urlDetector.ts +++ b/app/src/lib/urlDetector.ts @@ -70,8 +70,16 @@ export class UrlDetector { if (!flat) return; - // 3. Match URLs on the flattened string — spans across wrapped lines naturally - const urlRe = /https?:\/\/[^\s'"<>\x07]+/g; + // 3. Match URLs on the flattened string — spans across wrapped lines naturally. + // The negated class stops at anything illegal in a URL, which must + // include the *whole* C0 range and DEL, not just BEL: an escape or a NUL + // swallowed into the middle of a match becomes a URL that renders as one + // thing in the toast and resolves as another. Everything emitted here is + // still re-validated by `sanitizeRelayUrl` before it can reach `openUrl`; + // stopping the match early only means the legitimate prefix survives + // instead of the whole candidate being thrown away. + // eslint-disable-next-line no-control-regex + const urlRe = /https?:\/\/[^\s'"`<>\x00-\x20\x7f]+/g; let m: RegExpExecArray | null; while ((m = urlRe.exec(flat)) !== null) { diff --git a/app/src/lib/urlRelay.embedded.test.ts b/app/src/lib/urlRelay.embedded.test.ts new file mode 100644 index 0000000..fe3b712 --- /dev/null +++ b/app/src/lib/urlRelay.embedded.test.ts @@ -0,0 +1,131 @@ +import { describe, it, expect } from "vitest"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; +import { sanitizeRelayUrl, MAX_RELAY_URL_LENGTH } from "./urlRelay"; + +/** + * The web terminal (`src-tauri/src/web_terminal/terminal.html`) is embedded + * into the Rust binary with `include_str!()` and served as one standalone + * file, so it cannot import `urlRelay.ts`. It therefore carries a hand-copied + * duplicate of `sanitizeRelayUrl` — and a hand-copied security check that no + * test can reach is a check that quietly rots. + * + * This test reaches it: it pulls the marked block straight out of the HTML, + * evaluates it, and asserts it agrees with the TypeScript original on every + * case. Divergence fails here rather than shipping. + */ + +// Vitest runs with `app/` as its root; `import.meta.url` is an http URL under +// the jsdom environment, so resolve from the working directory instead. +const HTML_PATH = resolve( + process.cwd(), + "src-tauri/src/web_terminal/terminal.html", +); + +const START_MARKER = "─── shared-url-sanitizer "; +const END_MARKER = "─── end shared-url-sanitizer "; + +/** Extract and evaluate the embedded copy. */ +function loadEmbeddedSanitizer(): (raw: unknown) => string | null { + const html = readFileSync(HTML_PATH, "utf8"); + + const start = html.indexOf(START_MARKER); + const end = html.indexOf(END_MARKER); + if (start === -1 || end === -1 || end < start) { + throw new Error( + `Could not find the shared-url-sanitizer markers in ${HTML_PATH}. ` + + "If the block was renamed or removed, update this test — do not delete it.", + ); + } + + const block = html.slice(html.indexOf("\n", start) + 1, end); + if (!block.includes("function sanitizeRelayUrl(")) { + throw new Error( + "The shared-url-sanitizer block no longer defines sanitizeRelayUrl().", + ); + } + + // `RELAY_MAX_URL` is declared elsewhere in the page; supply it here with the + // same value the TypeScript module uses, which is also what the page sets. + const factory = new Function( + "RELAY_MAX_URL", + `${block}\nreturn sanitizeRelayUrl;`, + ); + return factory(MAX_RELAY_URL_LENGTH) as (raw: unknown) => string | null; +} + +const embeddedSanitize = loadEmbeddedSanitizer(); + +/** + * Every case both copies must agree on. Deliberately the union of the two + * threat models, not the easy half. + */ +const CASES: unknown[] = [ + // Accepted. + "https://example.com/", + "http://example.com/x", + "https://EXAMPLE.com", + "https://my-host.example.com/a-b_c~d/e.f?g=h-i#j-k", + "http://127.0.0.1:41703/callback?code=abc", + " https://example.com/padded ", + "https://example.com/x\n", + "https://claude.ai/oauth/authorize?code=true&client_id=abc", + + // Scheme. + "javascript:alert(1)", + "JavaScript:alert(1)", + "data:text/html,", + "file:///etc/passwd", + "vscode://x", + "java\nscript:alert(1)", + + // Malformed / hostile. + "", + " ", + "example.com", + "https://", + "https:///etc/passwd", + "https://user:pass@example.com/", + "https://claude.ai@evil.tld/oauth/authorize", + "https://example.com/a b", + "https://example.com/a\r\nb", + "https://example.com/\u001b]0;pwned\u0007", + "https://example.com/a\u0000b", + "https://example.com/a\u007fb", + "https://example.com/a\u0085b", + "https://example.com/a\u00a0b", + 'https://example.com/a"b', + "https://example.com/a'b", + "https://example.com/a`b", + `https://example.com/${"a".repeat(MAX_RELAY_URL_LENGTH)}`, + + // Non-strings. + null, + undefined, + 42, + {}, +]; + +describe("terminal.html's embedded sanitizer", () => { + it("is present and extractable", () => { + expect(typeof embeddedSanitize).toBe("function"); + }); + + it("agrees with lib/urlRelay.ts on every case", () => { + for (const input of CASES) { + expect( + embeddedSanitize(input), + `embedded copy disagrees for input: ${JSON.stringify(input)?.slice(0, 120)}`, + ).toEqual(sanitizeRelayUrl(input)); + } + }); + + it("rejects the userinfo spoof that reads as an Anthropic origin", () => { + expect(embeddedSanitize("https://claude.ai@evil.tld/oauth/authorize")).toBeNull(); + }); + + it("rejects quote characters, which the OS opener may treat as syntax", () => { + expect(embeddedSanitize('https://example.com/a"b')).toBeNull(); + expect(embeddedSanitize("https://example.com/a`b")).toBeNull(); + }); +}); diff --git a/app/src/lib/urlRelay.test.ts b/app/src/lib/urlRelay.test.ts index cef7f81..16246ca 100644 --- a/app/src/lib/urlRelay.test.ts +++ b/app/src/lib/urlRelay.test.ts @@ -1,10 +1,12 @@ import { describe, it, expect } from "vitest"; import { + ANTHROPIC_SIGN_IN_HOSTS, MAX_RELAY_URL_LENGTH, RelayRateLimiter, URL_RELAY_OSC, parseUrlRelayOsc, sanitizeRelayUrl, + urlOrigin, } from "./urlRelay"; /** Build the OSC 7777 payload the container shim emits for `url`. */ @@ -153,6 +155,86 @@ describe("sanitizeRelayUrl — rejects malformed and hostile input", () => { }); }); +describe("sanitizeRelayUrl — quote characters", () => { + // Latent today, because the only path that would exploit it is behind a + // feature flag. Latent is not the same as absent: the character class is the + // thing standing between a container-supplied string and an OS opener that + // on Windows has historically been reached through a command interpreter. + it("rejects a double quote", () => { + expect(sanitizeRelayUrl('https://example.com/a"b')).toBeNull(); + expect(sanitizeRelayUrl('https://example.com/?q="&x=1')).toBeNull(); + }); + + it("rejects a single quote and a backtick", () => { + expect(sanitizeRelayUrl("https://example.com/a'b")).toBeNull(); + expect(sanitizeRelayUrl("https://example.com/a`b")).toBeNull(); + }); + + it("still accepts the percent-encoded forms", () => { + expect(sanitizeRelayUrl("https://example.com/a%22b")).toBe( + "https://example.com/a%22b", + ); + }); + + it("rejects C1 controls and exotic whitespace new URL() would keep", () => { + expect(sanitizeRelayUrl("https://example.com/a\u0085b")).toBeNull(); + expect(sanitizeRelayUrl("https://example.com/a\u00a0b")).toBeNull(); + expect(sanitizeRelayUrl("https://example.com/a\u3000b")).toBeNull(); + }); +}); + +describe("sanitizeRelayUrl — host allowlist", () => { + const opts = { allowHosts: ANTHROPIC_SIGN_IN_HOSTS }; + + it("accepts the domain itself and its subdomains", () => { + expect(sanitizeRelayUrl("https://claude.ai/oauth/authorize", opts)).toBe( + "https://claude.ai/oauth/authorize", + ); + expect( + sanitizeRelayUrl("https://platform.claude.com/oauth/code/callback", opts), + ).toBe("https://platform.claude.com/oauth/code/callback"); + }); + + it("rejects a lookalike that merely contains the domain", () => { + expect(sanitizeRelayUrl("https://claude.ai.evil.tld/oauth", opts)).toBeNull(); + expect(sanitizeRelayUrl("https://notclaude.ai/oauth", opts)).toBeNull(); + expect(sanitizeRelayUrl("https://evil.tld/claude.ai/oauth", opts)).toBeNull(); + }); + + it("rejects the userinfo spoof even though it reads as an allowed host", () => { + expect( + sanitizeRelayUrl("https://claude.ai@evil.tld/oauth/authorize", opts), + ).toBeNull(); + }); + + it("is case-insensitive about the host", () => { + expect(sanitizeRelayUrl("https://CLAUDE.AI/oauth", opts)).toBe( + "https://claude.ai/oauth", + ); + }); + + it("allows any host when no allowlist is given — the relay's whole point", () => { + expect(sanitizeRelayUrl("https://github.com/login/device")).toBe( + "https://github.com/login/device", + ); + }); +}); + +describe("urlOrigin", () => { + it("returns the part that decides where credentials go", () => { + expect(urlOrigin("https://claude.ai/oauth/authorize?code=true")).toBe( + "https://claude.ai", + ); + expect(urlOrigin("http://127.0.0.1:41703/callback")).toBe( + "http://127.0.0.1:41703", + ); + }); + + it("returns null rather than guessing at unparseable input", () => { + expect(urlOrigin("not a url")).toBeNull(); + }); +}); + describe("parseUrlRelayOsc", () => { it("decodes the sequence the container shim emits", () => { const url = "https://github.com/login/device"; diff --git a/app/src/lib/urlRelay.ts b/app/src/lib/urlRelay.ts index b761ec4..1940fac 100644 --- a/app/src/lib/urlRelay.ts +++ b/app/src/lib/urlRelay.ts @@ -1,5 +1,6 @@ /** - * URL relay — host side of `container/triple-c-open`. + * URL relay — host side of `container/triple-c-open` — and the single URL + * validator every `openUrl` call site in the app is required to go through. * * A CLI inside the container has no browser. When it wants to open a URL * (`gh auth login`, `aws sso login`, `gcloud auth login`, anything honouring @@ -29,6 +30,19 @@ * * Opening is never automatic — see `RelayRateLimiter` and the confirmation * toast in TerminalView. + * + * The relay is not the only route from the container to the host's browser. + * The heuristic long-URL detector (`urlDetector.ts`) and the `claude + * setup-token` sign-in link (`useClaudeAuth.ts`) both scrape the same + * untrusted PTY byte stream, so they use this validator too — with an added + * host allowlist in the sign-in case, where exactly one origin is legitimate. + * Keep this the only implementation: a second copy is a second place for a + * rule to go missing. + * + * `web_terminal/terminal.html` is the one unavoidable duplicate — it is + * embedded standalone via `include_str!()` and cannot import this module. + * `urlRelay.embedded.test.ts` extracts that copy and runs it against the same + * table of cases, so the two cannot drift silently. */ /** Private OSC identifier used by the relay. Chosen to avoid the numbers in @@ -39,22 +53,79 @@ export const URL_RELAY_OSC = 7777; export const MAX_RELAY_URL_LENGTH = 8192; /** - * Validate a URL the container asked the host to open. + * Whether `candidate` contains a character that disqualifies it before it is + * ever parsed. + * + * Whitespace and C0/DEL matter most: `new URL()` silently *strips* tab, LF and + * CR, so `"java\nscript:alert(1)"` would otherwise parse as a `javascript:` + * URL. Quote characters are rejected on top of that: `"`, `'` and a backtick + * are all illegal in a URL per RFC 3986, and this string ends up as an + * argument to an OS-level opener — a path that on Windows has historically + * run through a command interpreter, where a quote ends the argument and + * whatever follows is the next command. Nothing legitimate loses out; a URL + * that really needs one carries it percent-encoded. + * + * Written as a scan rather than a regex literal so the C0 range is expressed + * as code points and cannot be quietly mangled by an editing tool. + */ +function hasForbiddenChar(candidate: string): boolean { + for (const ch of candidate) { + const code = ch.codePointAt(0) ?? 0; + // C0 controls, space, and DEL. + if (code <= 0x20 || code === 0x7f) return true; + // C1 controls — not stripped by `new URL()`, invisible in the toast. + if (code >= 0x80 && code <= 0x9f) return true; + if (ch === '"' || ch === "'" || ch === "`") return true; + // Any other Unicode whitespace (NBSP, ideographic space, ...). + if (ch.trim() === "") return true; + } + return false; +} + +/** + * Registrable domains the Anthropic sign-in flow may send the user to. + * + * `claude setup-token` prints a `claude.ai` authorize URL and redirects to + * `platform.claude.com`; `anthropic.com` covers the console. Anything else in + * the transcript is not a sign-in link, whatever it claims. + */ +export const ANTHROPIC_SIGN_IN_HOSTS = [ + "claude.ai", + "claude.com", + "anthropic.com", +] as const; + +export interface SanitizeUrlOptions { + /** + * Registrable domains the URL's host must match — either exactly, or as a + * subdomain (`platform.claude.com` matches `claude.com`). Omit to allow any + * host: the relay deliberately does, because opening a third-party OAuth + * page is the entire point of it. + */ + allowHosts?: readonly string[]; +} + +/** True when `host` is `domain` itself or a subdomain of it. */ +function hostMatches(host: string, domain: string): boolean { + return host === domain || host.endsWith(`.${domain}`); +} + +/** + * Validate a URL that something untrusted asked the host to open. * * @returns the normalized URL, or `null` if it must not be opened. */ -export function sanitizeRelayUrl(raw: unknown): string | null { +export function sanitizeRelayUrl( + raw: unknown, + options: SanitizeUrlOptions = {}, +): string | null { if (typeof raw !== "string") return null; const candidate = raw.trim(); if (candidate.length === 0) return null; if (candidate.length > MAX_RELAY_URL_LENGTH) return null; - // No whitespace or control characters anywhere. Rejecting these before - // parsing matters: `new URL()` silently strips tabs/newlines, so - // "java\nscript:alert(1)" would otherwise parse as a javascript: URL. - // eslint-disable-next-line no-control-regex - if (/[\s\u0000-\u0020\u007f]/.test(candidate)) return null; + if (hasForbiddenChar(candidate)) return null; let parsed: URL; try { @@ -70,15 +141,43 @@ export function sanitizeRelayUrl(raw: unknown): string | null { // resolves in surprising ways. if (parsed.hostname === "") return null; - // Embedded credentials spoof the displayed origin. + // Embedded credentials spoof the displayed origin: `https://claude.ai@evil.tld/x` + // reads as claude.ai in anything that truncates, and navigates to evil.tld. if (parsed.username !== "" || parsed.password !== "") return null; + if (options.allowHosts) { + const host = parsed.hostname.toLowerCase(); + if (!options.allowHosts.some((domain) => hostMatches(host, domain))) { + return null; + } + } + const normalized = parsed.toString(); if (normalized.length > MAX_RELAY_URL_LENGTH) return null; return normalized; } +/** + * The origin of an already-sanitized URL, for display. + * + * The origin is the only part of a URL that decides where the user's + * credentials end up, so it is the one part an ellipsis must never eat. Every + * place that shows a URL the user is about to open shows this separately, at + * full length, next to the truncatable remainder. + * + * Returns `null` for input that does not parse — callers pass + * {@link sanitizeRelayUrl} output, so that would be a bug rather than an + * attack. + */ +export function urlOrigin(url: string): string | null { + try { + return new URL(url).origin; + } catch { + return null; + } +} + /** * Parse the payload of an OSC 7777 sequence (everything between `ESC]7777;` * and the terminator).