diff --git a/src-tauri/src/commands.rs b/src-tauri/src/commands.rs index b1e963fba..c813c4653 100644 --- a/src-tauri/src/commands.rs +++ b/src-tauri/src/commands.rs @@ -771,6 +771,84 @@ fn persist_connection_uri_change( ) } +/// Keychain secrets a connection save wrote, each with the value it held +/// before. If the config file write fails afterwards they are put back, so a +/// save that never landed doesn't leave secrets behind in the keychain. +#[derive(Default)] +struct KeychainSecretWrites { + db_password: Option>, + ssh_password: Option>, + ssh_key_passphrase: Option>, +} + +impl KeychainSecretWrites { + /// Delete every secret this save wrote to the keychain. Used once the + /// config file landed with the secrets inline because a later keychain + /// write failed, so nothing the config doesn't point at is left behind. + fn discard(self, cache: &credential_cache::CredentialCache, connection_id: &str) { + Self { + db_password: self.db_password.map(|_| None), + ssh_password: self.ssh_password.map(|_| None), + ssh_key_passphrase: self.ssh_key_passphrase.map(|_| None), + } + .roll_back(cache, connection_id); + } + + fn roll_back(self, cache: &credential_cache::CredentialCache, connection_id: &str) { + fn restore( + label: &str, + connection_id: &str, + previous: Option, + set: impl FnOnce(&str) -> Result<(), String>, + delete: impl FnOnce() -> Result<(), String>, + ) { + let result = match previous.as_deref() { + Some(value) => set(value), + None => delete(), + }; + if let Err(err) = result { + log::warn!( + "Failed to roll back the {} for {} after a failed save: {}", + label, + connection_id, + err + ); + } + } + + if let Some(previous) = self.db_password { + restore( + "DB password", + connection_id, + previous, + |value| keychain_utils::set_db_password(connection_id, value), + || keychain_utils::delete_db_password(connection_id), + ); + credential_cache::invalidate_db_password(cache, connection_id); + } + if let Some(previous) = self.ssh_password { + restore( + "SSH password", + connection_id, + previous, + |value| keychain_utils::set_ssh_password(connection_id, value), + || keychain_utils::delete_ssh_password(connection_id), + ); + credential_cache::invalidate_ssh_password(cache, connection_id); + } + if let Some(previous) = self.ssh_key_passphrase { + restore( + "SSH key passphrase", + connection_id, + previous, + |value| keychain_utils::set_ssh_key_passphrase(connection_id, value), + || keychain_utils::delete_ssh_key_passphrase(connection_id), + ); + credential_cache::invalidate_ssh_key_passphrase(cache, connection_id); + } + } +} + /// Strip the URI out of the params that go to `connections.json`, leaving only /// the marker that says a keychain entry exists. fn params_for_persistence( @@ -1262,36 +1340,76 @@ pub async fn save_connection( let cache = app.state::>(); let connection_uri = runtime_connection_uri(¶ms).map(str::to_owned); let mut params_to_save = params_for_persistence(¶ms, connection_uri.is_some()); + let mut keychain_writes = KeychainSecretWrites::default(); if params.save_in_keychain.unwrap_or(false) { log::debug!("Storing passwords in keychain for connection: {}", name); + let mut keychain_stored = true; match keychain_utils::stored_password_change(params.password.as_deref()) { keychain_utils::StoredPasswordChange::Store(pwd) => { - keychain_utils::set_db_password(&id, pwd)?; - credential_cache::set_db_password_cached(&cache, &id, pwd); + match keychain_utils::set_db_password(&id, pwd) { + Ok(()) => { + keychain_writes.db_password = Some(None); + credential_cache::set_db_password_cached(&cache, &id, pwd); + } + Err(err) => { + log::warn!( + "Keychain unavailable while saving connection {}; storing secrets in the config file: {}", + name, err + ); + keychain_stored = false; + } + } } // A fresh id has nothing to delete; an empty password is simply // not stored (keyutils would reject it anyway). keychain_utils::StoredPasswordChange::Keep | keychain_utils::StoredPasswordChange::Delete => {} } - if params.ssh_enabled.unwrap_or(false) { + if keychain_stored && params.ssh_enabled.unwrap_or(false) { if let Some(ssh_pwd) = ¶ms.ssh_password { - keychain_utils::set_ssh_password(&id, ssh_pwd)?; - credential_cache::set_ssh_password_cached(&cache, &id, ssh_pwd); + match keychain_utils::set_ssh_password(&id, ssh_pwd) { + Ok(()) => { + keychain_writes.ssh_password = Some(None); + credential_cache::set_ssh_password_cached(&cache, &id, ssh_pwd); + } + Err(err) => { + log::warn!( + "Keychain unavailable while saving the SSH password for {}; storing secrets in the config file: {}", + name, err + ); + keychain_stored = false; + } + } } - if let Some(ssh_passphrase) = ¶ms.ssh_key_passphrase { - if !ssh_passphrase.trim().is_empty() { - keychain_utils::set_ssh_key_passphrase(&id, ssh_passphrase)?; - credential_cache::set_ssh_key_passphrase_cached(&cache, &id, ssh_passphrase); + if keychain_stored { + if let Some(ssh_passphrase) = ¶ms.ssh_key_passphrase { + if !ssh_passphrase.trim().is_empty() { + match keychain_utils::set_ssh_key_passphrase(&id, ssh_passphrase) { + Ok(()) => { + keychain_writes.ssh_key_passphrase = Some(None); + credential_cache::set_ssh_key_passphrase_cached( + &cache, + &id, + ssh_passphrase, + ); + } + Err(err) => { + log::warn!( + "Keychain unavailable while saving the SSH key passphrase for {}; storing secrets in the config file: {}", + name, err + ); + keychain_stored = false; + } + } + } } } } - params_to_save.password = None; - params_to_save.ssh_password = None; - params_to_save.ssh_key_passphrase = None; + persistence::apply_secret_persistence(&mut params_to_save, keychain_stored); } + let persisted_in_keychain = params_to_save.save_in_keychain.unwrap_or(false); let new_conn = SavedConnection { id: id.clone(), name: name.clone(), @@ -1304,18 +1422,30 @@ pub async fn save_connection( environment: validate_environment(environment)?, }; conn_file.connections.push(new_conn.clone()); - persist_connection_uri_change( + if let Err(err) = persist_connection_uri_change( &cache, &id, false, connection_uri.as_deref().map(Some), || save_connections_and_invalidate(&app, &path, &conn_file), - )?; + ) { + keychain_writes.roll_back(&cache, &id); + return Err(err); + } + if !persisted_in_keychain { + // A later keychain write failed and the secrets went to the config + // file instead; drop the ones that did reach the keychain. + keychain_writes.discard(&cache, &id); + } log::info!("Connection saved successfully: {} (ID: {})", name, id); + let requested_keychain = params.save_in_keychain.unwrap_or(false); let mut returned_conn = new_conn; returned_conn.params = params; // Return with password for frontend state + if requested_keychain && !persisted_in_keychain { + returned_conn.params.save_in_keychain = Some(false); + } Ok(returned_conn) } @@ -1427,41 +1557,85 @@ pub async fn update_connection( params_for_persistence(¶ms, connection_uri.is_some() || preserve_stored_uri); let cache = app.state::>(); + let mut keychain_writes = KeychainSecretWrites::default(); if params.save_in_keychain.unwrap_or(false) { + let mut keychain_stored = true; match keychain_utils::stored_password_change(params.password.as_deref()) { keychain_utils::StoredPasswordChange::Store(pwd) => { - keychain_utils::set_db_password(&id, pwd)?; - credential_cache::set_db_password_cached(&cache, &id, pwd); + let previous = keychain_utils::get_db_password(&id, "").ok(); + match keychain_utils::set_db_password(&id, pwd) { + Ok(()) => { + keychain_writes.db_password = Some(previous); + credential_cache::set_db_password_cached(&cache, &id, pwd); + } + Err(err) => { + log::warn!( + "Keychain unavailable while updating connection {}; storing secrets in the config file: {}", + id, err + ); + keychain_stored = false; + } + } } // The frontend sends an explicit empty password when a plugin hid // the login inputs. The previous secret must not survive, or the // next connect would hand a stale login to the driver. keychain_utils::StoredPasswordChange::Delete => { + let previous = keychain_utils::get_db_password(&id, "").ok(); keychain_utils::delete_db_password(&id)?; + keychain_writes.db_password = Some(previous); credential_cache::invalidate_db_password(&cache, &id); } keychain_utils::StoredPasswordChange::Keep => {} } - if params.ssh_enabled.unwrap_or(false) { + if keychain_stored && params.ssh_enabled.unwrap_or(false) { if let Some(ssh_pwd) = ¶ms.ssh_password { - keychain_utils::set_ssh_password(&id, ssh_pwd)?; - credential_cache::set_ssh_password_cached(&cache, &id, ssh_pwd); + let previous = keychain_utils::get_ssh_password(&id, "").ok(); + match keychain_utils::set_ssh_password(&id, ssh_pwd) { + Ok(()) => { + keychain_writes.ssh_password = Some(previous); + credential_cache::set_ssh_password_cached(&cache, &id, ssh_pwd); + } + Err(err) => { + log::warn!( + "Keychain unavailable while updating the SSH password for {}; storing secrets in the config file: {}", + id, err + ); + keychain_stored = false; + } + } } - if let Some(ssh_passphrase) = ¶ms.ssh_key_passphrase { - if !ssh_passphrase.trim().is_empty() { - keychain_utils::set_ssh_key_passphrase(&id, ssh_passphrase)?; - credential_cache::set_ssh_key_passphrase_cached(&cache, &id, ssh_passphrase); + if keychain_stored { + if let Some(ssh_passphrase) = ¶ms.ssh_key_passphrase { + if !ssh_passphrase.trim().is_empty() { + let previous = keychain_utils::get_ssh_key_passphrase(&id, "").ok(); + match keychain_utils::set_ssh_key_passphrase(&id, ssh_passphrase) { + Ok(()) => { + keychain_writes.ssh_key_passphrase = Some(previous); + credential_cache::set_ssh_key_passphrase_cached( + &cache, + &id, + ssh_passphrase, + ); + } + Err(err) => { + log::warn!( + "Keychain unavailable while updating the SSH key passphrase for {}; storing secrets in the config file: {}", + id, err + ); + keychain_stored = false; + } + } + } } } - } else { + } else if keychain_stored { keychain_utils::delete_ssh_password(&id).ok(); keychain_utils::delete_ssh_key_passphrase(&id).ok(); credential_cache::invalidate_ssh_password(&cache, &id); credential_cache::invalidate_ssh_key_passphrase(&cache, &id); } - params_to_save.password = None; - params_to_save.ssh_password = None; - params_to_save.ssh_key_passphrase = None; + persistence::apply_secret_persistence(&mut params_to_save, keychain_stored); } else { keychain_utils::delete_db_password(&id).ok(); keychain_utils::delete_ssh_password(&id).ok(); @@ -1481,6 +1655,7 @@ pub async fn update_connection( let original_appearance = conn_file.connections[conn_idx].appearance.clone(); // Tags are managed by set_connection_tags; preserve them across edits. let original_tag_ids = conn_file.connections[conn_idx].tag_ids.clone(); + let persisted_in_keychain = params_to_save.save_in_keychain.unwrap_or(false); let updated = SavedConnection { id: id.clone(), @@ -1496,9 +1671,19 @@ pub async fn update_connection( conn_file.connections[conn_idx] = updated.clone(); - persist_connection_uri_change(&cache, &id, existing_uri_in_keychain, uri_change, || { - save_connections_and_invalidate(&app, &path, &conn_file) - })?; + if let Err(err) = + persist_connection_uri_change(&cache, &id, existing_uri_in_keychain, uri_change, || { + save_connections_and_invalidate(&app, &path, &conn_file) + }) + { + keychain_writes.roll_back(&cache, &id); + return Err(err); + } + if !persisted_in_keychain { + // Same as save: the fallback put the secrets in the config file, so + // the keychain copies from this update are orphans. + keychain_writes.discard(&cache, &id); + } let mut metadata_changed = false; for driver_id in [&previous_driver_id, &updated.params.driver] { @@ -1548,8 +1733,12 @@ pub async fn update_connection( } } + let requested_keychain = params.save_in_keychain.unwrap_or(false); let mut returned_conn = updated; returned_conn.params = params; + if requested_keychain && !persisted_in_keychain { + returned_conn.params.save_in_keychain = Some(false); + } Ok(returned_conn) } @@ -1838,7 +2027,7 @@ async fn migrate_ssh_connections(app: &AppHandle) -> Result<(), S // Save migrated SSH connections let ssh_json = serde_json::to_string_pretty(&ssh_connections).map_err(|e| e.to_string())?; - fs::write(ssh_path, ssh_json).map_err(|e| e.to_string())?; + persistence::write_private_file(&ssh_path, ssh_json)?; // Save migrated connections using new format (preserving groups) conn_file.connections = migrated_connections; @@ -2014,7 +2203,7 @@ pub async fn save_ssh_connection( ssh_connections.push(ssh_to_save.clone()); let json = serde_json::to_string_pretty(&ssh_connections).map_err(|e| e.to_string())?; - fs::write(path, json).map_err(|e| e.to_string())?; + persistence::write_private_file(&path, json)?; let mut returned_ssh = ssh_to_save; returned_ssh.password = ssh.password; @@ -2083,7 +2272,7 @@ pub async fn update_ssh_connection( ssh_connections[ssh_idx] = ssh_to_save.clone(); let json = serde_json::to_string_pretty(&ssh_connections).map_err(|e| e.to_string())?; - fs::write(path, json).map_err(|e| e.to_string())?; + persistence::write_private_file(&path, json)?; let mut returned_ssh = ssh_to_save; returned_ssh.password = ssh.password; @@ -2115,7 +2304,7 @@ pub async fn delete_ssh_connection( credential_cache::invalidate_ssh_key_passphrase(&cache, &id); let json = serde_json::to_string_pretty(&ssh_connections).map_err(|e| e.to_string())?; - fs::write(path, json).map_err(|e| e.to_string())?; + persistence::write_private_file(&path, json)?; Ok(()) } @@ -2291,7 +2480,7 @@ pub async fn save_k8s_connection( connections.push(connection.clone()); let json = serde_json::to_string_pretty(&connections).map_err(|e| e.to_string())?; - fs::write(&path, json).map_err(|e| e.to_string())?; + persistence::write_private_file(&path, json)?; Ok(connection) } @@ -2331,7 +2520,7 @@ pub async fn update_k8s_connection( connections[idx] = connection.clone(); let json = serde_json::to_string_pretty(&connections).map_err(|e| e.to_string())?; - fs::write(&path, json).map_err(|e| e.to_string())?; + persistence::write_private_file(&path, json)?; Ok(connection) } @@ -2352,7 +2541,7 @@ pub async fn delete_k8s_connection( connections.retain(|c| c.id != id); let json = serde_json::to_string_pretty(&connections).map_err(|e| e.to_string())?; - fs::write(&path, json).map_err(|e| e.to_string())?; + persistence::write_private_file(&path, json)?; Ok(()) } @@ -6805,7 +6994,7 @@ pub async fn apply_export_payload( // Save files save_connections_and_invalidate(&app, &conn_path, ¤t_file)?; let ssh_json = serde_json::to_string_pretty(¤t_ssh).map_err(|e| e.to_string())?; - fs::write(ssh_path, ssh_json).map_err(|e| e.to_string())?; + persistence::write_private_file(&ssh_path, ssh_json)?; // Merge K8s connections let k8s_path = get_k8s_config_path(&app)?; @@ -6818,7 +7007,7 @@ pub async fn apply_export_payload( } } let k8s_json = serde_json::to_string_pretty(¤t_k8s).map_err(|e| e.to_string())?; - fs::write(k8s_path, k8s_json).map_err(|e| e.to_string())?; + persistence::write_private_file(&k8s_path, k8s_json)?; Ok(()) } diff --git a/src-tauri/src/persistence.rs b/src-tauri/src/persistence.rs index aa2488dbf..17ce233d7 100644 --- a/src-tauri/src/persistence.rs +++ b/src-tauri/src/persistence.rs @@ -1,4 +1,4 @@ -use crate::models::{ConnectionGroup, ConnectionsFile, SavedConnection}; +use crate::models::{ConnectionGroup, ConnectionParams, ConnectionsFile, SavedConnection}; use serde_json::{Map, Value}; use std::collections::HashMap; use std::fs; @@ -72,7 +72,54 @@ pub fn save_connections_file(path: &Path, file: &ConnectionsFile) -> Result<(), let mut json = serde_json::to_value(&to_save).map_err(|e| e.to_string())?; preserve_unknown_fields(path, &mut json); let json = serde_json::to_string_pretty(&json).map_err(|e| e.to_string())?; - fs::write(path, json).map_err(|e| e.to_string()) + write_private_file(path, json) +} + +/// Writes `contents` so that, on Unix, the file is readable and writable only +/// by the current user (`0o600`). An existing file is tightened to that mode +/// as well. Other platforms fall back to a normal write. +pub fn write_private_file(path: &Path, contents: impl AsRef<[u8]>) -> Result<(), String> { + #[cfg(unix)] + { + use std::io::Write; + use std::os::unix::fs::{OpenOptionsExt, PermissionsExt}; + + // `mode` only applies when the file is created, so tighten an existing + // file before the new contents land in it. + if path.exists() { + fs::set_permissions(path, fs::Permissions::from_mode(0o600)) + .map_err(|e| e.to_string())?; + } + let mut file = fs::OpenOptions::new() + .write(true) + .create(true) + .truncate(true) + .mode(0o600) + .open(path) + .map_err(|e| e.to_string())?; + file.write_all(contents.as_ref()).map_err(|e| e.to_string()) + } + #[cfg(not(unix))] + { + fs::write(path, contents).map_err(|e| e.to_string()) + } +} + +/// Keeps connection secrets out of the config file after a successful keychain +/// write. When the keychain is unavailable, the secrets stay on `params` and +/// `save_in_keychain` is cleared so the file is the source of truth and the +/// save can still succeed. +pub fn apply_secret_persistence(params: &mut ConnectionParams, keychain_stored: bool) { + if !params.save_in_keychain.unwrap_or(false) { + return; + } + if keychain_stored { + params.password = None; + params.ssh_password = None; + params.ssh_key_passphrase = None; + } else { + params.save_in_keychain = Some(false); + } } /// Fields of `raw` the current binary's structs do not know: anything present diff --git a/src-tauri/src/persistence_tests.rs b/src-tauri/src/persistence_tests.rs index fb3225eba..6b401507e 100644 --- a/src-tauri/src/persistence_tests.rs +++ b/src-tauri/src/persistence_tests.rs @@ -1,6 +1,9 @@ #[cfg(test)] mod tests { - use crate::persistence::{load_connections_file, save_connections_file}; + use crate::models::{ConnectionParams, ConnectionsFile, SavedConnection}; + use crate::persistence::{ + apply_secret_persistence, load_connections_file, save_connections_file, write_private_file, + }; #[test] fn save_preserves_unknown_fields_without_resurrecting_known_changes() { @@ -31,4 +34,117 @@ mod tests { assert!(connection["params"].get("connectionUri").is_none()); assert!(serde_json::from_value::(saved).is_ok()); } + + fn connection_with_secret(save_in_keychain: bool) -> SavedConnection { + SavedConnection { + id: "conn-1".to_string(), + name: "prod".to_string(), + params: ConnectionParams { + driver: "mysql".to_string(), + password: Some("db-secret".to_string()), + ssh_password: Some("ssh-secret".to_string()), + save_in_keychain: Some(save_in_keychain), + ..ConnectionParams::default() + }, + group_id: None, + sort_order: None, + detect_json_in_text_columns: None, + appearance: None, + tag_ids: None, + environment: None, + } + } + + #[test] + fn save_connections_file_with_keychain_default_writes_no_password() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("connections.json"); + let file = ConnectionsFile { + connections: vec![connection_with_secret(true)], + ..ConnectionsFile::default() + }; + + save_connections_file(&path, &file).unwrap(); + + let saved = std::fs::read_to_string(&path).unwrap(); + assert!(!saved.contains("db-secret")); + assert!(!saved.contains("ssh-secret")); + let parsed: serde_json::Value = serde_json::from_str(&saved).unwrap(); + assert_eq!(parsed["connections"][0]["params"]["save_in_keychain"], true); + assert!(parsed["connections"][0]["params"]["password"].is_null()); + } + + #[test] + fn save_connections_file_keeps_password_when_keychain_is_off() { + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("connections.json"); + let file = ConnectionsFile { + connections: vec![connection_with_secret(false)], + ..ConnectionsFile::default() + }; + + save_connections_file(&path, &file).unwrap(); + + let saved = std::fs::read_to_string(&path).unwrap(); + assert!(saved.contains("db-secret")); + assert!(saved.contains("ssh-secret")); + } + + #[test] + fn unavailable_keychain_keeps_the_secret_and_clears_the_flag() { + let mut params = connection_with_secret(true).params; + apply_secret_persistence(&mut params, false); + assert_eq!(params.password.as_deref(), Some("db-secret")); + assert_eq!(params.ssh_password.as_deref(), Some("ssh-secret")); + assert_eq!(params.save_in_keychain, Some(false)); + } + + #[test] + fn stored_keychain_strips_secrets() { + let mut params = connection_with_secret(true).params; + apply_secret_persistence(&mut params, true); + assert!(params.password.is_none()); + assert!(params.ssh_password.is_none()); + assert_eq!(params.save_in_keychain, Some(true)); + } + + #[cfg(unix)] + #[test] + fn write_private_file_creates_mode_0600_with_the_given_content() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("connections.json"); + write_private_file(&path, b"{\"secret\":\"db-secret\"}").unwrap(); + + let mode = std::fs::metadata(&path).unwrap().permissions().mode() & 0o777; + assert_eq!(mode, 0o600); + assert_eq!( + std::fs::read_to_string(&path).unwrap(), + "{\"secret\":\"db-secret\"}" + ); + } + + #[cfg(unix)] + #[test] + fn write_private_file_tightens_an_existing_0644_file() { + use std::os::unix::fs::PermissionsExt; + + let dir = tempfile::tempdir().unwrap(); + let path = dir.path().join("ssh_connections.json"); + std::fs::write(&path, b"old").unwrap(); + std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o644)).unwrap(); + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o644 + ); + + write_private_file(&path, b"new-secret").unwrap(); + + assert_eq!( + std::fs::metadata(&path).unwrap().permissions().mode() & 0o777, + 0o600 + ); + assert_eq!(std::fs::read_to_string(&path).unwrap(), "new-secret"); + } } diff --git a/src/components/modals/NewConnectionModal.tsx b/src/components/modals/NewConnectionModal.tsx index 638a34dbd..92632ca4b 100644 --- a/src/components/modals/NewConnectionModal.tsx +++ b/src/components/modals/NewConnectionModal.tsx @@ -40,6 +40,7 @@ import { SshConnectionsModal } from "./SshConnectionsModal"; import { K8sConnectionsModal } from "./K8sConnectionsModal"; import { Select } from "../ui/Select"; import { SlotAnchor } from "../ui/SlotAnchor"; +import { useAlert } from "../../hooks/useAlert"; import { useDrivers } from "../../hooks/useDrivers"; import { useSettings } from "../../hooks/useSettings"; import { usePluginSlotRegistry } from "../../hooks/usePluginSlotRegistry"; @@ -266,6 +267,7 @@ export const NewConnectionModal = ({ initialConnection, }: NewConnectionModalProps) => { const { t } = useTranslation(); + const { showAlert } = useAlert(); const { drivers, refresh: refreshDrivers } = useDrivers(); const { settings, updateSetting } = useSettings(); const { invalidate: invalidateK8sAsync, run: runK8sAsync } = useLatestAsync(); @@ -366,6 +368,11 @@ export const NewConnectionModal = ({ ssh_enabled: false, ssh_port: 22, k8s_enabled: false, + // New connections store secrets in the keychain. An edit starts from the + // saved flag so a slow credential load cannot flip an existing connection. + save_in_keychain: initialConnection + ? (initialConnection.params.save_in_keychain ?? false) + : true, }); const [selectedDatabasesState, setSelectedDatabasesState] = useState< string[] @@ -1934,6 +1941,7 @@ export const NewConnectionModal = ({ ssh_port: 22, k8s_enabled: false, ssm_enabled: false, + save_in_keychain: true, }); setSelectedDatabasesState([]); setLoadAllDatabases(true); @@ -1977,7 +1985,7 @@ export const NewConnectionModal = ({ ssh_password: undefined, ssh_key_file: undefined, ssh_key_passphrase: undefined, - save_in_keychain: false, + save_in_keychain: true, k8s_enabled: false, k8s_connection_id: undefined, k8s_context: undefined, @@ -2361,6 +2369,7 @@ export const NewConnectionModal = ({ setTestResult(null); try { let savedConnectionId: string; + let persistedInKeychain = params.save_in_keychain; if (initialConnection) { // An omitted password means "keep the stored one". While a plugin // hides the login inputs the empty password is deliberate: it is @@ -2369,25 +2378,32 @@ export const NewConnectionModal = ({ delete params.password; } if (!params.ssh_password?.trim()) delete params.ssh_password; - await invoke("update_connection", { + const updated = await invoke<{ + params?: { save_in_keychain?: boolean }; + }>("update_connection", { id: initialConnection.id, name, params, detectJsonInTextColumns: detectJsonInTextColumns ? true : null, environment: environment || null, }); + persistedInKeychain = updated?.params?.save_in_keychain; savedConnectionId = initialConnection.id; await invoke("set_connection_appearance", { id: initialConnection.id, appearance: appearancePayload ?? null, }); } else { - const saved = await invoke<{ id: string }>("save_connection", { + const saved = await invoke<{ + id: string; + params?: { save_in_keychain?: boolean }; + }>("save_connection", { name, params, detectJsonInTextColumns: detectJsonInTextColumns ? true : null, environment: environment || null, }); + persistedInKeychain = saved.params?.save_in_keychain; savedConnectionId = saved.id; if (appearancePayload) { await invoke("set_connection_appearance", { @@ -2415,6 +2431,12 @@ export const NewConnectionModal = ({ return; } + if (params.save_in_keychain && persistedInKeychain === false) { + showAlert(t("newConnection.secretsStoredInPlainText"), { + kind: "warning", + }); + } + if (onSave) onSave(); wasSavedRef.current = true; @@ -2905,19 +2927,34 @@ export const NewConnectionModal = ({ )} {/* Keychain */} - +
+ + {!formData.save_in_keychain && ( +

+ {t("newConnection.secretsStoredInPlainText")} +

+ )} +
)} diff --git a/src/components/ssh/SshConnectionsManager.tsx b/src/components/ssh/SshConnectionsManager.tsx index f4a72ce0a..2b9d17052 100644 --- a/src/components/ssh/SshConnectionsManager.tsx +++ b/src/components/ssh/SshConnectionsManager.tsx @@ -541,6 +541,11 @@ export function SshConnectionsManager({ type="checkbox" id="ssh-keychain-toggle" checked={!!formData.save_in_keychain} + aria-describedby={ + formData.save_in_keychain + ? undefined + : "ssh-secrets-plaintext-warning" + } onChange={(e) => { setPasswordDirty(true); setPassphraseDirty(true); @@ -562,6 +567,14 @@ export function SshConnectionsManager({ {t("newConnection.saveKeychain")} + {!formData.save_in_keychain && ( +

+ {t("newConnection.secretsStoredInPlainText")} +

+ )}
({ unlisten: vi.fn(), })); +const alertMocks = vi.hoisted(() => ({ + showAlert: vi.fn(), +})); + vi.mock("@tauri-apps/api/event", () => ({ listen: eventMocks.listen, })); +vi.mock("../../../src/hooks/useAlert", () => ({ + useAlert: () => ({ showAlert: alertMocks.showAlert }), +})); + vi.mock("../../../src/components/ui/Modal", () => ({ Modal: ({ isOpen, @@ -1700,3 +1708,148 @@ describe("NewConnectionModal extra_fields slot credential toggle", () => { expect(screen.getByTestId("plugin-extra")).toHaveTextContent("{}"); }); }); + +describe("NewConnectionModal keychain default", () => { + beforeEach(() => { + vi.clearAllMocks(); + driverState.defaultPort = 3306; + vi.mocked(invoke).mockImplementation((command) => { + if (command === "save_connection") { + return Promise.resolve({ + id: "new-connection", + params: { save_in_keychain: true }, + }); + } + return Promise.resolve(undefined); + }); + sshMocks.loadSshConnections.mockResolvedValue([]); + k8sMocks.loadK8sConnections.mockResolvedValue([]); + eventMocks.listen.mockResolvedValue(eventMocks.unlisten); + }); + + function keychainCheckbox() { + return screen.getByRole("checkbox", { name: "newConnection.saveKeychain" }); + } + + it("defaults a new connection to the keychain and keeps it on when the driver changes", () => { + renderModal(); + pickEngineFromCatalogue(); + + expect(keychainCheckbox()).toBeChecked(); + expect( + screen.queryByText("newConnection.secretsStoredInPlainText"), + ).not.toBeInTheDocument(); + + fireEvent.click(screen.getByRole("button", { name: "newConnection.changeDatabase" })); + pickEngineFromCatalogue(); + + expect(keychainCheckbox()).toBeChecked(); + expect( + screen.queryByText("newConnection.secretsStoredInPlainText"), + ).not.toBeInTheDocument(); + }); + + it("warns that secrets are stored in plain text when the keychain is turned off", () => { + renderModal(); + pickEngineFromCatalogue(); + + fireEvent.click(keychainCheckbox()); + + expect(keychainCheckbox()).not.toBeChecked(); + expect( + screen.getByText("newConnection.secretsStoredInPlainText"), + ).toBeInTheDocument(); + }); + + it("saves a new connection with the keychain enabled", async () => { + renderModal(); + pickEngineFromCatalogue(); + + fireEvent.change(screen.getByPlaceholderText("newConnection.namePlaceholder"), { + target: { value: "Local" }, + }); + fireEvent.change(screen.getByPlaceholderText("newConnection.dbNamePlaceholder"), { + target: { value: "shop" }, + }); + fireEvent.click(screen.getByText("newConnection.save")); + + await waitFor(() => { + expect(invoke).toHaveBeenCalledWith( + "save_connection", + expect.objectContaining({ + params: expect.objectContaining({ save_in_keychain: true }), + }), + ); + }); + expect(alertMocks.showAlert).not.toHaveBeenCalled(); + }); + + it("tells the user when a save fell back to plain text", async () => { + vi.mocked(invoke).mockImplementation((command) => { + if (command === "save_connection") { + return Promise.resolve({ + id: "new-connection", + params: { save_in_keychain: false }, + }); + } + return Promise.resolve(undefined); + }); + renderModal(); + pickEngineFromCatalogue(); + + fireEvent.change(screen.getByPlaceholderText("newConnection.namePlaceholder"), { + target: { value: "Local" }, + }); + fireEvent.change(screen.getByPlaceholderText("newConnection.dbNamePlaceholder"), { + target: { value: "shop" }, + }); + fireEvent.click(screen.getByText("newConnection.save")); + + await waitFor(() => { + expect(alertMocks.showAlert).toHaveBeenCalledWith( + "newConnection.secretsStoredInPlainText", + { kind: "warning" }, + ); + }); + }); + + it("keeps an existing connection's stored keychain setting", async () => { + renderModal( + createInitialConnection({ + save_in_keychain: false, + database: "shop", + }), + ); + + await waitFor(() => { + expect(invoke).toHaveBeenCalledWith( + "get_connection_by_id", + expect.anything(), + ); + }); + expect(keychainCheckbox()).not.toBeChecked(); + expect( + screen.getByText("newConnection.secretsStoredInPlainText"), + ).toBeInTheDocument(); + }); + + it("keeps an existing keychain-backed connection opted in", async () => { + renderModal( + createInitialConnection({ + save_in_keychain: true, + database: "shop", + }), + ); + + await waitFor(() => { + expect(invoke).toHaveBeenCalledWith( + "get_connection_by_id", + expect.anything(), + ); + }); + expect(keychainCheckbox()).toBeChecked(); + expect( + screen.queryByText("newConnection.secretsStoredInPlainText"), + ).not.toBeInTheDocument(); + }); +}); diff --git a/tests/components/ssh/SshConnectionsManager.test.tsx b/tests/components/ssh/SshConnectionsManager.test.tsx index befa494fc..0beb0c38a 100644 --- a/tests/components/ssh/SshConnectionsManager.test.tsx +++ b/tests/components/ssh/SshConnectionsManager.test.tsx @@ -120,6 +120,27 @@ describe("SshConnectionsManager", () => { expect(sshMocks.updateSshConnection).not.toHaveBeenCalled(); }); + it("warns about plain-text secrets when the keychain box is unticked", async () => { + render(); + await waitFor(() => screen.getByText("Prod bastion")); + fireEvent.click(screen.getByText("sshConnections.createNew")); + + const toggle = screen.getByLabelText("newConnection.saveKeychain"); + expect(toggle).toBeChecked(); + expect(toggle).not.toHaveAttribute("aria-describedby"); + expect( + screen.queryByText("newConnection.secretsStoredInPlainText"), + ).not.toBeInTheDocument(); + + fireEvent.click(toggle); + + expect(toggle).not.toBeChecked(); + const warning = screen.getByText("newConnection.secretsStoredInPlainText"); + expect(warning).toHaveAttribute("id", "ssh-secrets-plaintext-warning"); + expect(toggle).toHaveAttribute("aria-describedby", "ssh-secrets-plaintext-warning"); + expect(toggle).toHaveAccessibleDescription("newConnection.secretsStoredInPlainText"); + }); + it("asks for confirmation before deleting and deletes on confirm", async () => { render(); await screen.findByText("Prod bastion");