From 2dea30f4fb427fb21347cc36b151bd76a698bea0 Mon Sep 17 00:00:00 2001 From: Josh Knapp Date: Sat, 14 Nov 2026 15:46:05 -0800 Subject: [PATCH] feat(aws): fail-closed overwrite check, name local connections, disclose removals in import preview Co-Authored-By: Claude Opus 5.5 --- .../src/commands/settings_export_commands.rs | 2 +- app/src-tauri/src/models/settings_export.rs | 119 +++++++++++++++--- app/src-tauri/src/storage/secure.rs | 19 ++- .../settings/ImportSettingsModal.test.tsx | 14 ++- app/src/lib/settingsImportPreview.test.ts | 1 + app/src/lib/settingsImportPreview.ts | 28 ++++- app/src/lib/types.ts | 13 ++ 7 files changed, 165 insertions(+), 31 deletions(-) diff --git a/app/src-tauri/src/commands/settings_export_commands.rs b/app/src-tauri/src/commands/settings_export_commands.rs index cb42c4b..6abaccd 100644 --- a/app/src-tauri/src/commands/settings_export_commands.rs +++ b/app/src-tauri/src/commands/settings_export_commands.rs @@ -364,7 +364,7 @@ pub async fn preview_settings_import( 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 local_hosts = state.settings_store.get().hosts; - let preview = SettingsImportPreview::from_payload_against(&payload, &local_hosts, secure::aws_keys_entry_exists); + let preview = SettingsImportPreview::from_payload_against(&payload, &local_hosts, secure::aws_keys_entry_state); *state.pending_settings_import.lock().await = Some(PendingSettingsImport { path, diff --git a/app/src-tauri/src/models/settings_export.rs b/app/src-tauri/src/models/settings_export.rs index 801f178..715cbea 100644 --- a/app/src-tauri/src/models/settings_export.rs +++ b/app/src-tauri/src/models/settings_export.rs @@ -185,6 +185,37 @@ pub struct SettingsImportPreview { /// Non-blank AWS access-key entries the import restores to the keychain. #[serde(default)] pub aws_connection_key_count: usize, + /// Local connections the import drops, because it replaces the whole list. + /// Their keychain entries, if any, are left behind. + #[serde(default)] + pub removed_aws_connections: Vec, +} + +/// Cleaned, non-secret description of one connection. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] +pub struct AwsConnectionSummary { + pub name: String, + /// `"profile"` or `"keys"`. + pub auth: String, + pub profile_name: Option, + pub default_region: String, +} + +impl AwsConnectionSummary { + fn of(h: &crate::models::host_connection::HostConnection) -> Self { + use crate::models::host_connection::{AwsAuth, HostKind}; + let HostKind::Aws { auth, default_region } = &h.kind; + let (auth_kind, profile_name) = match auth { + AwsAuth::Profile { name } => ("profile", Some(sanitize_for_preview(name))), + AwsAuth::Keys { .. } => ("keys", None), + }; + Self { + name: sanitize_for_preview(&h.name), + auth: auth_kind.to_string(), + profile_name, + default_region: sanitize_for_preview(default_region), + } + } } /// One imported AWS connection as the confirmation dialog shows it. @@ -201,6 +232,14 @@ pub struct AwsConnectionPreview { /// The import carries keys for this id and a keychain entry for it /// already exists here (its value is never read into the preview). pub overwrites_keys: bool, + /// The import carries keys for this id and the keychain could not be + /// asked whether some are stored already. + #[serde(default)] + pub keys_check_failed: bool, + /// The local connection with this id, when the import replaces it, so an + /// imported name cannot hide which connection is being replaced. + #[serde(default)] + pub local: Option, } /// A cap on how much of a decrypted, not-yet-trusted string gets echoed back @@ -225,19 +264,20 @@ fn sanitize_for_preview(value: &str) -> String { } impl SettingsImportPreview { + #[cfg(test)] pub fn from_payload(payload: &SettingsExportPayload) -> Self { - Self::from_payload_against(payload, &[], |_| false) + Self::from_payload_against(payload, &[], |_| Some(false)) } - /// As [`Self::from_payload`], also flagging what the import would - /// overwrite: `local_hosts` are the connections held now and - /// `local_has_keys` says whether a keychain entry exists for an id. + /// Builds the preview, also flagging what the import would overwrite or + /// drop: `local_hosts` are the connections held now and `local_has_keys` + /// says whether a keychain entry exists for an id (`None`: could not be + /// determined). pub fn from_payload_against( payload: &SettingsExportPayload, local_hosts: &[crate::models::host_connection::HostConnection], - local_has_keys: impl Fn(&str) -> bool, + local_has_keys: impl Fn(&str) -> Option, ) -> Self { - use crate::models::host_connection::{AwsAuth, HostKind}; let carried_keys = |id: &str| { payload .secrets @@ -250,21 +290,29 @@ impl SettingsImportPreview { .hosts .iter() .map(|h| { - let HostKind::Aws { auth, default_region } = &h.kind; - let (auth_kind, profile_name) = match auth { - AwsAuth::Profile { name } => ("profile", Some(sanitize_for_preview(name))), - AwsAuth::Keys { .. } => ("keys", None), - }; + let summary = AwsConnectionSummary::of(h); + let local = local_hosts.iter().find(|l| l.id == h.id); + let overwrites_existing = local.is_some_and(|l| l != h); + let key_state = if carried_keys(&h.id) { local_has_keys(&h.id) } else { Some(false) }; AwsConnectionPreview { - name: sanitize_for_preview(&h.name), - auth: auth_kind.to_string(), - profile_name, - default_region: sanitize_for_preview(default_region), - overwrites_existing: local_hosts.iter().any(|l| l.id == h.id && l != h), - overwrites_keys: carried_keys(&h.id) && local_has_keys(&h.id), + name: summary.name, + auth: summary.auth, + profile_name: summary.profile_name, + default_region: summary.default_region, + overwrites_existing, + overwrites_keys: key_state == Some(true), + keys_check_failed: key_state.is_none(), + local: local + .filter(|_| overwrites_existing || key_state != Some(false)) + .map(AwsConnectionSummary::of), } }) .collect(); + let removed_aws_connections = local_hosts + .iter() + .filter(|l| !payload.settings.hosts.iter().any(|h| h.id == l.id)) + .map(AwsConnectionSummary::of) + .collect(); let non_blank = |s: &Option| s.as_deref().is_some_and(|v| !v.trim().is_empty()); let sanitized_non_blank = |s: &Option| { s.as_deref() @@ -316,6 +364,7 @@ impl SettingsImportPreview { .values() .filter(|v| !v.trim().is_empty()) .count(), + removed_aws_connections, } } } @@ -605,7 +654,7 @@ mod tests { ]; let local = vec![aws_host("same", "Same", false), aws_host("changed", "Old name", false)]; let preview = - SettingsImportPreview::from_payload_against(&payload, &local, |id| id == "k1"); + SettingsImportPreview::from_payload_against(&payload, &local, |id| Some(id == "k1")); let flags: Vec<_> = preview .aws_connections .iter() @@ -613,4 +662,38 @@ mod tests { .collect(); assert_eq!(flags, vec![(false, true), (false, false), (true, false), (false, false)]); } + + #[test] + fn aws_preview_names_the_local_connection_and_discloses_removals_and_failed_checks() { + let mut payload = payload_with(ExportedSecrets { + aws_connection_keys: BTreeMap::from([ + ("k1".to_string(), "{}".to_string()), + ("k2".to_string(), "{}".to_string()), + ]), + ..Default::default() + }); + payload.settings.hosts = vec![ + aws_host("k1", "Innocent", true), + aws_host("k2", "Other", true), + aws_host("p1", "Fresh Name", false), + ]; + let local = vec![ + aws_host("k1", "Real Prod", true), + aws_host("p1", "Real Dev", false), + aws_host("gone", "Dropped\n", false), + ]; + let preview = SettingsImportPreview::from_payload_against(&payload, &local, |id| match id { + "k1" => Some(true), + _ => None, + }); + let c = &preview.aws_connections; + assert!(c[0].overwrites_keys && !c[0].keys_check_failed); + assert_eq!(c[0].local.as_ref().unwrap().name, "Real Prod"); + assert!(c[1].keys_check_failed && !c[1].overwrites_keys && c[1].local.is_none()); + assert!(c[2].overwrites_existing); + assert_eq!(c[2].local.as_ref().unwrap().name, "Real Dev"); + assert_eq!(c[2].local.as_ref().unwrap().profile_name.as_deref(), Some("prod-profile")); + assert_eq!(preview.removed_aws_connections.len(), 1); + assert_eq!(preview.removed_aws_connections[0].name, "Dropped"); + } } diff --git a/app/src-tauri/src/storage/secure.rs b/app/src-tauri/src/storage/secure.rs index c001221..903cb6e 100644 --- a/app/src-tauri/src/storage/secure.rs +++ b/app/src-tauri/src/storage/secure.rs @@ -474,12 +474,19 @@ pub fn get_aws_keys(connection_id: &str) -> Result, String> { .map_err(|_| "The stored AWS access keys are unreadable".to_string()) } -/// Whether any entry exists for the connection, readable or not. Never -/// returns or logs a value. -pub fn aws_keys_entry_exists(connection_id: &str) -> bool { - aws_connection_service(connection_id) - .and_then(|svc| read_entry(&svc, "the AWS access keys")) - .is_ok_and(|v| v.is_some()) +/// Whether an entry exists for the connection, readable or not. `None` means +/// the keychain could not be asked, so callers must not read it as "absent". +/// Never returns or logs a value. +pub fn aws_keys_entry_state(connection_id: &str) -> Option { + let svc = aws_connection_service(connection_id).ok()?; + let entry = keyring::Entry::new(&svc, KEYCHAIN_ACCOUNT).ok()?; + match entry.get_password() { + Ok(_) => Some(true), + Err(keyring::Error::NoEntry) => Some(false), + // Present but not a plain string, etc.: something is there. + Err(keyring::Error::BadEncoding(_)) => Some(true), + Err(_) => None, + } } /// Missing entry is success. Unused until connection removal lands. diff --git a/app/src/components/settings/ImportSettingsModal.test.tsx b/app/src/components/settings/ImportSettingsModal.test.tsx index a1d1bdb..5fca998 100644 --- a/app/src/components/settings/ImportSettingsModal.test.tsx +++ b/app/src/components/settings/ImportSettingsModal.test.tsx @@ -38,6 +38,7 @@ const samplePreview: SettingsImportPreview = { marketplace_account_token_count: 0, aws_connections: [], aws_connection_key_count: 0, + removed_aws_connections: [], }; function outcome(settings: AppSettings, secretRestoreWarnings: string[] = []): SettingsImportOutcome { @@ -93,6 +94,8 @@ describe("ImportSettingsModal", () => { default_region: "us-west-2", overwrites_existing: true, overwrites_keys: true, + keys_check_failed: false, + local: { name: "Real Prod", auth: "keys", profile_name: null, default_region: "us-east-1" }, }, { name: "Dev", @@ -101,9 +104,14 @@ describe("ImportSettingsModal", () => { default_region: "eu-west-1", overwrites_existing: false, overwrites_keys: false, + keys_check_failed: true, + local: null, }, ], aws_connection_key_count: 1, + removed_aws_connections: [ + { name: "Old", auth: "profile", profile_name: "old-p", default_region: "ap-south-1" }, + ], }); render(); @@ -113,8 +121,10 @@ describe("ImportSettingsModal", () => { expect(await screen.findByText(/AWS connection "Prod" \(access keys, us-west-2\)/)).toBeInTheDocument(); expect(screen.getByText(/AWS connection "Dev" \(profile dev-profile, eu-west-1\)/)).toBeInTheDocument(); expect(screen.getByText(/1 AWS access key set$/)).toBeInTheDocument(); - expect(screen.getByText(/Overwrites the AWS access keys stored for "Prod"/)).toBeInTheDocument(); - expect(screen.getByText(/Replaces your existing AWS connection.*"Prod"/)).toBeInTheDocument(); + expect(screen.getByText(/Overwrites the AWS access keys stored for your connection "Real Prod"/)).toBeInTheDocument(); + expect(screen.getByText(/Replaces your connection "Real Prod" \(access keys, us-east-1\) with "Prod"/)).toBeInTheDocument(); + expect(screen.getByText(/Could not check whether keys are stored for "Dev".*may overwrite them/)).toBeInTheDocument(); + expect(screen.getByText(/Removes your AWS connection "Old" \(profile old-p, ap-south-1\)/)).toBeInTheDocument(); }); it("warns about a custom Docker image every time, not just on change", async () => { diff --git a/app/src/lib/settingsImportPreview.test.ts b/app/src/lib/settingsImportPreview.test.ts index 6973618..d2fe30d 100644 --- a/app/src/lib/settingsImportPreview.test.ts +++ b/app/src/lib/settingsImportPreview.test.ts @@ -26,6 +26,7 @@ function preview(overrides: Partial = {}): SettingsImport marketplace_account_token_count: 0, aws_connections: [], aws_connection_key_count: 0, + removed_aws_connections: [], ...overrides, }; } diff --git a/app/src/lib/settingsImportPreview.ts b/app/src/lib/settingsImportPreview.ts index aa31429..2ef338b 100644 --- a/app/src/lib/settingsImportPreview.ts +++ b/app/src/lib/settingsImportPreview.ts @@ -1,4 +1,4 @@ -import type { SettingsImportPreview } from "./types"; +import type { AwsConnectionSummary, SettingsImportPreview } from "./types"; /** Named things a `SettingsImportPreview` says an import will change, for * `ImportSettingsModal`'s confirmation list. Does not include anything @@ -91,12 +91,27 @@ export function describeImportWarnings(preview: SettingsImportPreview): string[] ); } for (const c of preview.aws_connections) { - if (c.overwrites_existing) { - warnings.push(`Replaces your existing AWS connection with different settings: "${c.name}".`); + const localDesc = c.local ? describeAws(c.local) : null; + if (c.overwrites_existing && c.local) { + warnings.push(`Replaces your connection ${localDesc} with "${c.name}" (${c.auth === "keys" ? "access keys" : "profile " + (c.profile_name ?? "")}, ${c.default_region}).`); } if (c.overwrites_keys) { - warnings.push(`Overwrites the AWS access keys stored for "${c.name}".`); + warnings.push( + c.local + ? `Overwrites the AWS access keys stored for your connection ${localDesc} (imported as "${c.name}").` + : `Overwrites AWS access keys already stored for the imported connection "${c.name}".`, + ); } + if (c.keys_check_failed) { + warnings.push( + `Could not check whether keys are stored for "${c.name}"${c.local ? ` (your connection ${localDesc})` : ""}; importing may overwrite them.`, + ); + } + } + for (const r of preview.removed_aws_connections) { + warnings.push( + `Removes your AWS connection ${describeAws(r)}, which is not in the file. Any access keys stored for it stay in your keychain.`, + ); } if (preview.image_source === "custom") { warnings.push( @@ -105,3 +120,8 @@ export function describeImportWarnings(preview: SettingsImportPreview): string[] } return warnings; } + +function describeAws(c: AwsConnectionSummary): string { + const auth = c.auth === "keys" ? "access keys" : `profile ${c.profile_name ?? ""}`.trim(); + return `"${c.name}" (${auth}, ${c.default_region})`; +} diff --git a/app/src/lib/types.ts b/app/src/lib/types.ts index e1ff4a7..1920d61 100644 --- a/app/src/lib/types.ts +++ b/app/src/lib/types.ts @@ -429,6 +429,15 @@ export interface SettingsImportPreview { aws_connections: AwsConnectionPreview[]; /** AWS access-key entries the import restores to the keychain. */ aws_connection_key_count: number; + /** Local AWS connections the import drops (it replaces the whole list). */ + removed_aws_connections: AwsConnectionSummary[]; +} + +export interface AwsConnectionSummary { + name: string; + auth: "profile" | "keys"; + profile_name: string | null; + default_region: string; } export interface AwsConnectionPreview { @@ -440,6 +449,10 @@ export interface AwsConnectionPreview { overwrites_existing: boolean; /** The import carries keys for an id that already has keys stored here. */ overwrites_keys: boolean; + /** The keychain could not be asked whether keys are already stored. */ + keys_check_failed: boolean; + /** The local connection with this id, when the import replaces it. */ + local: AwsConnectionSummary | null; } /** What `apply_settings_import` returns: the settings that were actually