From 97e58db3c1afe3b211f0182fd0d646e308c56115 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Thu, 27 Aug 2026 14:24:06 -0700 Subject: [PATCH] Close gateway-secret desync, TOCTOU, and undisclosed custom-image gaps Round 4 review findings: - Disclose and warn on a custom Docker image the import would set (HIGH): it's the image every project container is created from, so an undisclosed change here was a sharper version of the redirected-base-URL problem round 3 already flagged for the model backends. - Recreate a running gateway container when an import restores a new secret with the shape unchanged (MEDIUM): reconcile_gateway's shape comparison can't see a secret-only change, so the container would otherwise keep serving old key material indefinitely. - Report keychain write failures back to the caller instead of only logging them (MEDIUM): apply_settings_import now returns SettingsImportOutcome with secret_restore_warnings so a partial restore can't read as unqualified success. - Pin a hash of the previewed file's ciphertext and refuse to apply if it changed on disk (MEDIUM): closes a TOCTOU between preview and apply. - Sanitize and cap every free-form string a preview surfaces, and move the warning boxes above the replace list in the UI (MEDIUM): an unbounded base URL or image name could otherwise push the security warnings below the scroll fold. - Validate the Docker socket path on import the same as the SSH key and CA cert paths (LOW): it was the one mounted host path validate_settings_update didn't cover. - Fix ExportedSecrets::is_empty() to treat whitespace-only as blank, like every other secret-presence check in this feature (LOW). - Authenticate the file header as AEAD associated data (LOW, defense in depth) and correct two doc comments that overstated the password not being cached. --- CLAUDE.md | 52 ++++- .../src/commands/settings_commands.rs | 11 + .../src/commands/settings_export_commands.rs | 204 ++++++++++++++---- app/src-tauri/src/lib.rs | 8 +- app/src-tauri/src/models/settings_export.rs | 141 ++++++++++-- app/src-tauri/src/storage/settings_crypto.rs | 28 ++- .../settings/ImportSettingsModal.test.tsx | 42 +++- .../settings/ImportSettingsModal.tsx | 43 ++-- app/src/lib/settingsImportPreview.test.ts | 21 ++ app/src/lib/settingsImportPreview.ts | 12 ++ app/src/lib/tauri-commands.ts | 4 +- app/src/lib/types.ts | 14 ++ 12 files changed, 491 insertions(+), 89 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 55eafd2..c03fb36 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -588,11 +588,18 @@ deliberately out of scope — this is not a project backup. handing Rust a host path string is the exact shape of bug that produced this app's past criticals. `preview_settings_import` resolves the chosen path itself and remembers it (`AppState::pending_settings_import`) so `apply_settings_import` re-reads the same file without - a path ever crossing back over IPC. -- **The password is re-entered, not cached, between preview and apply.** Nothing here holds - decrypted plaintext — secrets included — in memory for longer than one command's execution. - `preview_settings_import` returns counts and presence flags only (`SettingsImportPreview`), - never a secret value, so it's safe to hand to the frontend and render directly. + a path ever crossing back over IPC. It also pins a hash of the file's ciphertext next to that + path, and `apply_settings_import` refuses to proceed if the file on disk no longer matches it — + otherwise confirming a preview would not actually be binding on what gets applied, which matters + given this feature's own threat model: a file shared between people may sit in a synced or + otherwise shared directory that changes between the two calls. +- **The decrypted payload is not cached between preview and apply — only the password is reused.** + The frontend holds the password in React state and passes it to both calls; nothing in Rust + holds decrypted plaintext — secrets included — in memory for longer than one command's + execution, so `apply_settings_import` always re-decrypts rather than reusing anything + `preview_settings_import` computed. `preview_settings_import` returns counts and presence flags + only (`SettingsImportPreview`), never a secret value, so it's safe to hand to the frontend and + render directly. - **Import replaces settings wholesale, but only writes secrets actually present in the file.** An import is "restore this environment," so the settings half is a full replace, not a field-by-field merge. Secrets are different on purpose: an absent secret in the export means @@ -601,6 +608,23 @@ deliberately out of scope — this is not a project backup. gateway key). Secrets are restored *before* the settings replace runs, not after — replacing settings is what triggers `reconcile_gateway`, and restoring the other way round leaves a real window where a gateway recreation happens against the destination's old keys. +- **A restored gateway secret nudges a running gateway container to recreate itself, even when + nothing about the gateway's *shape* changed.** `reconcile_gateway`'s `gateway_shape_changed` only + compares port/provider/base URL/models — deliberately, since that's what's rendered into the + container's config — so a secret-only change (same shape, new key) is invisible to it. Left + alone, a running container would keep serving the old key material indefinitely after an import + that restored a new one. `apply_settings_import` tracks whether either gateway secret was + actually written and, if the gateway is enabled and its container both exists and is running, + calls `docker::gateway::ensure_gateway_running` directly afterward — its own fingerprint already + includes the secret rotation id (`storage::secure::get_gateway_secret_version`), so it recreates + exactly when it should and no more. +- **A keychain write failing during import is reported back, not only logged.** Each of the three + `secure::store_*` calls collects its error into `SettingsImportOutcome::secret_restore_warnings` + in addition to logging it — an import that silently restores two of three secrets but not the + third must not read as unqualified success just because the settings half of the import (which + runs after, and is validated before any of this) went through. `apply_settings_import` returns + `SettingsImportOutcome { settings, secret_restore_warnings }` rather than bare `AppSettings` for + this reason; `ImportSettingsModal` shows any warnings alongside the "Settings imported" message. - **The imported settings are validated *before* any secret is written, not just before the settings replace.** `apply_settings_import` calls `settings_commands::validate_settings_update(¤t, &settings)` — the same checks @@ -634,6 +658,24 @@ deliberately out of scope — this is not a project backup. terminal token that arrives with the terminal left *off*: `start_web_terminal` only mints a fresh token when none is already set, so a planted token would otherwise activate silently the next time someone turns the terminal on, with no import-time signal that it wasn't freshly generated. +- **The preview also discloses a custom Docker image, and warns on one every time — not just on + change.** `custom_image_name`/`image_source` weren't in scope for the base-URL disclosure above, + but a review pointed out they're a sharper version of the same problem: this is the image *every* + project container is created from (`models::container_config::resolve_image_name`), so a crafted + export pointing it at an attacker-controlled image is a path to running arbitrary code with + whatever a project's containers are allowed to reach, not merely a redirected API endpoint. + `describeImportWarnings` fires on `image_source == Custom` unconditionally rather than only when + it differs from the destination's current value, since re-importing the same risky configuration + is still worth surfacing every time a user confirms an import. +- **Every free-form string a preview surfaces is sanitized and length-capped before it's built.** + `SettingsImportPreview::from_payload`'s `sanitize_for_preview` strips control characters and caps + at 100 characters (`MAX_PREVIEW_STRING_LEN`) for every base URL and the custom image name — a + review noted that, unlike the count- and boolean-derived fields the preview started with, these + are verbatim strings from a not-yet-trusted decrypted payload rendered directly into the + confirmation dialog. Unbounded, a single pathological value (very long, or holding embedded + newlines) could push the security warnings above the scroll fold in the dialog that exists + specifically to make them unmissable — the frontend's `
  • `/warning boxes also get `break-all` + as a second layer against the same failure mode. ## Testing diff --git a/app/src-tauri/src/commands/settings_commands.rs b/app/src-tauri/src/commands/settings_commands.rs index 060469d..13309bb 100644 --- a/app/src-tauri/src/commands/settings_commands.rs +++ b/app/src-tauri/src/commands/settings_commands.rs @@ -53,6 +53,17 @@ pub fn validate_settings_update( incoming.ca_cert_path.as_deref(), )?; + // Third host path this struct owns, same reasoning: any project with + // `allow_docker_access` bind-mounts this path in as the Docker socket + // (`project_commands.rs`'s container creation), so an unchecked value + // here is a read-write bind mount of whatever it names into every such + // project's container. + crate::commands::project_commands::validate_mounted_host_path( + "Docker socket path", + before.docker_socket_path.as_deref(), + incoming.docker_socket_path.as_deref(), + )?; + Ok(()) } diff --git a/app/src-tauri/src/commands/settings_export_commands.rs b/app/src-tauri/src/commands/settings_export_commands.rs index cf0860a..1bdd35b 100644 --- a/app/src-tauri/src/commands/settings_export_commands.rs +++ b/app/src-tauri/src/commands/settings_export_commands.rs @@ -17,9 +17,12 @@ //! (`AppState::pending_settings_import`) so `apply_settings_import` re-reads //! the same file without the path ever crossing back over IPC. //! -//! The password is re-entered (not cached) between preview and apply, so -//! that nothing here holds decrypted plaintext — export/import secrets -//! included — in memory for longer than one command's execution. +//! The *decrypted payload* is not cached between preview and apply — the +//! password the frontend passes to each call is what it already held for +//! the first, not a fresh secret extracted from the user, but nothing here +//! keeps the plaintext itself — export/import secrets included — around for +//! longer than one command's execution; `apply_settings_import` re-decrypts +//! the file rather than reusing anything `preview_settings_import` computed. //! //! **This is new attack surface**: a settings export is a file one person //! can hand another and ask them to import, together with a password, and @@ -29,19 +32,39 @@ //! and treat that as the standing example of the class of thing to keep //! checking for here, not a one-off fixed bug. -use std::path::{Path, PathBuf}; +#[cfg(test)] +use std::path::Path; +use std::path::PathBuf; +use sha2::{Digest, Sha256}; use tauri::State; use tauri_plugin_dialog::DialogExt; use zeroize::Zeroizing; use crate::models::{ - AppSettings, ExportedSecrets, SettingsExportPayload, SettingsImportPreview, - SETTINGS_EXPORT_FORMAT_VERSION, + AppSettings, ExportedSecrets, SettingsExportPayload, SettingsImportOutcome, + SettingsImportPreview, SETTINGS_EXPORT_FORMAT_VERSION, }; use crate::storage::{secure, settings_crypto}; use crate::AppState; +/// What `preview_settings_import` pins so `apply_settings_import` can tell +/// whether the file it's about to re-read is the same one the user actually +/// saw a preview of. Confirming a preview is only meaningful if it's binding +/// on what gets applied — without this, a file replaced on disk between the +/// two calls (this app's own stated threat model is a file shared between +/// people, which may sit in a synced or shared directory) would decrypt and +/// apply silently different content than what the confirmation dialog showed. +#[derive(Debug, Clone)] +pub struct PendingSettingsImport { + path: PathBuf, + ciphertext_hash: [u8; 32], +} + +fn hash_ciphertext(data: &[u8]) -> [u8; 32] { + Sha256::digest(data).into() +} + const FILE_EXTENSION: &str = "triplec"; /// Enforced here, not only in the export modal: the frontend's minimum is a @@ -166,10 +189,13 @@ pub async fn export_settings( /// preview (counts and presence flags only — never a secret value) for a /// confirmation UI. `Ok(None)` means the picker was dismissed. /// -/// Remembers the resolved path in `AppState::pending_settings_import` for -/// `apply_settings_import` to re-read; does **not** remember the decrypted -/// payload itself, so the password must be supplied again to actually apply -/// it — seeing the preview is not the same as committing to it. +/// Remembers the resolved path *and a hash of the file's ciphertext* in +/// `AppState::pending_settings_import` for `apply_settings_import` to check +/// against — does **not** remember the decrypted payload itself, so the +/// password must be supplied again to actually apply it — seeing the preview +/// is not the same as committing to it. The hash exists so it also can't be +/// swapped out from under that commitment: `apply_settings_import` refuses to +/// proceed if the file on disk no longer matches what was just previewed. #[tauri::command] pub async fn preview_settings_import( password: String, @@ -184,10 +210,14 @@ pub async fn preview_settings_import( return Ok(None); }; - let payload = read_and_decrypt(&path, &password)?; + let encrypted = std::fs::read(&path).map_err(|e| format!("Failed to read export file: {}", e))?; + let payload = read_and_decrypt_bytes(&encrypted, &password)?; let preview = SettingsImportPreview::from_payload(&payload); - *state.pending_settings_import.lock().await = Some(path); + *state.pending_settings_import.lock().await = Some(PendingSettingsImport { + path, + ciphertext_hash: hash_ciphertext(&encrypted), + }); Ok(Some(preview)) } @@ -196,7 +226,14 @@ pub async fn preview_settings_import( /// for. Fails if no preview is pending — this is not a general "decrypt and /// apply this file" entry point, deliberately: seeing the preview first is /// required, not just encouraged, since it is the only place a user is told -/// what an import is about to touch before it touches it. +/// what an import is about to touch before it touches it. That requirement +/// is only real if the file can't change out from under it, so this also +/// refuses to proceed if the file's ciphertext no longer matches the hash +/// `preview_settings_import` pinned — a file replaced on disk between the +/// two calls (this feature's own threat model is a file shared between +/// people, which may sit in a synced or shared directory) must not be able +/// to apply silently different content than what the confirmation dialog +/// showed. /// /// Global settings are replaced wholesale — an import is "restore this /// environment," not a field-by-field merge. Global secrets are handled @@ -227,30 +264,48 @@ pub async fn preview_settings_import( /// `reconcile_gateway`), so a gateway recreation that replace provokes sees /// the final key material rather than racing it — restoring the other way /// round left a real window where the running gateway and the keychain -/// briefly disagreed. +/// briefly disagreed. A gateway *secret* alone (same shape, new key) is +/// invisible to `reconcile_gateway`'s shape comparison, so this additionally +/// nudges a running gateway container to recreate itself whenever a secret +/// this import carried was actually written — otherwise the running +/// container keeps serving the old key material indefinitely while every +/// project container is handed the new one. /// -/// The pending path is only cleared on success. A failure here (rejected by -/// the validation above, or some other error) leaves the import pending so -/// the frontend can let the user retry `apply` without making them pick the -/// file and re-enter the password again — the preview's job was confirming -/// *what* to import, not spending the one attempt at applying it. +/// A keychain write failing is reported back rather than only logged: an +/// import that silently restores two of three secrets but not the third +/// must not read as unqualified success. +/// +/// The pending import is only cleared on success. A failure here (rejected +/// by the validation above, a stale-file mismatch, or some other error) +/// leaves it pending so the frontend can let the user retry `apply` without +/// making them pick the file and re-enter the password again — the +/// preview's job was confirming *what* to import, not spending the one +/// attempt at applying it. #[tauri::command] pub async fn apply_settings_import( password: String, state: State<'_, AppState>, -) -> Result { +) -> Result { if password.is_empty() { return Err("A password is required to import settings.".to_string()); } - let path = state + let pending = state .pending_settings_import .lock() .await .clone() .ok_or_else(|| "No import is pending — choose a file first.".to_string())?; - let payload = read_and_decrypt(&path, &password)?; + let encrypted = std::fs::read(&pending.path) + .map_err(|e| format!("Failed to read export file: {}", e))?; + if hash_ciphertext(&encrypted) != pending.ciphertext_hash { + return Err( + "This file changed since you reviewed it — choose it again to see an up-to-date preview." + .to_string(), + ); + } + let payload = read_and_decrypt_bytes(&encrypted, &password)?; let current = state.settings_store.get(); @@ -266,37 +321,81 @@ pub async fn apply_settings_import( crate::commands::settings_commands::validate_settings_update(¤t, &settings)?; + let mut secret_restore_warnings = Vec::new(); + let mut gateway_secret_changed = false; + if let Some(token) = non_blank(payload.secrets.claude_oauth_token) { if let Err(e) = secure::store_claude_oauth_token(&token) { log::warn!( "Settings import: could not restore the shared Claude login: {}", e ); + secret_restore_warnings + .push(format!("Could not restore your shared Claude login: {}", e)); } } if let Some(key) = non_blank(payload.secrets.gateway_api_key) { - if let Err(e) = secure::store_gateway_api_key(&key) { - log::warn!( - "Settings import: could not restore the gateway provider API key: {}", - e - ); + match secure::store_gateway_api_key(&key) { + Ok(()) => gateway_secret_changed = true, + Err(e) => { + log::warn!( + "Settings import: could not restore the gateway provider API key: {}", + e + ); + secret_restore_warnings.push(format!( + "Could not restore the gateway provider API key: {}", + e + )); + } } } if let Some(key) = non_blank(payload.secrets.gateway_master_key) { - if let Err(e) = secure::store_gateway_master_key(&key) { - log::warn!( - "Settings import: could not restore the gateway master key: {}", - e - ); + match secure::store_gateway_master_key(&key) { + Ok(()) => gateway_secret_changed = true, + Err(e) => { + log::warn!( + "Settings import: could not restore the gateway master key: {}", + e + ); + secret_restore_warnings + .push(format!("Could not restore the gateway master key: {}", e)); + } } } let saved = crate::commands::settings_commands::update_settings(settings, state.clone()).await?; + // `reconcile_gateway` (inside `update_settings`) only reacts to a changed + // *shape* — port, provider, base URL, models — because that's what's + // rendered into the container's config. A secret changing with the shape + // held constant is invisible to it, so a running gateway container would + // otherwise keep serving the old key material forever after an import + // that restored a new one, while `docker::gateway`'s own fingerprint + // (which does include the secret rotation id) means the *next* unrelated + // settings save would suddenly and confusingly recreate it instead. + if gateway_secret_changed && saved.gateway.enabled { + match crate::docker::gateway::gateway_container_presence().await { + Ok((true, true)) => { + if let Err(e) = crate::docker::gateway::ensure_gateway_running(&saved.gateway).await + { + log::error!( + "Settings import: could not apply the restored gateway credentials to the running gateway container: {}", + e + ); + } + } + Ok(_) => {} + Err(e) => log::debug!("Settings import: gateway reconcile skipped ({})", e), + } + } + state.pending_settings_import.lock().await.take(); - Ok(saved) + Ok(SettingsImportOutcome { + settings: saved, + secret_restore_warnings, + }) } fn non_blank(value: Option) -> Option { @@ -310,8 +409,22 @@ struct FormatVersionProbe { format_version: u32, } -/// Decrypt and parse an export file, checking the format version **before** -/// attempting to deserialize the full payload. +/// Read and decrypt an export file at `path`, then parse it — see +/// `read_and_decrypt_bytes` for why the format-version check runs before the +/// full parse. Every real caller already has the file's bytes in hand by the +/// time it needs this (`preview_settings_import`/`apply_settings_import` +/// both hash the ciphertext first) and calls `read_and_decrypt_bytes` +/// directly to avoid reading the file twice; this path-based wrapper only +/// exists now for tests that don't need that. +#[cfg(test)] +fn read_and_decrypt(path: &Path, password: &str) -> Result { + let encrypted = + std::fs::read(path).map_err(|e| format!("Failed to read export file: {}", e))?; + read_and_decrypt_bytes(&encrypted, password) +} + +/// Decrypt and parse an already-read export file's bytes, checking the +/// format version **before** attempting to deserialize the full payload. /// /// That ordering is not just tidiness: a version bump that isn't /// deserialize-compatible (a field's type changes, not just a new @@ -324,10 +437,8 @@ struct FormatVersionProbe { /// password), but the plaintext it decrypts to can hold a live credential, /// so neither error path below ever interpolates what `serde_json` /// actually says — only a fixed, generic message. -fn read_and_decrypt(path: &Path, password: &str) -> Result { - let encrypted = - std::fs::read(path).map_err(|e| format!("Failed to read export file: {}", e))?; - let plaintext = settings_crypto::decrypt(&encrypted, password)?; +fn read_and_decrypt_bytes(encrypted: &[u8], password: &str) -> Result { + let plaintext = settings_crypto::decrypt(encrypted, password)?; let probe: FormatVersionProbe = serde_json::from_slice(&plaintext) .map_err(|_| "This file doesn't look like a valid settings export.".to_string())?; @@ -356,6 +467,21 @@ mod tests { assert_eq!(non_blank(Some(" a ".to_string())), Some(" a ".to_string())); } + #[test] + fn ciphertext_hashing_is_deterministic_and_tamper_sensitive() { + // What `apply_settings_import` compares against the pinned hash from + // `preview_settings_import` to detect a file swapped out from under a + // pending import — this only defends anything if identical bytes + // always hash identically and any change to those bytes changes the + // hash. + let bytes = b"pretend this is an encrypted export file"; + assert_eq!(hash_ciphertext(bytes), hash_ciphertext(bytes)); + + let mut tampered = bytes.to_vec(); + tampered[0] ^= 0xFF; + assert_ne!(hash_ciphertext(bytes), hash_ciphertext(&tampered)); + } + fn write_export( dir: &std::path::Path, name: &str, diff --git a/app/src-tauri/src/lib.rs b/app/src-tauri/src/lib.rs index 679c3df..b789ad8 100644 --- a/app/src-tauri/src/lib.rs +++ b/app/src-tauri/src/lib.rs @@ -37,7 +37,13 @@ pub struct AppState { /// dangerous. Deliberately re-decrypted rather than cached in plaintext: /// nothing here holds a decrypted secret in memory for longer than one /// command's execution. - pub pending_settings_import: Arc>>, + /// + /// Also pins a hash of the file's ciphertext at preview time, so + /// `apply_settings_import` can refuse to proceed if the file on disk + /// changed underneath the pending import — otherwise confirming a + /// preview is not actually binding on what gets applied. + pub pending_settings_import: + Arc>>, } // ───────────────────────────────────────────────────────────────────────────── diff --git a/app/src-tauri/src/models/settings_export.rs b/app/src-tauri/src/models/settings_export.rs index 550d163..cf6c5d4 100644 --- a/app/src-tauri/src/models/settings_export.rs +++ b/app/src-tauri/src/models/settings_export.rs @@ -27,7 +27,7 @@ use serde::{Deserialize, Serialize}; -use super::AppSettings; +use super::{AppSettings, ImageSource}; /// Bumped when the shape of [`SettingsExportPayload`] changes in a way that /// isn't just an additive, `#[serde(default)]`-covered field — e.g. if a @@ -60,13 +60,26 @@ pub struct ExportedSecrets { impl ExportedSecrets { pub fn is_empty(&self) -> bool { - self.claude_oauth_token.is_none() - && self.gateway_api_key.is_none() - && self.gateway_master_key.is_none() - && self.web_terminal_access_token.is_none() + let blank = |s: &Option| s.as_deref().is_none_or(|v| v.trim().is_empty()); + blank(&self.claude_oauth_token) + && blank(&self.gateway_api_key) + && blank(&self.gateway_master_key) + && blank(&self.web_terminal_access_token) } } +/// What `apply_settings_import` hands back: the settings that were actually +/// saved, plus a human-readable note for each keychain secret this import +/// carried but could not be restored. A keychain write failing partway +/// through must not read as unqualified success just because the settings +/// half of the import went through. +#[derive(Debug, Clone, Serialize, Deserialize)] +pub struct SettingsImportOutcome { + pub settings: AppSettings, + #[serde(default)] + pub secret_restore_warnings: Vec, +} + /// The full plaintext payload — this is what gets encrypted on export and /// what decryption recovers on import. Never written to disk unencrypted; /// see `storage::settings_crypto`. @@ -122,12 +135,49 @@ pub struct SettingsImportPreview { pub openai_compatible_base_url: Option, #[serde(default)] pub gateway_api_base: Option, + /// Whether the import sets a custom Docker image, and its name if so — + /// disclosed for the same reason as the base URLs above, and arguably + /// more sharply: this is the image *every* project container is created + /// from (`models::container_config::resolve_image_name`), so a crafted + /// export pointing it at an attacker-controlled image is a path to + /// running arbitrary code with whatever a project's containers are + /// allowed to reach (the Docker socket, an SSH key, project files) — + /// not merely a redirected API endpoint. + #[serde(default)] + pub image_source: ImageSource, + #[serde(default)] + pub custom_image_name: Option, +} + +/// A cap on how much of a decrypted, not-yet-trusted string gets echoed back +/// into a preview a user reads and a UI renders without truncation of its +/// own. Applied to every field above that carries free-form text straight +/// from the import file rather than a count or a boolean — a base URL or an +/// image name a hostile export author controls has had no validation done +/// on it yet at preview time, and nothing stops it from being pathological +/// (embedded control characters, or long enough to blow out the confirmation +/// dialog and push the security warnings below it off screen). +const MAX_PREVIEW_STRING_LEN: usize = 100; + +fn sanitize_for_preview(value: &str) -> String { + let cleaned: String = value.chars().filter(|c| !c.is_control()).collect(); + let trimmed = cleaned.trim(); + if trimmed.chars().count() > MAX_PREVIEW_STRING_LEN { + let truncated: String = trimmed.chars().take(MAX_PREVIEW_STRING_LEN).collect(); + format!("{}…", truncated) + } else { + trimmed.to_string() + } } impl SettingsImportPreview { pub fn from_payload(payload: &SettingsExportPayload) -> Self { let non_blank = |s: &Option| s.as_deref().is_some_and(|v| !v.trim().is_empty()); - let non_blank_value = |s: &Option| s.clone().filter(|v| !v.trim().is_empty()); + let sanitized_non_blank = |s: &Option| { + s.as_deref() + .map(sanitize_for_preview) + .filter(|v| !v.is_empty()) + }; Self { exported_at: payload.exported_at.clone(), app_version: payload.app_version.clone(), @@ -139,12 +189,14 @@ impl SettingsImportPreview { has_gateway_master_key: non_blank(&payload.secrets.gateway_master_key), has_web_terminal_access_token: non_blank(&payload.secrets.web_terminal_access_token), enables_web_terminal: payload.settings.web_terminal.enabled, - ollama_base_url: non_blank_value(&payload.settings.global_ollama.base_url), - llamacpp_base_url: non_blank_value(&payload.settings.global_llamacpp.base_url), - openai_compatible_base_url: non_blank_value( + ollama_base_url: sanitized_non_blank(&payload.settings.global_ollama.base_url), + llamacpp_base_url: sanitized_non_blank(&payload.settings.global_llamacpp.base_url), + openai_compatible_base_url: sanitized_non_blank( &payload.settings.global_openai_compatible.base_url, ), - gateway_api_base: non_blank_value(&payload.settings.gateway.api_base), + gateway_api_base: sanitized_non_blank(&payload.settings.gateway.api_base), + image_source: payload.settings.image_source.clone(), + custom_image_name: sanitized_non_blank(&payload.settings.custom_image_name), } } } @@ -155,17 +207,19 @@ mod tests { use crate::models::AppSettings; fn payload_with(secrets: ExportedSecrets) -> SettingsExportPayload { - let mut settings = AppSettings::default(); - settings.global_custom_env_vars = vec![ - crate::models::EnvVar { - key: "A".to_string(), - value: "1".to_string(), - }, - crate::models::EnvVar { - key: "B".to_string(), - value: "2".to_string(), - }, - ]; + let settings = AppSettings { + global_custom_env_vars: vec![ + crate::models::EnvVar { + key: "A".to_string(), + value: "1".to_string(), + }, + crate::models::EnvVar { + key: "B".to_string(), + value: "2".to_string(), + }, + ], + ..AppSettings::default() + }; SettingsExportPayload { format_version: SETTINGS_EXPORT_FORMAT_VERSION, exported_at: "2026-08-27T00:00:00Z".to_string(), @@ -264,4 +318,49 @@ mod tests { } .is_empty()); } + + #[test] + fn a_secrets_bundle_holding_only_whitespace_still_reports_itself_as_empty() { + // Matches the "blank counts as absent" rule every other consumer of + // these fields applies (`has_claude_oauth_token` and friends above) — + // a keychain entry that exists but holds only whitespace carries + // nothing usable, so the export-time "nothing to export" log line + // must still fire for it. + assert!(ExportedSecrets { + claude_oauth_token: Some(" ".to_string()), + ..Default::default() + } + .is_empty()); + } + + #[test] + fn a_custom_docker_image_is_surfaced() { + let mut payload = payload_with(ExportedSecrets::default()); + payload.settings.image_source = crate::models::ImageSource::Custom; + payload.settings.custom_image_name = Some("ghcr.io/attacker/triple-c:latest".to_string()); + + let preview = SettingsImportPreview::from_payload(&payload); + assert_eq!(preview.image_source, crate::models::ImageSource::Custom); + assert_eq!( + preview.custom_image_name.as_deref(), + Some("ghcr.io/attacker/triple-c:latest") + ); + } + + #[test] + fn preview_strings_are_stripped_of_control_characters_and_capped_in_length() { + let mut payload = payload_with(ExportedSecrets::default()); + payload.settings.global_ollama.base_url = + Some(format!("http://example.test/{}\u{0007}bell", "x".repeat(200))); + + let preview = SettingsImportPreview::from_payload(&payload); + let shown = preview.ollama_base_url.expect("non-blank base url"); + assert!(!shown.contains('\u{0007}'), "control character leaked into the preview"); + // +1 for the trailing ellipsis appended when truncated. + assert!( + shown.chars().count() <= MAX_PREVIEW_STRING_LEN + 1, + "preview string was not capped: {} chars", + shown.chars().count() + ); + } } diff --git a/app/src-tauri/src/storage/settings_crypto.rs b/app/src-tauri/src/storage/settings_crypto.rs index 37d40df..385edf1 100644 --- a/app/src-tauri/src/storage/settings_crypto.rs +++ b/app/src-tauri/src/storage/settings_crypto.rs @@ -19,8 +19,14 @@ //! GCM's requirement that a (key, nonce) pair never repeat. Both hold //! because a fresh random value is drawn for each, on every call to //! [`encrypt`]. +//! +//! The whole header (magic + salt + nonce) is passed to AES-GCM as +//! associated data, not just placed alongside the ciphertext — free to do, +//! and it makes tampering with any header byte fail the same authentication +//! check the ciphertext gets, by construction rather than as a side effect +//! of the salt/nonce also feeding key derivation and the cipher. -use aes_gcm::aead::{Aead, KeyInit}; +use aes_gcm::aead::{Aead, KeyInit, Payload}; use aes_gcm::{Aes256Gcm, Nonce}; use argon2::{Algorithm, Argon2, Params, Version}; use rand::RngCore; @@ -70,16 +76,23 @@ pub fn encrypt(plaintext: &[u8], password: &str) -> Result, String> { rand::rng().fill_bytes(&mut nonce_bytes); let nonce = Nonce::from_slice(&nonce_bytes); + let mut header = Vec::with_capacity(HEADER_LEN); + header.extend_from_slice(MAGIC); + header.extend_from_slice(&salt); + header.extend_from_slice(&nonce_bytes); + let cipher = Aes256Gcm::new_from_slice(&*key) .map_err(|e| format!("Failed to initialize cipher: {}", e))?; + // The header (magic + salt + nonce) is authenticated as associated data + // even though none of it is secret: it costs nothing extra here, and it + // means tampering with any header byte is caught by the same tag check + // that already covers the ciphertext, by construction rather than as a + // side effect of the header also feeding key/nonce derivation. let ciphertext = cipher - .encrypt(nonce, plaintext) + .encrypt(nonce, Payload { msg: plaintext, aad: &header }) .map_err(|e| format!("Encryption failed: {}", e))?; - let mut out = Vec::with_capacity(HEADER_LEN + ciphertext.len()); - out.extend_from_slice(MAGIC); - out.extend_from_slice(&salt); - out.extend_from_slice(&nonce_bytes); + let mut out = header; out.extend_from_slice(&ciphertext); Ok(out) } @@ -102,6 +115,7 @@ pub fn decrypt(data: &[u8], password: &str) -> Result>, String if &data[..MAGIC.len()] != MAGIC { return Err("This does not look like a Triple-C settings export (unrecognized file).".to_string()); } + let header = &data[..HEADER_LEN]; let salt = &data[MAGIC.len()..MAGIC.len() + SALT_LEN]; let nonce_bytes = &data[MAGIC.len() + SALT_LEN..HEADER_LEN]; let ciphertext = &data[HEADER_LEN..]; @@ -111,7 +125,7 @@ pub fn decrypt(data: &[u8], password: &str) -> Result>, String .map_err(|e| format!("Failed to initialize cipher: {}", e))?; let nonce = Nonce::from_slice(nonce_bytes); cipher - .decrypt(nonce, ciphertext) + .decrypt(nonce, Payload { msg: ciphertext, aad: header }) .map(Zeroizing::new) .map_err(|_| "Wrong password, or the file is corrupted.".to_string()) } diff --git a/app/src/components/settings/ImportSettingsModal.test.tsx b/app/src/components/settings/ImportSettingsModal.test.tsx index 2d9807e..9a56f02 100644 --- a/app/src/components/settings/ImportSettingsModal.test.tsx +++ b/app/src/components/settings/ImportSettingsModal.test.tsx @@ -1,7 +1,7 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { fireEvent, render, screen, waitFor } from "@testing-library/react"; import ImportSettingsModal from "./ImportSettingsModal"; -import type { AppSettings, SettingsImportPreview } from "../../lib/types"; +import type { AppSettings, SettingsImportOutcome, SettingsImportPreview } from "../../lib/types"; const previewSettingsImport = vi.fn(); const applySettingsImport = vi.fn(); @@ -30,8 +30,14 @@ const samplePreview: SettingsImportPreview = { llamacpp_base_url: null, openai_compatible_base_url: null, gateway_api_base: null, + image_source: "registry", + custom_image_name: null, }; +function outcome(settings: AppSettings, secretRestoreWarnings: string[] = []): SettingsImportOutcome { + return { settings, secret_restore_warnings: secretRestoreWarnings }; +} + describe("ImportSettingsModal", () => { it("keeps 'Choose file' disabled until a password is entered", () => { render(); @@ -43,7 +49,7 @@ describe("ImportSettingsModal", () => { it("shows the preview and confirms with the same password used to open it", async () => { previewSettingsImport.mockResolvedValue(samplePreview); - applySettingsImport.mockResolvedValue({} as AppSettings); + applySettingsImport.mockResolvedValue(outcome({} as AppSettings)); const onImported = vi.fn(); render(); @@ -70,6 +76,38 @@ describe("ImportSettingsModal", () => { expect(await screen.findByText(/enables the remote web terminal/i)).toBeInTheDocument(); }); + it("warns about a custom Docker image every time, not just on change", async () => { + previewSettingsImport.mockResolvedValue({ + ...samplePreview, + image_source: "custom", + custom_image_name: "ghcr.io/attacker/triple-c:latest", + }); + render(); + + fireEvent.change(screen.getByLabelText("Password"), { target: { value: "hunter2" } }); + fireEvent.click(screen.getByRole("button", { name: /choose file/i })); + + expect( + await screen.findByText(/custom docker image: ghcr\.io\/attacker\/triple-c:latest/i), + ).toBeInTheDocument(); + }); + + it("shows a secret-restore warning alongside success rather than hiding it", async () => { + previewSettingsImport.mockResolvedValue(samplePreview); + applySettingsImport.mockResolvedValue( + outcome({} as AppSettings, ["Could not restore the gateway master key: keychain locked"]), + ); + render(); + + fireEvent.change(screen.getByLabelText("Password"), { target: { value: "hunter2" } }); + fireEvent.click(screen.getByRole("button", { name: /choose file/i })); + await screen.findByText(/2 global custom env vars/i); + + fireEvent.click(screen.getByRole("button", { name: /^import$/i })); + expect(await screen.findByText(/settings imported/i)).toBeInTheDocument(); + expect(await screen.findByText(/could not restore the gateway master key/i)).toBeInTheDocument(); + }); + it("closes quietly when the file picker is dismissed", async () => { previewSettingsImport.mockResolvedValue(null); const onClose = vi.fn(); diff --git a/app/src/components/settings/ImportSettingsModal.tsx b/app/src/components/settings/ImportSettingsModal.tsx index c0b79f4..c914c3b 100644 --- a/app/src/components/settings/ImportSettingsModal.tsx +++ b/app/src/components/settings/ImportSettingsModal.tsx @@ -27,6 +27,7 @@ export default function ImportSettingsModal({ onClose, onImported }: Props) { const [error, setError] = useState(null); const [preview, setPreview] = useState(null); const [applied, setApplied] = useState(false); + const [secretWarnings, setSecretWarnings] = useState([]); const handleChooseFile = async () => { setError(null); @@ -46,9 +47,10 @@ export default function ImportSettingsModal({ onClose, onImported }: Props) { setError(null); setBusy(true); try { - const settings = await applySettingsImport(password); + const outcome = await applySettingsImport(password); setApplied(true); - onImported(settings); + setSecretWarnings(outcome.secret_restore_warnings); + onImported(outcome.settings); } catch (e) { setError(String(e)); } finally { @@ -99,29 +101,46 @@ export default function ImportSettingsModal({ onClose, onImported }: Props) { } > {applied ? ( -

    Settings imported.

    +
    +

    Settings imported.

    + {secretWarnings.map((warning) => ( +

    + {warning} +

    + ))} +
    ) : preview ? (

    Exported {new Date(preview.exported_at).toLocaleString()} from Triple-C{" "} {preview.app_version}.

    -
    -

    This will replace:

    -
      - {describeImport(preview).map((item) => ( -
    • {item}
      • - ))} -
    -
    + {/* Warnings render before the replace list, deliberately: the list + * below can run long, and the one thing here that most needs to + * stay above the fold while scrolling is "this turns on a + * network-listening service" or "this runs a different image" — + * not a bullet buried among ordinary settings. */} {describeImportWarnings(preview).map((warning) => (

    {warning}

    ))} +
    +

    This will replace:

    +
      + {describeImport(preview).map((item) => ( +
    • + {item} +
      • + ))} +
    +
    {error &&

    {error}

    }
    ) : ( diff --git a/app/src/lib/settingsImportPreview.test.ts b/app/src/lib/settingsImportPreview.test.ts index 8124888..aa94e9d 100644 --- a/app/src/lib/settingsImportPreview.test.ts +++ b/app/src/lib/settingsImportPreview.test.ts @@ -18,6 +18,8 @@ function preview(overrides: Partial = {}): SettingsImport llamacpp_base_url: null, openai_compatible_base_url: null, gateway_api_base: null, + image_source: "registry", + custom_image_name: null, ...overrides, }; } @@ -76,6 +78,18 @@ describe("describeImport", () => { expect(items.some((i) => i.includes("llama.cpp"))).toBe(false); expect(items.some((i) => i.includes("OpenAI-compatible"))).toBe(false); }); + + it("names a custom Docker image when set, falling back to a placeholder if unnamed", () => { + expect( + describeImport(preview({ image_source: "custom", custom_image_name: "ghcr.io/me/triple-c" })), + ).toContain("Docker image: ghcr.io/me/triple-c"); + expect(describeImport(preview({ image_source: "custom", custom_image_name: null }))).toContain( + "Docker image: (no image name set)", + ); + expect(describeImport(preview({ image_source: "registry" })).some((i) => i.includes("Docker image"))).toBe( + false, + ); + }); }); describe("describeImportWarnings", () => { @@ -101,4 +115,11 @@ describe("describeImportWarnings", () => { "Includes a web terminal access token that will activate the next time the web terminal is turned on.", ]); }); + + it("warns about a custom Docker image every time, not only when it changes", () => { + expect( + describeImportWarnings(preview({ image_source: "custom", custom_image_name: "evil:latest" })), + ).toEqual(["Runs every project container from a custom Docker image: evil:latest."]); + expect(describeImportWarnings(preview({ image_source: "registry" }))).toEqual([]); + }); }); diff --git a/app/src/lib/settingsImportPreview.ts b/app/src/lib/settingsImportPreview.ts index 6e9c099..c8e05e6 100644 --- a/app/src/lib/settingsImportPreview.ts +++ b/app/src/lib/settingsImportPreview.ts @@ -25,6 +25,9 @@ export function describeImport(preview: SettingsImportPreview): string[] { items.push(`OpenAI-compatible server: ${preview.openai_compatible_base_url}`); } if (preview.gateway_api_base) items.push(`Gateway upstream: ${preview.gateway_api_base}`); + if (preview.image_source === "custom") { + items.push(`Docker image: ${preview.custom_image_name ?? "(no image name set)"}`); + } return items; } @@ -41,6 +44,10 @@ export function describeImport(preview: SettingsImportPreview): string[] { * would silently become live the next time someone flips the terminal on * through the UI, with no import-time signal that it wasn't freshly * generated. + * + * A custom Docker image gets a warning every time, not just on change: it's + * the image every project container is created from, so it's worth calling + * out regardless of what was configured before the import. */ export function describeImportWarnings(preview: SettingsImportPreview): string[] { const warnings: string[] = []; @@ -51,5 +58,10 @@ export function describeImportWarnings(preview: SettingsImportPreview): string[] "Includes a web terminal access token that will activate the next time the web terminal is turned on.", ); } + if (preview.image_source === "custom") { + warnings.push( + `Runs every project container from a custom Docker image: ${preview.custom_image_name ?? "(no image name set)"}.`, + ); + } return warnings; } diff --git a/app/src/lib/tauri-commands.ts b/app/src/lib/tauri-commands.ts index dbceeec..c9a2b1f 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, ProjectRemovalReport, ProjectResetOutcome, ContainerInfo, AppSettings, SettingsImportPreview, UpdateInfo, ImageUpdateInfo, FileEntry, FileContents, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, BrowserViewPopoutState, BrowserPageState, PlaywrightDetection, BrowserSetupOutcome, BrowserInstallTarget, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState, ClearTokenOutcome, CaCertInfo, UploadOutcome } from "./types"; +import type { Project, ProjectPath, ProjectRemovalReport, ProjectResetOutcome, ContainerInfo, AppSettings, SettingsImportPreview, SettingsImportOutcome, UpdateInfo, ImageUpdateInfo, FileEntry, FileContents, WebTerminalInfo, SttStatus, GatewayStatus, InstallOptions, ClaudeSession, ContainerCapabilities, ScheduledTask, ScheduledTaskInput, SchedulerNotification, AuthBridgeStatus, BrowserViewStatus, BrowserViewPopoutState, BrowserPageState, PlaywrightDetection, BrowserSetupOutcome, BrowserInstallTarget, ContainerStaleness, MigrationOptions, MigrationReport, MigrationState, ClearTokenOutcome, CaCertInfo, UploadOutcome } from "./types"; // Docker export const checkDocker = () => invoke("check_docker"); @@ -49,7 +49,7 @@ export const exportSettings = (password: string) => export const previewSettingsImport = (password: string) => invoke("preview_settings_import", { password }); export const applySettingsImport = (password: string) => - invoke("apply_settings_import", { password }); + invoke("apply_settings_import", { password }); // AWS export const awsSsoRefresh = (projectId: string) => diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index 3ddb549..ae2250c 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -316,6 +316,20 @@ export interface SettingsImportPreview { llamacpp_base_url: string | null; openai_compatible_base_url: string | null; gateway_api_base: string | null; + /** Whether the import sets a custom Docker image, and its name if so — + * this is the image every project container is created from, so worth + * more attention than an ordinary setting. */ + image_source: ImageSource; + custom_image_name: string | null; +} + +/** What `apply_settings_import` returns: the settings that were actually + * saved, plus a note for each keychain secret the import carried but could + * not be restored (a partial keychain failure must not read as unqualified + * success just because the settings half went through). */ +export interface SettingsImportOutcome { + settings: AppSettings; + secret_restore_warnings: string[]; } /** What `inspect_ca_cert_path` reports about a corporate CA path. Errors ride