diff --git a/CHANGELOG.md b/CHANGELOG.md index a6f4997b..8bc5da6f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,10 +96,13 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 was running, which on Windows is the common case (the OpenSSH Authentication Agent service is off by default), and then reported "no public key was accepted" when no key had ever been sent. `id_ed25519`, - `id_ecdsa` and `id_rsa` are now offered after the connection's own files - and before the agent, OpenSSH-style, deduplicated against the explicit list - by canonical path so one key spelled two ways is offered once — every offer - spends one of the server's `MaxAuthTries`. A discovered key that is + `id_ecdsa` and `id_rsa` now stand in for the identity list when the + connection names no key of its own, the way `IdentityFile`'s default works + in ssh_config — naming a key replaces them rather than adding to them. The + agent is asked before either, because every public key offered spends one + of the server's `MaxAuthTries` whether or not it is wanted, and the key the + user loaded into the agent is the one most likely to work. A discovered key + that is encrypted is used only when its passphrase is already in the OS keychain: russh has no offer-without-signing probe, so asking would spend a prompt on a key the server may not even want. A key named in the profile still asks, diff --git a/crates/tty7-core/src/daemon/protocol.rs b/crates/tty7-core/src/daemon/protocol.rs index 103577f3..547b6615 100644 --- a/crates/tty7-core/src/daemon/protocol.rs +++ b/crates/tty7-core/src/daemon/protocol.rs @@ -570,6 +570,17 @@ pub enum AuthPromptKind { port: u16, algorithm: String, fingerprint_sha256: String, + /// The algorithm this host is already on file under, when the key is + /// new only because its algorithm is. + /// + /// A field rather than a variant of its own on purpose. This enum is + /// externally tagged and travels both to the GUI and to the remote + /// `tty7-server`, either of which may be an older build; an unknown + /// variant fails the whole decode, while an unknown field is ignored + /// and a missing one defaults. So an old peer shows the plain + /// unknown-host confirmation, which is the right prompt either way. + #[serde(default)] + previously_known_as: Option, }, HostKeyChanged { host: String, @@ -1954,6 +1965,28 @@ mod tests { assert_eq!(info.title, ""); } + /// The reason `previously_known_as` is a field and not a variant: this + /// prompt is decoded by whatever GUI or `tty7-server` is at the other end, + /// and one of them is regularly older than the build that sent it. A + /// missing field defaults; an unknown variant would fail the whole frame. + #[test] + fn an_unknown_host_prompt_from_an_older_peer_still_decodes() { + let prompt: AuthPromptKind = serde_json::from_str( + r#"{"HostKeyUnknown":{"host":"h","port":22,"algorithm":"ssh-ed25519","fingerprint_sha256":"SHA256:x"}}"#, + ) + .unwrap(); + assert_eq!( + prompt, + AuthPromptKind::HostKeyUnknown { + host: "h".into(), + port: 22, + algorithm: "ssh-ed25519".into(), + fingerprint_sha256: "SHA256:x".into(), + previously_known_as: None, + } + ); + } + fn sample_native_spec() -> NativeSshSpec { let mut passphrases = std::collections::HashMap::new(); passphrases.insert("~/.ssh/id_ed25519".to_string(), "topsecret".to_string()); diff --git a/crates/tty7-core/src/daemon/ssh/auth.rs b/crates/tty7-core/src/daemon/ssh/auth.rs index 91622639..b031cd8f 100644 --- a/crates/tty7-core/src/daemon/ssh/auth.rs +++ b/crates/tty7-core/src/daemon/ssh/auth.rs @@ -441,49 +441,20 @@ async fn try_publickeys( let mut last: Option = None; let mut round = KeyRound::default(); - if spec.auth_mode != SshAuthMode::Agent { - // OpenSSH parity (#484): the `~/.ssh` default identities are appended - // after the explicit ones (there is no `IdentitiesOnly` yet), and - // deduped against them by canonical path — the explicit list may spell - // the same key with different separators or casing, and every offer - // spends one of the server's MaxAuthTries. Dedup compares the *expanded* - // explicit paths, the same ones `try_identity_file` opens: a spec entry - // still carrying `~` or `%h` names a real file, and comparing it raw - // would fail to canonicalize and offer that key a second time. - let explicit: Vec = spec - .identity_files - .iter() - .map(|p| { - crate::core::ssh_profile::expand_identity_placeholders(p, &spec.host, &spec.user) - }) - .collect(); - let discovered = dedup_candidates( - crate::core::ssh_profile::default_identity_candidates(), - &explicit, - canonical_key, - ); - let files = spec - .identity_files - .iter() - .map(|p| (p.clone(), KeySource::Explicit)) - .chain(discovered.into_iter().map(|p| (p, KeySource::Discovered))); - for (path, source) in files { - match try_identity_file(handle, spec, broker, &path, source, &mut round).await { - Outcome::Authenticated => return Outcome::Authenticated, - Outcome::Failed { - remaining_methods, .. - } => { - if remaining_methods.is_some() { - last = remaining_methods; - } - } - Outcome::Skipped => {} - } - } - } + let named_own_keys = !spec.identity_files.is_empty(); + let files = identity_offers( + &spec.identity_files, + crate::core::ssh_profile::default_identity_candidates, + ); - if spec.auth_mode != SshAuthMode::PublicKey { - match try_agent(handle, spec, &mut round).await { + for step in auth_steps(spec.auth_mode, named_own_keys) { + let outcome = match step { + AuthStep::IdentityFiles => { + try_identity_files(handle, spec, broker, &files, &mut round).await + } + AuthStep::Agent => try_agent(handle, spec, &mut round).await, + }; + match outcome { Outcome::Authenticated => return Outcome::Authenticated, Outcome::Failed { remaining_methods, .. @@ -498,10 +469,70 @@ async fn try_publickeys( Outcome::Failed { remaining_methods: last, - reason: Some(round.reason(spec.auth_mode)), + reason: Some(round.reason(spec.auth_mode, !spec.identity_files.is_empty())), } } +/// One leg of a publickey round. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum AuthStep { + IdentityFiles, + Agent, +} + +/// The order a publickey round works through its sources, most plainly asked +/// for first (#513). Every key offered spends one of the server's +/// `MaxAuthTries` — six by default — whether or not the server wants it, so +/// the order decides who gets locked out when the budget runs dry: +/// +/// 1. a key the profile **names** — the user said "use this one" +/// 2. the **agent** — the user said "I loaded these" +/// 3. the `~/.ssh` **defaults** — nobody said anything and we are guessing +/// +/// Steps 1 and 3 are the same leg: `identity_offers` already makes the two +/// lists alternatives, so a profile that names a key has no defaults to +/// reach and its named key goes first, while one that names none has only +/// guesses and puts them after the agent. Offering the guesses first is what +/// let three stale keys in `~/.ssh` exhaust the budget ahead of a working +/// agent; offering the agent ahead of a *named* key would do the same to +/// someone who spelled out exactly which key to use. +fn auth_steps(mode: SshAuthMode, named_own_keys: bool) -> Vec { + let mut steps = Vec::new(); + if mode != SshAuthMode::Agent && named_own_keys { + steps.push(AuthStep::IdentityFiles); + } + if mode != SshAuthMode::PublicKey { + steps.push(AuthStep::Agent); + } + if mode != SshAuthMode::Agent && !named_own_keys { + steps.push(AuthStep::IdentityFiles); + } + steps +} + +/// The identity files one publickey round will offer, in order, each tagged +/// with where it came from. The `~/.ssh` defaults are the list a profile +/// falls back to, not a list appended to its own: `IdentityFile` in +/// ssh_config replaces the defaults the same way, and appending instead made +/// a profile that names a key spend *more* of the server's `MaxAuthTries` +/// than one that names none (#513). `defaults` is a thunk so that the common +/// path — a profile with its own key — never goes looking for `$HOME`. +fn identity_offers( + explicit: &[String], + defaults: impl FnOnce() -> Vec, +) -> Vec<(String, KeySource)> { + if explicit.is_empty() { + return defaults() + .into_iter() + .map(|path| (path, KeySource::Discovered)) + .collect(); + } + explicit + .iter() + .map(|path| (path.clone(), KeySource::Explicit)) + .collect() +} + /// Where an identity file came from. Provenance decides failure behaviour: /// an explicit key is the user's own choice, so its failures are said aloud /// and its encrypted form may ask for a passphrase; a discovered `~/.ssh` @@ -513,41 +544,6 @@ enum KeySource { Discovered, } -/// Canonical path for dedup: the same key reached via `~`, an absolute path, -/// or different separator/casing spellings must be offered once, not twice — -/// each offer spends one of the server's MaxAuthTries. Files that cannot be -/// canonicalized (missing) never enter the set; the read step skips them. -fn canonical_key(path: &str) -> Option { - std::fs::canonicalize(path) - .ok() - .map(|p| p.to_string_lossy().into_owned()) -} - -/// Drop default candidates an explicit entry already names, comparing by -/// canonical path. Pure apart from the injected canonicalizer, so tests never -/// touch the filesystem. -fn dedup_candidates( - candidates: Vec, - explicit: &[String], - canon: impl Fn(&str) -> Option, -) -> Vec { - let mut seen: std::collections::HashSet = - explicit.iter().filter_map(|p| canon(p)).collect(); - let mut out = Vec::new(); - for candidate in candidates { - match canon(&candidate) { - Some(key) if seen.contains(&key) => {} - Some(key) => { - seen.insert(key); - out.push(candidate); - } - // Not canonicalizable means not readable; the read step skips it. - None => out.push(candidate), - } - } - out -} - /// What one publickey round learned, kept so the final error can distinguish /// the two situations "no public key was accepted" used to paper over /// (#484): nothing local could be offered at all, or keys went to the server @@ -571,7 +567,11 @@ struct KeyRound { } impl KeyRound { - fn reason(&self, mode: SshAuthMode) -> String { + /// `named_own_keys` is whether the profile listed identity files of its + /// own, which is what decides whether the `~/.ssh` defaults were in play + /// — the two are alternatives, so the "checked" list must name one or + /// the other and never both. + fn reason(&self, mode: SshAuthMode, named_own_keys: bool) -> String { if !self.rejected_files.is_empty() || self.agent_rejected > 0 { let mut what = self.rejected_files.clone(); if self.agent_rejected > 0 { @@ -590,8 +590,11 @@ impl KeyRound { if self.offered_files.is_empty() && self.agent_offered == 0 { let mut looked: Vec = Vec::new(); if mode != SshAuthMode::Agent { - looked.push("identity files".to_string()); - looked.push("~/.ssh default keys".to_string()); + looked.push(if named_own_keys { + "identity files".to_string() + } else { + "~/.ssh default keys".to_string() + }); } if mode != SshAuthMode::PublicKey { looked.push(if self.agent_available { @@ -699,6 +702,36 @@ fn load_identity( } } +/// Offer an identity list in order, stopping at the first key the server +/// takes. A file that cannot be offered at all is skipped rather than ending +/// the leg — `round` is what remembers why, for the failure text. +async fn try_identity_files( + handle: &mut Handle, + spec: &NativeSshSpec, + broker: &Arc, + files: &[(String, KeySource)], + round: &mut KeyRound, +) -> Outcome { + let mut last: Option = None; + for (path, source) in files { + match try_identity_file(handle, spec, broker, path, *source, round).await { + Outcome::Authenticated => return Outcome::Authenticated, + Outcome::Failed { + remaining_methods, .. + } => { + if remaining_methods.is_some() { + last = remaining_methods; + } + } + Outcome::Skipped => {} + } + } + Outcome::Failed { + remaining_methods: last, + reason: None, + } +} + async fn try_identity_file( handle: &mut Handle, spec: &NativeSshSpec, @@ -1278,35 +1311,81 @@ mod tests { } #[test] - fn default_candidates_dedup_against_explicit_by_canonical_path() { - // The fake canonicalizer collapses spelling differences; two strings - // with the same canonical form are one file, and the explicit entry - // wins the offer slot. - let canon = |p: &str| Some(p.replace("//", "/")); - let out = dedup_candidates( - vec![ - "/home/me/.ssh/id_ed25519".to_string(), - "/home/me/.ssh/id_ecdsa".to_string(), - "/home/me/.ssh/id_rsa".to_string(), - ], - &["/home/me//.ssh/id_rsa".to_string()], - canon, + fn a_named_key_outranks_the_agent_and_the_agent_outranks_a_guess() { + // #513: the budget is spent in order of how plainly the user asked + // for the key. Naming one puts it first; naming none leaves only + // guesses, which go behind the agent. + assert_eq!( + auth_steps(SshAuthMode::Auto, true), + vec![AuthStep::IdentityFiles, AuthStep::Agent], + "a key the profile names is offered before the agent's" ); assert_eq!( - out, - vec![ - "/home/me/.ssh/id_ed25519".to_string(), - "/home/me/.ssh/id_ecdsa".to_string() - ] + auth_steps(SshAuthMode::Auto, false), + vec![AuthStep::Agent, AuthStep::IdentityFiles], + "the ~/.ssh guesses come after the agent, never ahead of it" ); } #[test] - fn candidates_that_do_not_canonicalize_pass_through() { - // A missing default is the normal case; the read step skips it, so - // dedup must not drop it here either. - let out = dedup_candidates(vec!["/missing/id_ed25519".to_string()], &[], |_| None); - assert_eq!(out, vec!["/missing/id_ed25519".to_string()]); + fn a_pinned_mode_runs_only_its_own_step() { + for named in [true, false] { + assert_eq!( + auth_steps(SshAuthMode::PublicKey, named), + vec![AuthStep::IdentityFiles], + "publickey-only never reaches the agent (named: {named})" + ); + assert_eq!( + auth_steps(SshAuthMode::Agent, named), + vec![AuthStep::Agent], + "agent-only never reads a file (named: {named})" + ); + } + } + + #[test] + fn the_defaults_stand_in_only_for_a_profile_that_names_no_key() { + // #513: the two lists are alternatives, never a concatenation. A + // profile naming one key must spend one attempt, not four. + let defaults = || { + vec![ + "/home/me/.ssh/id_ed25519".to_string(), + "/home/me/.ssh/id_rsa".to_string(), + ] + }; + + assert_eq!( + identity_offers(&[], defaults), + vec![ + ( + "/home/me/.ssh/id_ed25519".to_string(), + KeySource::Discovered + ), + ("/home/me/.ssh/id_rsa".to_string(), KeySource::Discovered), + ], + "no key of its own falls back to the ~/.ssh defaults" + ); + + assert_eq!( + identity_offers(&["~/keys/work".to_string()], defaults), + vec![("~/keys/work".to_string(), KeySource::Explicit)], + "naming a key replaces the defaults rather than adding to them" + ); + } + + #[test] + fn a_named_key_is_offered_in_the_order_it_was_written() { + let offers = identity_offers( + &["~/keys/first".to_string(), "~/keys/second".to_string()], + Vec::new, + ); + assert_eq!( + offers + .iter() + .map(|(p, _)| p.as_str()) + .collect::>(), + vec!["~/keys/first", "~/keys/second"] + ); } const PASSPHRASE: &str = "correct horse battery staple"; @@ -1465,7 +1544,7 @@ mod tests { let mut round = KeyRound::default(); round.offered_files = vec!["/home/me/.ssh/id_ed25519".to_string()]; round.rejected_files = round.offered_files.clone(); - let msg = round.reason(SshAuthMode::Auto); + let msg = round.reason(SshAuthMode::Auto, true); assert_eq!( msg, "server rejected public key(s): /home/me/.ssh/id_ed25519" @@ -1473,7 +1552,7 @@ mod tests { round.agent_offered = 2; round.agent_rejected = 2; - let msg = round.reason(SshAuthMode::Auto); + let msg = round.reason(SshAuthMode::Auto, true); assert_eq!( msg, "server rejected public key(s): /home/me/.ssh/id_ed25519, 2 agent identities" @@ -1483,7 +1562,7 @@ mod tests { #[test] fn reason_for_nothing_offered_says_where_it_looked() { let round = KeyRound::default(); - let msg = round.reason(SshAuthMode::Auto); + let msg = round.reason(SshAuthMode::Auto, false); assert!(msg.contains("no usable private key was found"), "{msg}"); assert!(msg.contains("~/.ssh default keys"), "{msg}"); assert!(msg.contains("agent (unavailable)"), "{msg}"); @@ -1492,14 +1571,14 @@ mod tests { // "unavailable". let mut round = KeyRound::default(); round.agent_available = true; - let msg = round.reason(SshAuthMode::Auto); + let msg = round.reason(SshAuthMode::Auto, false); assert!(msg.contains("the SSH agent"), "{msg}"); assert!(!msg.contains("unavailable"), "{msg}"); // Pinned modes name only what they would have used. - let msg = KeyRound::default().reason(SshAuthMode::Agent); + let msg = KeyRound::default().reason(SshAuthMode::Agent, false); assert!(!msg.contains("default keys"), "{msg}"); - let msg = KeyRound::default().reason(SshAuthMode::PublicKey); + let msg = KeyRound::default().reason(SshAuthMode::PublicKey, false); assert!(!msg.contains("agent"), "{msg}"); } @@ -1509,7 +1588,7 @@ mod tests { round .unusable .push("cannot read identity file /bad/key: denied".to_string()); - let msg = round.reason(SshAuthMode::PublicKey); + let msg = round.reason(SshAuthMode::PublicKey, true); assert!( msg.contains("cannot read identity file /bad/key: denied"), "{msg}" @@ -1524,7 +1603,7 @@ mod tests { "public-key auth error with /home/me/.ssh/id_ed25519: connection lost".to_string(), ); assert_eq!( - round.reason(SshAuthMode::Auto), + round.reason(SshAuthMode::Auto, true), "public-key auth error with /home/me/.ssh/id_ed25519: connection lost" ); } diff --git a/crates/tty7-core/src/daemon/ssh/connect.rs b/crates/tty7-core/src/daemon/ssh/connect.rs index b0957fd7..cc1e56ac 100644 --- a/crates/tty7-core/src/daemon/ssh/connect.rs +++ b/crates/tty7-core/src/daemon/ssh/connect.rs @@ -9,6 +9,7 @@ use tokio::net::TcpStream; use crate::daemon::protocol::{NativeSshSpec, SshAlgorithms, SshProxy}; +use super::known_hosts; use super::session::SshConnection; pub enum Transport { @@ -369,8 +370,11 @@ async fn http_connect( } pub fn build_config(spec: &NativeSshSpec) -> Arc { + // Every hop comes through here — the target and each jump host build their + // own config from their own spec — so each asks known_hosts about itself. + let known = known_hosts::known_algorithms(&spec.host, spec.port); let mut cfg = russh::client::Config { - preferred: build_preferred(&spec.algorithms), + preferred: build_preferred(&spec.algorithms, &known), ..Default::default() }; if let Some(iv) = spec.keepalive_interval_s.filter(|v| *v > 0) { @@ -382,7 +386,10 @@ pub fn build_config(spec: &NativeSshSpec) -> Arc { Arc::new(cfg) } -fn build_preferred(a: &SshAlgorithms) -> russh::Preferred { +fn build_preferred( + a: &SshAlgorithms, + known_host_keys: &[russh::keys::Algorithm], +) -> russh::Preferred { let mut p = russh::Preferred::DEFAULT; if !a.kex.is_empty() { let v: Vec = a @@ -423,6 +430,21 @@ fn build_preferred(a: &SshAlgorithms) -> russh::Preferred { if !v.is_empty() { p.key = Cow::Owned(v); } + } else if !known_host_keys.is_empty() { + // What OpenSSH's `order_hostkeyalgs()` does: offer the algorithms this + // host already has entries for ahead of the rest, so a server that has + // both an ssh-rsa key on file and an ed25519 key on offer answers with + // the one the user has already accepted. Without it the default order + // picks ed25519, and a host known only by ssh-rsa raises a + // key-you-have-never-seen prompt on every single connection. + // + // Reordering only, never filtering — a host whose keys have genuinely + // rotated must still be able to negotiate — and skipped entirely when + // the user pinned `HostKeyAlgorithms`, which OpenSSH also treats as the + // last word. + let mut ordered: Vec = p.key.iter().cloned().collect(); + ordered.sort_by_key(|alg| !known_host_keys.iter().any(|k| same_key_type(k, alg))); + p.key = Cow::Owned(ordered); } if !a.compression.is_empty() { let v: Vec = a @@ -437,6 +459,21 @@ fn build_preferred(a: &SshAlgorithms) -> russh::Preferred { p } +/// Whether two host-key algorithms stand for the same *key*. +/// +/// An `ssh-rsa` line in known_hosts parses to `Rsa { hash: None }`, but what +/// gets negotiated is one of `rsa-sha2-512`, `rsa-sha2-256` or `ssh-rsa` — three +/// signature algorithms over one key. Comparing with `==` would float only the +/// SHA-1 spelling to the front of the list, which is a downgrade dressed up as a +/// preference; OpenSSH's `order_hostkeyalgs()` matches on the key type too. +fn same_key_type(a: &russh::keys::Algorithm, b: &russh::keys::Algorithm) -> bool { + use russh::keys::Algorithm; + match (a, b) { + (Algorithm::Rsa { .. }, Algorithm::Rsa { .. }) => true, + _ => a == b, + } +} + #[cfg(test)] mod tests { use super::*; @@ -473,9 +510,10 @@ mod tests { #[test] fn build_preferred_keeps_defaults_for_empty_lists() { let a = SshAlgorithms::default(); - let p = build_preferred(&a); + let p = build_preferred(&a, &[]); assert_eq!(p.kex, russh::Preferred::DEFAULT.kex); assert_eq!(p.cipher, russh::Preferred::DEFAULT.cipher); + assert_eq!(p.key, russh::Preferred::DEFAULT.key); } #[test] @@ -484,8 +522,61 @@ mod tests { cipher: vec!["totally-not-a-cipher".into(), "aes256-ctr".into()], ..Default::default() }; - let p = build_preferred(&a); + let p = build_preferred(&a, &[]); let aes = russh::cipher::Name::try_from("aes256-ctr").unwrap(); assert_eq!(p.cipher.as_ref(), &[aes]); } + + /// The default order leads with ed25519, so a host known only by an + /// ssh-rsa key was greeted with a key it had never seen on every connect. + #[test] + fn build_preferred_floats_known_hosts_algorithms_to_the_front() { + let rsa = russh::keys::Algorithm::Rsa { hash: None }; + let p = build_preferred(&SshAlgorithms::default(), &[rsa]); + // All three RSA spellings sign for the one key on file, so all three + // lead — anything less would pin the host to SHA-1 signatures. + assert!( + p.key + .iter() + .take(3) + .all(|a| matches!(a, russh::keys::Algorithm::Rsa { .. })), + "expected the RSA spellings first, got {:?}", + p.key + ); + } + + #[test] + fn build_preferred_reordering_never_drops_an_algorithm() { + let rsa = russh::keys::Algorithm::Rsa { hash: None }; + let p = build_preferred(&SshAlgorithms::default(), &[rsa]); + let mut got: Vec = p.key.iter().map(|a| a.to_string()).collect(); + let mut want: Vec = russh::Preferred::DEFAULT + .key + .iter() + .map(|a| a.to_string()) + .collect(); + got.sort(); + want.sort(); + assert_eq!(got, want); + } + + /// OpenSSH stops reordering once `HostKeyAlgorithms` is explicit, because + /// the user has already said what order they want. + #[test] + fn build_preferred_leaves_a_pinned_host_key_list_alone() { + let a = SshAlgorithms { + host_key: vec!["ssh-ed25519".into(), "rsa-sha2-512".into()], + ..Default::default() + }; + let p = build_preferred(&a, &[russh::keys::Algorithm::Rsa { hash: None }]); + assert_eq!( + p.key.as_ref(), + &[ + russh::keys::Algorithm::Ed25519, + russh::keys::Algorithm::Rsa { + hash: Some(russh::keys::HashAlg::Sha512) + }, + ] + ); + } } diff --git a/crates/tty7-core/src/daemon/ssh/forward.rs b/crates/tty7-core/src/daemon/ssh/forward.rs index 4996fd5e..c1e28ead 100644 --- a/crates/tty7-core/src/daemon/ssh/forward.rs +++ b/crates/tty7-core/src/daemon/ssh/forward.rs @@ -199,6 +199,53 @@ enum ForwardCancel { None, } +/// A forward's status, shared with the task that serves it. +/// +/// The status used to be a plain field written once when the forward was set +/// up and never again, so a forward whose accept loop had already exited went +/// on reporting `Listening` for as long as the pane stayed open — which is +/// forever, because a pane is deliberately not closed when its SSH connection +/// dies. The task that discovers the truth is the one that has to be able to +/// record it. +type SharedStatus = Arc>; + +/// Why a forward's accept loop stopped. +enum LoopExit { + /// `accept()` failed over and over: the listening socket is no longer + /// usable, so nothing can even reach the forward any more. + ListenerLost, + /// The SSH transport went away under it. The port may still be bound — + /// a connection to it is accepted and then dropped — but there is nothing + /// on the far side of it. + ConnectionLost, +} + +/// The status a forward is left in once its loop has exited. +/// +/// `ForwardStatus::Error` rather than a `Stopped` variant of its own, on +/// purpose: `ForwardStatus` is serialised across the protocol to remote +/// `tty7-server` builds, and a variant an older remote has never heard of is a +/// deserialisation failure rather than an unknown status. `Error` already +/// draws as a danger badge with its message beside it. +fn loop_exit_status(exit: LoopExit) -> ForwardStatus { + ForwardStatus::Error( + match exit { + LoopExit::ListenerLost => "stopped: the listening socket closed", + LoopExit::ConnectionLost => "stopped: the SSH connection went away", + } + .to_string(), + ) +} + +/// A forward that never got as far as a listening socket. It has no task, so +/// nothing will ever move it off this status. +fn bind_failed(rule: &SshForwardRule, e: io::Error) -> SharedStatus { + Arc::new(Mutex::new(ForwardStatus::Error(format!( + "bind {}:{} failed: {e}", + rule.bind_host, rule.bind_port + )))) +} + struct ForwardEntry { id: u64, kind: SshForwardKind, @@ -207,7 +254,7 @@ struct ForwardEntry { target_host: String, target_port: u16, description: Option, - status: ForwardStatus, + status: SharedStatus, cancel: ForwardCancel, auto_local: bool, } @@ -229,7 +276,7 @@ impl ForwardEntry { target_host: self.target_host.clone(), target_port: self.target_port, description: self.description.clone(), - status: self.status.clone(), + status: self.status.lock().unwrap().clone(), } } } @@ -390,18 +437,11 @@ impl SshForwardRegistry { &self, conn: &Arc, rule: &SshForwardRule, - ) -> (u16, ForwardStatus, ForwardCancel) { + ) -> (u16, SharedStatus, ForwardCancel) { let listener = match TcpListener::bind((rule.bind_host.as_str(), rule.bind_port)).await { Ok(l) => l, Err(e) => { - return ( - rule.bind_port, - ForwardStatus::Error(format!( - "bind {}:{} failed: {e}", - rule.bind_host, rule.bind_port - )), - ForwardCancel::None, - ); + return (rule.bind_port, bind_failed(rule, e), ForwardCancel::None); } }; let bound = listener @@ -411,14 +451,16 @@ impl SshForwardRegistry { let conn = conn.clone(); let target_host = rule.target_host.clone(); let target_port = rule.target_port; + let status: SharedStatus = Arc::new(Mutex::new(ForwardStatus::Listening)); + let task_status = status.clone(); let handle = tokio::spawn(async move { - loop { + let exit = loop { let sock = match accept_retrying(&listener).await { Some((sock, _peer)) => sock, - None => break, + None => break LoopExit::ListenerLost, }; if !conn.is_alive() { - break; + break LoopExit::ConnectionLost; } let conn = conn.clone(); let target_host = target_host.clone(); @@ -432,27 +474,21 @@ impl SshForwardRegistry { } } }); - } + }; + *task_status.lock().unwrap() = loop_exit_status(exit); }); - (bound, ForwardStatus::Listening, ForwardCancel::Task(handle)) + (bound, status, ForwardCancel::Task(handle)) } async fn start_dynamic( &self, conn: &Arc, rule: &SshForwardRule, - ) -> (u16, ForwardStatus, ForwardCancel) { + ) -> (u16, SharedStatus, ForwardCancel) { let listener = match TcpListener::bind((rule.bind_host.as_str(), rule.bind_port)).await { Ok(l) => l, Err(e) => { - return ( - rule.bind_port, - ForwardStatus::Error(format!( - "bind {}:{} failed: {e}", - rule.bind_host, rule.bind_port - )), - ForwardCancel::None, - ); + return (rule.bind_port, bind_failed(rule, e), ForwardCancel::None); } }; let bound = listener @@ -460,14 +496,16 @@ impl SshForwardRegistry { .map(|a| a.port()) .unwrap_or(rule.bind_port); let conn = conn.clone(); + let status: SharedStatus = Arc::new(Mutex::new(ForwardStatus::Listening)); + let task_status = status.clone(); let handle = tokio::spawn(async move { - loop { + let exit = loop { let sock = match accept_retrying(&listener).await { Some((sock, _peer)) => sock, - None => break, + None => break LoopExit::ListenerLost, }; if !conn.is_alive() { - break; + break LoopExit::ConnectionLost; } let conn = conn.clone(); tokio::spawn(async move { @@ -492,16 +530,21 @@ impl SshForwardRegistry { } } }); - } + }; + *task_status.lock().unwrap() = loop_exit_status(exit); }); - (bound, ForwardStatus::Listening, ForwardCancel::Task(handle)) + (bound, status, ForwardCancel::Task(handle)) } + /// Unlike the two above, a remote forward has no accept loop of its own to + /// notice a dead transport: the far end opens the channels and russh hands + /// them to the connection's handler. Its status stays whatever the + /// `tcpip-forward` request answered. async fn start_remote( &self, conn: &Arc, rule: &SshForwardRule, - ) -> (u16, ForwardStatus, ForwardCancel) { + ) -> (u16, SharedStatus, ForwardCancel) { match conn .add_remote_forward( &rule.bind_host, @@ -513,7 +556,7 @@ impl SshForwardRegistry { { Ok(bound) => ( bound, - ForwardStatus::Listening, + Arc::new(Mutex::new(ForwardStatus::Listening)), ForwardCancel::Remote { conn: Arc::downgrade(conn), bind_host: rule.bind_host.clone(), @@ -522,7 +565,9 @@ impl SshForwardRegistry { ), Err(e) => ( rule.bind_port, - ForwardStatus::Error(format!("remote forward request denied: {e}")), + Arc::new(Mutex::new(ForwardStatus::Error(format!( + "remote forward request denied: {e}" + )))), ForwardCancel::None, ), } @@ -576,7 +621,7 @@ impl SshForwardRegistry { }; let id = self.next_id.fetch_add(1, Ordering::Relaxed); let (bind_port, status, cancel) = self.start_local(&conn, &rule).await; - if let ForwardStatus::Error(e) = &status { + if let ForwardStatus::Error(e) = &*status.lock().unwrap() { return Err(io::Error::other(e.clone())); } let entry = ForwardEntry { @@ -617,7 +662,9 @@ impl SshForwardRegistry { && e.kind == SshForwardKind::Local && e.target_host == remote_host && e.target_port == remote_port - && matches!(e.status, ForwardStatus::Listening) + // Now that a dead loop says so, this stops handing out the + // port of a forward that no longer serves anything. + && matches!(*e.status.lock().unwrap(), ForwardStatus::Listening) }) .map(|e| e.bind_port) } @@ -814,6 +861,47 @@ mod tests { assert_eq!(table.lookup("localhost", 9000), None); } + #[test] + fn a_stopped_forward_says_why_it_stopped() { + let listener = loop_exit_status(LoopExit::ListenerLost); + let connection = loop_exit_status(LoopExit::ConnectionLost); + assert_ne!( + listener, connection, + "a socket that closed and a transport that died are different problems" + ); + for status in [&listener, &connection] { + assert!( + matches!(status, ForwardStatus::Error(_)), + "an older remote has to be able to deserialise this, so no new variant" + ); + } + } + + /// A forward's status used to be copied into `ManagedForward` from a field + /// written once when the forward was set up, so a forward whose loop had + /// long since exited answered `Listening` to every poll of the panel. + #[tokio::test] + async fn a_forward_whose_loop_exited_stops_reporting_listening() { + let reg = SshForwardRegistry::default(); + push_listener(®, ForwardOwner::Pane(7), 0).await; + assert!(matches!(reg.list(7)[0].status, ForwardStatus::Listening)); + + // What the accept loop does on its way out. The entry is already in the + // registry by then, which is the whole reason the status is shared. + let status = reg.owners.lock().unwrap()[&ForwardOwner::Pane(7)][0] + .status + .clone(); + *status.lock().unwrap() = loop_exit_status(LoopExit::ConnectionLost); + + match ®.list(7)[0].status { + ForwardStatus::Error(msg) => assert!( + msg.contains("SSH connection"), + "the panel needs a reason, not just a red badge: {msg}" + ), + other => panic!("a dead forward still reports {other:?}"), + } + } + #[tokio::test] async fn registry_add_list_remove_teardown_bookkeeping() { let reg = SshForwardRegistry::default(); @@ -827,7 +915,7 @@ mod tests { target_host: "h".into(), target_port: 80, description: None, - status: ForwardStatus::Listening, + status: Arc::new(Mutex::new(ForwardStatus::Listening)), cancel: ForwardCancel::Task(task), auto_local: false, } @@ -872,7 +960,7 @@ mod tests { target_host: "h".into(), target_port: 80, description: None, - status: ForwardStatus::Listening, + status: Arc::new(Mutex::new(ForwardStatus::Listening)), cancel: ForwardCancel::Task(handle), auto_local: false, } @@ -933,7 +1021,7 @@ mod tests { target_host: "127.0.0.1".into(), target_port: 3000, description: None, - status: ForwardStatus::Listening, + status: Arc::new(Mutex::new(ForwardStatus::Listening)), cancel: ForwardCancel::Task(handle), auto_local: true, }; diff --git a/crates/tty7-core/src/daemon/ssh/handler.rs b/crates/tty7-core/src/daemon/ssh/handler.rs index 56a2063f..170b336a 100644 --- a/crates/tty7-core/src/daemon/ssh/handler.rs +++ b/crates/tty7-core/src/daemon/ssh/handler.rs @@ -28,8 +28,22 @@ impl ClientHandler { remember, } => { if remember { - if let Err(e) = known_hosts::append_trusted(&self.host, self.port, key) { - log::warn!("failed to record host key in known_hosts: {e}"); + // The superseded line has to go before the new one lands. + // `known_hosts::check` answers `Known` on any + // same-algorithm match, so an override that only appended + // left the key the user had just rejected trusted for good. + // If it cannot be dropped, do not append either: being + // asked again next time is the better half of that trade. + match known_hosts::forget_superseded(&self.host, self.port, key) { + Ok(()) => { + if let Err(e) = known_hosts::append_trusted(&self.host, self.port, key) + { + log::warn!("failed to record host key in known_hosts: {e}"); + } + } + Err(e) => log::warn!( + "not recording host key: the superseded known_hosts line could not be removed: {e}" + ), } } true @@ -75,6 +89,28 @@ impl russh::client::Handler for ClientHandler { port: self.port, algorithm, fingerprint_sha256, + previously_known_as: None, + }) + .await; + Ok(self.apply_decision(resp, server_public_key)) + } + // Deliberately the unknown-host prompt and not a variant of its + // own: `AuthPromptKind` crosses to the GUI *and* to whatever + // `tty7-server` the far end happens to be running, and a new + // externally-tagged variant is a hard decode failure on any peer + // that predates it. The extra field is additive in both + // directions. + HostKeyStatus::ChangedAlgorithm { + known_algorithm, .. + } => { + let resp = self + .broker + .prompt(AuthPromptKind::HostKeyUnknown { + host: self.host.clone(), + port: self.port, + algorithm, + fingerprint_sha256, + previously_known_as: Some(known_algorithm), }) .await; Ok(self.apply_decision(resp, server_public_key)) diff --git a/crates/tty7-core/src/daemon/ssh/known_hosts.rs b/crates/tty7-core/src/daemon/ssh/known_hosts.rs index 0eb7c066..641186a4 100644 --- a/crates/tty7-core/src/daemon/ssh/known_hosts.rs +++ b/crates/tty7-core/src/daemon/ssh/known_hosts.rs @@ -1,13 +1,27 @@ use std::io::Write as _; use std::path::{Path, PathBuf}; -use russh::keys::ssh_key::{HashAlg, PublicKey}; +use russh::keys::ssh_key::{Algorithm, HashAlg, PublicKey}; #[derive(Debug, Clone, PartialEq, Eq)] pub enum HostKeyStatus { Known, Unknown, - Changed { old_fingerprint_sha256: String }, + Changed { + old_fingerprint_sha256: String, + }, + /// The host is on file, but only under some *other* key algorithm. + /// + /// A server that grows an ed25519 key beside the ssh-rsa one it has always + /// had has not been tampered with, and OpenSSH says so: it treats a key of + /// an algorithm the host has no entry for as simply unknown, and saves the + /// man-in-the-middle warning for a key that contradicts one on file. The + /// algorithm travels with the status so the confirmation can name what the + /// host was known by. + ChangedAlgorithm { + known_fingerprint_sha256: String, + known_algorithm: String, + }, Revoked, } @@ -71,7 +85,7 @@ pub fn check_in_str(contents: &str, host: &str, port: u16, key: &PublicKey) -> H } let mut changed: Option = None; - let mut changed_other_alg: Option = None; + let mut changed_other_alg: Option<(String, String)> = None; for line in contents.lines() { let Some(entry) = KnownHostsLine::parse(line) else { continue; @@ -84,9 +98,11 @@ pub fn check_in_str(contents: &str, host: &str, port: u16, key: &PublicKey) -> H Some(Marker::Revoked) => continue, None => { let Some(stored) = entry.key() else { continue }; - if stored.algorithm() != our_alg { + let stored_alg = stored.algorithm(); + if stored_alg != our_alg { if changed_other_alg.is_none() { - changed_other_alg = Some(fingerprint_sha256(&stored)); + changed_other_alg = + Some((fingerprint_sha256(&stored), stored_alg.as_str().to_string())); } continue; } @@ -100,14 +116,62 @@ pub fn check_in_str(contents: &str, host: &str, port: u16, key: &PublicKey) -> H } } - match changed.or(changed_other_alg) { - Some(old_fingerprint_sha256) => HostKeyStatus::Changed { + // A same-algorithm contradiction outranks everything else on file: the host + // has an entry that says this key is wrong, and no amount of other-algorithm + // company softens that. + if let Some(old_fingerprint_sha256) = changed { + return HostKeyStatus::Changed { old_fingerprint_sha256, + }; + } + match changed_other_alg { + Some((known_fingerprint_sha256, known_algorithm)) => HostKeyStatus::ChangedAlgorithm { + known_fingerprint_sha256, + known_algorithm, }, None => HostKeyStatus::Unknown, } } +/// The host-key algorithms this host already has entries for, in file order. +/// +/// Negotiation reads this so the algorithms already on file are offered first — +/// see `connect::build_preferred`. Markers are skipped: a `@cert-authority` line +/// names the authority's key rather than the host's, and a `@revoked` one names +/// a key that would be refused, so neither predicts what the host will present. +pub fn known_algorithms(host: &str, port: u16) -> Vec { + match default_path() { + Some(path) => known_algorithms_in_file(&path, host, port), + None => Vec::new(), + } +} + +pub fn known_algorithms_in_file(path: &Path, host: &str, port: u16) -> Vec { + match std::fs::read_to_string(path) { + Ok(contents) => known_algorithms_in_str(&contents, host, port), + Err(_) => Vec::new(), + } +} + +pub fn known_algorithms_in_str(contents: &str, host: &str, port: u16) -> Vec { + let token = host_token(host, port); + let mut out: Vec = Vec::new(); + for line in contents.lines() { + let Some(entry) = KnownHostsLine::parse(line) else { + continue; + }; + if entry.marker.is_some() || !entry.matches_host(&token) { + continue; + } + let Some(stored) = entry.key() else { continue }; + let alg = stored.algorithm(); + if !out.contains(&alg) { + out.push(alg); + } + } + out +} + pub fn append_trusted(host: &str, port: u16, key: &PublicKey) -> std::io::Result<()> { let path = default_path().ok_or_else(|| { std::io::Error::new(std::io::ErrorKind::NotFound, "no home dir for known_hosts") @@ -252,6 +316,75 @@ pub fn delete_in_str(contents: &str, id: &KnownHostId) -> (String, bool) { (out, removed) } +/// Drop the lines `key` replaces before it is appended for `host`. +/// +/// Appending on its own is not enough when the user overrides a *changed* key: +/// `check_in_str` answers `Known` on any same-algorithm match, so the line the +/// new key contradicts — the one the user just decided was wrong, and which in +/// the case the warning exists for is an attacker's — would go on being trusted +/// forever, with no warning ever shown again. OpenSSH rewrites the file for the +/// same reason. +/// +/// A no-op for a host that was merely unknown, which by definition has no +/// same-algorithm line to drop. +pub fn forget_superseded(host: &str, port: u16, key: &PublicKey) -> std::io::Result<()> { + match default_path() { + Some(path) => forget_superseded_in_file(&path, host, port, key), + None => Ok(()), + } +} + +pub fn forget_superseded_in_file( + path: &Path, + host: &str, + port: u16, + key: &PublicKey, +) -> std::io::Result<()> { + let contents = match std::fs::read_to_string(path) { + Ok(c) => c, + Err(e) if e.kind() == std::io::ErrorKind::NotFound => return Ok(()), + Err(e) => return Err(e), + }; + for id in superseded_ids_in_str(&contents, host, port, key) { + delete_in_file(path, &id)?; + } + Ok(()) +} + +pub fn superseded_ids_in_str( + contents: &str, + host: &str, + port: u16, + key: &PublicKey, +) -> Vec { + let token = host_token(host, port); + let our_alg = key.algorithm(); + let mut out = Vec::new(); + for line in contents.lines() { + let Some(entry) = KnownHostsLine::parse(line) else { + continue; + }; + // Only a plain line for this one host: a `@revoked` line is a standing + // refusal that an override of a different key must not lift, and a glob + // or a comma list also speaks for hosts nobody is connecting to — the + // rest of `*.example.com` should not be forgotten because one machine + // behind it rotated its key. + if entry.marker.is_some() || !entry.names_only_host(&token) { + continue; + } + let Some(stored) = entry.key() else { continue }; + if stored.algorithm() != our_alg || &stored == key { + continue; + } + out.push(KnownHostId { + host: entry.hosts.to_string(), + key_type: entry.keytype.to_string(), + keyblob: entry.keyblob.to_string(), + }); + } + out +} + fn split_keep_terminators(text: &str) -> Vec<&str> { let mut segments = Vec::new(); let mut start = 0; @@ -314,6 +447,29 @@ impl<'a> KnownHostsLine<'a> { PublicKey::from_openssh(&format!("{} {}", self.keytype, self.keyblob)).ok() } + /// Whether this line's host field names `token` and nothing else. + /// + /// `matches_host` is the right question for "does this entry apply here"; + /// this is the stricter one to ask before *deleting* a line, because a glob + /// or a comma list carries other hosts with it. A hashed pattern names + /// exactly one token, so it passes. + fn names_only_host(&self, token: &str) -> bool { + if self.hosts.contains(',') { + return false; + } + let pattern = self.hosts.trim(); + match pattern.strip_prefix("|1|") { + Some(hashed) => hashed_host_matches(hashed, token), + None => { + !pattern + .as_bytes() + .iter() + .any(|&b| b == b'*' || b == b'?' || b == b'!') + && pattern.eq_ignore_ascii_case(token) + } + } + } + fn matches_host(&self, token: &str) -> bool { let mut matched = false; for pattern in self.hosts.split(',') { @@ -555,13 +711,19 @@ mod tests { } } + const KEY_ECDSA: &str = "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBCdv5xfuuCGyVbYZSTqcFjQWE7YtIsx8fqlXF1+v728j1RUnELLVrmgsC6gZ0zObXAzJ39JEynaQv9tf/v16V58="; + + /// Not `Changed`, which is the man-in-the-middle alarm: a host that has + /// grown a second key type has not contradicted anything on file, and + /// OpenSSH asks the same mild question it asks about any unseen key. #[test] - fn different_key_type_for_a_known_host_reports_changed_not_unknown() { - const KEY_ECDSA: &str = "ecdsa-sha2-nistp256 AAAAE2VjZHNhLXNoYTItbmlzdHAyNTYAAAAIbmlzdHAyNTYAAABBBCdv5xfuuCGyVbYZSTqcFjQWE7YtIsx8fqlXF1+v728j1RUnELLVrmgsC6gZ0zObXAzJ39JEynaQv9tf/v16V58="; + fn a_key_of_a_new_algorithm_is_flagged_as_the_algorithm_being_new() { let file = format!("example.com {KEY_A}\n"); match check_in_str(&file, "example.com", 22, &key(KEY_ECDSA)) { - HostKeyStatus::Changed { .. } => {} - other => panic!("expected Changed, got {other:?}"), + HostKeyStatus::ChangedAlgorithm { + known_algorithm, .. + } => assert_eq!(known_algorithm, "ssh-ed25519"), + other => panic!("expected ChangedAlgorithm, got {other:?}"), } let file = format!("example.com {KEY_A}\nexample.com {KEY_ECDSA}\n"); assert_eq!( @@ -570,6 +732,48 @@ mod tests { ); } + #[test] + fn a_different_key_of_the_same_algorithm_is_still_a_change() { + let file = format!("example.com {KEY_A}\n"); + match check_in_str(&file, "example.com", 22, &key(KEY_B)) { + HostKeyStatus::Changed { + old_fingerprint_sha256, + } => assert_eq!(old_fingerprint_sha256, fingerprint_sha256(&key(KEY_A))), + other => panic!("expected Changed, got {other:?}"), + } + } + + /// The downgrade this split has to not open: an attacker offering a key of + /// an algorithm the host also has an entry for gets the full alarm, however + /// many other-algorithm lines sit alongside it. + #[test] + fn a_same_algorithm_mismatch_outranks_an_other_algorithm_entry() { + let file = format!("example.com {KEY_ECDSA}\nexample.com {KEY_A}\n"); + match check_in_str(&file, "example.com", 22, &key(KEY_B)) { + HostKeyStatus::Changed { .. } => {} + other => panic!("expected Changed, got {other:?}"), + } + } + + #[test] + fn known_algorithms_lists_each_algorithm_once_in_file_order() { + let file = format!( + "example.com {KEY_ECDSA}\nexample.com {KEY_A}\nexample.com {KEY_B}\nother.com {KEY_A}\n" + ); + let algs = known_algorithms_in_str(&file, "example.com", 22); + assert_eq!( + algs.iter().map(|a| a.as_str()).collect::>(), + vec!["ecdsa-sha2-nistp256", "ssh-ed25519"] + ); + assert!(known_algorithms_in_str(&file, "nowhere.com", 22).is_empty()); + } + + #[test] + fn known_algorithms_ignores_revoked_and_cert_authority_lines() { + let file = format!("@revoked example.com {KEY_A}\n@cert-authority example.com {KEY_B}\n"); + assert!(known_algorithms_in_str(&file, "example.com", 22).is_empty()); + } + #[test] fn non_default_port_uses_bracket_syntax() { let ka = key(KEY_A); @@ -771,6 +975,58 @@ mod tests { assert_eq!(after, contents); } + /// Overriding a changed key used to append and leave the old line in + /// place, and `check_in_str` answers `Known` on any same-algorithm match — + /// so the key the user had just refused stayed trusted, silently, forever. + #[test] + fn overriding_a_changed_key_stops_trusting_the_one_it_replaces() { + let dir = std::env::temp_dir().join(format!("tty7-kh-supersede-{}", std::process::id())); + std::fs::create_dir_all(&dir).unwrap(); + let path = dir.join("known_hosts"); + std::fs::write(&path, format!("example.com {KEY_A}\n")).unwrap(); + + let kb = key(KEY_B); + forget_superseded_in_file(&path, "example.com", 22, &kb).unwrap(); + append_trusted_to(&path, "example.com", 22, &kb).unwrap(); + + let contents = std::fs::read_to_string(&path).unwrap(); + assert!( + !contents.contains(KEY_A.split_whitespace().nth(1).unwrap()), + "the superseded key is still on file: {contents}" + ); + assert_eq!( + check_in_str(&contents, "example.com", 22, &kb), + HostKeyStatus::Known + ); + assert!(matches!( + check_in_str(&contents, "example.com", 22, &key(KEY_A)), + HostKeyStatus::Changed { .. } + )); + std::fs::remove_dir_all(&dir).ok(); + } + + #[test] + fn nothing_is_superseded_by_a_key_of_another_algorithm_or_another_host() { + let file = format!("example.com {KEY_A}\nother.com {KEY_B}\n"); + assert!(superseded_ids_in_str(&file, "example.com", 22, &key(KEY_ECDSA)).is_empty()); + assert!(superseded_ids_in_str(&file, "example.com", 22, &key(KEY_A)).is_empty()); + assert_eq!( + superseded_ids_in_str(&file, "example.com", 22, &key(KEY_B)).len(), + 1 + ); + } + + /// A wildcard or comma-list line speaks for hosts nobody is connecting to, + /// and a `@revoked` line is a standing refusal — one host's override must + /// not quietly drop either. + #[test] + fn superseding_never_touches_a_shared_or_revoked_line() { + let file = format!( + "*.example.com {KEY_A}\nweb.example.com,db.example.com {KEY_A}\n@revoked web.example.com {KEY_A}\n" + ); + assert!(superseded_ids_in_str(&file, "web.example.com", 22, &key(KEY_B)).is_empty()); + } + #[test] fn glob_matcher_edge_cases() { assert!(glob_match(b"*", b"")); diff --git a/src/core/ssh_config.rs b/src/core/ssh_config.rs index a78db670..f1ca8a7a 100644 --- a/src/core/ssh_config.rs +++ b/src/core/ssh_config.rs @@ -1,4 +1,4 @@ -use std::collections::HashSet; +use std::collections::{BTreeMap, HashSet}; use std::path::{Path, PathBuf}; use crate::core::ssh_profile::{ForwardKind, ForwardRule, HostPort, SshProfile as ManagedProfile}; @@ -163,6 +163,45 @@ pub struct ImportedProfile { pub proxy_jump: Option, } +/// A keyword the file sets for a host tty7 is importing, that no `SshProfile` +/// field can hold — `IdentityAgent`, `CertificateFile`, `AddKeysToAgent` and +/// the rest of what `resolve_alias` walks past. +/// +/// Grouped by keyword rather than by host because that is the question someone +/// reads the report to answer — "what did it not keep?" — and because the same +/// keyword under a two-alias `Host` line is one omission, not two. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct IgnoredOption { + /// Spelled as the file spells it. A report that says `identityagent` when + /// the file says `IdentityAgent` sends the reader hunting for a typo that + /// is tty7's, not theirs. + pub option: String, + pub hosts: Vec, +} + +/// Everything one pass over `~/.ssh/config` found, including the parts of it +/// that went nowhere. +/// +/// `import_profiles_from` answers only the first field, because it is also on a +/// render path; the import button wants the rest so it can say what happened +/// instead of leaving the person to diff the host list by eye. +#[derive(Clone, Debug, PartialEq, Eq)] +pub struct ImportReport { + pub profiles: Vec, + pub ignored: Vec, + pub source: PathBuf, + /// Whether `source` itself could be opened. An `Include` that matches + /// nothing is ordinary — most of these files carry one for a directory + /// that may or may not exist — but a root that cannot be read is the whole + /// import, and the two are indistinguishable in `profiles`, which comes + /// back empty either way. + pub source_read: bool, + /// `source` plus every `Include` that resolved to a file that was read, + /// for the log line. Someone who edited the wrong `conf.d` fragment finds + /// out here that tty7 never opened it. + pub files_read: usize, +} + #[allow(dead_code)] pub fn import_profiles() -> Vec { let Some(home) = home_dir() else { @@ -172,11 +211,40 @@ pub fn import_profiles() -> Vec { } pub fn import_profiles_from(root: PathBuf, home: &Path) -> Vec { - let blocks = parse_config_blocks(root, home); + profiles_from_blocks(&parse_config(&root, home).blocks) +} +pub fn import_report() -> ImportReport { + let Some(home) = home_dir() else { + // Without a home directory there is no path to have failed at, and the + // caller still has to name one. `~/.ssh/config` is the name the button + // itself uses, so it is the name the failure uses too. + return ImportReport { + profiles: Vec::new(), + ignored: Vec::new(), + source: PathBuf::from("~/.ssh/config"), + source_read: false, + files_read: 0, + }; + }; + import_report_from(home.join(".ssh/config"), &home) +} + +pub fn import_report_from(root: PathBuf, home: &Path) -> ImportReport { + let parsed = parse_config(&root, home); + ImportReport { + profiles: profiles_from_blocks(&parsed.blocks), + ignored: ignored_options(&parsed.blocks), + source: root, + source_read: parsed.root_read, + files_read: parsed.files_read, + } +} + +fn profiles_from_blocks(blocks: &[HostBlock]) -> Vec { let mut aliases: Vec = Vec::new(); let mut seen = HashSet::new(); - for block in &blocks { + for block in blocks { for pat in &block.patterns { if concrete_host_alias(pat) && seen.insert(pat.clone()) { aliases.push(pat.clone()); @@ -188,7 +256,7 @@ pub fn import_profiles_from(root: PathBuf, home: &Path) -> Vec aliases .into_iter() .map(|alias| { - let resolved = resolve_alias(&alias, &blocks); + let resolved = resolve_alias(&alias, blocks); let mut profile = ManagedProfile::new(alias.clone()); profile.group = Some(IMPORTED_GROUP.to_string()); let proxy_jump = apply_resolved(&mut profile, &alias, resolved); @@ -216,7 +284,7 @@ pub fn resolve_alias_to_profile_from( home: &Path, alias: &str, ) -> Option { - let blocks = parse_config_blocks(root, home); + let blocks = parse_config(&root, home).blocks; let matched = blocks.iter().any(|block| block_matches(block, alias)); let resolved = resolve_alias(alias, &blocks); if !matched && resolved.hostname.is_none() { @@ -262,8 +330,26 @@ fn apply_resolved(profile: &mut ManagedProfile, alias: &str, r: ResolvedHost) -> r.proxy_jump } +/// What a merge did, in the three counts the person who pressed the button is +/// owed: a host that arrived, one whose details moved, and one that already +/// said the right thing. +/// +/// The third is the one worth having. Importing the same unedited file twice +/// changes nothing — the suite pins that — and a report that called those +/// hosts "updated" would be the old silence with a number on it. +#[derive(Clone, Copy, Debug, Default, PartialEq, Eq)] +pub struct MergeStats { + pub added: usize, + pub updated: usize, + pub unchanged: usize, +} + #[allow(dead_code)] -pub fn merge_imported(existing: &mut Vec, imported: Vec) { +pub fn merge_imported( + existing: &mut Vec, + imported: Vec, +) -> MergeStats { + let mut stats = MergeStats::default(); let mut jump_targets: Vec<(String, String)> = Vec::new(); for entry in imported { @@ -276,6 +362,20 @@ pub fn merge_imported(existing: &mut Vec, imported: Vec { + // Asked before the assignments below, and only about the fields + // they write: afterwards every one of them agrees by + // construction, so the answer would always be "updated". + let changed = current.host != profile.host + || current.port != profile.port + || current.user != profile.user + || current.identity_files != profile.identity_files + || current.proxy_command != profile.proxy_command + || current.agent_forward != profile.agent_forward; + if changed { + stats.updated += 1; + } else { + stats.unchanged += 1; + } current.host = profile.host; current.port = profile.port; current.user = profile.user; @@ -283,7 +383,10 @@ pub fn merge_imported(existing: &mut Vec, imported: Vec existing.push(profile), + None => { + stats.added += 1; + existing.push(profile); + } } } @@ -299,6 +402,8 @@ pub fn merge_imported(existing: &mut Vec, imported: Vec Option { struct HostBlock { patterns: Vec, - options: Vec<(String, String)>, + options: Vec, +} + +/// One `Keyword value` line, kept twice over: `key` is what the resolver +/// matches on, `spelled` is what the import report shows a human. +struct ConfigOption { + key: String, + spelled: String, + value: String, } #[derive(Default)] @@ -338,30 +451,46 @@ struct ResolvedHost { forwards: Vec, } -fn parse_config_blocks(root: PathBuf, home: &Path) -> Vec { - let mut blocks = Vec::new(); - let mut seen = HashSet::new(); - parse_config_file(&root, home, 0, &mut blocks, &mut seen); - blocks +struct ParsedConfig { + blocks: Vec, + root_read: bool, + files_read: usize, } +fn parse_config(root: &Path, home: &Path) -> ParsedConfig { + let mut blocks = Vec::new(); + let mut seen = HashSet::new(); + let mut files_read = 0; + let root_read = parse_config_file(root, home, 0, &mut blocks, &mut seen, &mut files_read); + ParsedConfig { + blocks, + root_read, + files_read, + } +} + +/// Returns whether this file was read, which only the root's answer is worth +/// anything: an include that resolves to nothing is a normal config, a root +/// that does not is a failed import. fn parse_config_file( path: &Path, home: &Path, depth: usize, blocks: &mut Vec, seen: &mut HashSet, -) { + files_read: &mut usize, +) -> bool { if depth > MAX_INCLUDE_DEPTH || seen.len() >= MAX_CONFIG_FILES { - return; + return false; } let path = expand_path(path, home); if !seen.insert(path.clone()) { - return; + return false; } let Ok(text) = std::fs::read_to_string(&path) else { - return; + return false; }; + *files_read += 1; let base = path.parent().unwrap_or(home).to_path_buf(); let mut current: Option = None; @@ -403,11 +532,15 @@ fn parse_config_file( } for token in split_words(rest) { for include in expand_include(&token, &base, home) { - parse_config_file(&include, home, depth + 1, blocks, seen); + parse_config_file(&include, home, depth + 1, blocks, seen, files_read); } } } else if !in_match { - let opt = (key.to_ascii_lowercase(), rest.to_string()); + let opt = ConfigOption { + key: key.to_ascii_lowercase(), + spelled: key.to_string(), + value: rest.to_string(), + }; match current.as_mut() { Some(block) => block.options.push(opt), None => global @@ -426,6 +559,91 @@ fn parse_config_file( if let Some(block) = global.take() { blocks.push(block); } + true +} + +/// Group the keywords no `SshProfile` field can hold, by keyword, over the +/// blocks that name at least one host tty7 is actually importing. +/// +/// Grouping happens here rather than in `resolve_alias` because `resolve_alias` +/// runs once per alias: an option set under a pattern that matches ten hosts +/// would be reported ten times, and the `Host *` block would drag every +/// keyword in the file into the report for hosts that never set it. +/// +/// `Host *` and `Match` blocks are left out for the same reason. A shared +/// config's `SendEnv LANG` at the top is not something the import dropped from +/// anyone's host; it is a line about hosts tty7 was never asked to import. +fn ignored_options(blocks: &[HostBlock]) -> Vec { + let mut by_keyword: BTreeMap<&str, IgnoredOption> = BTreeMap::new(); + for block in blocks { + let mut hosts: Vec<&String> = block + .patterns + .iter() + .filter(|pat| concrete_host_alias(pat)) + .collect(); + if hosts.is_empty() { + continue; + } + hosts.sort(); + hosts.dedup(); + for opt in &block.options { + if option_is_supported(&opt.key) { + continue; + } + // Keyed on the lowercased form so `IdentityAgent` under one host + // and `identityagent` under another are one entry; the spelling + // shown is the first one the file uses. + by_keyword + .entry(&opt.key) + .or_insert_with(|| IgnoredOption { + option: opt.spelled.clone(), + hosts: Vec::new(), + }) + .hosts + .extend(hosts.iter().map(|host| (*host).clone())); + } + } + by_keyword + .into_values() + .map(|mut entry| { + entry.hosts.sort(); + entry.hosts.dedup(); + entry + }) + .collect() +} + +/// The keywords `resolve_alias` below knows how to carry into an `SshProfile`. +/// +/// A list of its own rather than a second one written out inside +/// `ignored_options`, because the report and the resolver have to answer the +/// same question and a hand-copied pair of lists is a thing that drifts. +/// `every_supported_keyword_is_kept` sets all twenty of these in one config and +/// fails if any of them comes back in the report as dropped. +fn option_is_supported(key: &str) -> bool { + matches!( + key, + "hostname" + | "user" + | "port" + | "identityfile" + | "proxyjump" + | "proxycommand" + | "forwardagent" + | "connecttimeout" + | "serveraliveinterval" + | "serveralivecountmax" + | "ciphers" + | "macs" + | "kexalgorithms" + | "hostkeyalgorithms" + | "compression" + | "forwardx11" + | "stricthostkeychecking" + | "localforward" + | "remoteforward" + | "dynamicforward" + ) } fn resolve_alias(alias: &str, blocks: &[HostBlock]) -> ResolvedHost { @@ -434,7 +652,10 @@ fn resolve_alias(alias: &str, blocks: &[HostBlock]) -> ResolvedHost { if !block_matches(block, alias) { continue; } - for (key, val) in &block.options { + for ConfigOption { + key, value: val, .. + } in &block.options + { match key.as_str() { "hostname" if r.hostname.is_none() => { r.hostname = first_word(val); @@ -754,7 +975,15 @@ mod tests { let prod_id = existing[0].id; let imported = import_profiles_from(ssh.join("config"), &root); - merge_imported(&mut existing, imported); + let stats = merge_imported(&mut existing, imported); + assert_eq!( + stats, + MergeStats { + added: 1, + updated: 1, + unchanged: 0 + } + ); assert_eq!(existing.len(), 2); let prod = existing.iter().find(|p| p.name == "prod").unwrap(); @@ -769,8 +998,18 @@ mod tests { let snapshot = existing.clone(); let imported_again = import_profiles_from(ssh.join("config"), &root); - merge_imported(&mut existing, imported_again); + let stats = merge_imported(&mut existing, imported_again); assert_eq!(existing, snapshot); + // The same invariant the line above pins, said in the words the button + // reports: a second import of an unedited file updates nothing. + assert_eq!( + stats, + MergeStats { + added: 0, + updated: 0, + unchanged: 2 + } + ); } #[test] @@ -947,6 +1186,117 @@ mod tests { assert_eq!(bastion.profile.user, "jumper"); } + #[test] + fn report_groups_ignored_options_by_keyword_as_the_file_spells_them() { + let root = temp_root("report-ignored"); + let ssh = root.join(".ssh"); + std::fs::create_dir_all(&ssh).unwrap(); + std::fs::write( + ssh.join("config"), + concat!( + "Host prod web\n", + " HostName 10.0.0.5\n", + " IdentityAgent /run/agent.sock\n", + " CertificateFile ~/.ssh/id-cert.pub\n", + "Host db\n", + " identityagent /run/other.sock\n", + " AddKeysToAgent yes\n", + "Host *\n", + " SendEnv LANG\n", + "Match host prod\n", + " PKCS11Provider /usr/lib/x.so\n", + ), + ) + .unwrap(); + + let report = import_report_from(ssh.join("config"), &root); + assert!(report.source_read); + assert_eq!(report.files_read, 1); + let ignored: Vec<(&str, Vec<&str>)> = report + .ignored + .iter() + .map(|opt| { + ( + opt.option.as_str(), + opt.hosts.iter().map(String::as_str).collect(), + ) + }) + .collect(); + assert_eq!( + ignored, + vec![ + ("AddKeysToAgent", vec!["db"]), + ("CertificateFile", vec!["prod", "web"]), + // One entry despite the two spellings, under the first of them, + // and one entry per host rather than one per `Host` line. + ("IdentityAgent", vec!["db", "prod", "web"]), + ] + ); + } + + #[test] + fn every_supported_keyword_is_kept() { + let root = temp_root("report-supported"); + let ssh = root.join(".ssh"); + std::fs::create_dir_all(&ssh).unwrap(); + std::fs::write( + ssh.join("config"), + concat!( + "Host everything\n", + " HostName 10.0.0.5\n", + " User deploy\n", + " Port 2222\n", + " IdentityFile ~/.ssh/id_prod\n", + " ProxyJump bastion\n", + " ProxyCommand nc %h %p\n", + " ForwardAgent yes\n", + " ConnectTimeout 15\n", + " ServerAliveInterval 30\n", + " ServerAliveCountMax 4\n", + " Ciphers aes256-ctr\n", + " MACs hmac-sha2-256\n", + " KexAlgorithms curve25519-sha256\n", + " HostKeyAlgorithms ssh-ed25519\n", + " Compression yes\n", + " ForwardX11 no\n", + " StrictHostKeyChecking no\n", + " LocalForward 8080 localhost:80\n", + " RemoteForward 9000 127.0.0.1:3000\n", + " DynamicForward 1080\n", + ), + ) + .unwrap(); + + let report = import_report_from(ssh.join("config"), &root); + assert_eq!(report.ignored, Vec::new()); + } + + #[test] + fn report_says_when_the_root_config_could_not_be_read() { + let root = temp_root("report-missing"); + let missing = root.join(".ssh/config"); + + let report = import_report_from(missing.clone(), &root); + assert_eq!(report.source, missing); + assert!(!report.source_read); + assert_eq!(report.files_read, 0); + assert!(report.profiles.is_empty()); + } + + #[test] + fn a_wildcard_only_config_was_still_read() { + let root = temp_root("report-wildcard"); + let ssh = root.join(".ssh"); + std::fs::create_dir_all(&ssh).unwrap(); + std::fs::write(ssh.join("config"), "Host *\n User fallback\n").unwrap(); + + let report = import_report_from(ssh.join("config"), &root); + assert!(report.source_read); + assert_eq!(report.files_read, 1); + assert!(report.profiles.is_empty()); + assert!(report.ignored.is_empty()); + } + fn temp_root(name: &str) -> PathBuf { let dir = std::env::temp_dir().join(format!( "tty7-ssh-config-test-{name}-{}", diff --git a/src/terminal/remote.rs b/src/terminal/remote.rs index 4077ca3f..8f4d1e0e 100644 --- a/src/terminal/remote.rs +++ b/src/terminal/remote.rs @@ -1489,21 +1489,26 @@ impl RemoteTerminal { query(job_id).unwrap_or_default() } - pub fn sftp_transfer_list(pane_id: u64) -> Vec { - fn query(pane_id: u64) -> anyhow::Result> { + /// A failed poll is not an empty transfer list: the caller has to be able + /// to keep the jobs it already knows about, so this reports the failure + /// the way `sftp_list` does rather than answering with an empty `Vec`. + pub fn sftp_transfer_list(pane_id: u64) -> Result, String> { + fn query(pane_id: u64) -> anyhow::Result, String>> { let mut stream = connect()?; ClientMsg::SftpTransferList { pane_id }.encode(&mut stream)?; - match DaemonMsg::read(&mut stream)? { + Ok(match DaemonMsg::read(&mut stream)? { DaemonMsg::SftpTransferProgress(jobs) => Ok(jobs), - other => Err(anyhow::anyhow!( - "unexpected reply to SftpTransferList: {other:?}" - )), - } + DaemonMsg::Error(msg) => Err(msg), + other => Err(format!("unexpected reply to SftpTransferList: {other:?}")), + }) } - query(pane_id).unwrap_or_default() + query(pane_id).unwrap_or_else(|e| Err(e.to_string())) } - pub fn add_forward(pane_id: u64, rule: SshForwardRule) -> Vec { + /// `None` when the request never got a list back — which is not the same + /// as getting an empty one, because only the caller of a *failed* request + /// still has to keep showing what it had. + pub fn add_forward(pane_id: u64, rule: SshForwardRule) -> Option> { fn query(pane_id: u64, rule: SshForwardRule) -> anyhow::Result> { let mut stream = connect()?; ClientMsg::AddForward { pane_id, rule }.encode(&mut stream)?; @@ -1513,10 +1518,13 @@ impl RemoteTerminal { other => Err(anyhow::anyhow!("unexpected reply to AddForward: {other:?}")), } } - query(pane_id, rule).unwrap_or_default() + query(pane_id, rule) + .inspect_err(|e| log::warn!("AddForward failed: {e}")) + .ok() } - pub fn remove_forward(pane_id: u64, forward_id: u64) -> Vec { + /// `None` when the request never got a list back — see `add_forward`. + pub fn remove_forward(pane_id: u64, forward_id: u64) -> Option> { fn query(pane_id: u64, forward_id: u64) -> anyhow::Result> { let mut stream = connect()?; ClientMsg::RemoveForward { @@ -1531,7 +1539,9 @@ impl RemoteTerminal { )), } } - query(pane_id, forward_id).unwrap_or_default() + query(pane_id, forward_id) + .inspect_err(|e| log::warn!("RemoveForward failed: {e}")) + .ok() } pub fn list_forwards(pane_id: u64) -> Vec { diff --git a/src/terminal/view.rs b/src/terminal/view.rs index 401124ee..9f741f42 100644 --- a/src/terminal/view.rs +++ b/src/terminal/view.rs @@ -2654,6 +2654,9 @@ impl TerminalView { cx.background_spawn(async move { route.transfer_list() }) .await }; + // A poll that failed says nothing about the job — keep asking + // until it answers or the budget above runs out. + let Ok(listed) = listed else { continue }; let Some(progress) = listed.into_iter().find(|j| j.job_id == job) else { // Pruned after the retention window, or the daemon restarted: // there is nothing left to report either way. diff --git a/src/ui/app.rs b/src/ui/app.rs index f3336c97..e24a78e4 100644 --- a/src/ui/app.rs +++ b/src/ui/app.rs @@ -27,6 +27,7 @@ use crate::core::window_state::{WindowGeometry as _, WindowState}; use crate::daemon::protocol::{RemoteContext, ShellSpec, ssh_option_takes_value}; use crate::daemon::spawn::DaemonMismatch; use crate::terminal::view::{ChildExited, TerminalView}; +use crate::ui::forwards::{ForwardFields, added_forward, rule_of}; use crate::ui::host_registry::HostId; use crate::ui::i18n::{L10nKey, set_locale, t, t_fmt, t_plural}; use crate::ui::palette::{ @@ -398,7 +399,13 @@ pub(crate) struct LoopbackForwardPanelState { pub(crate) mf_target_host: Entity, pub(crate) mf_target_port: Entity, pub(crate) mf_description: Entity, - pub(crate) mf_editing: Option, + /// The rule the form is editing, whole rather than by id: an edit that + /// fails has to be able to put back what it took out, and the id alone + /// cannot describe the rule it named. + pub(crate) mf_editing: Option, + /// Why the last Add or Save did not take, in the far side's own words. + /// Cleared the moment the form is closed or the edit is abandoned. + pub(crate) mf_error: Option, } pub struct Tty7App { @@ -985,6 +992,7 @@ impl Tty7App { mf_target_port, mf_description, mf_editing: None, + mf_error: None, }, sftp_panel, right_panel: Default::default(), @@ -2196,74 +2204,105 @@ impl Tty7App { cx.notify(); } + /// The managed-forward form's fields as plain text, for the two callers + /// that have to agree on what they add up to. + pub(crate) fn managed_forward_fields(&self, cx: &gpui::App) -> ForwardFields { + let val = |input: &Entity| input.read(cx).value().to_string(); + ForwardFields { + kind: self.loopback_panel.mf_kind, + bind_host: val(&self.loopback_panel.mf_bind_host), + bind_port: val(&self.loopback_panel.mf_bind_port), + target_host: val(&self.loopback_panel.mf_target_host), + target_port: val(&self.loopback_panel.mf_target_port), + description: val(&self.loopback_panel.mf_description), + } + } + pub(crate) fn add_managed_forward( &mut self, pane_id: u64, window: &mut Window, cx: &mut Context, ) { - use crate::daemon::protocol::{SshForwardKind, SshForwardRule}; - let kind = self.loopback_panel.mf_kind; - let bind_host = self - .loopback_panel - .mf_bind_host - .read(cx) - .value() - .trim() - .to_string(); - let bind_host = if bind_host.is_empty() { - "127.0.0.1".to_string() - } else { - bind_host - }; - let Ok(bind_port) = self - .loopback_panel - .mf_bind_port - .read(cx) - .value() - .trim() - .parse::() - else { + use crate::daemon::protocol::ForwardStatus; + + let Some(rule) = self.managed_forward_fields(cx).collect() else { + // Add is disabled while the fields do not make a rule and the form + // already says what is missing, so there is nothing to do here and + // nothing left to explain. return; }; - let target_host = self - .loopback_panel - .mf_target_host - .read(cx) - .value() - .trim() - .to_string(); - let target_port = self - .loopback_panel - .mf_target_port - .read(cx) - .value() - .trim() - .parse::() - .unwrap_or(0); - if kind != SshForwardKind::Dynamic && (target_host.is_empty() || target_port == 0) { - return; - } - let description = self - .loopback_panel - .mf_description - .read(cx) - .value() - .trim() - .to_string(); - let rule = SshForwardRule { - kind, - bind_host, - bind_port, - target_host, - target_port, - description: (!description.is_empty()).then_some(description), - }; let route = self.forward_route(pane_id, cx); - if let Some(old_id) = self.loopback_panel.mf_editing.take() { - let _ = route.remove(old_id); + let previous = self.loopback_panel.mf_editing.clone(); + // A saved edit is a replace, and the rule being replaced has to come + // out first: the ordinary edit keeps the bind port, and the far side + // really does bind it, so adding first would collide with the very + // rule it is replacing and fail every edit that only renames a rule or + // moves its target. + if let Some(old) = &previous { + let Some(list) = route.remove(old.id) else { + // Nothing came back, so what the far side still has is + // unknown — most likely the old rule, still listening. Adding + // on top of that would collide with it, and putting it back + // afterwards would leave two of it. Stop while nothing has + // changed. + self.loopback_panel.mf_error = Some(t(L10nKey::ForwardRequestFailed).to_string()); + cx.notify(); + return; + }; + self.loopback_panel.managed = list; } - self.loopback_panel.managed = route.add(rule); + + let before: Vec = self.loopback_panel.managed.iter().map(|m| m.id).collect(); + let mut failure = None; + match route.add(rule) { + // The request never got an answer. An empty list here is not "this + // pane has no forwards", it is "nobody said" — assigning it is what + // used to blank the panel on a dropped connection. + None => failure = Some(t(L10nKey::ForwardRequestFailed).to_string()), + Some(list) => { + // A rule that could not be started is registered all the same, + // with the reason in its status, so whether the add worked is a + // question about the entry it appended rather than about + // whether the call returned. + let broken = added_forward(&before, &list).and_then(|added| match &added.status { + ForwardStatus::Error(msg) => Some((added.id, msg.clone())), + ForwardStatus::Listening => None, + }); + self.loopback_panel.managed = list; + if let Some((id, msg)) = broken { + if let Some(list) = route.remove(id) { + self.loopback_panel.managed = list; + } + failure = Some(msg); + } + } + } + + if let Some(msg) = failure { + // Put back what the edit took out, so the worst a failed Save can + // do is leave everything exactly as it was — with the form still + // open on the rule and the reason underneath it. + if let Some(old) = &previous { + let before: Vec = self.loopback_panel.managed.iter().map(|m| m.id).collect(); + if let Some(list) = route.add(rule_of(old)) { + // The rule comes back under a new id and the form is still + // editing it, so the form has to be pointed at the entry + // that now exists — otherwise the next Save would remove + // an id nobody has and add a second copy of the rule. + if let Some(restored) = added_forward(&before, &list) { + self.loopback_panel.mf_editing = Some(restored.clone()); + } + self.loopback_panel.managed = list; + } + } + self.loopback_panel.mf_error = Some(msg); + cx.notify(); + return; + } + + self.loopback_panel.mf_editing = None; + self.loopback_panel.mf_error = None; self.loopback_panel.form_pane_id = None; for input in [ &self.loopback_panel.mf_bind_port, @@ -2283,8 +2322,8 @@ impl Tty7App { cx: &mut Context, ) { self.loopback_panel.mf_kind = forward.kind; - self.loopback_panel.mf_editing = Some(forward.id); self.loopback_panel.form_pane_id = Some(forward.pane_id); + self.loopback_panel.mf_error = None; let target_port = if forward.target_port == 0 { String::new() } else { @@ -2309,6 +2348,7 @@ impl Tty7App { for (input, value) in fields { input.update(cx, |input, cx| input.set_value(&value, window, cx)); } + self.loopback_panel.mf_editing = Some(forward); cx.notify(); } @@ -2318,6 +2358,7 @@ impl Tty7App { cx: &mut Context, ) { self.loopback_panel.mf_editing = None; + self.loopback_panel.mf_error = None; for input in [ &self.loopback_panel.mf_bind_port, &self.loopback_panel.mf_target_host, @@ -2338,7 +2379,12 @@ impl Tty7App { forward_id: u64, cx: &mut Context, ) { - self.loopback_panel.managed = self.forward_route(pane_id, cx).remove(forward_id); + // Only what the far side actually answered with. A request that never + // got a reply knows nothing about the remaining forwards, and writing + // its empty list into the panel would blank a list that is still there. + if let Some(list) = self.forward_route(pane_id, cx).remove(forward_id) { + self.loopback_panel.managed = list; + } cx.notify(); } @@ -5910,18 +5956,26 @@ impl ForwardRoute { ) } + /// The list a forward request answered with, or `None` when it did not + /// answer at all. + /// + /// The two are not the same and the panel has to be able to tell them + /// apart: an empty list is a pane with no forwards left, while a request + /// that failed says nothing about what the far side still has. Reporting + /// the second as the first is what blanked the panel whenever the daemon + /// was briefly unreachable. fn forwards( reply: anyhow::Result, - ) -> Vec { + ) -> Option> { match reply { - Ok(crate::daemon::protocol::DaemonMsg::ForwardList(list)) => list, + Ok(crate::daemon::protocol::DaemonMsg::ForwardList(list)) => Some(list), Ok(other) => { log::warn!("unexpected reply to a workspace forward request: {other:?}"); - Vec::new() + None } Err(e) => { log::warn!("a workspace forward request failed: {e}"); - Vec::new() + None } } } @@ -5931,13 +5985,13 @@ impl ForwardRoute { else { return crate::terminal::RemoteTerminal::list_forwards(self.pane_id); }; - Self::forwards(crate::terminal::RemoteTerminal::on_workspace(req)) + Self::forwards(crate::terminal::RemoteTerminal::on_workspace(req)).unwrap_or_default() } pub(crate) fn add( &self, rule: crate::daemon::protocol::SshForwardRule, - ) -> Vec { + ) -> Option> { let Some(req) = self .workspace_op(crate::daemon::protocol::WorkspaceOp::AddForward { rule: rule.clone() }) else { @@ -5951,10 +6005,13 @@ impl ForwardRoute { else { return Vec::new(); }; - Self::forwards(crate::terminal::RemoteTerminal::on_workspace(req)) + Self::forwards(crate::terminal::RemoteTerminal::on_workspace(req)).unwrap_or_default() } - pub(crate) fn remove(&self, forward_id: u64) -> Vec { + pub(crate) fn remove( + &self, + forward_id: u64, + ) -> Option> { let Some(req) = self.workspace_op(crate::daemon::protocol::WorkspaceOp::RemoveForward { forward_id }) else { @@ -8148,3 +8205,97 @@ mod rename_gpui_tests { }); } } + +// A test window has no daemon behind it — its socket path is under the pinned +// test config dir and nothing is listening on it — so every forward request +// fails. That is exactly the case these are about: what the panel and the form +// are left holding when the far side does not answer. +#[cfg(all(test, unix))] +mod managed_forward_gpui_tests { + use gpui::TestAppContext; + use gpui_component::input::InputState; + + use crate::daemon::protocol::{ForwardStatus, ManagedForward, SshForwardKind}; + use crate::ui::app::test_window::harness_with_tabs; + + fn listening(id: u64) -> ManagedForward { + ManagedForward { + id, + pane_id: 1, + kind: SshForwardKind::Local, + bind_host: "127.0.0.1".to_string(), + bind_port: 8080, + target_host: "10.0.0.5".to_string(), + target_port: 80, + description: None, + status: ForwardStatus::Listening, + } + } + + #[gpui::test] + fn an_add_that_never_reaches_the_session_leaves_the_panel_as_it_was(cx: &mut TestAppContext) { + let (app, mut vcx, _streams) = harness_with_tabs(cx, 1); + + app.update_in(&mut vcx, |app, window, cx| { + app.loopback_panel.managed = vec![listening(1)]; + app.loopback_panel.form_pane_id = Some(1); + let typed: [(&gpui::Entity, &str); 3] = [ + (&app.loopback_panel.mf_bind_port, "9000"), + (&app.loopback_panel.mf_target_host, "127.0.0.1"), + (&app.loopback_panel.mf_target_port, "22"), + ]; + for (input, value) in typed { + input.update(cx, |input, cx| input.set_value(value, window, cx)); + } + + app.add_managed_forward(1, window, cx); + + assert_eq!( + app.loopback_panel.managed.len(), + 1, + "a request that failed says nothing about the forwards that are up" + ); + assert!( + app.loopback_panel.mf_error.is_some(), + "and the form has to say why the Add did nothing" + ); + assert_eq!( + app.loopback_panel.form_pane_id, + Some(1), + "the form stays open on what was typed" + ); + }); + } + + #[gpui::test] + fn a_save_that_cannot_be_made_leaves_the_rule_it_would_replace_alone(cx: &mut TestAppContext) { + let (app, mut vcx, _streams) = harness_with_tabs(cx, 1); + + app.update_in(&mut vcx, |app, window, cx| { + app.loopback_panel.managed = vec![listening(1)]; + app.loopback_panel.form_pane_id = Some(1); + app.loopback_panel.mf_editing = Some(listening(1)); + let typed: [(&gpui::Entity, &str); 3] = [ + (&app.loopback_panel.mf_bind_port, "8080"), + (&app.loopback_panel.mf_target_host, "10.0.0.6"), + (&app.loopback_panel.mf_target_port, "80"), + ]; + for (input, value) in typed { + input.update(cx, |input, cx| input.set_value(value, window, cx)); + } + + app.add_managed_forward(1, window, cx); + + assert_eq!( + app.loopback_panel.managed, + vec![listening(1)], + "the rule being edited must survive an edit that could not be made" + ); + assert!( + app.loopback_panel.mf_editing.is_some(), + "the form is still editing it" + ); + assert!(app.loopback_panel.mf_error.is_some()); + }); + } +} diff --git a/src/ui/file_tree.rs b/src/ui/file_tree.rs index 08a14a1f..2b0057db 100644 --- a/src/ui/file_tree.rs +++ b/src/ui/file_tree.rs @@ -66,6 +66,9 @@ pub(crate) enum TreeNote { /// The search stopped at `SEARCH_LIMIT`; the list is a prefix, not the /// whole answer, and has to say so. SearchCapped, + /// The search never ran to an answer — the host refused it or the link to + /// it went away. An empty list here means nothing at all. + SearchFailed, } /// `landed` is how many entries the listing returned, or `None` when nothing @@ -122,6 +125,10 @@ struct SearchState { pending: String, hidden: bool, hits: Vec, + /// Whether the last search came back as a failure rather than as no hits. + /// The two used to print the same "Nothing matches …", which is the same + /// lie `unreadable` was added to stop a directory listing from telling. + failed: bool, } impl SearchState { @@ -134,22 +141,25 @@ impl SearchState { self.hidden = show_hidden; if query.is_empty() { self.hits.clear(); + self.failed = false; return None; } Some(self.generation) } - fn accept(&mut self, generation: u64, hits: Vec) -> bool { + fn accept(&mut self, generation: u64, ok: bool, hits: Vec) -> bool { if self.generation != generation { return false; } self.hits = hits; + self.failed = !ok; true } fn restart(&mut self) { self.generation += 1; self.pending.clear(); + self.failed = false; } } @@ -438,7 +448,14 @@ impl FileTreeState { host, cx, move |h| { - h.search(&roots, &query, SEARCH_LIMIT, SEARCH_MAX_DIRS, show_hidden) + // `(ok, hits)` the way `spawn_load` reports a listing: + // a search the host refused is not a search with no + // hits, and the column has to be able to tell them + // apart before it says "Nothing matches". + let found = + h.search(&roots, &query, SEARCH_LIMIT, SEARCH_MAX_DIRS, show_hidden); + let ok = found.is_ok(); + let hits = found .unwrap_or_default() .into_iter() .map(|hit| TreeEntry { @@ -447,10 +464,11 @@ impl FileTreeState { is_dir: hit.is_dir, ignored: hit.ignored, }) - .collect::>() + .collect::>(); + (ok, hits) }, - move |app, hits, cx| { - if app.file_tree.search.accept(generation, hits) { + move |app, (ok, hits), cx| { + if app.file_tree.search.accept(generation, ok, hits) { cx.notify(); } }, @@ -461,35 +479,51 @@ impl FileTreeState { } fn search_rows(&self) -> Vec { - let mut rows: Vec = self - .search - .hits - .iter() - .map(|e| TreeRow { - entry: e.clone(), - depth: 0, - is_root: false, - expanded: false, - note: None, - }) - .collect(); - if rows.len() >= SEARCH_LIMIT { - rows.push(TreeRow { - entry: TreeEntry { - name: String::new(), - path: PathBuf::new(), - is_dir: false, - ignored: false, - }, - depth: 0, - is_root: false, - expanded: false, - note: Some(TreeNote::SearchCapped), - }); - } - rows + search_rows(&self.search) } +} +/// The rows a search puts in the column, and the note that stands for whatever +/// they do not say by themselves. +fn search_rows(search: &SearchState) -> Vec { + let mut rows: Vec = search + .hits + .iter() + .map(|e| TreeRow { + entry: e.clone(), + depth: 0, + is_root: false, + expanded: false, + note: None, + }) + .collect(); + let note = if search.failed { + // Ahead of the cap: a failed search has no hits to have capped, and + // this is the one thing worth saying about it. + Some(TreeNote::SearchFailed) + } else if rows.len() >= SEARCH_LIMIT { + Some(TreeNote::SearchCapped) + } else { + None + }; + if let Some(note) = note { + rows.push(TreeRow { + entry: TreeEntry { + name: String::new(), + path: PathBuf::new(), + is_dir: false, + ignored: false, + }, + depth: 0, + is_root: false, + expanded: false, + note: Some(note), + }); + } + rows +} + +impl FileTreeState { pub(crate) fn visible_rows( &self, host: HostId, @@ -1116,6 +1150,20 @@ impl Tty7App { } let target = new_path.clone(); + // The same gap `file_tree_delete` closed: on its own, "Permission + // denied (os error 13)" says neither which file nor what was being + // done to it, and those are the only two things worth knowing here. + // A rename is named by the name it is leaving, which is the one still + // on screen to find. + let (context, failed_name) = match &edit { + TreeEdit::Rename { path, .. } => ( + L10nKey::FileTreeRenameFailed, + path.file_name() + .map(|n| n.to_string_lossy().to_string()) + .unwrap_or_else(|| name.clone()), + ), + _ => (L10nKey::FileTreeCreateFailed, name.clone()), + }; HostOps::run_in( host, window, @@ -1141,8 +1189,12 @@ impl Tty7App { { code.selected = None; } - use gpui_component::WindowExt as _; - window.push_notification(format!("{e}"), cx); + HostOps::notify_err( + window, + cx, + &t_fmt(context, &[("name", &failed_name)]), + &e, + ); } } cx.notify(); @@ -1503,6 +1555,7 @@ impl Tty7App { TreeNote::HiddenOnly => (L10nKey::TreeDirHiddenOnly, muted), TreeNote::Unreadable => (L10nKey::TreeDirUnreadable, cx.theme().danger), TreeNote::SearchCapped => (L10nKey::TreeSearchCapped, muted), + TreeNote::SearchFailed => (L10nKey::TreeSearchFailed, cx.theme().danger), }; return vec![ h_flex() @@ -1519,9 +1572,10 @@ impl Tty7App { TreeNote::SearchCapped => t_fmt(key, &[("n", &SEARCH_LIMIT.to_string())]), _ => t(key).to_string(), }) - // Every note but the capped-search one stands for a real - // directory, and carries its path; that one stands for the - // rest of a search and has nowhere to put anything. + // Every note but the two search ones stands for a real + // directory, and carries its path; those stand for the rest + // of a search, or for one that never ran, and have nowhere + // to put anything. .when(!path.as_os_str().is_empty(), |d| { d.drag_over::(|s, _, _, cx| { s.bg(cx.theme().drag_border.opacity(0.14)) @@ -2058,6 +2112,28 @@ mod tests { ); } + #[test] + fn a_failed_search_says_so_instead_of_drawing_no_rows() { + let mut search = SearchState::default(); + let walk = search.retarget("foo", false).expect("a new query walks"); + search.accept(walk, false, Vec::new()); + assert_eq!( + search_rows(&search) + .iter() + .filter_map(|r| r.note) + .collect::>(), + vec![TreeNote::SearchFailed], + "without a note the column falls through to \"Nothing matches\"" + ); + + // A search that ran and found nothing still draws nothing. + let walk = search + .retarget("bar", false) + .expect("a changed query walks"); + search.accept(walk, true, Vec::new()); + assert!(search_rows(&search).is_empty()); + } + #[test] fn a_listing_superseded_in_flight_is_still_shown() { let mut loads: InFlight = InFlight::default(); @@ -2426,10 +2502,10 @@ mod tests { assert_ne!(first, second); assert!( - !search.accept(first, vec![entry("stale.rs", false)]), + !search.accept(first, true, vec![entry("stale.rs", false)]), "the overtaken walk's hits are dropped" ); - assert!(search.accept(second, vec![entry("foo.rs", false)])); + assert!(search.accept(second, true, vec![entry("foo.rs", false)])); assert_eq!(search.hits.len(), 1); let third = search @@ -2445,6 +2521,30 @@ mod tests { assert!(search.retarget("foo", true).is_some(), "restart re-walks"); } + #[test] + fn a_search_that_failed_is_not_a_search_with_no_hits() { + let mut search = SearchState::default(); + let walk = search.retarget("foo", false).expect("a new query walks"); + assert!(search.accept(walk, false, Vec::new())); + assert!( + search.failed, + "an empty list from a host that refused the walk is not an answer" + ); + + // And it is not carried past the query it belongs to. + let next = search + .retarget("food", false) + .expect("a changed query walks"); + assert!(search.accept(next, true, vec![entry("food.rs", false)])); + assert!(!search.failed); + + let last = search.retarget("foodie", false).expect("and again"); + assert!(search.accept(last, false, Vec::new())); + assert!(search.failed); + search.retarget("", false); + assert!(!search.failed, "an emptied box has nothing to report"); + } + #[test] fn the_tree_reads_the_same_listing_out_of_the_host() { let host = tty7_core::host::local::LocalHost::new(); diff --git a/src/ui/forwards.rs b/src/ui/forwards.rs index 7bd32be6..a9784dfa 100644 --- a/src/ui/forwards.rs +++ b/src/ui/forwards.rs @@ -1,9 +1,11 @@ use gpui::{AnyElement, Context, Div, Entity, FontWeight, Stateful, div, prelude::*, px, rems}; use gpui_component::button::{Button, ButtonVariants as _}; use gpui_component::input::Input; -use gpui_component::{ActiveTheme as _, Icon, IconName, Sizable as _, h_flex, v_flex}; +use gpui_component::{ + ActiveTheme as _, Disableable as _, Icon, IconName, Sizable as _, h_flex, v_flex, +}; -use crate::daemon::protocol::{ForwardStatus, ManagedForward, SshForwardKind}; +use crate::daemon::protocol::{ForwardStatus, ManagedForward, SshForwardKind, SshForwardRule}; use crate::terminal::view::TerminalView; use crate::ui::app::{CONTENT_INSET, TILE_GLYPH_SM, TILE_SIZE_SM, Tty7App}; use crate::ui::i18n::{L10nKey, t, t_fmt}; @@ -14,6 +16,103 @@ use crate::ui::right_panel::{META, TEXT, TEXT_MONO}; /// draw the same row, so they fade it by the same amount. pub(crate) const NO_TARGET_FADE: f32 = 0.4; +/// The managed-forward form's five text fields, read out of their inputs. +/// +/// Split out so the question "do these make a rule?" can be asked without a +/// `Window` and answered the same way twice: `add_managed_forward` needs the +/// rule, and `forward_form` needs to know whether there is one yet — that is +/// what decides whether Add is live and whether the form says what is missing. +pub(crate) struct ForwardFields { + pub(crate) kind: SshForwardKind, + pub(crate) bind_host: String, + pub(crate) bind_port: String, + pub(crate) target_host: String, + pub(crate) target_port: String, + pub(crate) description: String, +} + +impl ForwardFields { + /// The rule these fields describe, or `None` while they do not describe + /// one yet. + /// + /// The same conditions the settings sheet's `ForwardRuleForm::collect` + /// applies, so a rule typed here and a rule typed there are accepted or + /// refused alike — including port 0, which parses as a `u16` but asks the + /// OS to pick the port, and there is nowhere in either form to say which + /// one it picked. + pub(crate) fn collect(&self) -> Option { + let bind_port: u16 = self.bind_port.trim().parse().ok().filter(|p| *p > 0)?; + let (target_host, target_port) = if self.kind == SshForwardKind::Dynamic { + (String::new(), 0) + } else { + let port: u16 = self.target_port.trim().parse().ok().filter(|p| *p > 0)?; + let host = self.target_host.trim(); + if host.is_empty() { + return None; + } + (host.to_string(), port) + }; + let bind_host = match self.bind_host.trim() { + // The panel's own default, and the one the strip's tooltip + // promises: an empty bind host is loopback, not every interface. + "" => "127.0.0.1".to_string(), + host => host.to_string(), + }; + let description = self.description.trim(); + Some(SshForwardRule { + kind: self.kind, + bind_host, + bind_port, + target_host, + target_port, + description: (!description.is_empty()).then(|| description.to_string()), + }) + } + + /// Whether the form is still empty enough that saying what is missing + /// would be nagging rather than helping — the same restraint the settings + /// sheet shows through `ForwardRuleForm::is_blank`. + pub(crate) fn is_blank(&self) -> bool { + [ + &self.bind_host, + &self.bind_port, + &self.target_host, + &self.target_port, + &self.description, + ] + .iter() + .all(|v| v.trim().is_empty()) + } +} + +/// The entry a forward request just appended: the one the panel did not have +/// before it asked. +/// +/// Ids come from a counter that only goes up, so "none of the ids from before" +/// names the new entry exactly — and it is the new entry that says whether the +/// rule is listening or why it is not. +pub(crate) fn added_forward<'a>( + before: &[u64], + list: &'a [ManagedForward], +) -> Option<&'a ManagedForward> { + list.iter().find(|m| !before.contains(&m.id)) +} + +/// The rule a live forward was made from. +/// +/// An edit removes the old forward before adding the new one, so when the new +/// one will not come up this is what puts the old one back. +pub(crate) fn rule_of(forward: &ManagedForward) -> SshForwardRule { + SshForwardRule { + kind: forward.kind, + bind_host: forward.bind_host.clone(), + bind_port: forward.bind_port, + target_host: forward.target_host.clone(), + target_port: forward.target_port, + description: forward.description.clone(), + } +} + impl Tty7App { pub(crate) fn render_ssh_status_strip( &self, @@ -315,9 +414,17 @@ impl Tty7App { fn forward_form(&self, pane_id: u64, cx: &mut Context) -> Div { let theme = cx.theme(); let muted = theme.muted_foreground; + let danger = theme.danger; let sf = cx.global::().sidebar; let kind = self.loopback_panel.mf_kind; let editing = self.loopback_panel.mf_editing.is_some(); + let fields = self.managed_forward_fields(cx); + // The form used to accept a click on Add and then do nothing at all + // when the fields did not make a rule. Now Add is only live when there + // is something to add, and the line below the form says what is still + // missing — but not while the form has barely been touched. + let complete = fields.collect().is_some(); + let incomplete = !complete && !fields.is_blank(); let selected = match kind { SshForwardKind::Local => 0, SshForwardKind::Remote => 1, @@ -387,6 +494,17 @@ impl Tty7App { )), ) .child(Input::new(&self.loopback_panel.mf_description).xsmall()) + .when(incomplete, |form| { + form.child(div().text_size(rems(META)).text_color(danger).child( + match needs_target { + true => t(L10nKey::SettingsFwdNeedsBoth), + false => t(L10nKey::SettingsFwdNeedsListen), + }, + )) + }) + .when_some(self.loopback_panel.mf_error.clone(), |form, msg| { + form.child(div().text_size(rems(META)).text_color(danger).child(msg)) + }) .child( h_flex() .justify_end() @@ -410,6 +528,7 @@ impl Tty7App { }) .primary() .xsmall() + .disabled(!complete) .on_click(cx.listener(move |this, _, window, cx| { this.add_managed_forward(pane_id, window, cx) })), @@ -417,3 +536,127 @@ impl Tty7App { ) } } + +#[cfg(test)] +mod tests { + use super::*; + + fn fields(kind: SshForwardKind, bind_port: &str, host: &str, port: &str) -> ForwardFields { + ForwardFields { + kind, + bind_host: "127.0.0.1".to_string(), + bind_port: bind_port.to_string(), + target_host: host.to_string(), + target_port: port.to_string(), + description: String::new(), + } + } + + fn managed(id: u64, bind_port: u16) -> ManagedForward { + ManagedForward { + id, + pane_id: 7, + kind: SshForwardKind::Local, + bind_host: "127.0.0.1".to_string(), + bind_port, + target_host: "10.0.0.5".to_string(), + target_port: 80, + description: Some("the staging box".to_string()), + status: ForwardStatus::Listening, + } + } + + #[test] + fn a_complete_local_rule_is_collected() { + let rule = fields(SshForwardKind::Local, "8080", "10.0.0.5", "80") + .collect() + .expect("a bind port and a target make a rule"); + assert_eq!(rule.bind_port, 8080); + assert_eq!(rule.target_host, "10.0.0.5"); + assert_eq!(rule.target_port, 80); + assert_eq!(rule.description, None); + } + + #[test] + fn a_half_typed_rule_is_not_a_rule() { + assert!( + fields(SshForwardKind::Local, "", "10.0.0.5", "80") + .collect() + .is_none() + ); + assert!( + fields(SshForwardKind::Local, "8080", "", "80") + .collect() + .is_none() + ); + assert!( + fields(SshForwardKind::Local, "8080", "10.0.0.5", "") + .collect() + .is_none() + ); + assert!( + fields(SshForwardKind::Local, "http", "10.0.0.5", "80") + .collect() + .is_none(), + "a service name is not a port" + ); + } + + #[test] + fn port_zero_is_refused_rather_than_quietly_ephemeral() { + assert!( + fields(SshForwardKind::Local, "0", "10.0.0.5", "80") + .collect() + .is_none(), + "there is nowhere in this form to say which port the OS picked" + ); + assert!( + fields(SshForwardKind::Local, "8080", "10.0.0.5", "0") + .collect() + .is_none() + ); + } + + #[test] + fn a_socks_proxy_needs_nothing_but_a_port_to_listen_on() { + let rule = fields(SshForwardKind::Dynamic, "1080", "", "") + .collect() + .expect("a dynamic forward has no target"); + assert_eq!(rule.bind_port, 1080); + assert_eq!(rule.target_host, ""); + assert_eq!(rule.target_port, 0); + } + + #[test] + fn an_untouched_form_is_blank_and_a_touched_one_is_not() { + let mut form = fields(SshForwardKind::Local, "", "", ""); + form.bind_host = String::new(); + assert!(form.is_blank()); + form.description = " ".to_string(); + assert!(form.is_blank(), "whitespace is not typing"); + form.bind_port = "8".to_string(); + assert!(!form.is_blank()); + } + + #[test] + fn a_rule_survives_the_round_trip_through_a_live_forward() { + let rule = rule_of(&managed(3, 8080)); + assert_eq!(rule.kind, SshForwardKind::Local); + assert_eq!(rule.bind_host, "127.0.0.1"); + assert_eq!(rule.bind_port, 8080); + assert_eq!(rule.target_host, "10.0.0.5"); + assert_eq!(rule.target_port, 80); + assert_eq!(rule.description.as_deref(), Some("the staging box")); + } + + #[test] + fn the_entry_an_add_appended_is_the_one_that_was_not_there_before() { + let list = vec![managed(1, 8080), managed(4, 9090)]; + let added = added_forward(&[1], &list).expect("the new entry"); + assert_eq!(added.id, 4); + assert!( + added_forward(&[1, 4], &list).is_none(), + "nothing was added, so there is nothing to point at" + ); + } +} diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 88e2ce0b..a2630d2b 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -39,6 +39,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::TreeDirHiddenOnly => "Only hidden files", L10nKey::TreeDirUnreadable => "Could not be read", L10nKey::TreeSearchCapped => "First {n} matches", + L10nKey::TreeSearchFailed => "Search failed", L10nKey::FileChangedOnDisk => "File changed on disk", L10nKey::Reload => "Reload", L10nKey::KeepMine => "Keep mine", @@ -197,12 +198,30 @@ pub fn translate_en(key: L10nKey) -> &'static str { "Re-reads the file and adds anything new. Edits you make here are stored by tty7 — the file itself is never written." } L10nKey::SettingsImportNow => "Import now", + L10nKey::SettingsImportUnreadable => "Could not read {path} — nothing was imported.", + L10nKey::SettingsImportNoHosts => { + "{path} names no hosts to import — only wildcard or Match rules." + } + L10nKey::SettingsImportSummary => { + "{count} hosts added — {updated} updated, {unchanged} already current" + } + L10nKey::SettingsImportIgnored => { + "{count} options have no setting in tty7 and were left in the file: {options}" + } + L10nKey::SettingsImportMoreOptions => "+{count} more", L10nKey::SettingsDefaultsIntro => { "Every host starts from these. Any host can override one under its own Advanced." } 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." @@ -232,6 +251,8 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::SettingsNameDesc => "A label for this connection.", L10nKey::SettingsHost => "Host", L10nKey::SettingsHostDesc => "Hostname or IP address.", + L10nKey::SettingsHostRequired => "Needs a host — won't be saved.", + L10nKey::SettingsPortInvalid => "Port must be 1-65535 — blank means 22.", L10nKey::SettingsUser => "User", L10nKey::SettingsUserDesc => "Login user (blank = resolve at connect).", L10nKey::SettingsAuth => "Auth", @@ -245,6 +266,8 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::SettingsJumpHostDesc => { "Name of another profile to tunnel through (blank = direct)." } + L10nKey::SettingsJumpHostUnknown => "No host profile named {jump_name} — won't be saved.", + L10nKey::SettingsJumpHostSelf => "A host can't be its own jump host — won't be saved.", L10nKey::SettingsNoneSummary => "(none)", L10nKey::SettingsPortForwarding => "Port forwarding", L10nKey::SettingsRulesOpenedWithConnection => "1 rule, opened with the connection", @@ -277,6 +300,9 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::SettingsSocks5ProxyDesc => "host:port (blank = none).", L10nKey::SettingsHttpProxy => "HTTP proxy", L10nKey::SettingsHttpProxyDesc => "host:port (blank = none).", + L10nKey::SettingsProxyPortInvalid => { + "Port must be 1-65535 — the host on its own takes the default port." + } L10nKey::SettingsKexAlgorithms => "KEX algorithms", L10nKey::SettingsKexAlgorithmsDesc => "Comma-separated (blank = library default).", L10nKey::SettingsCiphers => "Ciphers", @@ -819,6 +845,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::SftpTransferDone => "done", L10nKey::SftpTransferCancelled => "cancelled", L10nKey::SftpTransferError => "error", + L10nKey::SftpTransferListFailed => "Could not check transfers: {error}", L10nKey::SftpImagePasteUploadFailed => { "Could not upload the pasted image to {host}: {error}" } @@ -835,6 +862,7 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::ForwardToLabel => "to", L10nKey::ForwardSocksLabel => "SOCKS", L10nKey::ForwardAdd => "Add", + L10nKey::ForwardRequestFailed => "Could not reach the session — nothing changed.", L10nKey::FileTreePlaceholderFileName => "file name", L10nKey::FileTreePlaceholderFolderName => "folder name", L10nKey::FileTreePlaceholderNewName => "new name", @@ -848,6 +876,8 @@ pub fn translate_en(key: L10nKey) -> &'static str { "The file will be deleted on {host}. There is no trash on the far side." } L10nKey::FileTreeDeleteFailed => "Could not delete {name}", + L10nKey::FileTreeCreateFailed => "Could not create {name}", + L10nKey::FileTreeRenameFailed => "Could not rename {name}", L10nKey::FileTreeContextOpen => "Open", L10nKey::FileTreeContextCdHere => "cd Here", L10nKey::FileTreeContextInsertPath => "Insert Path in Terminal", @@ -872,6 +902,11 @@ pub fn translate_en(key: L10nKey) -> &'static str { L10nKey::FileDropFailedMany => "Could not copy {name}, and {n} more failed", L10nKey::SshPromptNewKey => "new {fingerprint}", L10nKey::SshPromptOldKey => "old {old_fingerprint}", + L10nKey::SshPromptHostKeyNewAlgorithm => { + "You already know this host by a {previous_algorithm} key. This is a new \ + {algorithm} key, not a replacement for that one." + } + L10nKey::SshPromptTypeYesToOverride => "Type \"yes\" to enable Override.", L10nKey::EditorCantOpen => "Could not open {path}: {e}", L10nKey::EditorCantRead => "Could not read {path}: {e}", L10nKey::EditorNotUtf8 => "\"{path}\" is not valid UTF-8", @@ -1586,6 +1621,22 @@ pub fn translate_variant_en(key: L10nKey, branch: &'static str) -> Option<&'stat (L10nKey::SettingsAliasesLinked, "zero") => "No aliases linked yet.", (L10nKey::SettingsAliasesLinked, "one") => "1 alias linked.", (L10nKey::SettingsAliasesLinked, "other") => "{count} aliases linked.", + (L10nKey::SettingsImportSummary, "zero") => { + "Nothing new — {updated} updated, {unchanged} already current" + } + (L10nKey::SettingsImportSummary, "one") => { + "1 host added — {updated} updated, {unchanged} already current" + } + (L10nKey::SettingsImportSummary, "other") => { + "{count} hosts added — {updated} updated, {unchanged} already current" + } + (L10nKey::SettingsImportIgnored, "zero") => "Every option in the file has a tty7 setting.", + (L10nKey::SettingsImportIgnored, "one") => { + "1 option has no setting in tty7 and was left in the file: {options}" + } + (L10nKey::SettingsImportIgnored, "other") => { + "{count} options have no setting in tty7 and were left in the file: {options}" + } (L10nKey::SettingsRulesOpenedWithConnection, "zero") => { "0 rules, opened with the connection" } @@ -1602,6 +1653,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 d92baeff..b035b2a2 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -39,6 +39,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::TreeDirHiddenOnly => "隠しファイルのみ", L10nKey::TreeDirUnreadable => "読み取れません", L10nKey::TreeSearchCapped => "最初の {n} 件のみ", + L10nKey::TreeSearchFailed => "検索に失敗しました", L10nKey::FileChangedOnDisk => "ディスク上でファイルが変更されました", L10nKey::Reload => "再読み込み", L10nKey::KeepMine => "自分の変更を保持", @@ -198,12 +199,32 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { "ファイルを再読み込みして新しい項目を追加します。ここでの編集は tty7 が保存します — ファイル自体には書き込まれません" } L10nKey::SettingsImportNow => "今すぐインポート", + L10nKey::SettingsImportUnreadable => { + "{path} を読み取れませんでした — 何もインポートされていません" + } + L10nKey::SettingsImportNoHosts => { + "{path} にインポートできるホストがありません — ワイルドカードや Match のルールだけです" + } + L10nKey::SettingsImportSummary => { + "ホスト {count} 件を追加 — {updated} 件を更新、{unchanged} 件は変更なし" + } + L10nKey::SettingsImportIgnored => { + "tty7 に設定のないオプションが {count} 件あり、ファイルに残されています: {options}" + } + L10nKey::SettingsImportMoreOptions => "他 {count} 件", L10nKey::SettingsDefaultsIntro => { "すべてのホストはこの設定から始まります。各ホストは詳細設定で個別に上書きできます" } L10nKey::SettingsCopyAddress => "アドレスをコピー", L10nKey::SettingsDuplicate => "複製", L10nKey::SettingsForgetPassword => "パスワードを消去", + L10nKey::SettingsForgetPasswordTitle => "{endpoint} の保存されたパスワードを消去しますか?", + L10nKey::SettingsForgetPasswordBody => { + "次に接続するときに、もう一度パスワードを尋ねられます。このホストの他の設定は変わりません" + } + L10nKey::SettingsForgetPasswordSharedBody => { + "他にも {count} 件のホストプロファイルが {endpoint} を使っているため、それらの接続でもパスワードの再入力が必要になります" + } L10nKey::SettingsForgotPasswordFor => "{endpoint} の保存されたパスワードを消去しました", L10nKey::SettingsDeleteProfileBody => { "保存されたパスワードも一緒に削除されます。同じアドレスを使う接続が他にある場合は残ります。" @@ -229,6 +250,8 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsNameDesc => "この接続の表示名", L10nKey::SettingsHost => "ホスト名", L10nKey::SettingsHostDesc => "ホスト名または IP アドレス", + L10nKey::SettingsHostRequired => "ホスト名が必要です — 保存されません", + L10nKey::SettingsPortInvalid => "ポートは 1-65535 の範囲です — 空欄なら 22 です", L10nKey::SettingsUser => "ユーザー名", L10nKey::SettingsUserDesc => "ログインユーザー (空欄 = 接続時に解決)", L10nKey::SettingsAuth => "認証方式", @@ -242,6 +265,12 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsJumpHostDesc => { "トンネリングに使用する別のプロファイル名 (空欄 = 直接接続)" } + L10nKey::SettingsJumpHostUnknown => { + "{jump_name} という名前のホストプロファイルはありません — 保存されません" + } + L10nKey::SettingsJumpHostSelf => { + "ホストを自分自身のジャンプホストにはできません — 保存されません" + } L10nKey::SettingsNoneSummary => "(なし)", L10nKey::SettingsPortForwarding => "ポートフォワーディング", L10nKey::SettingsRulesOpenedWithConnection => "接続と同時に開くルール 1 件", @@ -274,6 +303,9 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsSocks5ProxyDesc => "host:port(空欄 = なし)", L10nKey::SettingsHttpProxy => "HTTP プロキシ", L10nKey::SettingsHttpProxyDesc => "host:port(空欄 = なし)", + L10nKey::SettingsProxyPortInvalid => { + "ポートは 1-65535 の範囲です — ホストだけならデフォルトポートを使います" + } L10nKey::SettingsKexAlgorithms => "KEX アルゴリズム", L10nKey::SettingsKexAlgorithmsDesc => "カンマ区切り(空欄 = ライブラリのデフォルト)", L10nKey::SettingsCiphers => "暗号方式", @@ -861,6 +893,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::SftpTransferDone => "完了", L10nKey::SftpTransferCancelled => "キャンセル済み", L10nKey::SftpTransferError => "エラー", + L10nKey::SftpTransferListFailed => "転送状況を取得できませんでした: {error}", L10nKey::SftpImagePasteUploadFailed => { "貼り付けた画像を {host} にアップロードできませんでした: {error}" } @@ -877,6 +910,7 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::ForwardToLabel => "転送先", L10nKey::ForwardSocksLabel => "SOCKS", L10nKey::ForwardAdd => "追加", + L10nKey::ForwardRequestFailed => "セッションに届きませんでした。何も変更していません", L10nKey::FileTreePlaceholderFileName => "ファイル名", L10nKey::FileTreePlaceholderFolderName => "フォルダ名", L10nKey::FileTreePlaceholderNewName => "新しい名前", @@ -890,6 +924,8 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { "{host} 上でファイルが削除されます。リモート側にゴミ箱はありません。" } L10nKey::FileTreeDeleteFailed => "{name} を削除できませんでした", + L10nKey::FileTreeCreateFailed => "{name} を作成できませんでした", + L10nKey::FileTreeRenameFailed => "{name} の名前を変更できませんでした", L10nKey::FileTreeContextOpen => "開く", L10nKey::FileTreeContextCdHere => "ここで cd", L10nKey::FileTreeContextInsertPath => "ターミナルにパスを挿入", @@ -914,6 +950,10 @@ pub fn translate_ja(key: L10nKey) -> Option<&'static str> { L10nKey::FileDropFailedMany => "{name} をコピーできませんでした。他に {n} 件も失敗しました", L10nKey::SshPromptNewKey => "新しいキー {fingerprint}", L10nKey::SshPromptOldKey => "以前のキー {old_fingerprint}", + L10nKey::SshPromptHostKeyNewAlgorithm => { + "このホストはすでに {previous_algorithm} キーで登録されています。これはそれを置き換えるものではなく、新しい {algorithm} キーです" + } + L10nKey::SshPromptTypeYesToOverride => "「yes」を入力すると「上書き」が有効になります", L10nKey::EditorCantOpen => "{path} を開けません: {e}", L10nKey::EditorCantRead => "{path} を読み取れません: {e}", L10nKey::EditorNotUtf8 => "「{path}」は有効な UTF-8 ではありません", @@ -1633,6 +1673,24 @@ pub fn translate_variant_ja(key: L10nKey, branch: &'static str) -> Option<&'stat (L10nKey::SettingsAliasesLinked, "zero") => "エイリアスはまだリンクされていません", (L10nKey::SettingsAliasesLinked, "one") => "エイリアス 1 件がリンクされています", (L10nKey::SettingsAliasesLinked, "other") => "エイリアス {count} 件がリンクされています", + (L10nKey::SettingsImportSummary, "zero") => { + "新しいホストはありません — {updated} 件を更新、{unchanged} 件は変更なし" + } + (L10nKey::SettingsImportSummary, "one") => { + "ホスト 1 件を追加 — {updated} 件を更新、{unchanged} 件は変更なし" + } + (L10nKey::SettingsImportSummary, "other") => { + "ホスト {count} 件を追加 — {updated} 件を更新、{unchanged} 件は変更なし" + } + (L10nKey::SettingsImportIgnored, "zero") => { + "ファイル内のすべてのオプションに tty7 側の設定があります" + } + (L10nKey::SettingsImportIgnored, "one") => { + "tty7 に設定のないオプションが 1 件あり、ファイルに残されています: {options}" + } + (L10nKey::SettingsImportIgnored, "other") => { + "tty7 に設定のないオプションが {count} 件あり、ファイルに残されています: {options}" + } (L10nKey::SettingsRulesOpenedWithConnection, "zero") => "接続と同時に開くルール 0 件", (L10nKey::SettingsRulesOpenedWithConnection, "one") => "接続と同時に開くルール 1 件", (L10nKey::SettingsRulesOpenedWithConnection, "other") => { @@ -1647,6 +1705,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 86c4e815..e41833ab 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -107,6 +107,7 @@ l10n_keys! { TreeDirHiddenOnly, TreeDirUnreadable, TreeSearchCapped, + TreeSearchFailed, FileChangedOnDisk, Reload, KeepMine, @@ -224,10 +225,18 @@ l10n_keys! { SettingsImportAliases, SettingsImportAliasesDesc, SettingsImportNow, + SettingsImportUnreadable, + SettingsImportNoHosts, + SettingsImportSummary, + SettingsImportIgnored, + SettingsImportMoreOptions, SettingsDefaultsIntro, SettingsCopyAddress, SettingsDuplicate, SettingsForgetPassword, + SettingsForgetPasswordTitle, + SettingsForgetPasswordBody, + SettingsForgetPasswordSharedBody, SettingsForgotPasswordFor, SettingsDeleteProfileBody, SettingsCouldntForgetPassword, @@ -245,6 +254,8 @@ l10n_keys! { SettingsNameDesc, SettingsHost, SettingsHostDesc, + SettingsHostRequired, + SettingsPortInvalid, SettingsUser, SettingsUserDesc, SettingsAuth, @@ -256,6 +267,8 @@ l10n_keys! { SettingsAuthMode2Fa, SettingsJumpHost, SettingsJumpHostDesc, + SettingsJumpHostUnknown, + SettingsJumpHostSelf, SettingsNoneSummary, SettingsNoneLower, SettingsPortForwarding, @@ -285,6 +298,7 @@ l10n_keys! { SettingsSocks5ProxyDesc, SettingsHttpProxy, SettingsHttpProxyDesc, + SettingsProxyPortInvalid, SettingsKexAlgorithms, SettingsKexAlgorithmsDesc, SettingsCiphers, @@ -625,6 +639,7 @@ l10n_keys! { SftpTransferDone, SftpTransferCancelled, SftpTransferError, + SftpTransferListFailed, SftpImagePasteUploadFailed, ForwardPanelTitle, ForwardDisconnected, @@ -639,6 +654,7 @@ l10n_keys! { ForwardToLabel, ForwardSocksLabel, ForwardAdd, + ForwardRequestFailed, FileTreePlaceholderFileName, FileTreePlaceholderFolderName, FileTreePlaceholderNewName, @@ -648,6 +664,8 @@ l10n_keys! { SftpDeleteFolderBody, SftpDeleteFileBody, FileTreeDeleteFailed, + FileTreeCreateFailed, + FileTreeRenameFailed, FileTreeContextOpen, FileTreeContextCdHere, FileTreeContextInsertPath, @@ -670,6 +688,8 @@ l10n_keys! { FileDropFailedMany, SshPromptNewKey, SshPromptOldKey, + SshPromptHostKeyNewAlgorithm, + SshPromptTypeYesToOverride, EditorCantOpen, EditorCantRead, EditorNotUtf8, @@ -1993,8 +2013,11 @@ mod tests { fn plural_and_select_branches_are_translated() { let plural_keys = [ L10nKey::SettingsAliasesLinked, + L10nKey::SettingsImportSummary, + L10nKey::SettingsImportIgnored, 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 8fa6a742..63584aa4 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -39,6 +39,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::TreeDirHiddenOnly => "只有隐藏文件", L10nKey::TreeDirUnreadable => "无法读取", L10nKey::TreeSearchCapped => "只显示前 {n} 个匹配", + L10nKey::TreeSearchFailed => "搜索失败", L10nKey::FileChangedOnDisk => "文件在磁盘上已被修改", L10nKey::Reload => "重新加载", L10nKey::KeepMine => "保留我的版本", @@ -176,12 +177,28 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { "重新读取文件并添加新内容。你在这里做的编辑由 tty7 保存——不会写入该文件本身。" } L10nKey::SettingsImportNow => "立即导入", + L10nKey::SettingsImportUnreadable => "无法读取 {path}——没有导入任何内容。", + L10nKey::SettingsImportNoHosts => "{path} 中没有可导入的主机——只有通配符或 Match 规则。", + L10nKey::SettingsImportSummary => { + "新增 {count} 个主机——更新 {updated} 个,{unchanged} 个已是最新" + } + L10nKey::SettingsImportIgnored => { + "有 {count} 个选项在 tty7 中没有对应设置,仍留在文件里:{options}" + } + L10nKey::SettingsImportMoreOptions => "还有 {count} 个", L10nKey::SettingsDefaultsIntro => { "所有主机都从这些设置开始。每个主机都可以在自己的高级选项中覆盖某项。" } L10nKey::SettingsCopyAddress => "复制地址", L10nKey::SettingsDuplicate => "复制", L10nKey::SettingsForgetPassword => "清除已保存的密码", + L10nKey::SettingsForgetPasswordTitle => "要清除 {endpoint} 的已保存密码吗?", + L10nKey::SettingsForgetPasswordBody => { + "下次连接它时会重新询问密码。这台主机的其他设置不受影响。" + } + L10nKey::SettingsForgetPasswordSharedBody => { + "还有 {count} 个主机配置同样使用 {endpoint},那些连接也需要重新输入密码。" + } L10nKey::SettingsForgotPasswordFor => "已清除 {endpoint} 的已保存密码", L10nKey::SettingsDeleteProfileBody => { "为它保存的密码也会一并删除,除非还有别的连接用着同一个地址。" @@ -205,6 +222,8 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsNameDesc => "此连接的标签。", L10nKey::SettingsHost => "主机", L10nKey::SettingsHostDesc => "主机名或 IP 地址。", + L10nKey::SettingsHostRequired => "需要填写主机——不会被保存。", + L10nKey::SettingsPortInvalid => "端口必须在 1-65535 之间——留空表示 22。", L10nKey::SettingsUser => "用户", L10nKey::SettingsUserDesc => "登录用户(留空表示连接时解析)。", L10nKey::SettingsAuth => "认证", @@ -216,6 +235,8 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsAuthMode2Fa => "2FA", L10nKey::SettingsJumpHost => "跳板主机", L10nKey::SettingsJumpHostDesc => "用于中转的另一个主机配置的名称(留空 = 直连)。", + L10nKey::SettingsJumpHostUnknown => "没有名为 {jump_name} 的主机配置——不会被保存。", + L10nKey::SettingsJumpHostSelf => "主机不能把自己当作跳板——不会被保存。", L10nKey::SettingsNoneSummary => "(无)", L10nKey::SettingsPortForwarding => "端口转发", L10nKey::SettingsRulesOpenedWithConnection => "1 条规则,随连接打开", @@ -244,6 +265,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SettingsSocks5ProxyDesc => "host:port(留空 = 无)。", L10nKey::SettingsHttpProxy => "HTTP 代理", L10nKey::SettingsHttpProxyDesc => "host:port(留空 = 无)。", + L10nKey::SettingsProxyPortInvalid => "端口必须在 1-65535 之间——只写主机则使用默认端口。", L10nKey::SettingsKexAlgorithms => "KEX 算法", L10nKey::SettingsKexAlgorithmsDesc => "逗号分隔(留空 = 库默认值)。", L10nKey::SettingsCiphers => "加密算法", @@ -786,6 +808,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::SftpTransferDone => "完成", L10nKey::SftpTransferCancelled => "已取消", L10nKey::SftpTransferError => "错误", + L10nKey::SftpTransferListFailed => "无法获取传输状态:{error}", L10nKey::SftpImagePasteUploadFailed => "无法将粘贴的图片上传到 {host}:{error}", L10nKey::ForwardPanelTitle => "端口转发", L10nKey::ForwardDisconnected => "已断开", @@ -800,6 +823,7 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::ForwardToLabel => "到", L10nKey::ForwardSocksLabel => "SOCKS", L10nKey::ForwardAdd => "添加", + L10nKey::ForwardRequestFailed => "联系不上这个会话——什么都没有改动。", L10nKey::FileTreePlaceholderFileName => "文件名", L10nKey::FileTreePlaceholderFolderName => "文件夹名", L10nKey::FileTreePlaceholderNewName => "新名称", @@ -811,6 +835,8 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { } L10nKey::SftpDeleteFileBody => "该文件将在 {host} 上被删除。远端没有回收站。", L10nKey::FileTreeDeleteFailed => "无法删除 {name}", + L10nKey::FileTreeCreateFailed => "无法创建 {name}", + L10nKey::FileTreeRenameFailed => "无法重命名 {name}", L10nKey::FileTreeContextOpen => "打开", L10nKey::FileTreeContextCdHere => "cd 到此处", L10nKey::FileTreeContextInsertPath => "在终端中插入路径", @@ -833,6 +859,10 @@ pub fn translate_zh(key: L10nKey) -> Option<&'static str> { L10nKey::FileDropFailedMany => "无法复制 {name},另有 {n} 个也失败了", L10nKey::SshPromptNewKey => "新 {fingerprint}", L10nKey::SshPromptOldKey => "旧 {old_fingerprint}", + L10nKey::SshPromptHostKeyNewAlgorithm => { + "你已经通过一把 {previous_algorithm} 密钥认识这台主机。这是一把新的 {algorithm} 密钥,并不是用来替换那一把的。" + } + L10nKey::SshPromptTypeYesToOverride => "输入 yes 才能启用“覆盖”。", L10nKey::EditorCantOpen => "无法打开 {path}:{e}", L10nKey::EditorCantRead => "无法读取 {path}:{e}", L10nKey::EditorNotUtf8 => "“{path}”不是有效的 UTF-8", @@ -1506,6 +1536,22 @@ pub fn translate_variant_zh(key: L10nKey, branch: &'static str) -> Option<&'stat (L10nKey::SettingsAliasesLinked, "zero") => "还没有关联别名。", (L10nKey::SettingsAliasesLinked, "one") => "已关联 1 个别名。", (L10nKey::SettingsAliasesLinked, "other") => "已关联 {count} 个别名。", + (L10nKey::SettingsImportSummary, "zero") => { + "没有新主机——更新 {updated} 个,{unchanged} 个已是最新" + } + (L10nKey::SettingsImportSummary, "one") => { + "新增 1 个主机——更新 {updated} 个,{unchanged} 个已是最新" + } + (L10nKey::SettingsImportSummary, "other") => { + "新增 {count} 个主机——更新 {updated} 个,{unchanged} 个已是最新" + } + (L10nKey::SettingsImportIgnored, "zero") => "文件里的每个选项在 tty7 中都有对应设置。", + (L10nKey::SettingsImportIgnored, "one") => { + "有 1 个选项在 tty7 中没有对应设置,仍留在文件里:{options}" + } + (L10nKey::SettingsImportIgnored, "other") => { + "有 {count} 个选项在 tty7 中没有对应设置,仍留在文件里:{options}" + } (L10nKey::SettingsRulesOpenedWithConnection, "zero") => "0 条规则,随连接打开", (L10nKey::SettingsRulesOpenedWithConnection, "one") => "1 条规则,随连接打开", (L10nKey::SettingsRulesOpenedWithConnection, "other") => "{count} 条规则,随连接打开", @@ -1518,6 +1564,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 7048c942..96803083 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -9,6 +9,7 @@ use gpui_component::color_picker::{ColorPicker, ColorPickerState}; use gpui_component::input::{Input, InputEvent, InputState}; use gpui_component::link::Link; use gpui_component::menu::{ContextMenuExt as _, DropdownMenu as _, PopupMenu, PopupMenuItem}; +use gpui_component::notification::{Notification, NotificationType}; use gpui_component::select::{SearchableVec, Select, SelectState}; use gpui_component::sidebar::{Sidebar, SidebarCollapsible, SidebarMenu, SidebarMenuItem}; use gpui_component::slider::{Slider, SliderState}; @@ -797,6 +798,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, @@ -840,6 +862,19 @@ pub(crate) struct SshProfileForm { _subs: Vec, } +impl SshProfileForm { + /// Whether the group that identifies the host — name, host, port, user — + /// is still untouched. Every field notifies on change, so the form + /// re-renders on each keystroke; without this a new host would be told it + /// needs a host before anyone had the chance to type one. Same deal the + /// forward rows strike with `ForwardRuleForm::is_blank`. + fn core_is_blank(&self, cx: &App) -> bool { + [&self.name, &self.host, &self.port, &self.user] + .iter() + .all(|e| e.read(cx).value().trim().is_empty()) + } +} + pub(crate) struct ForwardRuleForm { pub(crate) kind: ForwardKind, pub(crate) bind_host: Entity, @@ -911,14 +946,39 @@ pub(crate) fn humanize_action(action: &str) -> String { out } -fn parse_host_port(s: &str) -> Option { +/// What a blank port field means. The same number `SshProfile`'s serde default +/// writes for a config that never mentioned a port, which is why leaving the +/// field empty has to stay legal: every host imported from `~/.ssh/config` +/// leaves it empty. +const DEFAULT_SSH_PORT: u16 = 22; + +/// The port each proxy scheme listens on when the field names only a host. +const DEFAULT_SOCKS_PORT: u16 = 1080; +const DEFAULT_HTTP_PROXY_PORT: u16 = 8080; + +/// A port as a form field spells it. Nothing here accepts 0: every port in a +/// profile is one something has to connect to, and no listener answers on 0. +fn parse_port(s: &str) -> Option { + s.trim().parse::().ok().filter(|p| *p > 0) +} + +/// A proxy address as the form spells it: blank is "no proxy", a bare host +/// takes the scheme's default port, and anything else has to carry a port that +/// exists. This used to be `parse().unwrap_or(0)`, so `proxy.example.com:88O` +/// saved a proxy on port 0 and the failure surfaced far away, in the socket +/// layer. The default port is not a secret either — `host_port_text` writes it +/// back into the field the next time the form opens. +fn parse_host_port_checked(s: &str, default_port: u16) -> Result, SshFieldError> { let s = s.trim(); if s.is_empty() { - return None; + return Ok(None); } match s.rsplit_once(':') { - Some((h, p)) => Some(HostPort::new(h.trim(), p.trim().parse().unwrap_or(0))), - None => Some(HostPort::new(s, 0)), + Some((h, p)) => match parse_port(p) { + Some(port) => Ok(Some(HostPort::new(h.trim(), port))), + None => Err(SshFieldError::ProxyPortRange), + }, + None => Ok(Some(HostPort::new(s, default_port))), } } @@ -944,6 +1004,207 @@ fn split_lines(s: &str) -> Vec { .collect() } +/// Why one field of the SSH profile form cannot be saved. A value rather than +/// a finished sentence, so the rules stay a plain function a test can call — +/// the wording, and the locale it is written in, belong to the render pass. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) enum SshFieldError { + /// Nothing to connect to. Saved anyway, the profile used to render as an + /// empty row in the host list and hand `TcpStream::connect` an empty name. + HostMissing, + /// A port field that is neither blank nor a port. + PortRange, + /// The same, for the port half of a proxy address. + ProxyPortRange, + /// The jump field names a profile no host list has. + JumpUnknown(String), + /// The jump field names the profile being edited. + JumpIsSelf, +} + +impl SshFieldError { + fn message(&self) -> String { + match self { + Self::HostMissing => t(L10nKey::SettingsHostRequired).to_string(), + Self::PortRange => t(L10nKey::SettingsPortInvalid).to_string(), + Self::ProxyPortRange => t(L10nKey::SettingsProxyPortInvalid).to_string(), + Self::JumpUnknown(name) => { + t_fmt(L10nKey::SettingsJumpHostUnknown, &[("jump_name", name)]) + } + Self::JumpIsSelf => t(L10nKey::SettingsJumpHostSelf).to_string(), + } + } +} + +/// What the form has to fix before it can be saved, one slot per field so each +/// complaint can be printed under the control it is about. +#[derive(Debug, Clone, Default, PartialEq, Eq)] +pub(crate) struct SshFormErrors { + host: Option, + port: Option, + jump: Option, + socks: Option, + http: Option, +} + +impl SshFormErrors { + fn is_empty(&self) -> bool { + self.host.is_none() + && self.port.is_none() + && self.jump.is_none() + && self.socks.is_none() + && self.http.is_none() + } +} + +/// The SSH profile form as plain text, lifted out of the `InputState` entities +/// it lives in. The rules that turn it into a profile are the part worth +/// testing, and a GPUI entity is not something a unit test can hand them, so +/// the window layer's job stops at reading the strings out. +#[derive(Debug, Clone, Default)] +pub(crate) struct SshFormDraft { + id: Uuid, + name: String, + group: Option, + host: String, + port: String, + user: String, + jump: String, + proxy_command: String, + socks: String, + http: String, + auth: AuthMode, + identity_files: String, + agent_forward: bool, + credential_ref: Option, + forwards: Vec, + keepalive_interval: String, + keepalive_count: String, + connect_timeout: String, + warn_on_close: Option, + skip_banner: bool, + shell_integration: bool, + login_scripts: String, + x11: bool, + kex: String, + cipher: String, + mac: String, + hostkey: String, + compression: String, + verify_host_keys: Option, +} + +/// The one place that decides what the form would save and what is wrong with +/// it. Both, always — never one or the other: the Escape prompt asks whether +/// the form differs from what is on disk, and a form that is merely invalid +/// still holds everything the user typed. Handing back only the errors would +/// make a brand-new invalid profile compare equal to the nothing on disk, and +/// Escape would throw the typing away without asking. +/// +/// A missing `name` is deliberately not an error: the host list already falls +/// back to the host for a nameless profile, and requiring one would refuse +/// every host imported from `~/.ssh/config`. +fn validate_ssh_draft(draft: SshFormDraft, profiles: &[SshProfile]) -> (SshProfile, SshFormErrors) { + let mut errors = SshFormErrors::default(); + + let host = draft.host.trim().to_string(); + if host.is_empty() { + errors.host = Some(SshFieldError::HostMissing); + } + + let port_text = draft.port.trim(); + let port = match port_text.is_empty() { + true => DEFAULT_SSH_PORT, + false => parse_port(port_text).unwrap_or_else(|| { + errors.port = Some(SshFieldError::PortRange); + DEFAULT_SSH_PORT + }), + }; + + // The field is a name but the profile stores an id, so a jump host already + // survives its target being renamed. What it never survived was a name + // nobody has: the lookup returned `None`, the profile saved as a direct + // connection, and reopening the form showed an empty field. + let jump_name = draft.jump.trim(); + let jump_host = if jump_name.is_empty() { + None + } else { + let named = |p: &&SshProfile| p.name == jump_name; + // Duplicate names resolve to whichever profile comes first, as they + // always have. The one profile that can never be the answer is the one + // being edited, and typing its own name is worth saying out loud + // rather than quietly connecting direct. + match profiles.iter().filter(named).find(|p| p.id != draft.id) { + Some(p) => Some(p.id), + None => { + errors.jump = Some(match profiles.iter().any(|p| p.name == jump_name) { + true => SshFieldError::JumpIsSelf, + false => SshFieldError::JumpUnknown(jump_name.to_string()), + }); + None + } + } + }; + + let proxy = |text: &str, default_port: u16, slot: &mut Option| { + match parse_host_port_checked(text, default_port) { + Ok(hp) => hp, + Err(e) => { + *slot = Some(e); + None + } + } + }; + let socks_proxy = proxy(&draft.socks, DEFAULT_SOCKS_PORT, &mut errors.socks); + let http_proxy = proxy(&draft.http, DEFAULT_HTTP_PROXY_PORT, &mut errors.http); + + let proxy_command = draft.proxy_command.trim(); + let profile = SshProfile { + id: draft.id, + name: draft.name.trim().to_string(), + group: draft.group, + host, + port, + user: draft.user.trim().to_string(), + jump_host, + proxy_command: (!proxy_command.is_empty()).then(|| proxy_command.to_string()), + socks_proxy, + http_proxy, + auth: draft.auth, + identity_files: split_lines(&draft.identity_files), + agent_forward: draft.agent_forward, + credential_ref: draft.credential_ref, + forwards: draft.forwards, + keepalive_interval_s: draft.keepalive_interval.trim().parse().ok(), + keepalive_count_max: draft.keepalive_count.trim().parse().ok(), + connect_timeout_s: draft.connect_timeout.trim().parse().ok(), + warn_on_close: draft.warn_on_close, + skip_banner: draft.skip_banner, + shell_integration: draft.shell_integration, + login_scripts: split_lines(&draft.login_scripts), + x11: draft.x11, + algorithms: Algorithms { + kex: split_list(&draft.kex), + cipher: split_list(&draft.cipher), + mac: split_list(&draft.mac), + hostkey: split_list(&draft.hostkey), + compression: split_list(&draft.compression), + }, + verify_host_keys: draft.verify_host_keys, + }; + (profile, errors) +} + +/// The inline complaint under a field: one line, in the danger colour, in the +/// column the control sits in. Built before the row rather than inside a +/// `when` closure so it borrows the app for the length of one call. +fn field_error(message: impl Into, cx: &App) -> Div { + div() + .text_xs() + .text_color(cx.theme().danger) + .child(message.into()) +} + fn forward_row_inputs(row: &ForwardRuleForm) -> [&Entity; 5] { [ &row.bind_host, @@ -2347,8 +2608,9 @@ impl Tty7App { .item( PopupMenuItem::new(t(L10nKey::SettingsImportFromSshConfig)).on_click({ let app = app.clone(); - move |_, _window, cx| { - let _ = app.update(cx, |this, cx| this.import_ssh_config_profiles(cx)); + move |_, window, cx| { + let _ = + app.update(cx, |this, cx| this.import_ssh_config_profiles(window, cx)); } }), ) @@ -2726,9 +2988,9 @@ impl Tty7App { Button::new("ssh-empty-import") .label(t(L10nKey::Link)) .small() - .on_click( - cx.listener(|this, _, _w, cx| this.import_ssh_config_profiles(cx)), - ), + .on_click(cx.listener(|this, _, window, cx| { + this.import_ssh_config_profiles(window, cx) + })), ), ); } @@ -2772,9 +3034,9 @@ impl Tty7App { Button::new("ssh-defaults-import") .label(t(L10nKey::SettingsImportNow)) .small() - .on_click( - cx.listener(|this, _, _w, cx| this.import_ssh_config_profiles(cx)), - ) + .on_click(cx.listener(|this, _, window, cx| { + this.import_ssh_config_profiles(window, cx) + })) .into_any_element(), cx, ), @@ -2834,13 +3096,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)); } }), ) @@ -3049,59 +3306,62 @@ impl Tty7App { cx.notify(); } - fn ssh_form_collect(&self, cx: &App) -> Option { + /// Reads the form out of its entities and runs it past + /// [`validate_ssh_draft`]. The profile that comes back is what the form + /// would save; the errors are what stands in the way. + fn ssh_form_collect(&self, cx: &App) -> Option<(SshProfile, SshFormErrors)> { let form = self.active_settings()?.ssh_form.as_ref()?; - let id = form.editing; let val = |e: &Entity| e.read(cx).value().trim().to_string(); + // The multi-line and comma-separated fields do their own splitting, so + // they travel whole rather than trimmed. + let raw = |e: &Entity| e.read(cx).value().to_string(); - let jump_name = val(&form.jump); - let jump_host = if jump_name.is_empty() { - None - } else { - cx.global::() - .ssh_profiles - .iter() - .find(|p| p.name == jump_name && p.id != id) - .map(|p| p.id) - }; - - Some(SshProfile { - id, + let draft = SshFormDraft { + id: form.editing, name: val(&form.name), group: form.carry_group.clone(), host: val(&form.host), - port: val(&form.port).parse().unwrap_or(22), + port: val(&form.port), user: val(&form.user), - jump_host, - proxy_command: (!val(&form.proxy_command).is_empty()).then(|| val(&form.proxy_command)), - socks_proxy: parse_host_port(&val(&form.socks)), - http_proxy: parse_host_port(&val(&form.http)), + jump: val(&form.jump), + proxy_command: val(&form.proxy_command), + socks: val(&form.socks), + http: val(&form.http), auth: form.auth, - identity_files: split_lines(&form.identity_files.read(cx).value()), + identity_files: raw(&form.identity_files), agent_forward: form.agent_forward, credential_ref: form.carry_credential_ref.clone(), forwards: form.forwards.iter().filter_map(|r| r.collect(cx)).collect(), - keepalive_interval_s: val(&form.keepalive_interval).parse().ok(), - keepalive_count_max: val(&form.keepalive_count).parse().ok(), - connect_timeout_s: val(&form.connect_timeout).parse().ok(), + keepalive_interval: val(&form.keepalive_interval), + keepalive_count: val(&form.keepalive_count), + connect_timeout: val(&form.connect_timeout), warn_on_close: form.warn_on_close, skip_banner: form.skip_banner, shell_integration: form.shell_integration, - login_scripts: split_lines(&form.login_scripts.read(cx).value()), + login_scripts: raw(&form.login_scripts), x11: form.x11, - algorithms: Algorithms { - kex: split_list(&form.kex.read(cx).value()), - cipher: split_list(&form.cipher.read(cx).value()), - mac: split_list(&form.mac.read(cx).value()), - hostkey: split_list(&form.hostkey.read(cx).value()), - compression: split_list(&form.compression.read(cx).value()), - }, + kex: raw(&form.kex), + cipher: raw(&form.cipher), + mac: raw(&form.mac), + hostkey: raw(&form.hostkey), + compression: raw(&form.compression), verify_host_keys: form.verify_host_keys, - }) + }; + Some(validate_ssh_draft( + draft, + &cx.global::().ssh_profiles, + )) } pub(crate) fn save_editing_profile(&mut self, cx: &mut Context) -> Option { - let profile = self.ssh_form_collect(cx)?; + let (profile, errors) = self.ssh_form_collect(cx)?; + // Save and Connect are both disabled while anything is wrong, but this + // is the door all of them go through, and what gets past it lands in + // the config file — where a host-less profile is a blank row nobody + // can identify or delete on sight. + if !errors.is_empty() { + return None; + } let id = profile.id; self.update_config(cx, |cfg| { if let Some(slot) = cfg.ssh_profiles.iter_mut().find(|p| p.id == id) { @@ -3120,7 +3380,9 @@ impl Tty7App { /// Whether the SSH profile form on screen holds edits that were never /// saved. Save is enabled off exactly this, so closing on it is the same - /// question the button already answers. + /// question the button already answers — and it compares what the form + /// would save even when the form cannot be saved yet, so a half-typed new + /// host is still something Escape has to ask about. pub(crate) fn ssh_form_dirty(&self, cx: &App) -> bool { let Some(form) = self.active_settings().and_then(|s| s.ssh_form.as_ref()) else { return false; @@ -3131,7 +3393,7 @@ impl Tty7App { .iter() .find(|p| p.id == form.editing) .cloned(); - self.ssh_form_collect(cx) != saved + self.ssh_form_collect(cx).map(|(profile, _)| profile) != saved } /// Closing from Escape or the X is the user leaving; every other caller @@ -3224,11 +3486,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); @@ -3273,15 +3531,107 @@ impl Tty7App { cx.notify(); } - pub(crate) fn import_ssh_config_profiles(&mut self, cx: &mut Context) { - let imported = crate::core::ssh_config::import_profiles(); - if imported.is_empty() { + /// Import `~/.ssh/config`, and say what that did. + /// + /// Every branch here ends in a notification because every branch used to + /// end in nothing: a missing file, a file of nothing but `Host *`, and a + /// clean import of six hosts were all the same silent button press, and the + /// only way to tell them apart was to go count the host list. + pub(crate) fn import_ssh_config_profiles( + &mut self, + window: &mut Window, + cx: &mut Context, + ) { + // One id for all three outcomes, so pressing the button again replaces + // what it said last time instead of stacking a second toast on top of + // an answer that is now out of date. + const NOTIFICATION: &str = "ssh-config-import"; + // A toast is 448pt wide. A config with a dozen unsupported keywords in + // it would push the counts out of view, so the notification names the + // first few and the log line below carries the whole list, with the + // hosts each keyword was set on. + const OPTIONS_SHOWN: usize = 5; + + let report = crate::core::ssh_config::import_report(); + let source = report.source.display().to_string(); + + if !report.source_read { + window.push_notification( + Notification::error(t_fmt( + L10nKey::SettingsImportUnreadable, + &[("path", &source)], + )) + .id1::(NOTIFICATION), + cx, + ); return; } + if report.profiles.is_empty() { + window.push_notification( + Notification::warning(t_fmt(L10nKey::SettingsImportNoHosts, &[("path", &source)])) + .id1::(NOTIFICATION), + cx, + ); + return; + } + + let read = report.profiles.len(); + let ignored = report.ignored; + let mut stats = crate::core::ssh_config::MergeStats::default(); self.update_config(cx, |cfg| { - crate::core::ssh_config::merge_imported(&mut cfg.ssh_profiles, imported); + stats = crate::core::ssh_config::merge_imported(&mut cfg.ssh_profiles, report.profiles); }); - cx.notify(); + + let dropped: Vec = ignored + .iter() + .map(|opt| format!("{} ({})", opt.option, opt.hosts.join(", "))) + .collect(); + log::info!( + "imported {read} alias(es) from {source} ({} file(s) read): {} added, {} updated, \ + {} unchanged; no tty7 setting for: [{}]", + report.files_read, + stats.added, + stats.updated, + stats.unchanged, + dropped.join("; ") + ); + + let mut notification = Notification::new() + .with_type(NotificationType::Success) + .title(t_plural( + L10nKey::SettingsImportSummary, + stats.added, + &[ + ("updated", &stats.updated.to_string()), + ("unchanged", &stats.unchanged.to_string()), + ], + )) + .id1::(NOTIFICATION); + if !ignored.is_empty() { + let mut options: Vec = ignored + .iter() + .take(OPTIONS_SHOWN) + .map(|opt| opt.option.clone()) + .collect(); + let rest = ignored.len() - options.len(); + if rest > 0 { + options.push(t_fmt( + L10nKey::SettingsImportMoreOptions, + &[("count", &rest.to_string())], + )); + } + notification = notification + .message(t_plural( + L10nKey::SettingsImportIgnored, + ignored.len(), + &[("options", &options.join(", "))], + )) + // A list of what the import could not carry is something to + // read and act on, and four seconds is not long enough to do + // either. The counts alone still fade on their own. + .autohide(false); + } + window.push_notification(notification, cx); } pub(crate) fn copy_profile_connect_string(&mut self, id: Uuid, cx: &mut Context) { @@ -3297,6 +3647,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, @@ -3337,7 +3741,8 @@ impl Tty7App { .iter() .find(|p| p.id == editing) .cloned(); - let collected = self.ssh_form_collect(cx); + let (collected, errors) = self.ssh_form_collect(cx).unzip(); + let errors = errors.unwrap_or_default(); let dirty = collected != saved; let address = collected .as_ref() @@ -3406,20 +3811,38 @@ impl Tty7App { Button::new("ssh-form-save") .label(t(L10nKey::Save)) .small() - .disabled(!dirty) + .disabled(!dirty || !errors.is_empty()) .on_click(cx.listener(|this, _, _w, cx| this.save_ssh_form(cx))), ) .child( + // Connect saves first, so it answers to the same + // rules. Before this it answered to none at all, and + // an empty host reached the socket layer as a DNS + // error about a name nobody typed. Button::new("ssh-form-connect") .label(t(L10nKey::Connect)) .primary() .small() + .disabled(!errors.is_empty()) .on_click(cx.listener(|this, _, window, cx| { this.save_and_connect_profile(window, cx) })), ), ); + // Every field notifies on change, so this form re-renders on each + // keystroke: telling a brand-new host that it needs a host is + // something it would say before the user had typed a character. Hold + // that one line back until the group it belongs to has something in + // it. A malformed value has nothing to wait for and says so at once. + let core_blank = form.core_is_blank(cx); + let host_error = errors + .host + .as_ref() + .filter(|_| !core_blank) + .map(|e| field_error(e.message(), cx)); + let port_error = errors.port.as_ref().map(|e| field_error(e.message(), cx)); + let core = v_flex() .gap_3() .child( @@ -3438,21 +3861,28 @@ impl Tty7App { self.settings_row( t(L10nKey::SettingsHost), t(L10nKey::SettingsHostDesc), - h_flex() - .gap_2() + v_flex() + .gap_1() .max_w_full() .child( - div() - .w(px(172.)) - .min_w_0() - .child(Input::new(&form.host).small()), - ) - .child( - div() - .w(px(80.)) - .flex_shrink_0() - .child(Input::new(&form.port).small()), + h_flex() + .gap_2() + .max_w_full() + .child( + div() + .w(px(172.)) + .min_w_0() + .child(Input::new(&form.host).small()), + ) + .child( + div() + .w(px(80.)) + .flex_shrink_0() + .child(Input::new(&form.port).small()), + ), ) + .when_some(host_error, |col, line| col.child(line)) + .when_some(port_error, |col, line| col.child(line)) .into_any_element(), cx, ), @@ -3505,9 +3935,9 @@ impl Tty7App { .gap_4() .child(header) .child(core) - .child(self.render_ssh_profile_jump_section(form, cx)) + .child(self.render_ssh_profile_jump_section(form, &errors, cx)) .child(self.render_ssh_profile_forwards_section(form, cx)) - .child(self.render_ssh_profile_advanced_section(form, cx)) + .child(self.render_ssh_profile_advanced_section(form, &errors, cx)) .into_any_element() } @@ -3553,6 +3983,7 @@ impl Tty7App { fn render_ssh_profile_jump_section( &self, form: &SshProfileForm, + errors: &SshFormErrors, cx: &mut Context, ) -> AnyElement { let summary = { @@ -3563,11 +3994,15 @@ impl Tty7App { name } }; + // A complaint nobody can see is a Save button that is greyed out for + // no reason the user can read, and the field keeps its text whether + // this section is folded or not — so an error holds it open. + let open = form.show_jump || errors.jump.is_some(); let mut section = v_flex().child(self.disclosure_header( "ssh-sec-jump", t(L10nKey::SettingsJumpHost), &summary, - form.show_jump, + open, cx, |this, cx| { if let Some(f) = this.ssh_form_mut() { @@ -3576,15 +4011,18 @@ impl Tty7App { } }, )); - if form.show_jump { + if open { + let error = errors.jump.as_ref().map(|e| field_error(e.message(), cx)); section = section.child( self.settings_row( t(L10nKey::SettingsJumpHost), t(L10nKey::SettingsJumpHostDesc), - div() + v_flex() + .gap_1() .w(px(260.)) .max_w_full() .child(Input::new(&form.jump).small()) + .when_some(error, |col, line| col.child(line)) .into_any_element(), cx, ), @@ -3662,14 +4100,21 @@ impl Tty7App { cx: &mut Context, ) -> AnyElement { let muted = cx.theme().muted_foreground; - let danger = cx.theme().danger; let needs_target = row.kind != ForwardKind::Dynamic; let kind_idx = match row.kind { ForwardKind::Local => 0, ForwardKind::Remote => 1, ForwardKind::Dynamic => 2, }; - let incomplete = row.collect(cx).is_none() && !row.is_blank(cx); + let incomplete = (row.collect(cx).is_none() && !row.is_blank(cx)).then(|| { + field_error( + match needs_target { + true => t(L10nKey::SettingsFwdNeedsBoth), + false => t(L10nKey::SettingsFwdNeedsListen), + }, + cx, + ) + }); // Below `SPLIT_FORWARD_ROW_BELOW` the five controls stop fitting on one // line. The kind switch, the description and the remove button keep the @@ -3773,13 +4218,7 @@ impl Tty7App { .gap_0p5() .py_1() .child(rule) - .when(incomplete, |col| { - col.child(div().text_xs().text_color(danger).child(if needs_target { - t(L10nKey::SettingsFwdNeedsBoth) - } else { - t(L10nKey::SettingsFwdNeedsListen) - })) - }) + .when_some(incomplete, |col, line| col.child(line)) .into_any_element() } @@ -3815,13 +4254,18 @@ impl Tty7App { fn render_ssh_profile_advanced_section( &self, form: &SshProfileForm, + errors: &SshFormErrors, cx: &mut Context, ) -> AnyElement { + // This section opens folded, and a proxy address saved back when the + // form wrote port 0 is wrong the moment the profile is opened. Let the + // error unfold it, or Save is disabled over something out of sight. + let open = form.show_advanced || errors.socks.is_some() || errors.http.is_some(); let mut section = v_flex().child(self.disclosure_header( "ssh-sec-adv", t(L10nKey::SettingsAdvanced), t(L10nKey::SettingsAdvancedSummary), - form.show_advanced, + open, cx, |this, cx| { if let Some(f) = this.ssh_form_mut() { @@ -3830,7 +4274,7 @@ impl Tty7App { } }, )); - if !form.show_advanced { + if !open { return section.into_any_element(); } @@ -3850,6 +4294,28 @@ impl Tty7App { cx, ) }; + // The two proxy addresses are the only advanced fields with a rule of + // their own, so they carry room for the complaint under the control. + let proxy_row = |this: &Self, + label: &str, + desc: &str, + input: &Entity, + error: Option<&SshFieldError>, + cx: &mut Context| { + let line = error.map(|e| field_error(e.message(), cx)); + this.settings_row( + label.to_string(), + desc.to_string(), + v_flex() + .gap_1() + .w(px(260.)) + .max_w_full() + .child(Input::new(input).small()) + .when_some(line, |col, line| col.child(line)) + .into_any_element(), + cx, + ) + }; let on_off = |b: bool| { if b { @@ -3904,18 +4370,20 @@ impl Tty7App { &form.proxy_command, cx, )) - .child(text_row( + .child(proxy_row( self, t(L10nKey::SettingsSocks5Proxy), t(L10nKey::SettingsSocks5ProxyDesc), &form.socks, + errors.socks.as_ref(), cx, )) - .child(text_row( + .child(proxy_row( self, t(L10nKey::SettingsHttpProxy), t(L10nKey::SettingsHttpProxyDesc), &form.http, + errors.http.as_ref(), cx, )) .child(self.subgroup_header(L10nKey::SettingsGroupAlgorithms, cx)) @@ -5801,19 +6269,14 @@ impl Tty7App { let http_proxy_value = http_proxy_input.read(cx).value().trim().to_string(); let http_proxy_invalid = !http_proxy_value.is_empty() && !tty7_core::daemon::install::proxy::is_valid_manual(&http_proxy_value); + let http_proxy_error = + http_proxy_invalid.then(|| field_error(t(L10nKey::SettingsAppHttpProxyInvalid), cx)); let http_proxy_control = v_flex() .gap_1() .w(px(260.)) .max_w_full() .child(Input::new(&http_proxy_input).small()) - .when(http_proxy_invalid, |this| { - this.child( - div() - .text_xs() - .text_color(danger) - .child(t(L10nKey::SettingsAppHttpProxyInvalid)), - ) - }) + .when_some(http_proxy_error, |this, line| this.child(line)) .into_any_element(); let logo = Arc::new(Image::from_bytes( @@ -6599,11 +7062,217 @@ mod tests { #[test] fn parse_host_port_handles_blank_and_ports() { - assert!(parse_host_port(" ").is_none()); - let hp = parse_host_port("example.com:2222").unwrap(); + assert!( + parse_host_port_checked(" ", DEFAULT_SOCKS_PORT) + .unwrap() + .is_none() + ); + let hp = parse_host_port_checked("example.com:2222", DEFAULT_SOCKS_PORT) + .unwrap() + .unwrap(); assert_eq!(hp.host, "example.com"); assert_eq!(hp.port, 2222); - assert_eq!(parse_host_port("host").unwrap().port, 0); + // Used to be port 0, which no proxy answers on. + assert_eq!( + parse_host_port_checked("host", DEFAULT_SOCKS_PORT) + .unwrap() + .unwrap() + .port, + DEFAULT_SOCKS_PORT + ); + } + + /// A form with the one field that is genuinely required, and nothing else. + fn draft_with_host() -> SshFormDraft { + SshFormDraft { + host: "example.com".to_string(), + ..Default::default() + } + } + + #[test] + fn a_profile_with_no_host_is_not_saveable() { + let (_, errors) = validate_ssh_draft(SshFormDraft::default(), &[]); + assert_eq!(errors.host, Some(SshFieldError::HostMissing)); + assert!(!errors.is_empty()); + } + + #[test] + fn spaces_are_not_a_host() { + let draft = SshFormDraft { + host: " ".to_string(), + ..Default::default() + }; + let (profile, errors) = validate_ssh_draft(draft, &[]); + assert_eq!(errors.host, Some(SshFieldError::HostMissing)); + assert_eq!(profile.host, ""); + } + + #[test] + fn a_name_is_not_required() { + // Every host imported from ~/.ssh/config arrives without one, and the + // list falls back to the address. + let (profile, errors) = validate_ssh_draft(draft_with_host(), &[]); + assert_eq!(profile.name, ""); + assert!(errors.is_empty()); + } + + #[test] + fn a_blank_port_still_means_22() { + let (profile, errors) = validate_ssh_draft(draft_with_host(), &[]); + assert_eq!(profile.port, 22); + assert_eq!(errors.port, None); + } + + #[test] + fn a_port_that_is_not_a_port_is_refused() { + // "0" parses as a u16 and used to be saved as written; the other two + // failed to parse and were silently rewritten to 22. + for text in ["0", "abc", "70000", "-1", "22 "] { + let draft = SshFormDraft { + port: text.to_string(), + ..draft_with_host() + }; + let (profile, errors) = validate_ssh_draft(draft, &[]); + match text { + "22 " => { + assert_eq!(errors.port, None, "{text:?} is a port with spare space"); + assert_eq!(profile.port, 22); + } + _ => { + assert_eq!(errors.port, Some(SshFieldError::PortRange), "{text:?}"); + assert!(!errors.is_empty()); + } + } + } + } + + #[test] + fn a_jump_host_that_exists_is_kept_by_id() { + let bastion = SshProfile::new("bastion"); + let draft = SshFormDraft { + jump: "bastion".to_string(), + ..draft_with_host() + }; + let (profile, errors) = validate_ssh_draft(draft, &[bastion.clone()]); + assert_eq!(profile.jump_host, Some(bastion.id)); + assert!(errors.is_empty()); + } + + #[test] + fn a_mistyped_jump_host_says_which_name_it_could_not_find() { + let draft = SshFormDraft { + jump: "bastian".to_string(), + ..draft_with_host() + }; + let (profile, errors) = validate_ssh_draft(draft, &[SshProfile::new("bastion")]); + assert_eq!( + errors.jump, + Some(SshFieldError::JumpUnknown("bastian".to_string())) + ); + assert_eq!( + profile.jump_host, None, + "a typo never saves as a direct connection" + ); + } + + #[test] + fn a_host_cannot_jump_through_itself() { + let me = SshProfile::new("prod"); + let draft = SshFormDraft { + id: me.id, + jump: "prod".to_string(), + ..draft_with_host() + }; + let (profile, errors) = validate_ssh_draft(draft, &[me]); + assert_eq!(errors.jump, Some(SshFieldError::JumpIsSelf)); + assert_eq!(profile.jump_host, None); + } + + #[test] + fn a_bare_proxy_host_takes_the_scheme_default_port() { + let draft = SshFormDraft { + socks: "socks.example.com".to_string(), + http: "http.example.com".to_string(), + ..draft_with_host() + }; + let (profile, errors) = validate_ssh_draft(draft, &[]); + assert_eq!( + profile.socks_proxy, + Some(HostPort::new("socks.example.com", 1080)) + ); + assert_eq!( + profile.http_proxy, + Some(HostPort::new("http.example.com", 8080)) + ); + assert!(errors.is_empty()); + } + + #[test] + fn a_proxy_address_with_a_colon_and_no_port_is_refused() { + for text in ["proxy.example.com:", "proxy.example.com:abc", "proxy:0"] { + let draft = SshFormDraft { + socks: text.to_string(), + ..draft_with_host() + }; + let (profile, errors) = validate_ssh_draft(draft, &[]); + assert_eq!( + errors.socks, + Some(SshFieldError::ProxyPortRange), + "{text:?}" + ); + assert_eq!(profile.socks_proxy, None, "{text:?}"); + } + } + + #[test] + fn a_form_that_cannot_be_saved_still_reports_what_it_would_save() { + // The Escape prompt asks whether the form differs from the config, so + // an invalid form has to hand back a profile to compare — otherwise a + // half-typed new host looks identical to the nothing on disk and + // Escape throws it away without asking. + let draft = SshFormDraft { + name: "half typed".to_string(), + ..Default::default() + }; + let (profile, errors) = validate_ssh_draft(draft, &[]); + assert!(!errors.is_empty()); + assert_eq!(profile.name, "half typed"); + } + + 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); } } diff --git a/src/ui/sftp.rs b/src/ui/sftp.rs index 18a2d781..e723fd15 100644 --- a/src/ui/sftp.rs +++ b/src/ui/sftp.rs @@ -115,21 +115,18 @@ impl SftpRoute { } } - pub(crate) fn transfer_list(&self) -> Vec { + pub(crate) fn transfer_list(&self) -> Result, String> { let Some(req) = self.workspace_op(crate::daemon::protocol::WorkspaceOp::SftpTransferList) else { return RemoteTerminal::sftp_transfer_list(self.pane_id); }; match RemoteTerminal::on_workspace(req) { - Ok(crate::daemon::protocol::DaemonMsg::SftpTransferProgress(jobs)) => jobs, - Ok(other) => { - log::warn!("unexpected reply to a workspace transfer list: {other:?}"); - Vec::new() - } - Err(e) => { - log::warn!("workspace transfer list failed: {e}"); - Vec::new() - } + Ok(crate::daemon::protocol::DaemonMsg::SftpTransferProgress(jobs)) => Ok(jobs), + Ok(other) => Err(t_fmt( + L10nKey::SftpErrorUnexpectedReply, + &[("reply", &format!("{other:?}"))], + )), + Err(e) => Err(e.to_string()), } } } @@ -143,6 +140,12 @@ pub(crate) struct SftpPanelState { pub(crate) filter_input: gpui::Entity, pub(crate) error: Option, pub(crate) jobs: Vec, + /// Why the last transfer poll came back empty-handed, if it did. + /// + /// Kept apart from `error`, which blanks the directory listing: a poll + /// that could not reach the daemon says nothing about the listing already + /// on screen, and the transfer tray is the only place it belongs. + jobs_error: Option, /// Uploads this panel started whose landing it has not listed yet. /// /// An upload is written to `.tty7-upload-` and renamed into @@ -188,6 +191,7 @@ impl SftpPanelState { filter_input, error: None, jobs: Vec::new(), + jobs_error: None, uploads_awaiting_listing: HashSet::new(), claimed_downloads: HashSet::new(), dismissed_jobs: HashSet::new(), @@ -223,6 +227,24 @@ fn uploads_still_running(owed: &HashSet, jobs: &[SftpJobProgress]) -> HashS .collect() } +/// What the tray shows after a poll: the jobs to draw, and the failure to say +/// out loud beside them. +/// +/// A poll that failed used to come back as an empty `Vec`, which reads as "the +/// transfers are all gone" — the tray disappeared and every upload the panel +/// was waiting on counted as landed. Over a link that is down that is not a +/// blink but the permanent answer, so a failure keeps the previous list and is +/// reported instead of replacing it. +fn apply_poll( + previous: Vec, + reply: Result, String>, +) -> (Vec, Option) { + match reply { + Ok(jobs) => (jobs, None), + Err(e) => (previous, Some(e)), + } +} + fn is_dir_like(e: &SftpEntry) -> bool { matches!(e.kind, SftpEntryKind::Dir) || (matches!(e.kind, SftpEntryKind::Symlink) && e.target_is_dir) @@ -362,6 +384,7 @@ impl Tty7App { self.sftp_panel.editing_path = None; self.sftp_panel.editing_path_sub.clear(); self.sftp_panel.jobs.clear(); + self.sftp_panel.jobs_error = None; self.sftp_panel.open_workspace = None; self.sftp_panel.poll_gen = self.sftp_panel.poll_gen.wrapping_add(1); cx.notify(); @@ -1007,12 +1030,28 @@ impl Tty7App { /// listing on screen was the one taken while the temporary name existed — /// and it stayed, so a finished upload read as a file with a hash glued to /// its name. - fn sftp_apply_jobs(&mut self, jobs: Vec, cx: &mut Context) { + /// + /// A poll that failed is not a job list, so it settles nothing: the uploads + /// still owe their listing, and asking for one now would only refresh from + /// the same unreachable daemon. + fn sftp_apply_jobs( + &mut self, + reply: Result, String>, + cx: &mut Context, + ) { + let previous = std::mem::take(&mut self.sftp_panel.jobs); + let (jobs, failure) = apply_poll(previous, reply); + let failed = failure.is_some(); + self.sftp_panel.jobs = jobs; + self.sftp_panel.jobs_error = failure; + if failed { + cx.notify(); + return; + } let owed = &self.sftp_panel.uploads_awaiting_listing; - let still_running = uploads_still_running(owed, &jobs); + let still_running = uploads_still_running(owed, &self.sftp_panel.jobs); let settled = still_running.len() != owed.len(); self.sftp_panel.uploads_awaiting_listing = still_running; - self.sftp_panel.jobs = jobs; cx.notify(); if settled { self.sftp_refresh(cx); @@ -1568,7 +1607,11 @@ impl Tty7App { .iter() .filter(|j| history || !self.sftp_panel.dismissed_jobs.contains(&j.job_id)) .collect(); - if jobs.is_empty() && !history { + // A poll that failed is worth a tray of its own. Without one the whole + // footer vanishes at the moment the panel stops being able to say + // anything about the transfers, which reads as "they are all finished". + let jobs_error = self.sftp_panel.jobs_error.as_ref(); + if jobs.is_empty() && !history && jobs_error.is_none() { return None; } @@ -1596,7 +1639,12 @@ impl Tty7App { } else { 0.0 }; - let summary = if running > 0 { + // The failed poll outranks the counts, because the counts are only as + // fresh as the last poll that got through and the summary is the one + // line a collapsed tray gets to say. + let summary = if let Some(e) = jobs_error { + t_fmt(L10nKey::SftpTransferListFailed, &[("error", e)]) + } else if running > 0 { t_fmt( L10nKey::SftpTransferSummaryRunning, &[ @@ -1612,7 +1660,7 @@ impl Tty7App { } else { t(L10nKey::SftpTransferSummaryIdle).to_string() }; - let summary_color = if running == 0 && failed > 0 { + let summary_color = if jobs_error.is_some() || (running == 0 && failed > 0) { danger } else { muted @@ -1671,13 +1719,23 @@ impl Tty7App { let body = expanded.then(|| { let inner: Div = if jobs.is_empty() { + // The summary above says the same thing when a poll failed, but + // it is a single truncated line; this one wraps, so it is where + // the reason is actually readable. + let (text, color): (gpui::SharedString, _) = match jobs_error { + Some(e) => ( + t_fmt(L10nKey::SftpTransferListFailed, &[("error", e)]).into(), + danger, + ), + None => (t(L10nKey::SftpNoTransfers).into(), muted), + }; v_flex().child( div() .px(px(CONTENT_INSET)) .py(px(3.)) .text_size(rems(META)) - .text_color(muted) - .child(t(L10nKey::SftpNoTransfers)), + .text_color(color) + .child(text), ) } else { let mut list = v_flex().px(px(CONTENT_INSET)).pb(px(6.)).gap(px(6.)); @@ -1883,6 +1941,36 @@ mod tests { assert_eq!(running, HashSet::from([8])); } + #[test] + fn a_failed_poll_keeps_the_transfers_it_cannot_see() { + let previous = vec![upload(7, SftpJobState::Running)]; + let (jobs, failure) = apply_poll(previous.clone(), Err("broken pipe".into())); + assert_eq!(jobs.len(), 1, "the last list anyone saw is still the truth"); + assert_eq!(jobs[0].job_id, 7); + assert_eq!(failure.as_deref(), Some("broken pipe")); + + // And the upload is still owed its listing, so nothing settles behind + // a link that has gone quiet. + assert_eq!( + uploads_still_running(&HashSet::from([7]), &jobs), + HashSet::from([7]) + ); + } + + #[test] + fn a_poll_that_got_through_replaces_the_list_and_clears_the_failure() { + let previous = vec![upload(7, SftpJobState::Running)]; + let (jobs, failure) = apply_poll(previous, Ok(vec![upload(8, SftpJobState::Done)])); + assert_eq!(jobs.len(), 1); + assert_eq!(jobs[0].job_id, 8); + assert!(failure.is_none()); + + // An empty reply from a daemon that answered really is an empty list. + let (jobs, failure) = apply_poll(jobs, Ok(Vec::new())); + assert!(jobs.is_empty()); + assert!(failure.is_none()); + } + #[test] fn a_second_download_is_numbered_rather_than_written_over_the_first() { let dir = tempfile::tempdir().expect("tempdir"); diff --git a/src/ui/ssh_connect.rs b/src/ui/ssh_connect.rs index f3d2e94c..9dc31482 100644 --- a/src/ui/ssh_connect.rs +++ b/src/ui/ssh_connect.rs @@ -191,13 +191,18 @@ fn build_spec_inner( let mut key_passphrases: HashMap = HashMap::new(); if matches!(profile.auth, AuthMode::Auto | AuthMode::PublicKey) { - // Explicit files, then the same `~/.ssh` defaults the daemon probes - // (#484): it looks passphrases up by the candidate string, so both - // sides must iterate the one shared list. - for path in identity_files - .iter() - .chain(crate::core::ssh_profile::default_identity_candidates().iter()) - { + // Whichever list the daemon will actually offer (#484, #513): the + // profile's own files, or — only when it names none — the same + // `~/.ssh` defaults, from the one shared candidate list. The daemon + // looks passphrases up by the candidate string, so both sides must + // spell them identically. + let owned = identity_files.clone(); + let probed = if owned.is_empty() { + crate::core::ssh_profile::default_identity_candidates() + } else { + owned + }; + for path in &probed { let Ok(bytes) = std::fs::read(path) else { continue; }; @@ -271,8 +276,12 @@ fn map_proxy(profile: &SshProfile) -> SshProxy { return SshProxy::Command(cmd.clone()); } } + // Port 0 is not somewhere a proxy listens. The settings form used to write + // it whenever the address had no port or an unparseable one, so configs + // carrying it are already on disk; connecting direct is the honest reading + // of an address that names nowhere. if let Some(HostPort { host, port }) = &profile.socks_proxy { - if !host.is_empty() { + if !host.is_empty() && *port != 0 { return SshProxy::Socks { host: host.clone(), port: *port, @@ -280,7 +289,7 @@ fn map_proxy(profile: &SshProfile) -> SshProxy { } } if let Some(HostPort { host, port }) = &profile.http_proxy { - if !host.is_empty() { + if !host.is_empty() && *port != 0 { return SshProxy::Http { host: host.clone(), port: *port, @@ -620,4 +629,22 @@ mod tests { SshProxy::Command(_) )); } + + #[test] + fn a_proxy_on_port_zero_is_no_proxy() { + // What the settings form wrote for `proxy.example.com` before it + // checked the port, and what is still sitting in configs saved then. + let store = InMemoryCredentialStore::new(); + let mut p = profile("web", "h", "u"); + p.socks_proxy = Some(HostPort::new("socks", 0)); + assert!(matches!( + build_native_ssh_spec(&p, &[], &store, true).proxy, + SshProxy::None + )); + p.http_proxy = Some(HostPort::new("http", 8080)); + assert!(matches!( + build_native_ssh_spec(&p, &[], &store, true).proxy, + SshProxy::Http { .. } + )); + } } diff --git a/src/ui/ssh_prompt.rs b/src/ui/ssh_prompt.rs index 34fe217b..9a31c98b 100644 --- a/src/ui/ssh_prompt.rs +++ b/src/ui/ssh_prompt.rs @@ -5,7 +5,7 @@ use gpui::{ use gpui_component::button::{Button, ButtonVariants as _}; use gpui_component::checkbox::Checkbox; use gpui_component::input::{Input, InputEvent, InputState}; -use gpui_component::{ActiveTheme as _, Sizable as _, h_flex, v_flex}; +use gpui_component::{ActiveTheme as _, Disableable as _, Sizable as _, h_flex, v_flex}; use crate::core::keychain::{CredentialStore as _, OsCredentialStore}; use crate::daemon::protocol::{AuthPromptKind, AuthResponse, SshPhase}; @@ -60,6 +60,9 @@ pub(crate) enum PromptModel { port: u16, algorithm: String, fingerprint: String, + /// Set when the host is already on file under some other algorithm, so + /// the sheet can say why a known host is offering an unseen key. + previously_known_as: Option, }, HostKeyChanged { host: String, @@ -139,11 +142,13 @@ impl PromptModel { port, algorithm, fingerprint_sha256, + previously_known_as, } => PromptModel::HostKeyUnknown { host, port, algorithm, fingerprint: fingerprint_sha256, + previously_known_as, }, AuthPromptKind::HostKeyChanged { host, @@ -371,10 +376,14 @@ impl Tty7App { subs.push(cx.subscribe_in( input, window, - |this, _input, ev: &InputEvent, window, cx| { - if matches!(ev, InputEvent::PressEnter { .. }) { - this.submit_ssh_prompt(window, cx); - } + |this, _input, ev: &InputEvent, window, cx| match ev { + InputEvent::PressEnter { .. } => this.submit_ssh_prompt(window, cx), + // The changed-host sheet enables Override off the + // typed text, so the flag has to be recomputed + // between keystrokes rather than at whatever + // repaint happened to come along. + InputEvent::Change => cx.notify(), + _ => {} }, )); } @@ -421,10 +430,12 @@ impl Tty7App { subs.push(cx.subscribe_in( input, window, - |this, _input, ev: &InputEvent, window, cx| { - if matches!(ev, InputEvent::PressEnter { .. }) { - this.submit_ssh_prompt(window, cx); - } + |this, _input, ev: &InputEvent, window, cx| match ev { + InputEvent::PressEnter { .. } => this.submit_ssh_prompt(window, cx), + // Same reason as the pane-routed path above: Override's + // enabled state is read off the input on every repaint. + InputEvent::Change => cx.notify(), + _ => {} }, )); } @@ -456,6 +467,18 @@ impl Tty7App { .map(|i| i.read(cx).value().to_string()) .collect(); + // A changed host key is the one prompt where submitting the wrong thing + // is indistinguishable from aborting: the decision it sends for + // anything but "yes" is byte-for-byte what Abort sends, and the sheet + // closed either way. So Enter on a half-typed answer looked like the + // app had swallowed the connection. Leave the sheet up instead; the + // rejection stays available on Abort, where the user meant it. + if let PromptModel::HostKeyChanged { .. } = &model { + if !changed_confirmed(values.first().map(String::as_str).unwrap_or_default()) { + return; + } + } + let (response, write) = match &model { PromptModel::Password { user, @@ -830,6 +853,7 @@ impl Tty7App { fingerprint, port, host, + previously_known_as, } => card .child(div().text_xs().child(format!("{host}:{port} {algorithm}"))) .child( @@ -838,6 +862,21 @@ impl Tty7App { .font_family("monospace") .child(fingerprint.clone()), ) + // A host that already has an entry under another algorithm is + // the ordinary way a server grows an ed25519 key beside its old + // ssh-rsa one. Saying so is the difference between "who is + // this?" and "this is the host you know, with a second key". + .when_some(previously_known_as.as_ref(), |c, previous| { + c.child( + div() + .text_xs() + .text_color(cx.theme().muted_foreground) + .child(crate::ui::i18n::t_fmt( + crate::ui::i18n::L10nKey::SshPromptHostKeyNewAlgorithm, + &[("previous_algorithm", previous), ("algorithm", algorithm)], + )), + ) + }) .child( h_flex() .justify_end() @@ -866,8 +905,20 @@ impl Tty7App { old_fingerprint, port, host, - } => card - .child(div().text_xs().text_color(danger).child(crate::ui::i18n::t( + } => { + // Override without "yes" typed used to send the *rejection* — + // the same bytes Abort sends — and close the sheet, so the + // button read as a way through and behaved as a way out. It is + // dead until the word is there, which is what the line above + // the field has been claiming all along. + let typed = self + .ssh_prompt + .inputs + .first() + .map(|i| i.read(cx).value().to_string()) + .unwrap_or_default(); + let can_override = changed_confirmed(&typed); + card.child(div().text_xs().text_color(danger).child(crate::ui::i18n::t( crate::ui::i18n::L10nKey::SshPromptHostKeyChangedBody, ))) .child(div().text_xs().child(format!("{host}:{port} {algorithm}"))) @@ -894,6 +945,18 @@ impl Tty7App { crate::ui::i18n::L10nKey::HostKeyOverrideMessage, ))) .child(self.render_ssh_input(0)) + // Only once they have typed something: an empty field is not a + // mistake to be corrected, it is where everyone starts. + .when(!typed.trim().is_empty() && !can_override, |c| { + c.child( + div() + .text_xs() + .text_color(cx.theme().muted_foreground) + .child(crate::ui::i18n::t( + crate::ui::i18n::L10nKey::SshPromptTypeYesToOverride, + )), + ) + }) .child( h_flex() .justify_end() @@ -905,6 +968,7 @@ impl Tty7App { Button::new("ssh-hkc-override") .label(crate::ui::i18n::t(crate::ui::i18n::L10nKey::Override)) .small() + .disabled(!can_override) .on_click(cx.listener(|this, _, window, cx| { this.submit_ssh_prompt(window, cx) })), @@ -918,7 +982,8 @@ impl Tty7App { this.cancel_ssh_prompt(window, cx) })), ), - ), + ) + } }; card.into_any_element() @@ -1234,6 +1299,58 @@ mod tests { } ); } + + /// `changed_confirmed` is also what enables the Override button, so the + /// button and the decision can never disagree about what "yes" means — the + /// bug was a button that offered a way through and sent the rejection. + #[test] + fn override_is_enabled_by_exactly_what_accepts() { + for typed in ["", " ", "y", "no", "yesss"] { + assert!( + !changed_confirmed(typed), + "{typed:?} must leave Override disabled" + ); + assert_eq!( + host_key_changed_decision(typed), + AuthResponse::HostKeyDecision { + accept: false, + remember: false + } + ); + } + for typed in ["yes", "YES", " yes "] { + assert!(changed_confirmed(typed), "{typed:?} must enable Override"); + } + } + + /// The host is known, just not by this algorithm — the mild confirmation, + /// carrying the algorithm it *is* known by, and never the danger sheet. + #[test] + fn a_new_algorithm_raises_the_unknown_host_sheet_not_the_changed_one() { + let m = PromptModel::from_prompt( + AuthPromptKind::HostKeyUnknown { + host: "example.com".into(), + port: 22, + algorithm: "ssh-ed25519".into(), + fingerprint_sha256: "SHA256:new".into(), + previously_known_as: Some("ssh-rsa".into()), + }, + None, + false, + ) + .unwrap(); + assert_eq!( + m, + PromptModel::HostKeyUnknown { + host: "example.com".into(), + port: 22, + algorithm: "ssh-ed25519".into(), + fingerprint: "SHA256:new".into(), + previously_known_as: Some("ssh-rsa".into()), + } + ); + assert_eq!(m.input_count(), 0); + } } #[cfg(test)] @@ -1281,6 +1398,7 @@ mod focus_tests { port: 22, algorithm: "ssh-ed25519".into(), fingerprint: "SHA256:zzz".into(), + previously_known_as: None, }); window.focus(&app.ssh_prompt.focus_handle, cx); // A headless harness has no live pane, so `focus_active`