From 84947b2deb468cc7eb2c65524a1240b41e76a4b5 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Tue, 11 Aug 2026 19:58:33 +0800 Subject: [PATCH] fix(settings): refuse a half-filled SSH profile instead of saving it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The SSH profile form saved whatever was in it. An empty host wrote a profile that renders as a blank row in the host list and hands `TcpStream::connect` an empty name — and Connect had no gate at all, so the first thing the user heard about it was a DNS error naming nothing. A jump host with a typo in it resolved to `None` and saved as a direct connection, with the field blank the next time the form opened. And `parse_host_port` was `parse().unwrap_or(0)`, so `proxy.example.com`, `proxy.example.com:` and `proxy.example.com:88O` all saved a proxy on port 0, which the socket layer got to explain (#492, #493, #494). The rules now live in `validate_ssh_draft`, a plain function over a plain-String snapshot of the form, which returns both the profile the form would save and what is wrong with it. Both, always: the Escape prompt asks whether the form differs from the config, and handing back only the errors would make a brand-new invalid profile compare equal to the nothing on disk — Escape would throw the typing away without asking. Only the host is required. A name is not, because the list already falls back to the address and every host imported from ~/.ssh/config arrives without one. A blank port still means 22, but a non-empty one has to be a port, so "0", "abc" and "70000" are refused rather than saved as written or quietly rewritten. A proxy address with no port takes the scheme's default (1080 / 8080) and `host_port_text` writes that back into the field, so the number it picked is visible; a colon with nothing usable after it is an error. `map_proxy` also stops treating port 0 as a proxy, because configs written before this are already on disk. Each complaint prints under the field it is about, and Save and Connect are disabled while any of them stands; a section holding one unfolds so the disabled button always has a visible reason. The "needs a host" line waits until the name/host/port/user group has something in it — every field notifies per keystroke, so otherwise a new host would be told off before anyone had typed a character. Consequence: on a pristine new form Save is now disabled where it used to be enabled. --- src/ui/i18n/en.rs | 7 + src/ui/i18n/ja.rs | 11 + src/ui/i18n/mod.rs | 5 + src/ui/i18n/zh.rs | 5 + src/ui/settings.rs | 640 ++++++++++++++++++++++++++++++++++++------ src/ui/ssh_connect.rs | 26 +- 6 files changed, 609 insertions(+), 85 deletions(-) diff --git a/src/ui/i18n/en.rs b/src/ui/i18n/en.rs index 6914debd..0ddf3543 100644 --- a/src/ui/i18n/en.rs +++ b/src/ui/i18n/en.rs @@ -229,6 +229,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", @@ -242,6 +244,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", @@ -274,6 +278,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", diff --git a/src/ui/i18n/ja.rs b/src/ui/i18n/ja.rs index a8b2cccb..7ff4622b 100644 --- a/src/ui/i18n/ja.rs +++ b/src/ui/i18n/ja.rs @@ -226,6 +226,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 => "認証方式", @@ -239,6 +241,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 件", @@ -271,6 +279,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 => "暗号方式", diff --git a/src/ui/i18n/mod.rs b/src/ui/i18n/mod.rs index 489d29f7..a57784d1 100644 --- a/src/ui/i18n/mod.rs +++ b/src/ui/i18n/mod.rs @@ -244,6 +244,8 @@ l10n_keys! { SettingsNameDesc, SettingsHost, SettingsHostDesc, + SettingsHostRequired, + SettingsPortInvalid, SettingsUser, SettingsUserDesc, SettingsAuth, @@ -255,6 +257,8 @@ l10n_keys! { SettingsAuthMode2Fa, SettingsJumpHost, SettingsJumpHostDesc, + SettingsJumpHostUnknown, + SettingsJumpHostSelf, SettingsNoneSummary, SettingsNoneLower, SettingsPortForwarding, @@ -284,6 +288,7 @@ l10n_keys! { SettingsSocks5ProxyDesc, SettingsHttpProxy, SettingsHttpProxyDesc, + SettingsProxyPortInvalid, SettingsKexAlgorithms, SettingsKexAlgorithmsDesc, SettingsCiphers, diff --git a/src/ui/i18n/zh.rs b/src/ui/i18n/zh.rs index ec546971..6ce149f7 100644 --- a/src/ui/i18n/zh.rs +++ b/src/ui/i18n/zh.rs @@ -204,6 +204,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 => "认证", @@ -215,6 +217,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 条规则，随连接打开", @@ -243,6 +247,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 => "加密算法", diff --git a/src/ui/settings.rs b/src/ui/settings.rs index 704e9f3e..05db1a97 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -840,6 +840,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 +924,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 +982,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, @@ -3049,59 +3288,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 +3362,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 +3375,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 @@ -3310,7 +3554,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() @@ -3379,20 +3624,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( @@ -3411,21 +3674,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, ), @@ -3478,9 +3748,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() } @@ -3526,6 +3796,7 @@ impl Tty7App { fn render_ssh_profile_jump_section( &self, form: &SshProfileForm, + errors: &SshFormErrors, cx: &mut Context, ) -> AnyElement { let summary = { @@ -3536,11 +3807,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() { @@ -3549,15 +3824,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, ), @@ -3635,14 +3913,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 @@ -3746,13 +4031,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() } @@ -3788,13 +4067,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() { @@ -3803,7 +4087,7 @@ impl Tty7App { } }, )); - if !form.show_advanced { + if !open { return section.into_any_element(); } @@ -3823,6 +4107,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 { @@ -3877,18 +4183,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)) @@ -5774,19 +6082,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( @@ -6572,11 +6875,182 @@ 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"); } } diff --git a/src/ui/ssh_connect.rs b/src/ui/ssh_connect.rs index 06c6699b..70475da4 100644 --- a/src/ui/ssh_connect.rs +++ b/src/ui/ssh_connect.rs @@ -265,8 +265,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, @@ -274,7 +278,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, @@ -551,4 +555,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 { .. } + )); + } }