From f4f9b2bac40f14bd0fb898ca3e9cd83fb5a2acb2 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:47:07 +0800 Subject: [PATCH] fix(settings): confirm before forgetting a password other profiles share MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit "Forget Password" was a bare menu item: one click deleted the keychain entry, with no confirmation and nothing said about who else it took down. The entry is keyed by `user@host:port`, so two profiles that reach the same endpoint — one direct, one through a jump host — share exactly one secret, and forgetting from either row signed both of them out. The notification even worded itself by endpoint while the action hung off a single profile's menu. It now asks first, the way deleting a profile does, and when the endpoint is shared the dialog names the blast radius instead of leaving it to turn up at the next connect on a host nobody touched. Deleting a profile stays conservative on purpose — the menu that could remove the secret is about to disappear — so the two paths keep their different policies. What they no longer keep is two copies of the "is this endpoint shared" question: `profiles_sharing_endpoint` is now the one place that answers it, and it has the test. --- src/ui/i18n/en.rs | 13 +++++ src/ui/i18n/ja.rs | 13 +++++ src/ui/i18n/mod.rs | 4 ++ src/ui/i18n/zh.rs | 13 +++++ src/ui/settings.rs | 125 ++++++++++++++++++++++++++++++++++++++++----- 5 files changed, 156 insertions(+), 12 deletions(-) diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 6914debd..d9ceef7a 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -200,6 +200,13 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::SettingsCopyAddress => "Copy Address", L10nKey::SettingsDuplicate => "Duplicate", L10nKey::SettingsForgetPassword => "Forget Password", + L10nKey::SettingsForgetPasswordTitle => "Forget the saved password for {endpoint}?", + L10nKey::SettingsForgetPasswordBody => { + "The next connection to it asks for the password again. Nothing else about this host changes." + } + L10nKey::SettingsForgetPasswordSharedBody => { + "{count} other host profiles use {endpoint} as well, so those connections will have to enter the password again too." + } L10nKey::SettingsForgotPasswordFor => "Forgot saved password for {endpoint}", L10nKey::SettingsDeleteProfileBody => { "The password saved for it goes too, unless another connection still uses the same address." @@ -1599,6 +1606,12 @@ pub fn translate_variant_en(key: L10nKey, branch: &'static str) -> Option<&'stat (L10nKey::SettingsOfflineMachines, "other") => { "{count} more saved machines are not connected — open a workspace on one to install its hooks there." } + (L10nKey::SettingsForgetPasswordSharedBody, "one") => { + "1 other host profile uses {endpoint} as well, so that connection will have to enter the password again too." + } + (L10nKey::SettingsForgetPasswordSharedBody, "other") => { + "{count} other host profiles use {endpoint} as well, so those connections will have to enter the password again too." + } (L10nKey::SftpReplaceBody, "one") => { "{names} already exists in this folder. Uploading overwrites it." } diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index a8b2cccb..3077bb63 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -201,6 +201,13 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsCopyAddress => "アドレスをコピー", L10nKey::SettingsDuplicate => "複製", L10nKey::SettingsForgetPassword => "パスワードを消去", + L10nKey::SettingsForgetPasswordTitle => "{endpoint} の保存されたパスワードを消去しますか？", + L10nKey::SettingsForgetPasswordBody => { + "次に接続するときに、もう一度パスワードを尋ねられます。このホストの他の設定は変わりません" + } + L10nKey::SettingsForgetPasswordSharedBody => { + "他にも {count} 件のホストプロファイルが {endpoint} を使っているため、それらの接続でもパスワードの再入力が必要になります" + } L10nKey::SettingsForgotPasswordFor => "{endpoint} の保存されたパスワードを消去しました", L10nKey::SettingsDeleteProfileBody => { "保存されたパスワードも一緒に削除されます。同じアドレスを使う接続が他にある場合は残ります。" @@ -1644,6 +1651,12 @@ pub fn translate_variant_ja(key: L10nKey, branch: &'static str) -> Option<&'stat (L10nKey::SettingsOfflineMachines, "other") => { "未接続の保存済みマシンがさらに {count} 台あります — いずれかでワークスペースを開くと、そこにフックをインストールできます" } + (L10nKey::SettingsForgetPasswordSharedBody, "one") => { + "他にも 1 件のホストプロファイルが {endpoint} を使っているため、その接続でもパスワードの再入力が必要になります" + } + (L10nKey::SettingsForgetPasswordSharedBody, "other") => { + "他にも {count} 件のホストプロファイルが {endpoint} を使っているため、それらの接続でもパスワードの再入力が必要になります" + } (L10nKey::SftpReplaceBody, "one") => { "{names} はこのフォルダに既に存在します。アップロードすると上書きされます。" } diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 489d29f7..d1cdd455 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -227,6 +227,9 @@ l10n_keys! { SettingsCopyAddress, SettingsDuplicate, SettingsForgetPassword, + SettingsForgetPasswordTitle, + SettingsForgetPasswordBody, + SettingsForgetPasswordSharedBody, SettingsForgotPasswordFor, SettingsDeleteProfileBody, SettingsCouldntForgetPassword, @@ -1994,6 +1997,7 @@ mod tests { L10nKey::SettingsAliasesLinked, L10nKey::SettingsRulesOpenedWithConnection, L10nKey::SettingsOfflineMachines, + L10nKey::SettingsForgetPasswordSharedBody, L10nKey::PanelMoreChangedFiles, L10nKey::ScmFilesChanged, L10nKey::ScmStagedFileCount, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index ec546971..bf766e2b 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -181,6 +181,13 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsCopyAddress => "复制地址", L10nKey::SettingsDuplicate => "复制", L10nKey::SettingsForgetPassword => "清除已保存的密码", + L10nKey::SettingsForgetPasswordTitle => "要清除 {endpoint} 的已保存密码吗？", + L10nKey::SettingsForgetPasswordBody => { + "下次连接它时会重新询问密码。这台主机的其他设置不受影响。" + } + L10nKey::SettingsForgetPasswordSharedBody => { + "还有 {count} 个主机配置同样使用 {endpoint}，那些连接也需要重新输入密码。" + } L10nKey::SettingsForgotPasswordFor => "已清除 {endpoint} 的已保存密码", L10nKey::SettingsDeleteProfileBody => { "为它保存的密码也会一并删除，除非还有别的连接用着同一个地址。" @@ -1517,6 +1524,12 @@ pub fn translate_variant_zh(key: L10nKey, branch: &'static str) -> Option<&'stat (L10nKey::SettingsOfflineMachines, "other") => { "还有 {count} 台已保存的机器未连接——在其中一台上打开工作区，即可在那台机器上安装 hook。" } + (L10nKey::SettingsForgetPasswordSharedBody, "one") => { + "还有 1 个主机配置同样使用 {endpoint}，那个连接也需要重新输入密码。" + } + (L10nKey::SettingsForgetPasswordSharedBody, "other") => { + "还有 {count} 个主机配置同样使用 {endpoint}，那些连接也需要重新输入密码。" + } (L10nKey::SftpReplaceBody, "one") => "{names} 在这个文件夹里已经存在，上传会覆盖它。", (L10nKey::SftpReplaceBody, "other") => "{names} 在这个文件夹里已经存在，上传会覆盖它们。", (L10nKey::AppTabsNotRestored, "one") => "上次的 1 个标签页没能重新打开", diff --git a/src/ui/settings.rs b/src/ui/settings.rs index 704e9f3e..e9baced3 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -797,6 +797,27 @@ fn ssh_row_matches(p: &SshProfile, query: &str) -> bool { hit(&p.name) || hit(&p.host) || hit(&p.user) || hit(&p.port.to_string()) } +/// How many *other* profiles reach the same `user@host:port` as this one. +/// +/// The keychain is keyed by the endpoint, not by the profile, so two hosts that +/// differ only in how they get there — one direct, one through a jump host — +/// hand the same saved password back and forth. Every path that is about to +/// remove that password has to know this first: deleting a profile keeps the +/// secret while someone else still needs it, and forgetting one says out loud +/// who else it takes down. Both used to work the answer out on their own, which +/// is exactly how the two policies would have drifted apart. +fn profiles_sharing_endpoint(cfg: &Config, id: Uuid) -> usize { + let Some(profile) = cfg.ssh_profiles.iter().find(|p| p.id == id) else { + return 0; + }; + cfg.ssh_profiles + .iter() + .filter(|p| { + p.id != id && (&p.user, &p.host, p.port) == (&profile.user, &profile.host, profile.port) + }) + .count() +} + pub(crate) struct SshProfileForm { editing: Uuid, carry_group: Option, @@ -2834,13 +2855,8 @@ impl Tty7App { PopupMenuItem::new(t(L10nKey::SettingsForgetPassword)).on_click({ let app = app.clone(); move |_, window, cx| { - if let Some(msg) = app - .update(cx, |this, cx| this.forget_profile_password(id, cx)) - .ok() - .flatten() - { - window.push_notification(msg, cx); - } + let _ = + app.update(cx, |this, cx| this.forget_profile_password(id, window, cx)); } }), ) @@ -3224,11 +3240,7 @@ impl Tty7App { .iter() .find(|p| p.id == id) .map(|p| (p.user.clone(), p.host.clone(), p.port)); - let shared = endpoint.as_ref().is_some_and(|(user, host, port)| { - cfg.ssh_profiles - .iter() - .any(|p| p.id != id && (&p.user, &p.host, p.port) == (user, host, *port)) - }); + let shared = profiles_sharing_endpoint(cfg, id) > 0; if let Some((user, host, port)) = endpoint.filter(|_| !shared) { use crate::core::keychain::{CredentialStore, OsCredentialStore}; let _ = OsCredentialStore.delete_password(&user, &host, port); @@ -3270,6 +3282,60 @@ impl Tty7App { } pub(crate) fn forget_profile_password( + &mut self, + id: Uuid, + window: &mut Window, + cx: &mut Context, + ) { + let cfg = cx.global::(); + let Some(endpoint) = cfg + .ssh_profiles + .iter() + .find(|p| p.id == id) + .map(|p| format!("{}@{}:{}", p.user, p.host, p.port)) + else { + return; + }; + // One click used to be the whole gesture, and the thing it removed does + // not come back. Worse, the entry is the endpoint's rather than this + // row's, so a menu opened on one host can sign several of them out — + // name that count here instead of letting it turn up at the next + // connect on a host nobody touched. + let others = profiles_sharing_endpoint(cfg, id); + let body = if others == 0 { + t(L10nKey::SettingsForgetPasswordBody).to_string() + } else { + t_plural( + L10nKey::SettingsForgetPasswordSharedBody, + others, + &[("endpoint", &endpoint)], + ) + }; + let answer = window.prompt( + gpui::PromptLevel::Warning, + &t_fmt( + L10nKey::SettingsForgetPasswordTitle, + &[("endpoint", &endpoint)], + ), + Some(&body), + &crate::ui::confirm_answers(t(L10nKey::SettingsForgetPassword), t(L10nKey::Cancel)), + cx, + ); + cx.spawn_in(window, async move |this, cx| { + let Ok(0) = answer.await else { return }; + // The notification is the only sign the keychain was touched, and + // by now the click that asked for it is long gone — so it has to be + // raised from in here, on the window the prompt belonged to. + let _ = this.update_in(cx, |this, window, cx| { + if let Some(msg) = this.forget_profile_password_confirmed(id, cx) { + window.push_notification(msg, cx); + } + }); + }) + .detach(); + } + + fn forget_profile_password_confirmed( &mut self, id: Uuid, cx: &mut Context, @@ -6578,6 +6644,41 @@ mod tests { assert_eq!(hp.port, 2222); assert_eq!(parse_host_port("host").unwrap().port, 0); } + + fn profile_at(name: &str, user: &str, host: &str, port: u16) -> SshProfile { + let mut p = SshProfile::new(name); + p.user = user.to_string(); + p.host = host.to_string(); + p.port = port; + p + } + + /// The saved password belongs to `user@host:port`, so what counts as + /// "shared" is exactly that triple — a different name or a jump host in + /// front of it changes nothing, and a different port makes it a different + /// secret entirely. + #[test] + fn the_same_endpoint_under_two_names_counts_as_shared() { + let direct = profile_at("direct", "ana", "build.example.com", 22); + let mut via_jump = profile_at("via bastion", "ana", "build.example.com", 22); + via_jump.jump_host = Some(direct.id); + let staging = profile_at("staging", "ana", "build.example.com", 2222); + let other_user = profile_at("root", "root", "build.example.com", 22); + + let mut cfg = Config::default(); + let (direct_id, jump_id, staging_id) = (direct.id, via_jump.id, staging.id); + cfg.ssh_profiles = vec![direct, via_jump, staging, other_user]; + + // The two that reach the same endpoint see each other, and neither + // counts itself. + assert_eq!(profiles_sharing_endpoint(&cfg, direct_id), 1); + assert_eq!(profiles_sharing_endpoint(&cfg, jump_id), 1); + // A port apart is a keychain entry apart, so this one is alone even + // though the user and host match two of the others. + assert_eq!(profiles_sharing_endpoint(&cfg, staging_id), 0); + // A profile that is no longer on the list shares with nobody. + assert_eq!(profiles_sharing_endpoint(&cfg, Uuid::new_v4()), 0); + } } #[cfg(test)]