From 253ef61da77d0e585edd9ebd1b0b937d4f607c12 Mon Sep 17 00:00:00 2001 From: Shadowy Super Coder Date: Tue, 1 Sep 2026 12:19:37 -0600 Subject: [PATCH 1/2] fix(start-os): filter the gateway-reported address for both mapping protocols Fixes #3784. `check_port` short-circuited to "open" on the port-map client's record of its last grant, and the two protocols disagreed about what the address in that record had to be. PCP reported whatever the gateway returned, so behind a second NAT a private address passed the check and nothing outside could connect. UPnP discarded a non-routable address at record time and fell through to the echo probe, so the same topology answered red. Same reachability, opposite verdicts by protocol. The shard's `ExternalIp` handler is where both protocols' addresses converge, so the filter moves there and applies to whichever protocol reported it. v6 passes through untouched: the GUA is the box's own address and `check_gua_port` never reads the mapping's. No new echoip traffic beyond what a UPnP gateway already generates on this topology: the fall-through is the path UPnP has always taken, and PCP now joins it only where the gateway's own address rules out a direct answer. Claude-Session: https://claude.ai/code/session_01RwXsV4feB6gSVXTedUrzxT --- projects/start-os/CHANGELOG.md | 8 ++++++++ shared-libs/crates/start-core/src/net/port_map/client.rs | 7 +++++++ 2 files changed, 15 insertions(+) diff --git a/projects/start-os/CHANGELOG.md b/projects/start-os/CHANGELOG.md index 7825fff85..84b3184dd 100644 --- a/projects/start-os/CHANGELOG.md +++ b/projects/start-os/CHANGELOG.md @@ -77,6 +77,14 @@ file tracks notable changes since the move to the monorepo. ### Fixed +- **The port-forwarding test reports a port as open to the Internet only where + it is reachable from the Internet.** Where StartOS's port-forward request was + granted by a router that sits behind another router, the test could pass a + port that nothing outside could reach, and two otherwise identical setups + could disagree depending on which forwarding protocol the router spoke. + StartOS now measures the port from the Internet in that case and reports + what it finds. + - **A downgrade to a version that cannot take over the service's data is refused before anything is downloaded or stopped**, with an explanation of what to do instead. diff --git a/shared-libs/crates/start-core/src/net/port_map/client.rs b/shared-libs/crates/start-core/src/net/port_map/client.rs index f88f70ab1..df83bb682 100644 --- a/shared-libs/crates/start-core/src/net/port_map/client.rs +++ b/shared-libs/crates/start-core/src/net/port_map/client.rs @@ -371,6 +371,9 @@ impl PortMapController { /// Gateway-assigned external IP if a TCP mapping is active for /// `(local_ip, external_port)`, else `None`. `Some` means the TCP port was /// forwarded automatically, so a remote reachability check can be skipped. + /// A v4 address the gateway reports from outside publicly routable space + /// yields `None`: the gateway is itself behind a NAT, and only a probe can + /// say whether anything reaches it. pub async fn mapped_external_ip(&self, local_ip: IpAddr, external_port: u16) -> Option { let (resp, rx) = oneshot::channel(); self.shard(local_ip) @@ -408,6 +411,10 @@ fn spawn_shard( .and_then(|(_, a)| match a { Active::Pcp(m) => m.external_ip(), Active::Upnp { external_ip } => external_ip.map(IpAddr::V4), + }) + .filter(|ip| match ip { + IpAddr::V4(v4) => upnp::is_wan_candidate(*v4), + IpAddr::V6(_) => true, }); let _ = resp.send(ip); } From 8178ae7076d7d9b2d1b101d05b0e1fa26057901e Mon Sep 17 00:00:00 2001 From: Shadowy Super Coder Date: Tue, 1 Sep 2026 12:30:53 -0600 Subject: [PATCH 2/2] test(start-os): cover the shard's external-address filter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Command::ExternalIp`'s body was inline in the shard's select loop, so nothing could reach it without spawning a shard against real interfaces. Extract it as `external_ip_of`, with the per-protocol match funnelling through one `routable_external_ip`. The UPnP arm and the key selection (right port, TCP only) are covered directly. The PCP arm is not constructible here — `crab_nat::PortMapping` has private fields and no public constructor — so it reaches the filter by construction: `external_ip_of` is the one call site and it wraps both arms. A live check is what proves a real PCP gateway behind a second NAT reports the private address this rejects. Claude-Session: https://claude.ai/code/session_018ZmVQMf744jaqpKUm5gthK --- .../start-core/src/net/port_map/client.rs | 118 +++++++++++++++--- 1 file changed, 103 insertions(+), 15 deletions(-) diff --git a/shared-libs/crates/start-core/src/net/port_map/client.rs b/shared-libs/crates/start-core/src/net/port_map/client.rs index df83bb682..cf0e5af41 100644 --- a/shared-libs/crates/start-core/src/net/port_map/client.rs +++ b/shared-libs/crates/start-core/src/net/port_map/client.rs @@ -386,6 +386,30 @@ impl PortMapController { } } +/// Gateway-reported external address of the active TCP mapping on +/// `external_port`, kept only where the public Internet can reach it. +fn external_ip_of(active: &BTreeMap, external_port: u16) -> Option { + active + .iter() + .find(|(k, _)| k.1 == external_port && k.3 == TransportProtocol::Tcp) + .and_then(|(_, a)| { + routable_external_ip(match a { + Active::Pcp(m) => m.external_ip(), + Active::Upnp { external_ip } => external_ip.map(IpAddr::V4), + }) + }) +} + +/// Discards a v4 address outside publicly routable space: the gateway is itself +/// behind a NAT, and only a probe can say whether anything reaches it. A v6 +/// mapping is on the host's own address and is kept as reported. +fn routable_external_ip(reported: Option) -> Option { + reported.filter(|ip| match ip { + IpAddr::V4(v4) => upnp::is_wan_candidate(*v4), + IpAddr::V6(_) => true, + }) +} + fn spawn_shard( interfaces: Watch>, ) -> mpsc::UnboundedSender { @@ -402,21 +426,7 @@ fn spawn_shard( Some(Command::Ensure { key, spec }) => state.ensure(&interfaces, key, spec).await, Some(Command::Remove { key }) => state.remove(key).await, Some(Command::ExternalIp { external_port, resp }) => { - let ip = state - .active - .iter() - .find(|(k, _)| { - k.1 == external_port && k.3 == TransportProtocol::Tcp - }) - .and_then(|(_, a)| match a { - Active::Pcp(m) => m.external_ip(), - Active::Upnp { external_ip } => external_ip.map(IpAddr::V4), - }) - .filter(|ip| match ip { - IpAddr::V4(v4) => upnp::is_wan_candidate(*v4), - IpAddr::V6(_) => true, - }); - let _ = resp.send(ip); + let _ = resp.send(external_ip_of(&state.active, external_port)); } None => break, }, @@ -1038,6 +1048,84 @@ mod tests { Watch::new(OrdMap::new()) } + // The filter both protocols' reported addresses funnel through. PCP grants + // cannot be built here (`crab_nat::PortMapping` has private fields), so the + // Pcp arm reaches this only by construction: `external_ip_of` has the one + // call site, and it wraps the match over both arms. + #[test] + fn a_gateway_behind_another_nat_reports_no_usable_address() { + for ip in ["192.168.1.1", "10.0.0.1", "172.16.0.1"] { + let reported: IpAddr = ip.parse::().unwrap().into(); + assert_eq!( + routable_external_ip(Some(reported)), + None, + "{ip} is not reachable from the public Internet" + ); + } + } + + #[test] + fn a_routable_address_is_reported_as_given() { + for ip in ["1.2.3.4", "93.184.216.34"] { + let reported: IpAddr = ip.parse::().unwrap().into(); + assert_eq!(routable_external_ip(Some(reported)), Some(reported), "{ip}"); + } + } + + // v6 has no NAT: the mapping is on the host's own GUA, and `check_gua_port` + // only asks whether a pinhole exists. + #[test] + fn a_v6_pinhole_is_kept_as_reported() { + let gua: IpAddr = "2001:470:1f0b:1::1".parse().unwrap(); + assert_eq!(routable_external_ip(Some(gua)), Some(gua)); + } + + // NAT-PMP grants carry no external address. + #[test] + fn an_addressless_grant_has_no_answer() { + assert_eq!(routable_external_ip(None), None); + } + + #[test] + fn a_upnp_mapping_answers_only_for_its_own_tcp_port() { + let ip: IpAddr = Ipv4Addr::new(10, 59, 0, 2).into(); + let public = Ipv4Addr::new(1, 2, 3, 4); + let mut active = BTreeMap::new(); + active.insert( + (ip, 443, None, TransportProtocol::Tcp), + Active::Upnp { + external_ip: Some(public), + }, + ); + active.insert( + (ip, 8080, None, TransportProtocol::Udp), + Active::Upnp { + external_ip: Some(public), + }, + ); + + assert_eq!(external_ip_of(&active, 443), Some(IpAddr::V4(public))); + assert_eq!(external_ip_of(&active, 8080), None, "UDP is not TCP"); + assert_eq!( + external_ip_of(&active, 444), + None, + "no mapping on that port" + ); + } + + #[test] + fn a_upnp_mapping_behind_another_nat_answers_nothing() { + let ip: IpAddr = Ipv4Addr::new(10, 59, 0, 2).into(); + let mut active = BTreeMap::new(); + active.insert( + (ip, 443, None, TransportProtocol::Tcp), + Active::Upnp { + external_ip: Some(Ipv4Addr::new(192, 168, 8, 1)), + }, + ); + assert_eq!(external_ip_of(&active, 443), None); + } + // Distinct hostnames on the same external port are independent mappings; // removing one (or adding a plain mapping) never clobbers the others. #[tokio::test]