manawenuz/wz-phone
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fix(reflect): drop LAN/private reflex addrs from NAT classification

Browse Source 
Real-world report: a user with one LAN relay + one internet relay
got "Multiple IPs — treating as symmetric" because the LAN relay
saw the client's LAN IP (172.16.81.172) while the internet relay
saw the WAN IP (150.228.49.65). Two observations of "different
public IPs" from the classifier's perspective, but semantically
they describe two different network paths and shouldn't be
compared.

The LAN relay's reflection is always true, just not useful for
public NAT classification: there's no NAT between the client and
the LAN relay, so that path's reflex addr is always the LAN
interface IP regardless of what the public-facing NAT beyond it
looks like.

Fix: new `is_private_or_loopback` helper filters the probe set
before classification. Drops:
 - 127.0.0.0/8 loopback
 - 10/8, 172.16/12, 192.168/16 RFC1918 private
 - 169.254/16 link-local
 - 100.64/10 CGNAT shared-transition (same reasoning: a relay
   that sees the client with a CGNAT addr is on the same carrier
   network and can't describe public NAT state)
 - IPv6 loopback, unspecified, fe80::/10 link-local

Failed probes still filtered out of classification (they were
already) but now dimmed in the UI list instead of highlighted
amber. Same rationale: a momentarily-offline probe target isn't
a warning-worthy state, it's just a fact about the probe run.

UI palette rebalance: only Cone gets green, everything else
neutral text-dim. Wording changed from warning-tone
"⚠ must use relay" to informational "ℹ P2P falls back to relay,
calls still work" — symmetric NAT isn't broken state, it just
means media takes the relay path.

Tests added (4 new in wzp_client::reflect):
- classify_drops_private_ip_probes — LAN + public → Unknown
- classify_drops_loopback_probes — loopback + 2 public → Cone
- classify_drops_cgnat_probes — CGNAT + 2 public same-IP-
  diff-port → SymmetricPort
- classify_two_lan_probes_is_unknown_not_cone — all LAN → Unknown

Existing multi_reflect integration test updated: two loopback
relays now correctly classify as Unknown (because loopback reflex
addrs are filtered) with the plumbing-works invariant preserved.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
Siavash Sameni
2026-04-11 18:29:09 +04:00
parent da08723fe7
commit 00deb97a5d
3 changed files with 157 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files

111
crates/wzp-client/src/reflect.rs
View File

@@ -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<String>) {
let successes: Vec<SocketAddr> = probes
// First: parse every successful probe's observed addr.
let parsed: Vec<SocketAddr> = 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<SocketAddr> = 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![

50
crates/wzp-relay/tests/multi_reflect.rs
View File

@@ -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<String> = 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());
}
// -----------------------------------------------------------------------

33
desktop/src/main.ts
View File

@@ -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]</div>`;
} else {
return `<div style="color:var(--yellow)">• ${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 `<div style="color:var(--text-dim);opacity:0.7">• ${escapeHtml(
p.relay_name
)} (${escapeHtml(p.relay_addr)}) → ${escapeHtml(
p.error ?? "probe failed"