From ec7a7a2fa56f451a7234eb4ca7b4faebfdbde784 Mon Sep 17 00:00:00 2001 From: l0ng-ai <24760907+l0ng-ai@users.noreply.github.com> Date: Sat, 22 Aug 2026 23:15:52 +0800 Subject: [PATCH] fix(ssh): a forward with no bind address binds loopback, not the resolver's guess MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three forms build a port forward: the settings sheet, the side panel, and a `LocalForward` line read out of `~/.ssh/config`. Two of them turned a blank bind address into `127.0.0.1`. The settings sheet passed the empty string through to the bind. Where that lands is not ours to decide once it leaves: `""` is whatever getaddrinfo makes of it. On macOS and glibc today that is loopback, which is why nothing looked wrong — but it is the resolver's answer, not the app's, and under `AI_PASSIVE` semantics the same string means every interface. An SSH tunnel reachable from the network is not a state to arrive at through a default nobody chose, and the sheet's own field already shows "localhost" as its placeholder, so a user leaving it blank has been told what they are getting. The side panel's `collect` claimed, in its doc, to apply "the same conditions the settings sheet's `ForwardRuleForm::collect` applies". It did not, and that is the sort of comment that stops anyone checking. Both now call one function beside `HostPort`, along with the ssh_config parser, which had the third copy of the same literal. Tested from both ends: the rule the settings sheet builds from a blank field, and the rule the side panel builds from the same blank, are asserted equal — and against the unfixed sheet the first half fails. --- crates/tty7-core/src/core/ssh_profile.rs | 45 +++++++++++++++++ src/core/ssh_config.rs | 14 ++---- src/ui/forwards.rs | 9 ++-- src/ui/settings.rs | 62 +++++++++++++++++++++++- 4 files changed, 112 insertions(+), 18 deletions(-) diff --git a/crates/tty7-core/src/core/ssh_profile.rs b/crates/tty7-core/src/core/ssh_profile.rs index 5e189d5d..e9e9f0a8 100644 --- a/crates/tty7-core/src/core/ssh_profile.rs +++ b/crates/tty7-core/src/core/ssh_profile.rs @@ -112,6 +112,27 @@ impl HostPort { } } +/// The address a forward binds when the user did not name one. +/// +/// Loopback, always. Left empty, the address is whatever the platform's +/// resolver makes of `""` — loopback on macOS and glibc today, but that is +/// getaddrinfo's choice and not ours, and the same string under `AI_PASSIVE` +/// semantics means every interface. An SSH tunnel is exactly the thing that +/// must not be opened to the network by a default nobody chose. +/// +/// One function because there are three forms that build a forward — the +/// settings sheet, the side panel, and `LocalForward` lines read out of +/// `~/.ssh/config` — and a rule typed in one has to mean what it means in the +/// others. Two of them already normalized, with the same literal written +/// twice; the settings sheet did not, so a blank there was the only one that +/// left the decision to the resolver. +pub fn bind_host_or_loopback(host: &str) -> String { + match host.trim() { + "" => "127.0.0.1".to_string(), + named => named.to_string(), + } +} + #[derive(Debug, Clone, Copy, PartialEq, Eq, Default, Deserialize, Serialize)] #[serde(rename_all = "kebab-case")] pub enum AuthMode { @@ -539,6 +560,30 @@ mod tests { ); } + /// A forward with no bind address binds loopback, never the world. + /// + /// Left to the platform, `""` is whatever getaddrinfo decides — loopback + /// on macOS and glibc, every interface under `AI_PASSIVE` semantics. An + /// SSH tunnel reachable from the network is not a thing to arrive at by + /// way of an unset default, so the answer is given here instead of asked + /// for. + #[test] + fn a_forward_with_no_bind_address_binds_loopback() { + assert_eq!(bind_host_or_loopback(""), "127.0.0.1"); + assert_eq!( + bind_host_or_loopback(" "), + "127.0.0.1", + "and blank is empty" + ); + assert_eq!(bind_host_or_loopback("\t"), "127.0.0.1"); + + // A named address is the user saying it out loud, including the one + // that does mean every interface. + assert_eq!(bind_host_or_loopback("0.0.0.0"), "0.0.0.0"); + assert_eq!(bind_host_or_loopback("localhost"), "localhost"); + assert_eq!(bind_host_or_loopback(" ::1 "), "::1", "and it is trimmed"); + } + #[test] fn profile_serde_defaults_and_round_trip() { let p: SshProfile = serde_json::from_str(r#"{"name":"min","host":"h"}"#).unwrap(); diff --git a/src/core/ssh_config.rs b/src/core/ssh_config.rs index 021271b3..8b606f37 100644 --- a/src/core/ssh_config.rs +++ b/src/core/ssh_config.rs @@ -1,7 +1,9 @@ use std::collections::{BTreeMap, HashSet}; use std::path::{Path, PathBuf}; -use crate::core::ssh_profile::{ForwardKind, ForwardRule, HostPort, SshProfile as ManagedProfile}; +use crate::core::ssh_profile::{ + ForwardKind, ForwardRule, HostPort, SshProfile as ManagedProfile, bind_host_or_loopback, +}; const MAX_INCLUDE_DEPTH: usize = 8; const MAX_CONFIG_FILES: usize = 256; @@ -902,7 +904,7 @@ fn parse_algorithm_list(value: &str) -> Option> { fn parse_forward_rule(kind: ForwardKind, value: &str) -> Option { let words = split_words(value); let (bind_host, bind_port) = parse_forward_endpoint(words.first()?)?; - let bind = HostPort::new(forward_bind_host(bind_host), bind_port); + let bind = HostPort::new(bind_host_or_loopback(&bind_host), bind_port); let target = match kind { ForwardKind::Dynamic => HostPort::default(), ForwardKind::Local | ForwardKind::Remote => { @@ -938,14 +940,6 @@ fn parse_forward_endpoint(token: &str) -> Option<(String, u16)> { } } -fn forward_bind_host(host: String) -> String { - if host.is_empty() { - "127.0.0.1".to_string() - } else { - host - } -} - #[cfg(test)] mod tests { diff --git a/src/ui/forwards.rs b/src/ui/forwards.rs index a9784dfa..ad8012be 100644 --- a/src/ui/forwards.rs +++ b/src/ui/forwards.rs @@ -52,12 +52,9 @@ impl ForwardFields { } (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(), - }; + // The panel's own default, and the one the strip's tooltip promises: + // an empty bind host is loopback, not every interface. + let bind_host = crate::core::ssh_profile::bind_host_or_loopback(&self.bind_host); let description = self.description.trim(); Some(SshForwardRule { kind: self.kind, diff --git a/src/ui/settings.rs b/src/ui/settings.rs index 416c4a00..bb7c19ff 100644 --- a/src/ui/settings.rs +++ b/src/ui/settings.rs @@ -28,7 +28,8 @@ use crate::core::config::{ }; use crate::core::keychain::CredentialRef; use crate::core::ssh_profile::{ - Algorithms, AuthMode, ForwardKind, ForwardRule, HostPort, SshProfile, to_connect_string, + Algorithms, AuthMode, ForwardKind, ForwardRule, HostPort, SshProfile, bind_host_or_loopback, + to_connect_string, }; use crate::daemon::protocol::{SshTestNeed, SshTestReport}; use crate::ui::app::{ @@ -1070,7 +1071,10 @@ impl ForwardRuleForm { fn collect(&self, cx: &App) -> Option { let val = |e: &Entity| e.read(cx).value().trim().to_string(); let bind_port: u16 = val(&self.bind_port).parse().ok().filter(|p| *p > 0)?; - let bind = HostPort::new(val(&self.bind_host), bind_port); + // Blank means loopback, the same as it does in the side panel's form + // and in a `LocalForward` line with no bind address. Left as typed, + // the address became whatever the platform's resolver made of `""`. + let bind = HostPort::new(bind_host_or_loopback(&val(&self.bind_host)), bind_port); let target = if self.kind == ForwardKind::Dynamic { HostPort::default() } else { @@ -8449,3 +8453,57 @@ mod search_index_tests { } } } + +#[cfg(all(test, unix))] +mod forward_bind_gpui_tests { + use super::*; + use crate::ui::app::test_window::harness; + use gpui::TestAppContext; + + /// The settings sheet and the side panel are two forms for the same rule, + /// and a blank bind address has to mean the same thing in both. + /// + /// It did not. `ForwardFields::collect` in the side panel normalized a + /// blank to loopback — and said in its own doc that it applies "the same + /// conditions the settings sheet's `ForwardRuleForm::collect` applies" — + /// while the sheet passed the empty string straight through to the bind. + /// `LocalForward` lines out of `~/.ssh/config` normalized too, so the + /// sheet was the one path of three that left the address to whatever the + /// platform's resolver makes of `""`. + #[gpui::test] + fn a_blank_bind_address_in_the_settings_sheet_is_loopback(cx: &mut TestAppContext) { + crate::core::config::pin_test_config_dir(); + let (app, mut vcx) = harness(cx); + + let rule = app.update_in(&mut vcx, |_app, window, cx| { + let row = seed_forward_row(window, cx, &ForwardRule::default()); + row.bind_port + .update(cx, |s, cx| s.set_value("8080", window, cx)); + row.target_host + .update(cx, |s, cx| s.set_value("10.0.0.9", window, cx)); + row.target_port + .update(cx, |s, cx| s.set_value("80", window, cx)); + // bind_host left as the placeholder shows it: empty. + row.collect(cx) + }); + + let rule = rule.expect("a rule with a port and a target is complete"); + assert_eq!( + rule.bind.host, "127.0.0.1", + "a blank bind address is loopback, not whatever `\"\"` resolves to" + ); + + // And the side panel, on the same blank, agrees. + let panel = crate::ui::forwards::ForwardFields { + kind: crate::daemon::protocol::SshForwardKind::Local, + bind_host: String::new(), + bind_port: "8080".into(), + target_host: "10.0.0.9".into(), + target_port: "80".into(), + description: String::new(), + } + .collect() + .expect("the same rule, typed in the other form"); + assert_eq!(panel.bind_host, rule.bind.host, "the two forms agree"); + } +}