mirror of
https://github.com/l0ng-ai/tty7.git
synced 2026-09-21 16:02:20 +00:00
fix(ssh): a forward with no bind address binds loopback, not the resolver's guess
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.
This commit is contained in:
@@ -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();
|
||||
|
||||
+4
-10
@@ -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<Vec<String>> {
|
||||
fn parse_forward_rule(kind: ForwardKind, value: &str) -> Option<ForwardRule> {
|
||||
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 {
|
||||
|
||||
|
||||
+3
-6
@@ -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,
|
||||
|
||||
+60
-2
@@ -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<ForwardRule> {
|
||||
let val = |e: &Entity<InputState>| 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");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user