From 2d5fc26db7eb274ec3b9101399d781207e263339 Mon Sep 17 00:00:00 2001 From: michel Date: Sun, 19 Jul 2026 21:46:46 +0200 Subject: [PATCH] test(net): e2e-test the UPnP path + add a portmap diagnostic MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the gap that the port mapper's UPnP path and gateway discovery had no automated coverage (only NAT-PMP did): - UPnP e2e: split map_via_gateway out of discovery and drive it through igd-next against an in-process mock IGD (a std HTTP server returning a canned SOAP envelope). Verifies the real add_port SOAP round-trip and that we request the exact external/internal port + LAN client. - default_gateway parsing is now pure, testable functions with unit tests for the macOS and Linux formats (including the 0.0.0.0 skip). - New 'portmap' CLI subcommand (needs no login): synchronously probes UPnP/NAT-PMP against the real router and reports whether the port was opened + the external address, then removes the test mapping — so a user can verify reachability on their own network. 9 port_mapping tests now cover the NAT-PMP protocol, the UPnP SOAP round-trip, gateway parsing, and the map/renew/cleanup lifecycle. --- README.md | 10 ++ soulseek-rs/src/cli.rs | 3 + soulseek-rs/src/main.rs | 13 ++ soulseek-rs/src/port_mapping/mod.rs | 183 ++++++++++++++++++++++++ soulseek-rs/src/port_mapping/nat_pmp.rs | 88 ++++++++---- 5 files changed, 271 insertions(+), 26 deletions(-) diff --git a/README.md b/README.md index 2dff3aa..4609632 100644 --- a/README.md +++ b/README.md @@ -141,6 +141,16 @@ you'll see a log line suggesting you forward the port manually. `--listener-port` you chose) to this machine on your router. - Pass `--disable-listener` to turn the listener (and port mapping) off. +Check whether it works on **your** network without launching the whole client: + +```bash +soulseek-rs portmap +``` + +It tries to open the port via UPnP/NAT-PMP and prints whether your router +allowed it (and your external address), then removes the test mapping. If it +reports failure, enable UPnP on your router or forward the port manually. + If both you and a peer are behind routers with no forwarded port, browsing that peer can't work — that's a fundamental Soulseek/peer-to-peer limitation, not a bug. diff --git a/soulseek-rs/src/cli.rs b/soulseek-rs/src/cli.rs index b378091..523ed59 100644 --- a/soulseek-rs/src/cli.rs +++ b/soulseek-rs/src/cli.rs @@ -120,6 +120,9 @@ pub enum Commands { #[arg(short, long, default_value = "30")] listen_secs: u64, }, + + /// Test whether your router lets us auto-open the listen port (UPnP/NAT-PMP) + Portmap, } pub fn parse_server_address(server: &str) -> color_eyre::Result<(String, u16)> { diff --git a/soulseek-rs/src/main.rs b/soulseek-rs/src/main.rs index 4ba08ac..2986365 100644 --- a/soulseek-rs/src/main.rs +++ b/soulseek-rs/src/main.rs @@ -41,6 +41,17 @@ fn main() -> Result<()> { }; } + // `portmap` is a local network diagnostic; it needs no server credentials, + // so handle it before requiring a username/password. + if matches!(cli.command, Some(Commands::Portmap)) { + println!( + "Testing automatic port mapping for TCP {}…", + cli.listener_port + ); + println!("{}", port_mapping::diagnose(cli.listener_port)); + return Ok(()); + } + let username = cli.username.ok_or_else(|| { color_eyre::eyre::eyre!( "Username required: use --username or set SOULSEEK_USERNAME env var" @@ -141,6 +152,8 @@ fn main() -> Result<()> { message.as_deref(), listen_secs, ), + // Handled before the credential check above. + Some(Commands::Portmap) => unreachable!(), None => { use ratatui::crossterm::{ event::EnableMouseCapture, diff --git a/soulseek-rs/src/port_mapping/mod.rs b/soulseek-rs/src/port_mapping/mod.rs index fc8fb2d..6fcfdb8 100644 --- a/soulseek-rs/src/port_mapping/mod.rs +++ b/soulseek-rs/src/port_mapping/mod.rs @@ -61,6 +61,46 @@ impl Drop for PortMapper { } } +/// Synchronously probe whether automatic port mapping works on this network: +/// try to map `port` (UPnP then NAT-PMP), report the result in human-readable +/// form, then remove the probe mapping. Used by the `portmap` CLI command so a +/// user can verify reachability on their actual router. +#[must_use] +pub fn diagnose(port: u16) -> String { + let Some(local_ip) = local_lan_ip() else { + return "Could not determine this machine's LAN IP — are you connected \ + to a network?" + .to_string(); + }; + + match map_once(local_ip, port) { + Some((backend, _lease)) => { + let detail = match &backend { + Backend::Upnp { gateway, .. } => { + let ip = gateway.get_external_ip().map_or_else( + |_| "unknown".to_string(), + |ip| ip.to_string(), + ); + format!("UPnP-IGD (external address {ip}:{port})") + } + Backend::NatPmp { .. } => "NAT-PMP".to_string(), + }; + remove(&backend); // clean up the probe mapping + format!( + "✅ Automatic port mapping works via {detail}.\n TCP {port} \ + was opened on your router, so firewalled peers and the server \ + can reach you. The app does this automatically while it runs." + ) + } + None => format!( + "❌ No UPnP/NAT-PMP router responded for TCP {port}.\n Automatic \ + mapping won't work on this network. To browse/download firewalled \ + peers, forward TCP {port} to this machine on your router (or \ + enable UPnP there)." + ), + } +} + /// Which backend created the active mapping, carrying what's needed to renew and /// remove it. enum Backend { @@ -132,6 +172,16 @@ fn map_upnp(local_ip: IpAddr, port: u16) -> Option<(Backend, u32)> { }; let gateway = search_gateway(options).ok()?; let local_addr = SocketAddr::new(local_ip, port); + map_via_gateway(gateway, local_addr, port) +} + +/// Map `port` on an already-discovered IGD `gateway`. Split out from discovery +/// so it can be exercised against a mock IGD in tests. +fn map_via_gateway( + gateway: igd_next::Gateway, + local_addr: SocketAddr, + port: u16, +) -> Option<(Backend, u32)> { // We advertise our listen port verbatim to the Soulseek server, so the // EXTERNAL port must equal it. If the exact port can't be mapped (e.g. // another LAN device already claimed it), a different external port would @@ -405,6 +455,139 @@ mod tests { ); } + /// A minimal in-process UPnP IGD: an HTTP server that answers every SOAP + /// control request with a canned envelope containing the Add/Delete/GetIP + /// response elements, and forwards each request body to the test. + fn mock_igd() -> (SocketAddr, mpsc::Receiver) { + use std::io::{Read, Write}; + use std::net::TcpListener; + + let listener = TcpListener::bind((Ipv4Addr::LOCALHOST, 0)).unwrap(); + let addr = listener.local_addr().unwrap(); + let (tx, rx) = mpsc::channel(); + thread::spawn(move || { + for stream in listener.incoming() { + let Ok(mut stream) = stream else { break }; + let mut buf = Vec::new(); + let mut chunk = [0u8; 1024]; + // Read headers, then Content-Length bytes of body. + let body = loop { + let n = match stream.read(&mut chunk) { + Ok(0) | Err(_) => break String::new(), + Ok(n) => n, + }; + buf.extend_from_slice(&chunk[..n]); + let text = String::from_utf8_lossy(&buf); + if let Some(hdr_end) = text.find("\r\n\r\n") { + let len = text + .lines() + .find_map(|l| { + l.strip_prefix("Content-Length: ").or_else( + || l.strip_prefix("content-length: "), + ) + }) + .and_then(|v| v.trim().parse::().ok()) + .unwrap_or(0); + if buf.len() >= hdr_end + 4 + len { + break text[hdr_end + 4..].to_string(); + } + } + }; + if tx.send(body).is_err() { + break; + } + let soap = r#" + + + + +203.0.113.7 + +"#; + let response = format!( + "HTTP/1.1 200 OK\r\nContent-Type: text/xml\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{soap}", + soap.len() + ); + let _ = stream.write_all(response.as_bytes()); + } + }); + (addr, rx) + } + + fn igd_control_schema() -> std::collections::HashMap> { + let mut schema = std::collections::HashMap::new(); + schema.insert( + "AddPortMapping".to_string(), + [ + "NewRemoteHost", + "NewExternalPort", + "NewProtocol", + "NewInternalPort", + "NewInternalClient", + "NewEnabled", + "NewPortMappingDescription", + "NewLeaseDuration", + ] + .iter() + .map(ToString::to_string) + .collect(), + ); + schema.insert( + "DeletePortMapping".to_string(), + ["NewRemoteHost", "NewExternalPort", "NewProtocol"] + .iter() + .map(ToString::to_string) + .collect(), + ); + schema + } + + #[test] + fn upnp_maps_a_port_against_a_mock_igd() { + let (igd_addr, requests) = mock_igd(); + let gateway = igd_next::Gateway { + addr: igd_addr, + root_url: format!("http://{igd_addr}/rootDesc.xml"), + control_url: "/ctl".to_string(), + control_schema_url: format!("http://{igd_addr}/scpd.xml"), + control_schema: igd_control_schema(), + }; + + let local_addr = + SocketAddr::new(IpAddr::V4(Ipv4Addr::new(192, 168, 1, 50)), 2234); + let result = map_via_gateway(gateway, local_addr, 2234); + + let (backend, _lease) = + result.expect("mapping against a healthy IGD should succeed"); + match backend { + Backend::Upnp { external_port, .. } => { + assert_eq!( + external_port, 2234, + "must map the exact listen port" + ); + } + Backend::NatPmp { .. } => panic!("expected a UPnP backend"), + } + + // The IGD must have received an AddPortMapping SOAP call quoting our + // external and internal port and the LAN client address. + let body = requests + .recv_timeout(StdDuration::from_secs(3)) + .expect("IGD should receive an AddPortMapping request"); + assert!( + body.contains("2234"), + "request should map external port 2234: {body}" + ); + assert!( + body.contains("2234"), + "request should target internal port 2234: {body}" + ); + assert!( + body.contains("192.168.1.50"), + "request should point at the LAN client: {body}" + ); + } + #[test] fn drop_does_not_hang_when_gateway_is_unresponsive() { // A bound-but-silent gateway: the mapper thread blocks in the NAT-PMP diff --git a/soulseek-rs/src/port_mapping/nat_pmp.rs b/soulseek-rs/src/port_mapping/nat_pmp.rs index 8440a2b..8c4f10b 100644 --- a/soulseek-rs/src/port_mapping/nat_pmp.rs +++ b/soulseek-rs/src/port_mapping/nat_pmp.rs @@ -175,36 +175,12 @@ pub fn default_gateway() -> Option { .args(["-n", "get", "default"]) .output() .ok()?; - let text = String::from_utf8_lossy(&out.stdout); - for line in text.lines() { - let line = line.trim(); - if let Some(rest) = line.strip_prefix("gateway:") { - return rest.trim().parse().ok(); - } - } - None + parse_macos_default_gateway(&String::from_utf8_lossy(&out.stdout)) } #[cfg(target_os = "linux")] { - // /proc/net/route: tab-separated; the default route has Destination - // 00000000, and Gateway is a little-endian hex u32. let text = std::fs::read_to_string("/proc/net/route").ok()?; - for line in text.lines().skip(1) { - let mut fields = line.split_whitespace(); - let _iface = fields.next()?; - let destination = fields.next()?; - let gateway = fields.next()?; - if destination == "00000000" { - let raw = u32::from_str_radix(gateway, 16).ok()?; - // A 0.0.0.0 gateway is a directly-connected default route (no - // next hop): not a usable NAT-PMP gateway. Keep scanning for a - // real one rather than returning the unspecified address. - if raw != 0 { - return Some(Ipv4Addr::from(raw.to_le_bytes())); - } - } - } - None + parse_linux_default_gateway(&text) } #[cfg(not(any(target_os = "macos", target_os = "linux")))] { @@ -212,6 +188,36 @@ pub fn default_gateway() -> Option { } } +/// Parse the gateway IP from `route -n get default` output (macOS). +#[cfg(any(target_os = "macos", test))] +#[must_use] +fn parse_macos_default_gateway(output: &str) -> Option { + output.lines().find_map(|line| { + line.trim().strip_prefix("gateway:")?.trim().parse().ok() + }) +} + +/// Parse the IPv4 default gateway from the contents of `/proc/net/route` +/// (Linux). Fields are whitespace-separated; the default route has Destination +/// `00000000` and stores the Gateway as a little-endian hex u32. A `00000000` +/// (unspecified) gateway is a directly-connected route with no next hop, so it +/// is skipped rather than returned as `0.0.0.0`. +#[cfg(any(target_os = "linux", test))] +#[must_use] +fn parse_linux_default_gateway(contents: &str) -> Option { + contents.lines().skip(1).find_map(|line| { + let mut fields = line.split_whitespace(); + let _iface = fields.next()?; + let destination = fields.next()?; + let gateway = fields.next()?; + if destination != "00000000" { + return None; + } + let raw = u32::from_str_radix(gateway, 16).ok()?; + (raw != 0).then(|| Ipv4Addr::from(raw.to_le_bytes())) + }) +} + #[cfg(test)] mod tests { use super::*; @@ -302,6 +308,36 @@ mod tests { } } + #[test] + fn parses_macos_default_gateway() { + let output = " route to: default\ndestination: default\n mask: default\n gateway: 192.168.1.254\n interface: en0\n"; + assert_eq!( + parse_macos_default_gateway(output), + Some(Ipv4Addr::new(192, 168, 1, 254)) + ); + // No default route → None. + assert_eq!(parse_macos_default_gateway("interface: en0\n"), None); + } + + #[test] + fn parses_linux_default_gateway_and_skips_unspecified() { + // Header + a directly-connected default (gateway 00000000) that must be + // skipped + the real default route (192.168.0.1 => 0100A8C0 LE). + let contents = "Iface\tDestination\tGateway\tFlags\tRefCnt\tUse\tMetric\tMask\n\ + tun0\t00000000\t00000000\t0001\t0\t0\t0\t00000000\n\ + eth0\t00000000\t0100A8C0\t0003\t0\t0\t100\t00000000\n\ + eth0\t0000A8C0\t00000000\t0001\t0\t0\t0\t00FFFFFF\n"; + assert_eq!( + parse_linux_default_gateway(contents), + Some(Ipv4Addr::new(192, 168, 0, 1)) + ); + + // Only a gateway-less default route → None (not 0.0.0.0). + let only_direct = "Iface\tDestination\tGateway\tFlags\n\ + tun0\t00000000\t00000000\t0001\n"; + assert_eq!(parse_linux_default_gateway(only_direct), None); + } + #[test] fn unmap_sends_zero_lifetime_and_port() { let (gateway, requests) = mock_gateway(map_reply(0, 2234, 0, 0));