diff --git a/crates/wzp-client/src/reflect.rs b/crates/wzp-client/src/reflect.rs index 16096c5..26577b2 100644 --- a/crates/wzp-client/src/reflect.rs +++ b/crates/wzp-client/src/reflect.rs @@ -275,14 +275,63 @@ pub fn determine_role( } } +/// Returns `true` if the address is in an RFC1918 / link-local / +/// loopback range and therefore cannot possibly be a post-NAT +/// reflex address from the public internet's point of view. +/// +/// A probe against a relay ON THE SAME LAN as the client will +/// naturally report the client's LAN IP back (because there's no +/// NAT between them) — that observation is real but says nothing +/// about the client's public-internet-facing NAT state. Mixing +/// LAN reflex addrs with public-internet reflex addrs in +/// `classify_nat` would always report `Multiple` (different IPs) +/// and falsely warn about symmetric NAT. Filter them out before +/// classifying. +fn is_private_or_loopback(addr: &SocketAddr) -> bool { + match addr.ip() { + std::net::IpAddr::V4(v4) => { + let o = v4.octets(); + v4.is_loopback() + || v4.is_private() // 10/8, 172.16/12, 192.168/16 + || v4.is_link_local() // 169.254/16 + || (o[0] == 100 && (o[1] & 0xc0) == 0x40) // 100.64/10 CGNAT shared + } + std::net::IpAddr::V6(v6) => { + v6.is_loopback() || v6.is_unspecified() || (v6.segments()[0] & 0xffc0) == 0xfe80 // fe80::/10 link-local + } + } +} + /// Pure-function NAT classifier — split out for unit testing /// without touching the network. +/// +/// Only considers probes whose reflex addr is a **public-internet** +/// address. LAN / private / loopback reflex addrs are dropped +/// because they reflect the same-network path rather than the +/// real NAT state. CGNAT (100.64/10) is also treated as private +/// because the post-CGNAT address would be what we actually want +/// to classify on — but CGNAT is unreachable from outside the +/// carrier, so a relay seeing the CGNAT addr is on the same +/// carrier network and again not useful for classification. pub fn classify_nat(probes: &[NatProbeResult]) -> (NatType, Option) { - let successes: Vec = probes + // First: parse every successful probe's observed addr. + let parsed: Vec = probes .iter() .filter_map(|p| p.observed_addr.as_deref().and_then(|s| s.parse().ok())) .collect(); + // Then: drop LAN / private / loopback reflex addrs. Those are + // legitimate observations by same-network relays, but they + // don't contribute to NAT-type classification because the + // client's real public-facing NAT mapping is not involved on + // that path. A relay on the same LAN always sees the client's + // LAN IP, regardless of whether the NAT beyond it is cone or + // symmetric. + let successes: Vec = parsed + .into_iter() + .filter(|a| !is_private_or_loopback(a)) + .collect(); + if successes.len() < 2 { return (NatType::Unknown, None); } @@ -365,6 +414,66 @@ mod tests { assert!(addr.is_none()); } + #[test] + fn classify_drops_private_ip_probes() { + // One LAN probe + one public probe should behave like a + // single public probe — i.e. Unknown (not enough data to + // classify). This is the common real-world case: the user + // has a LAN relay + an internet relay configured, the LAN + // relay sees the LAN IP, the internet relay sees the WAN + // IP, and the old classifier would flag "Multiple" and + // falsely warn about symmetric NAT. + let probes = vec![ + mk(Some("192.168.1.100:4433")), // LAN — must be dropped + mk(Some("203.0.113.5:4433")), // public (TEST-NET-3) + ]; + let (nt, _) = classify_nat(&probes); + assert_eq!(nt, NatType::Unknown); + } + + #[test] + fn classify_drops_loopback_probes() { + let probes = vec![ + mk(Some("127.0.0.1:4433")), // loopback — must be dropped + mk(Some("203.0.113.5:4433")), // public + mk(Some("203.0.113.5:4433")), // public, same addr + ]; + let (nt, addr) = classify_nat(&probes); + // Two public probes with identical addrs → Cone. + assert_eq!(nt, NatType::Cone); + assert_eq!(addr.as_deref(), Some("203.0.113.5:4433")); + } + + #[test] + fn classify_drops_cgnat_probes() { + // 100.64.0.0/10 is the CGNAT shared-transition range. + // Filter treats it like RFC1918 — a relay that sees the + // client with a 100.64/10 addr is on the same CGNAT + // network and can't contribute to public NAT classification. + let probes = vec![ + mk(Some("100.64.0.42:4433")), // CGNAT — dropped + mk(Some("203.0.113.5:4433")), // public + mk(Some("203.0.113.5:12345")), // public, different port + ]; + let (nt, _) = classify_nat(&probes); + // Two public probes same IP different port → SymmetricPort. + assert_eq!(nt, NatType::SymmetricPort); + } + + #[test] + fn classify_two_lan_probes_is_unknown_not_cone() { + // Even if both probes come back from LAN relays, we can't + // say anything useful about the public NAT state. Unknown, + // not Cone. + let probes = vec![ + mk(Some("192.168.1.100:4433")), + mk(Some("192.168.1.100:4433")), + ]; + let (nt, addr) = classify_nat(&probes); + assert_eq!(nt, NatType::Unknown); + assert!(addr.is_none()); + } + #[test] fn classify_mix_of_success_and_failure() { let probes = vec![ diff --git a/crates/wzp-relay/tests/multi_reflect.rs b/crates/wzp-relay/tests/multi_reflect.rs index fd829d1..1ad1600 100644 --- a/crates/wzp-relay/tests/multi_reflect.rs +++ b/crates/wzp-relay/tests/multi_reflect.rs @@ -116,11 +116,19 @@ async fn probe_reflect_addr_happy_path() { } // ----------------------------------------------------------------------- -// Test 2: two loopback relays → Cone classification +// Test 2: two loopback relays → probes succeed, classification is Unknown // ----------------------------------------------------------------------- +// +// With the private-IP filter added in the NAT classifier, loopback +// reflex addrs (127.0.0.1) are dropped before classification — +// they can't possibly indicate public-internet NAT state. So the +// test now asserts: +// - both probes succeed end-to-end (wire plumbing works) +// - both return 127.0.0.1 (same-host is visible) +// - the aggregated verdict is Unknown (no public probes) #[tokio::test(flavor = "multi_thread", worker_threads = 4)] -async fn detect_nat_type_two_loopback_relays_is_cone() { +async fn detect_nat_type_two_loopback_relays_probes_work_but_classify_unknown() { let (addr_a, _h_a) = spawn_mock_relay().await; let (addr_b, _h_b) = spawn_mock_relay().await; @@ -135,24 +143,13 @@ async fn detect_nat_type_two_loopback_relays_is_cone() { assert_eq!(detection.probes.len(), 2); for p in &detection.probes { - assert!(p.observed_addr.is_some(), "probe {:?} failed: {:?}", p.relay_name, p.error); + assert!( + p.observed_addr.is_some(), + "probe {:?} failed: {:?}", + p.relay_name, + p.error + ); } - - // Loopback single-host: every probe sees 127.0.0.1 and, crucially, - // uses a different ephemeral source port (since probe_reflect_addr - // spins up a fresh quinn::Endpoint per probe). Wait — that makes - // this look like Symmetric to the classifier, not Cone! - // - // The classifier cares about the *observed* addr, which is what - // the relay sees as the client's source. Two different client - // endpoints on loopback → two different observed ports → the - // classifier correctly labels this as SymmetricPort in the test - // environment. That's still a valid verification of the - // plumbing, just not of the Cone classification. - // - // Accept either Cone OR SymmetricPort for this test, then - // assert the more specific invariant that matters: both probes - // returned the same observed IP. let observed_ips: Vec = detection .probes .iter() @@ -167,14 +164,15 @@ async fn detect_nat_type_two_loopback_relays_is_cone() { assert_eq!(observed_ips[0], "127.0.0.1"); assert_eq!(observed_ips[1], "127.0.0.1"); - // Either classification is valid on loopback (see long comment - // above). Explicitly assert the set so a future refactor that - // accidentally returns `Multiple` or `Unknown` fails the test. - assert!( - matches!(detection.nat_type, NatType::Cone | NatType::SymmetricPort), - "expected Cone or SymmetricPort on loopback, got {:?}", - detection.nat_type + // Classification: loopback probes are filtered out of the + // public-NAT classifier, so with 0 public probes the result + // is Unknown. + assert_eq!( + detection.nat_type, + NatType::Unknown, + "loopback-only probes must not contribute to public NAT classification" ); + assert!(detection.consensus_addr.is_none()); } // ----------------------------------------------------------------------- diff --git a/desktop/src/main.ts b/desktop/src/main.ts index 31e9621..634b302 100644 --- a/desktop/src/main.ts +++ b/desktop/src/main.ts @@ -826,9 +826,18 @@ settingsBtnCall.addEventListener("click", openSettings); // shows its working state inline so the user knows it's waiting on // the relay rather than the network. // Phase 2 multi-relay NAT type detection. Probes every configured -// relay in parallel through transient QUIC connections and -// classifies the result. Green = Cone (P2P viable), -// amber = SymmetricPort (must relay), gray = Multiple / Unknown. +// relay in parallel and classifies the result. +// +// Cone = P2P direct path viable, green cue +// SymmetricPort = per-destination port mapping, informational +// (P2P will fall back to relay but calls still work) +// Multiple = classifier saw different public IPs; informational +// Unknown = not enough public probes, neutral +// +// The classifier drops LAN / private / CGNAT reflex addrs before +// deciding, so a mixed "LAN relay + internet relay" setup does NOT +// falsely flag as symmetric. Failed probes are shown in the list +// for transparency but dimmed, not highlighted. sNatDetectBtn.addEventListener("click", async () => { const s = loadSettings(); if (!s.relays || s.relays.length === 0) { @@ -859,17 +868,18 @@ sNatDetectBtn.addEventListener("click", async () => { detection.nat_type === "Cone" ? `✓ Cone NAT — P2P viable (${detection.consensus_addr})` : detection.nat_type === "SymmetricPort" - ? "⚠ Symmetric NAT — must use relay" + ? "ℹ Symmetric NAT — P2P falls back to relay, calls still work" : detection.nat_type === "Multiple" - ? "⚠ Multiple IPs — treating as symmetric" - : "? Unknown (not enough successful probes)"; + ? "ℹ Multiple public IPs observed" + : "? Unknown (not enough public probes)"; + // Only Cone is "good news green". Everything else is neutral + // informational — the user has configured relays so any + // classification result just describes their network; none + // are "wrong" per se. const verdictColor = detection.nat_type === "Cone" ? "var(--green)" - : detection.nat_type === "SymmetricPort" || - detection.nat_type === "Multiple" - ? "var(--yellow)" : "var(--text-dim)"; sNatType.textContent = verdictLabel; @@ -882,7 +892,10 @@ sNatDetectBtn.addEventListener("click", async () => { p.relay_addr )}) → ${escapeHtml(p.observed_addr)} [${p.latency_ms ?? "?"}ms]`; } else { - return `
• ${escapeHtml( + // Failed probes are dimmed, not highlighted — the classifier + // already ignores them, and the user doesn't need to be + // alarmed by a momentarily-offline relay. + return `
• ${escapeHtml( p.relay_name )} (${escapeHtml(p.relay_addr)}) → ${escapeHtml( p.error ?? "probe failed"