diff --git a/CLAUDE.md b/CLAUDE.md index 39d3024..7267b16 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -555,12 +555,26 @@ nobody had reason to open. Fixtures are never live values; there is no case wher ## Settings export/import `commands::settings_export_commands`, `storage::settings_crypto`, `models::settings_export` -(triple-c#35). Exports the *host* environment — global `AppSettings` (already the non-secret -shape persisted to `settings.json`) plus the global secrets that live in the OS keychain instead: -the shared Claude Code OAuth login and the model gateway's two keys. Per-project settings, -per-project secrets, and anything in a project's Docker volumes are deliberately out of scope — -this is not a project backup. +(triple-c#35). Exports the *host* environment — global `AppSettings` plus the global secrets that +live in the OS keychain instead: the shared Claude Code OAuth login and the model gateway's two +keys. Per-project settings, per-project secrets, and anything in a project's Docker volumes are +deliberately out of scope — this is not a project backup. +- **`AppSettings` is not entirely the non-secret shape it looks like, and a review of this feature + caught the one place that isn't.** `WebTerminalSettings::access_token` is a live bearer + credential for a server that binds every interface — exporting `AppSettings` wholesale would + have carried it along as if it were as inert as a port number, and importing it would have + applied `web_terminal.enabled` and the token together with no more warning than any other + setting, letting a crafted export silently stand up a LAN-listening terminal on the next launch. + `export_settings`/`apply_settings_import` carve this one field out into `ExportedSecrets` + instead, with the same "only overwrite what the import actually has" treatment as the other + three secrets — except "leave it alone" has to be done by hand in `apply_settings_import`, since + unlike the keychain secrets this one lives inside the `AppSettings` blob that gets replaced + wholesale. `SettingsImportPreview::enables_web_terminal` also exists because of this: `enabled` + and the token are independent fields, and "this turns on a listening service" must not hide + inside a generic "settings replaced" summary. Read this as the standing example of the class of + thing to keep checking for in this feature, not a one-off fixed bug — any other field that looks + like config but is actually a live credential would have the same problem. - **Encrypted because it can carry live credentials, not for appearance's sake.** Argon2id derives a 256-bit key from the user's password (memory-hard — meaningfully resistant to GPU/ASIC brute-forcing, unlike PBKDF2 at any reasonable iteration count), AES-256-GCM does the actual @@ -584,7 +598,22 @@ this is not a project backup. field-by-field merge. Secrets are different on purpose: an absent secret in the export means "the source machine never had this configured," not "delete this on import" — a user who wants to clear a secret already has dedicated UI for that (signing out of shared auth, clearing the - gateway key). + 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. +- **`read_and_decrypt` checks `format_version` before attempting to parse the full payload, not + after.** A version bump that isn't deserialize-compatible is exactly the case that check exists + for, and parsing the full struct first would fail on the shape mismatch before the version check + ever ran. Neither error path interpolates what `serde_json` actually says into the message + shown to the user — its type-mismatch errors quote the offending value inline, and the plaintext + here can hold a live credential. +- **The 8-character password minimum is enforced in `export_settings` itself, not only in the + export modal.** The frontend minimum is a UX nudge; the Rust command is the actual boundary a + weak password has to cross, and Argon2id's memory-hardness buys little against an attacker who + can just try a short password directly. The derived key and the decrypted plaintext are both + wrapped in `zeroize::Zeroizing` for the same reason every other secret in this codebase gets + handled carefully — cheap insurance (`zeroize` is already pulled in transitively via `aes-gcm`) + for material that exists only to hold or produce live credentials. ## Testing diff --git a/app/src-tauri/Cargo.lock b/app/src-tauri/Cargo.lock index b5b92bd..d663f61 100644 --- a/app/src-tauri/Cargo.lock +++ b/app/src-tauri/Cargo.lock @@ -5307,6 +5307,7 @@ dependencies = [ "tokio", "tower-http", "uuid", + "zeroize", ] [[package]] diff --git a/app/src-tauri/Cargo.toml b/app/src-tauri/Cargo.toml index 88bad73..ebc9648 100644 --- a/app/src-tauri/Cargo.toml +++ b/app/src-tauri/Cargo.toml @@ -38,6 +38,7 @@ rand = "0.9" local-ip-address = "0.6" argon2 = "0.5" aes-gcm = "0.10" +zeroize = "1" [dev-dependencies] # `test-util` (not part of tokio's `full`) lets the auto-start retry tests run diff --git a/app/src-tauri/src/commands/settings_export_commands.rs b/app/src-tauri/src/commands/settings_export_commands.rs index ebebc81..dc5c3e3 100644 --- a/app/src-tauri/src/commands/settings_export_commands.rs +++ b/app/src-tauri/src/commands/settings_export_commands.rs @@ -20,20 +20,35 @@ //! 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. +//! +//! **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 +//! `apply_settings_import` applies whatever `AppSettings` it decrypts to +//! wholesale — see the module doc on `models::settings_export` for the +//! `web_terminal.access_token` carve-out a review of this feature found, +//! 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::PathBuf; +use std::path::{Path, PathBuf}; use tauri::State; use tauri_plugin_dialog::DialogExt; use crate::models::{ - ExportedSecrets, SettingsExportPayload, SettingsImportPreview, SETTINGS_EXPORT_FORMAT_VERSION, + AppSettings, ExportedSecrets, SettingsExportPayload, SettingsImportPreview, + SETTINGS_EXPORT_FORMAT_VERSION, }; use crate::storage::{secure, settings_crypto}; use crate::AppState; const FILE_EXTENSION: &str = "triplec"; +/// Enforced here, not only in the export modal: the frontend's minimum is a +/// UX nudge, but `export_settings` is the actual boundary a weak password +/// has to cross, and Argon2id's memory-hardness buys little against an +/// attacker who can just try a three-character password directly. +const MIN_PASSWORD_LEN: usize = 8; + fn suggested_export_name() -> String { // Timestamped so exporting more than once doesn't silently overwrite an // earlier file just because the save dialog defaults to the same name. @@ -73,17 +88,28 @@ async fn pick_import_open_path(window: &tauri::Window) -> Option { rx.await.ok().flatten().and_then(|p| p.into_path().ok()) } -/// Gather the current global secrets. A missing secret reads as `None` — a -/// keychain read failure is treated as "nothing to export" for that one -/// entry rather than aborting the whole export, matching how the rest of -/// this app degrades a keychain error to "absent" (`has_claude_oauth_token`, -/// `has_gateway_api_key`) rather than surfacing it as a hard failure. -fn gather_secrets() -> ExportedSecrets { - ExportedSecrets { +/// Gather the current global secrets, and hand back the `AppSettings` to +/// export with the web-terminal token blanked out of it — see the module +/// doc comment on `models::settings_export` for why that field cannot +/// travel through `settings` like the rest of this struct. +/// +/// A missing keychain secret reads as `None` — a keychain read failure is +/// treated as "nothing to export" for that one entry rather than aborting +/// the whole export, matching how the rest of this app degrades a keychain +/// error to "absent" (`has_claude_oauth_token`, `has_gateway_api_key`) +/// rather than surfacing it as a hard failure. +fn split_settings_and_secrets(current: AppSettings) -> (AppSettings, ExportedSecrets) { + let mut settings = current; + let web_terminal_access_token = settings.web_terminal.access_token.take(); + + let secrets = ExportedSecrets { claude_oauth_token: secure::get_claude_oauth_token().unwrap_or_default(), gateway_api_key: secure::get_gateway_api_key().unwrap_or_default(), gateway_master_key: secure::get_gateway_master_key().unwrap_or_default(), - } + web_terminal_access_token, + }; + + (settings, secrets) } /// Export the current global settings and secrets to a password-encrypted @@ -96,15 +122,18 @@ pub async fn export_settings( window: tauri::Window, state: State<'_, AppState>, ) -> Result { - if password.is_empty() { - return Err("A password is required to export settings.".to_string()); + if password.len() < MIN_PASSWORD_LEN { + return Err(format!( + "Use a password of at least {} characters.", + MIN_PASSWORD_LEN + )); } let Some(dest) = pick_export_save_path(&window, &suggested_export_name()).await else { return Ok(false); }; - let secrets = gather_secrets(); + let (settings, secrets) = split_settings_and_secrets(state.settings_store.get()); if secrets.is_empty() { log::info!("Exporting settings with no global secrets configured on this machine"); } @@ -113,7 +142,7 @@ pub async fn export_settings( format_version: SETTINGS_EXPORT_FORMAT_VERSION, exported_at: chrono::Utc::now().to_rfc3339(), app_version: env!("CARGO_PKG_VERSION").to_string(), - settings: state.settings_store.get(), + settings, secrets, }; @@ -121,7 +150,7 @@ pub async fn export_settings( .map_err(|e| format!("Failed to prepare settings for export: {}", e))?; let encrypted = settings_crypto::encrypt(&plaintext, &password)?; - std::fs::write(&dest, encrypted).map_err(|e| format!("Failed to write export file: {}", e))?; + std::fs::write(&dest, &encrypted).map_err(|e| format!("Failed to write export file: {}", e))?; Ok(true) } @@ -170,11 +199,24 @@ pub async fn preview_settings_import( /// means "the source machine never had this configured," not "delete this /// on import." A user who wants to clear a secret already has dedicated UI /// for that (signing out of shared auth, clearing the gateway key). +/// +/// Order matters here: secrets are restored **before** the settings replace +/// runs (which is what triggers `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. +/// +/// The pending path is only cleared on success. A failure here (a rejected +/// host path, a keychain write failure surfaced some other way) 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. #[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()); } @@ -183,13 +225,11 @@ pub async fn apply_settings_import( .pending_settings_import .lock() .await - .take() + .clone() .ok_or_else(|| "No import is pending — choose a file first.".to_string())?; let payload = read_and_decrypt(&path, &password)?; - let saved = crate::commands::settings_commands::update_settings(payload.settings, state).await?; - 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); @@ -206,6 +246,20 @@ pub async fn apply_settings_import( } } + // The web-terminal token lives inside `AppSettings` itself rather than + // the keychain, so "leave an absent secret alone" has to be done by + // hand here: carry the destination's current token forward when the + // import doesn't have one, instead of letting the wholesale replace + // below blank it (every export writes `None` there — see + // `split_settings_and_secrets`). + let mut settings = payload.settings; + settings.web_terminal.access_token = non_blank(payload.secrets.web_terminal_access_token) + .or_else(|| state.settings_store.get().web_terminal.access_token); + + let saved = crate::commands::settings_commands::update_settings(settings, state.clone()).await?; + + state.pending_settings_import.lock().await.take(); + Ok(saved) } @@ -213,20 +267,150 @@ fn non_blank(value: Option) -> Option { value.filter(|v| !v.trim().is_empty()) } -fn read_and_decrypt(path: &std::path::Path, password: &str) -> Result { +/// Only the field `read_and_decrypt` needs before deciding whether the rest +/// of the payload is even worth attempting to parse. +#[derive(serde::Deserialize)] +struct FormatVersionProbe { + format_version: u32, +} + +/// Decrypt and parse an export file, 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 +/// `#[serde(default)]`-covered one) is exactly the case this check exists +/// for, and parsing the full struct first would fail on the shape mismatch +/// before the version check ever ran, surfacing a raw parse error instead +/// of "update Triple-C" — and, more seriously, `serde_json`'s type-mismatch +/// errors quote the offending value inline. This file is not attacker +/// content in the usual sense (it must still decrypt under the right +/// 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)?; - let payload: SettingsExportPayload = serde_json::from_slice(&plaintext) - .map_err(|e| format!("This file doesn't look like a valid settings export: {}", e))?; - - if payload.format_version > SETTINGS_EXPORT_FORMAT_VERSION { + let probe: FormatVersionProbe = serde_json::from_slice(&plaintext) + .map_err(|_| "This file doesn't look like a valid settings export.".to_string())?; + if probe.format_version > SETTINGS_EXPORT_FORMAT_VERSION { return Err(format!( "This export was made by a newer version of Triple-C (format {}, this app supports up to {}). \ Update Triple-C before importing it.", - payload.format_version, SETTINGS_EXPORT_FORMAT_VERSION + probe.format_version, SETTINGS_EXPORT_FORMAT_VERSION )); } - Ok(payload) + serde_json::from_slice(&plaintext) + .map_err(|_| "This file doesn't look like a valid settings export (unexpected shape).".to_string()) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn non_blank_treats_whitespace_only_as_absent() { + assert_eq!(non_blank(Some(" ".to_string())), None); + assert_eq!(non_blank(Some("".to_string())), None); + assert_eq!(non_blank(None), None); + assert_eq!(non_blank(Some(" a ".to_string())), Some(" a ".to_string())); + } + + fn write_export(dir: &std::path::Path, name: &str, payload: &SettingsExportPayload, password: &str) -> PathBuf { + let plaintext = serde_json::to_vec(payload).unwrap(); + let encrypted = settings_crypto::encrypt(&plaintext, password).unwrap(); + let path = dir.join(name); + std::fs::write(&path, &encrypted).unwrap(); + path + } + + fn sample_payload(format_version: u32) -> SettingsExportPayload { + SettingsExportPayload { + format_version, + exported_at: "2026-08-27T00:00:00Z".to_string(), + app_version: "0.4.14".to_string(), + settings: AppSettings::default(), + secrets: ExportedSecrets::default(), + } + } + + fn temp_dir(name: &str) -> PathBuf { + let dir = std::env::temp_dir().join(format!( + "triple-c-settings-export-test-{}-{}", + name, + uuid::Uuid::new_v4().simple() + )); + std::fs::create_dir_all(&dir).unwrap(); + dir + } + + #[test] + fn a_file_from_a_newer_format_is_refused_before_the_full_shape_is_parsed() { + let dir = temp_dir("newer-format"); + let path = write_export( + &dir, + "export.triplec", + &sample_payload(SETTINGS_EXPORT_FORMAT_VERSION + 1), + "correct password", + ); + + let err = read_and_decrypt(&path, "correct password").unwrap_err(); + assert!(err.contains("newer version"), "unexpected message: {}", err); + assert!(err.contains("Update Triple-C")); + + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn a_file_at_the_current_format_is_accepted() { + let dir = temp_dir("current-format"); + let path = write_export( + &dir, + "export.triplec", + &sample_payload(SETTINGS_EXPORT_FORMAT_VERSION), + "correct password", + ); + + let payload = read_and_decrypt(&path, "correct password").unwrap(); + assert_eq!(payload.format_version, SETTINGS_EXPORT_FORMAT_VERSION); + + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn a_malformed_payload_produces_a_generic_error_not_a_raw_serde_message() { + // Encrypt something that decrypts fine but isn't a valid payload + // shape at all — this must not happen in practice (only this app + // ever writes these files), but the error path must still never + // echo back plaintext content, generic malformed-shape or not. + let dir = temp_dir("malformed"); + let plaintext = b"{\"not\": \"a real export\"}".to_vec(); + let encrypted = settings_crypto::encrypt(&plaintext, "correct password").unwrap(); + let path = dir.join("export.triplec"); + std::fs::write(&path, &encrypted).unwrap(); + + let err = read_and_decrypt(&path, "correct password").unwrap_err(); + assert!(!err.contains("not a real export"), "leaked plaintext into the error: {}", err); + assert!(err.contains("doesn't look like a valid settings export")); + + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn the_wrong_password_is_reported_without_a_version_check_ever_running() { + let dir = temp_dir("wrong-password"); + let path = write_export( + &dir, + "export.triplec", + &sample_payload(SETTINGS_EXPORT_FORMAT_VERSION), + "correct password", + ); + + let err = read_and_decrypt(&path, "wrong password").unwrap_err(); + assert!(err.contains("Wrong password"), "unexpected message: {}", err); + + std::fs::remove_dir_all(&dir).ok(); + } } diff --git a/app/src-tauri/src/models/settings_export.rs b/app/src-tauri/src/models/settings_export.rs index 0e71df8..acf33bc 100644 --- a/app/src-tauri/src/models/settings_export.rs +++ b/app/src-tauri/src/models/settings_export.rs @@ -2,12 +2,28 @@ //! //! `SettingsExportPayload` is the whole plaintext export before encryption //! and after decryption (see `storage::settings_crypto`). It bundles -//! `AppSettings` (already the non-secret shape persisted to `settings.json`) -//! with the global secrets that live in the OS keychain instead — the shared -//! Claude Code OAuth login and the model gateway's two keys. Per-project -//! settings, per-project secrets, and anything living in a project's Docker -//! volumes are deliberately out of scope: this exports the *host* -//! environment, not any one project's. +//! `AppSettings` — with one field carved out, see below — with the global +//! secrets that live in the OS keychain instead: the shared Claude Code +//! OAuth login and the model gateway's two keys. Per-project settings, +//! per-project secrets, and anything living in a project's Docker volumes +//! are deliberately out of scope: this exports the *host* environment, not +//! any one project's. +//! +//! **`AppSettings` is not entirely the non-secret shape it looks like.** +//! `WebTerminalSettings::access_token` is a live bearer credential for a +//! server that binds every interface, stored as a plain field on the +//! struct that is otherwise safe to treat as config. A review of this +//! feature caught it: exporting `AppSettings` wholesale would have carried +//! that token along as if it were as inert as a port number, and — worse — +//! importing it would apply `web_terminal.enabled` and the token together +//! with no more warning than any other setting, letting a crafted export +//! silently stand up a LAN-listening terminal server with an +//! attacker-known token on the next launch. `export_settings` / +//! `apply_settings_import` blank this field out of the `settings` they +//! read from and write to, and it travels only through +//! [`ExportedSecrets::web_terminal_access_token`] instead, with the same +//! "only overwrite what the import actually has" treatment as the other +//! three secrets. use serde::{Deserialize, Serialize}; @@ -34,6 +50,12 @@ pub struct ExportedSecrets { pub gateway_api_key: Option, #[serde(default)] pub gateway_master_key: Option, + /// See the module doc comment — this is `AppSettings::web_terminal + /// .access_token`, carved out because it is a live bearer credential, + /// not config, despite living on a struct that is otherwise safe to + /// export wholesale. + #[serde(default)] + pub web_terminal_access_token: Option, } impl ExportedSecrets { @@ -41,6 +63,7 @@ impl ExportedSecrets { 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() } } @@ -78,31 +101,31 @@ pub struct SettingsImportPreview { pub has_claude_oauth_token: bool, pub has_gateway_api_key: bool, pub has_gateway_master_key: bool, + pub has_web_terminal_access_token: bool, + /// Whether the imported settings turn the web terminal on. Named + /// separately from the token above: `enabled` and the token are two + /// different fields, either can be true without the other, and + /// "this import turns on a service that listens on your network" is + /// exactly the kind of change a wholesale settings replace must not + /// bury in a generic "settings replaced" line — see the module doc + /// comment on why this field exists at all. + pub enables_web_terminal: bool, } 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()); Self { exported_at: payload.exported_at.clone(), app_version: payload.app_version.clone(), custom_env_var_count: payload.settings.global_custom_env_vars.len(), gateway_model_count: payload.settings.gateway.models.len(), has_claude_code_settings: payload.settings.global_claude_code_settings.is_some(), - has_claude_oauth_token: payload - .secrets - .claude_oauth_token - .as_deref() - .is_some_and(|t| !t.trim().is_empty()), - has_gateway_api_key: payload - .secrets - .gateway_api_key - .as_deref() - .is_some_and(|k| !k.trim().is_empty()), - has_gateway_master_key: payload - .secrets - .gateway_master_key - .as_deref() - .is_some_and(|k| !k.trim().is_empty()), + has_claude_oauth_token: non_blank(&payload.secrets.claude_oauth_token), + has_gateway_api_key: non_blank(&payload.secrets.gateway_api_key), + 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, } } } @@ -133,6 +156,7 @@ mod tests { claude_oauth_token: Some("sk-super-secret-token".to_string()), gateway_api_key: Some("sk-another-secret".to_string()), gateway_master_key: Some("sk-triple-c-yet-another".to_string()), + web_terminal_access_token: Some("wt-super-secret-token".to_string()), }); let preview = SettingsImportPreview::from_payload(&payload); let serialized = serde_json::to_string(&preview).unwrap(); @@ -140,9 +164,11 @@ mod tests { assert!(!serialized.contains("sk-super-secret-token")); assert!(!serialized.contains("sk-another-secret")); assert!(!serialized.contains("sk-triple-c-yet-another")); + assert!(!serialized.contains("wt-super-secret-token")); assert!(preview.has_claude_oauth_token); assert!(preview.has_gateway_api_key); assert!(preview.has_gateway_master_key); + assert!(preview.has_web_terminal_access_token); } #[test] @@ -154,11 +180,27 @@ mod tests { claude_oauth_token: Some(" ".to_string()), gateway_api_key: None, gateway_master_key: None, + web_terminal_access_token: Some(" ".to_string()), }); let preview = SettingsImportPreview::from_payload(&payload); assert!(!preview.has_claude_oauth_token); assert!(!preview.has_gateway_api_key); assert!(!preview.has_gateway_master_key); + assert!(!preview.has_web_terminal_access_token); + } + + #[test] + fn enabling_the_web_terminal_is_surfaced_regardless_of_whether_a_token_came_with_it() { + // `enabled` and the token are independent fields — a crafted export + // could set one without the other, and both are worth a user's + // attention: this is the field that exists specifically so "this + // import turns on a service that listens on your network" cannot + // hide inside a generic "settings replaced" summary. + let mut payload = payload_with(ExportedSecrets::default()); + payload.settings.web_terminal.enabled = true; + let preview = SettingsImportPreview::from_payload(&payload); + assert!(preview.enables_web_terminal); + assert!(!preview.has_web_terminal_access_token); } #[test] diff --git a/app/src-tauri/src/storage/settings_crypto.rs b/app/src-tauri/src/storage/settings_crypto.rs index 499e7d4..37d40df 100644 --- a/app/src-tauri/src/storage/settings_crypto.rs +++ b/app/src-tauri/src/storage/settings_crypto.rs @@ -24,6 +24,7 @@ use aes_gcm::aead::{Aead, KeyInit}; use aes_gcm::{Aes256Gcm, Nonce}; use argon2::{Algorithm, Argon2, Params, Version}; use rand::RngCore; +use zeroize::Zeroizing; /// Identifies the file as a Triple-C settings export and pins the format — /// a change to the salt/nonce lengths or the KDF/cipher choice below needs a @@ -44,11 +45,16 @@ fn argon2_params() -> Params { Params::new(19 * 1024, 2, 1, Some(KEY_LEN)).expect("hardcoded Argon2 params are valid") } -fn derive_key(password: &str, salt: &[u8]) -> Result<[u8; KEY_LEN], String> { +/// The derived key is wrapped in `Zeroizing` so it is overwritten with zeros +/// when it drops rather than left in freed memory for whatever reuses that +/// stack slot next — cheap insurance (`zeroize` is already in the dependency +/// tree via `aes-gcm`) for material that exists only to decrypt live +/// credentials. +fn derive_key(password: &str, salt: &[u8]) -> Result, String> { let argon2 = Argon2::new(Algorithm::Argon2id, Version::V0x13, argon2_params()); - let mut key = [0u8; KEY_LEN]; + let mut key = Zeroizing::new([0u8; KEY_LEN]); argon2 - .hash_password_into(password.as_bytes(), salt, &mut key) + .hash_password_into(password.as_bytes(), salt, &mut *key) .map_err(|e| format!("Failed to derive encryption key: {}", e))?; Ok(key) } @@ -64,7 +70,7 @@ pub fn encrypt(plaintext: &[u8], password: &str) -> Result, String> { rand::rng().fill_bytes(&mut nonce_bytes); let nonce = Nonce::from_slice(&nonce_bytes); - let cipher = Aes256Gcm::new_from_slice(&key) + let cipher = Aes256Gcm::new_from_slice(&*key) .map_err(|e| format!("Failed to initialize cipher: {}", e))?; let ciphertext = cipher .encrypt(nonce, plaintext) @@ -84,7 +90,12 @@ pub fn encrypt(plaintext: &[u8], password: &str) -> Result, String> { /// fails to verify for the wrong key on essentially any ciphertext, so there /// is no reliable way to tell "wrong password" from "corrupted file" apart, /// and guessing would be worse than saying so. -pub fn decrypt(data: &[u8], password: &str) -> Result, String> { +/// +/// Returns `Zeroizing>` rather than a plain `Vec` — the plaintext +/// this recovers is the whole settings-plus-secrets payload, so it gets the +/// same "wipe it when it drops" treatment as the derived key in +/// [`derive_key`]. +pub fn decrypt(data: &[u8], password: &str) -> Result>, String> { if data.len() < HEADER_LEN { return Err("This does not look like a Triple-C settings export (file too short).".to_string()); } @@ -96,11 +107,12 @@ pub fn decrypt(data: &[u8], password: &str) -> Result, String> { let ciphertext = &data[HEADER_LEN..]; let key = derive_key(password, salt)?; - let cipher = Aes256Gcm::new_from_slice(&key) + let cipher = Aes256Gcm::new_from_slice(&*key) .map_err(|e| format!("Failed to initialize cipher: {}", e))?; let nonce = Nonce::from_slice(nonce_bytes); cipher .decrypt(nonce, ciphertext) + .map(Zeroizing::new) .map_err(|_| "Wrong password, or the file is corrupted.".to_string()) } @@ -113,7 +125,7 @@ mod tests { let plaintext = b"{\"settings\": \"whatever\"}"; let encrypted = encrypt(plaintext, "correct horse battery staple").unwrap(); let decrypted = decrypt(&encrypted, "correct horse battery staple").unwrap(); - assert_eq!(decrypted, plaintext); + assert_eq!(&*decrypted, plaintext); } #[test] diff --git a/app/src/components/settings/ImportSettingsModal.test.tsx b/app/src/components/settings/ImportSettingsModal.test.tsx index 345d916..be8bc0e 100644 --- a/app/src/components/settings/ImportSettingsModal.test.tsx +++ b/app/src/components/settings/ImportSettingsModal.test.tsx @@ -24,6 +24,8 @@ const samplePreview: SettingsImportPreview = { has_claude_oauth_token: true, has_gateway_api_key: false, has_gateway_master_key: false, + has_web_terminal_access_token: false, + enables_web_terminal: false, }; describe("ImportSettingsModal", () => { @@ -54,6 +56,16 @@ describe("ImportSettingsModal", () => { expect(await screen.findByText(/settings imported/i)).toBeInTheDocument(); }); + it("shows a distinct warning when the import would enable the web terminal", async () => { + previewSettingsImport.mockResolvedValue({ ...samplePreview, enables_web_terminal: true }); + render(); + + fireEvent.change(screen.getByLabelText("Password"), { target: { value: "hunter2" } }); + fireEvent.click(screen.getByRole("button", { name: /choose file/i })); + + expect(await screen.findByText(/enables the remote web terminal/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 c8abb8d..c0b79f4 100644 --- a/app/src/components/settings/ImportSettingsModal.tsx +++ b/app/src/components/settings/ImportSettingsModal.tsx @@ -3,7 +3,7 @@ import Modal from "../ui/Modal"; import Button from "../ui/Button"; import Field, { inputClass } from "../ui/Field"; import { applySettingsImport, previewSettingsImport } from "../../lib/tauri-commands"; -import { describeImport } from "../../lib/settingsImportPreview"; +import { describeImport, describeImportWarnings } from "../../lib/settingsImportPreview"; import type { AppSettings, SettingsImportPreview } from "../../lib/types"; interface Props { @@ -114,6 +114,14 @@ export default function ImportSettingsModal({ onClose, onImported }: Props) { ))} + {describeImportWarnings(preview).map((warning) => ( +

+ {warning} +

+ ))} {error &&

{error}

} ) : ( diff --git a/app/src/lib/settingsImportPreview.test.ts b/app/src/lib/settingsImportPreview.test.ts index 4173f42..cad7b81 100644 --- a/app/src/lib/settingsImportPreview.test.ts +++ b/app/src/lib/settingsImportPreview.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from "vitest"; -import { describeImport } from "./settingsImportPreview"; +import { describeImport, describeImportWarnings } from "./settingsImportPreview"; import type { SettingsImportPreview } from "./types"; function preview(overrides: Partial = {}): SettingsImportPreview { @@ -12,6 +12,8 @@ function preview(overrides: Partial = {}): SettingsImport has_claude_oauth_token: false, has_gateway_api_key: false, has_gateway_master_key: false, + has_web_terminal_access_token: false, + enables_web_terminal: false, ...overrides, }; } @@ -52,4 +54,32 @@ describe("describeImport", () => { expect(items.some((i) => i.includes("env var"))).toBe(false); expect(items.some((i) => i.includes("gateway model"))).toBe(false); }); + + it("names the web terminal access token like any other present secret", () => { + const items = describeImport(preview({ has_web_terminal_access_token: true })); + expect(items).toContain("The web terminal access token"); + }); +}); + +describe("describeImportWarnings", () => { + it("is empty when nothing about the import needs extra attention", () => { + expect(describeImportWarnings(preview())).toEqual([]); + }); + + it("warns when the import enables the web terminal, regardless of the token", () => { + // `enabled` and the token are independent — the warning is about the + // service turning on, whether or not a token came with it. + expect(describeImportWarnings(preview({ enables_web_terminal: true }))).toEqual([ + "Enables the remote web terminal, which listens on your network.", + ]); + expect( + describeImportWarnings( + preview({ enables_web_terminal: true, has_web_terminal_access_token: true }), + ), + ).toHaveLength(1); + }); + + it("does not warn just because a web terminal token is present but the terminal is off", () => { + expect(describeImportWarnings(preview({ has_web_terminal_access_token: true }))).toEqual([]); + }); }); diff --git a/app/src/lib/settingsImportPreview.ts b/app/src/lib/settingsImportPreview.ts index dde8d46..0b34db1 100644 --- a/app/src/lib/settingsImportPreview.ts +++ b/app/src/lib/settingsImportPreview.ts @@ -1,7 +1,9 @@ import type { SettingsImportPreview } from "./types"; /** Named things a `SettingsImportPreview` says an import will change, for - * `ImportSettingsModal`'s confirmation list. */ + * `ImportSettingsModal`'s confirmation list. Does not include anything + * `describeImportWarnings` covers — those get their own, more visible + * treatment rather than blending into this list. */ export function describeImport(preview: SettingsImportPreview): string[] { const items: string[] = ["Your global settings (all of them — this replaces what's here now)"]; if (preview.custom_env_var_count > 0) { @@ -16,5 +18,22 @@ export function describeImport(preview: SettingsImportPreview): string[] { if (preview.has_claude_oauth_token) items.push("Your shared Claude login"); if (preview.has_gateway_api_key) items.push("The gateway provider API key"); if (preview.has_gateway_master_key) items.push("The gateway master key"); + if (preview.has_web_terminal_access_token) items.push("The web terminal access token"); return items; } + +/** + * Things about an import that deserve more attention than a bullet in a + * long list — currently just the one, but deliberately its own function + * rather than a flag inside `describeImport`: a setting that turns on a + * network-listening service is exactly the kind of change a "your settings + * were replaced" summary is bad at surfacing, on purpose or (if the file + * came from someone else) not. + */ +export function describeImportWarnings(preview: SettingsImportPreview): string[] { + const warnings: string[] = []; + if (preview.enables_web_terminal) { + warnings.push("Enables the remote web terminal, which listens on your network."); + } + return warnings; +} diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index baadc0b..bca46dc 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -304,6 +304,12 @@ export interface SettingsImportPreview { has_claude_oauth_token: boolean; has_gateway_api_key: boolean; has_gateway_master_key: boolean; + has_web_terminal_access_token: boolean; + /** Whether the import turns the web terminal on — surfaced separately + * from the token above since either can be true without the other, and + * "this enables a service that listens on your network" must not hide + * inside a generic "settings replaced" summary. */ + enables_web_terminal: boolean; } /** What `inspect_ca_cert_path` reports about a corporate CA path. Errors ride