From 603e80f72732971fbac0b8d410d8b00a08e0d2d3 Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Tue, 18 Aug 2026 20:00:54 +0200 Subject: [PATCH 1/2] test: stop aiming I12 transport tests at recycled ports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit wiremock's process-wide pool binds 127.0.0.1:0 for the process lifetime. The I12 transport-error tests bound an ephemeral 127.0.0.1 port, dropped the listener, and connected at the freed address expecting ECONNREFUSED. Under load that port can belong to a newly created pooled listener: unwrap_err panics on another test's corpus, .expect(1) over-counts a foreign request, or — worst — the I12 test goes green without sanitizing a transport error at all. Point those tests at 127.0.0.2:1 instead. That address is still loopback (127.0.0.0/8) so the kernel refuses immediately (or returns ENETUNREACH), and a pooled listener binds only 127.0.0.1 so it can never occupy it. A bounded probe refuses to return an address that accepted or timed out; the client call itself is capped at 2s so a proxy/routing hang cannot become the 30s client timeout; and the assertion requires reqwest's "error sending request" so an empty or HTTP-status error cannot pass a bare !contains(KEY). A mutation that retargets the helper at 127.0.0.1 fails the helper. I12 is unchanged: the key still must not appear in Display or Debug of a real transport error. Closes #115 --- crates/bugwarden-core/Cargo.toml | 2 +- crates/bugwarden-core/tests/guard_wiremock.rs | 114 +++++++++++------- crates/bugwarden/tests/common/mod.rs | 32 +++++ crates/bugwarden/tests/preflight_wiremock.rs | 31 ++--- crates/bugwarden/tests/tools_wiremock.rs | 28 +++-- 5 files changed, 137 insertions(+), 70 deletions(-) create mode 100644 crates/bugwarden/tests/common/mod.rs diff --git a/crates/bugwarden-core/Cargo.toml b/crates/bugwarden-core/Cargo.toml index 1df5aab..b89fc43 100644 --- a/crates/bugwarden-core/Cargo.toml +++ b/crates/bugwarden-core/Cargo.toml @@ -30,5 +30,5 @@ reqwest = { version = "0.13", default-features = false, features = [ ] } [dev-dependencies] -tokio = { version = "1", features = ["macros", "rt-multi-thread"] } +tokio = { version = "1", features = ["macros", "rt-multi-thread", "time"] } wiremock = "0.6" diff --git a/crates/bugwarden-core/tests/guard_wiremock.rs b/crates/bugwarden-core/tests/guard_wiremock.rs index cfb3487..536be68 100644 --- a/crates/bugwarden-core/tests/guard_wiremock.rs +++ b/crates/bugwarden-core/tests/guard_wiremock.rs @@ -32,6 +32,52 @@ fn client(server: &MockServer) -> BugzillaClient { BugzillaClient::new(&server.uri(), false, TEST_USER_AGENT).expect("client must build") } +/// Connect-time failure that wiremock's 127.0.0.1 pool cannot serve (#115). +/// +/// 127.0.0.2 is still loopback (`127.0.0.0/8`); port 1 is a system port. +/// A bounded probe refuses to return an address that accepted or timed out, +/// so the 30s client cannot hang on an unroutable extra-loopback. The URL +/// is built from the probed socket so the two cannot drift, and 127.0.0.1 +/// is rejected so a bind-then-drop mutation fails this helper. +fn refused_base_url() -> String { + let addr = std::net::SocketAddr::from(([127, 0, 0, 2], 1)); + assert_ne!( + addr.ip(), + std::net::IpAddr::V4(std::net::Ipv4Addr::LOCALHOST), + "I12 transport tests must not target 127.0.0.1; wiremock's pool binds there (#115)" + ); + match std::net::TcpStream::connect_timeout(&addr, std::time::Duration::from_millis(500)) { + Ok(_) => panic!( + "{addr} accepted a connection; I12 tests need a refused address \ + that wiremock's 127.0.0.1 pool cannot occupy (#115)" + ), + Err(e) if e.kind() == std::io::ErrorKind::TimedOut => panic!( + "{addr} timed out; refusing to point the 30s client at an address \ + that would hang the test (#115)" + ), + Err(_) => format!("http://{addr}"), + } +} + +/// Bound on the I12 client calls. Loopback refuse is immediate; a hang +/// here is a proxy or routing defect, not a 30s client timeout. +const REFUSED_CONNECT_BUDGET: std::time::Duration = std::time::Duration::from_secs(2); + +/// I12: the error must be a real reqwest transport failure, and neither +/// Display nor Debug may carry the API key. An empty or HTTP-status error +/// would pass a bare `!contains(KEY)` without exercising sanitization. +fn assert_key_absent_from_transport_error(err: &anyhow::Error) { + let full = format!("{err:#} {err:?}"); + assert!( + full.contains("error sending request"), + "expected a reqwest transport error (I12), got: {full}" + ); + assert!( + !full.contains(KEY), + "API key leaked into transport error: {full}" + ); +} + fn bug(id: u64, groups: &[&str], creation_time: &str) -> serde_json::Value { json!({ "id": id, @@ -408,20 +454,16 @@ async fn api_key_reaches_server_but_never_error_text_i12() { #[tokio::test] async fn api_key_absent_from_transport_error_i12() { - // Point at a closed port: reqwest yields a connect error whose URL would - // contain the api_key query parameter — sanitize must strip it. - let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); - let addr = listener.local_addr().expect("addr"); - drop(listener); // free the port so the connection is refused - let bz = - BugzillaClient::new(&format!("http://{addr}"), false, TEST_USER_AGENT).expect("client"); + // Point at an address the wiremock pool can never occupy: reqwest + // yields a connect error whose URL would contain the api_key query + // parameter — sanitize must strip it. + let bz = BugzillaClient::new(&refused_base_url(), false, TEST_USER_AGENT).expect("client"); - let err = bz.get_bugs(KEY, &[1], None).await.unwrap_err(); - let full = format!("{err:#} {err:?}"); - assert!( - !full.contains(KEY), - "API key leaked into transport error: {full}" - ); + let err = tokio::time::timeout(REFUSED_CONNECT_BUDGET, bz.get_bugs(KEY, &[1], None)) + .await + .expect("connect to the refused extra-loopback must not hang") + .unwrap_err(); + assert_key_absent_from_transport_error(&err); } #[tokio::test] @@ -475,11 +517,8 @@ async fn client_create_bug_posts_the_payload_to_rest_bug() { // The payload travels as the POST body, untouched. // // Select the request by method and path rather than by position (#93). - // wiremock hands out mock servers from a process-wide pool of listeners, - // and the I12 transport-error tests in this binary deliberately aim a - // request at a just-freed ephemeral port; when that port has meanwhile - // been taken by a pooled listener, their bodyless GET is recorded here - // too. `[0]` would then assert against that request instead. + // The recording can contain more than the create POST — `[0]` would + // then assert against the wrong request. let reqs = server.received_requests().await.expect("recording enabled"); let posts: Vec<&Request> = reqs .iter() @@ -1138,18 +1177,13 @@ async fn client_whoami_missing_non_string_or_blank_name_is_a_failure() { async fn whoami_api_key_absent_from_transport_error_i12() { // Same pattern as api_key_absent_from_transport_error_i12: a connect // error's URL would carry api_key=... — sanitize must strip it. - let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); - let addr = listener.local_addr().expect("addr"); - drop(listener); // free the port so the connection is refused - let bz = - BugzillaClient::new(&format!("http://{addr}"), false, TEST_USER_AGENT).expect("client"); + let bz = BugzillaClient::new(&refused_base_url(), false, TEST_USER_AGENT).expect("client"); - let err = bz.whoami(KEY).await.unwrap_err(); - let full = format!("{err:#} {err:?}"); - assert!( - !full.contains(KEY), - "API key leaked into whoami transport error: {full}" - ); + let err = tokio::time::timeout(REFUSED_CONNECT_BUDGET, bz.whoami(KEY)) + .await + .expect("connect to the refused extra-loopback must not hang") + .unwrap_err(); + assert_key_absent_from_transport_error(&err); } // --------------------------------------------------------------------------- @@ -1218,18 +1252,16 @@ async fn client_valid_login_unusable_shape_is_an_error_never_false() { #[tokio::test] async fn valid_login_api_key_absent_from_transport_error_i12() { - let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); - let addr = listener.local_addr().expect("addr"); - drop(listener); // free the port so the connection is refused - let bz = - BugzillaClient::new(&format!("http://{addr}"), false, TEST_USER_AGENT).expect("client"); - - let err = bz.valid_login(KEY, "svc@example.com").await.unwrap_err(); - let full = format!("{err:#} {err:?}"); - assert!( - !full.contains(KEY), - "API key leaked into valid_login transport error: {full}" - ); + let bz = BugzillaClient::new(&refused_base_url(), false, TEST_USER_AGENT).expect("client"); + + let err = tokio::time::timeout( + REFUSED_CONNECT_BUDGET, + bz.valid_login(KEY, "svc@example.com"), + ) + .await + .expect("connect to the refused extra-loopback must not hang") + .unwrap_err(); + assert_key_absent_from_transport_error(&err); } #[tokio::test] diff --git a/crates/bugwarden/tests/common/mod.rs b/crates/bugwarden/tests/common/mod.rs new file mode 100644 index 0000000..28d56dd --- /dev/null +++ b/crates/bugwarden/tests/common/mod.rs @@ -0,0 +1,32 @@ +//! Shared helpers for bugwarden integration tests. + +/// Connect-time failure that wiremock's 127.0.0.1 pool cannot serve (#115). +/// +/// 127.0.0.2 is still loopback (`127.0.0.0/8`); port 1 is a system port. +/// A bounded probe refuses to return an address that accepted or timed out, +/// so the 30s client cannot hang on an unroutable extra-loopback. The URL +/// is built from the probed socket so the two cannot drift, and 127.0.0.1 +/// is rejected so a bind-then-drop mutation fails this helper. +pub fn refused_base_url() -> String { + let addr = std::net::SocketAddr::from(([127, 0, 0, 2], 1)); + assert_ne!( + addr.ip(), + std::net::IpAddr::V4(std::net::Ipv4Addr::LOCALHOST), + "I12 transport tests must not target 127.0.0.1; wiremock's pool binds there (#115)" + ); + match std::net::TcpStream::connect_timeout(&addr, std::time::Duration::from_millis(500)) { + Ok(_) => panic!( + "{addr} accepted a connection; I12 tests need a refused address \ + that wiremock's 127.0.0.1 pool cannot occupy (#115)" + ), + Err(e) if e.kind() == std::io::ErrorKind::TimedOut => panic!( + "{addr} timed out; refusing to point the 30s client at an address \ + that would hang the test (#115)" + ), + Err(_) => format!("http://{addr}"), + } +} + +/// Bound on the I12 client calls. Loopback refuse is immediate; a hang +/// here is a proxy or routing defect, not a 30s client timeout. +pub const REFUSED_CONNECT_BUDGET: std::time::Duration = std::time::Duration::from_secs(2); diff --git a/crates/bugwarden/tests/preflight_wiremock.rs b/crates/bugwarden/tests/preflight_wiremock.rs index 5e94073..d059003 100644 --- a/crates/bugwarden/tests/preflight_wiremock.rs +++ b/crates/bugwarden/tests/preflight_wiremock.rs @@ -19,6 +19,8 @@ use std::sync::Arc; +mod common; + use bugwarden::config::Cli; use bugwarden::server::{BugWarden, USER_AGENT}; use bugwarden_core::client::BugzillaClient; @@ -262,18 +264,16 @@ async fn preflight_declared_login_transport_error_names_the_endpoint() { #[tokio::test] async fn preflight_transport_error_does_not_leak_the_api_key_i12() { - // Point the server at a closed port: the whoami lookup fails at the - // transport level, where the unsanitized error would carry the - // request URL with api_key=... in it. Nothing in the preflight error - // text may contain the key. - let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); - let addr = listener.local_addr().expect("addr"); - drop(listener); // free the port so every connection is refused + // Point the server at an address the wiremock pool can never occupy: + // the whoami lookup fails at the transport level, where the + // unsanitized error would carry the request URL with api_key=... in + // it. Nothing in the preflight error text may contain the key. + let base = common::refused_base_url(); let mut cli = Cli::parse_from([ "bugwarden", "--bugzilla-server", - &format!("http://{addr}"), + &base, "--transport", "stdio", "--api-key", @@ -284,17 +284,18 @@ async fn preflight_transport_error_does_not_leak_the_api_key_i12() { let guard = Arc::new(Guard { policy: Policy::from_toml_str(IDENTITY_POLICY).expect("test policy must parse"), }); - let bz = Arc::new( - BugzillaClient::new(&format!("http://{addr}"), false, USER_AGENT) - .expect("client must build"), - ); + let bz = Arc::new(BugzillaClient::new(&base, false, USER_AGENT).expect("client must build")); let server = BugWarden::new(cfg, guard, bz).expect("server must build"); - let err = server - .preflight() + let err = tokio::time::timeout(common::REFUSED_CONNECT_BUDGET, server.preflight()) .await + .expect("connect to the refused extra-loopback must not hang") .expect_err("an unreachable whoami endpoint must fail preflight"); - let msg = format!("{err:#}"); + let msg = format!("{err:#} {err:?}"); + assert!( + msg.contains("error sending request"), + "expected a whoami transport error (I12), got: {msg}" + ); assert!( !msg.contains("SUPERSECRETKEY123"), "API key leaked into a preflight error: {msg}" diff --git a/crates/bugwarden/tests/tools_wiremock.rs b/crates/bugwarden/tests/tools_wiremock.rs index d47343e..3a89e57 100644 --- a/crates/bugwarden/tests/tools_wiremock.rs +++ b/crates/bugwarden/tests/tools_wiremock.rs @@ -21,6 +21,8 @@ use std::sync::Arc; +mod common; + use bugwarden::config::Cli; use bugwarden::server::{BugWarden, USER_AGENT, WRITE_TOOLS}; use bugwarden_core::client::BugzillaClient; @@ -1642,18 +1644,16 @@ async fn created_by_me_reaches_the_write_gate_add_comment_too() { #[tokio::test] async fn whoami_transport_error_does_not_leak_the_api_key_i12() { - // Point the server at a closed port: the whoami lookup (and everything - // after it) fails at the transport level, where the unsanitized error - // would carry the request URL with api_key=... in it. Nothing the - // client sees may contain the key. - let listener = std::net::TcpListener::bind("127.0.0.1:0").expect("bind"); - let addr = listener.local_addr().expect("addr"); - drop(listener); // free the port so every connection is refused + // Point the server at an address the wiremock pool can never occupy: + // the whoami lookup (and everything after it) fails at the transport + // level, where the unsanitized error would carry the request URL with + // api_key=... in it. Nothing the client sees may contain the key. + let base = common::refused_base_url(); let mut cli = Cli::parse_from([ "bugwarden", "--bugzilla-server", - &format!("http://{addr}"), + &base, "--transport", "stdio", "--api-key", @@ -1664,10 +1664,7 @@ async fn whoami_transport_error_does_not_leak_the_api_key_i12() { let guard = Arc::new(Guard { policy: Policy::from_toml_str(IDENTITY_POLICY).expect("test policy must parse"), }); - let bz = Arc::new( - BugzillaClient::new(&format!("http://{addr}"), false, USER_AGENT) - .expect("client must build"), - ); + let bz = Arc::new(BugzillaClient::new(&base, false, USER_AGENT).expect("client must build")); let server = BugWarden::new(cfg, guard, bz).expect("server must build"); let (client_io, server_io) = tokio::io::duplex(1 << 16); tokio::spawn(async move { @@ -1680,7 +1677,12 @@ async fn whoami_transport_error_does_not_leak_the_api_key_i12() { .await .expect("MCP handshake must succeed"); - let result = call(&client, "bug_info", json!({ "bug_ids": [7] })).await; + let result = tokio::time::timeout( + common::REFUSED_CONNECT_BUDGET, + call(&client, "bug_info", json!({ "bug_ids": [7] })), + ) + .await + .expect("connect to the refused extra-loopback must not hang"); let text = serde_json::to_string(&result).unwrap(); assert!( !text.contains("SUPERSECRETKEY123"), From c6c3e9331f2d11cf5d673f5b4dac703492604142 Mon Sep 17 00:00:00 2001 From: Martin Pluskal Date: Tue, 18 Aug 2026 20:11:30 +0200 Subject: [PATCH 2/2] fix(test): use a portable refused address for I12 transport tests 127.0.0.2 is extra loopback on Linux but is not aliased on macOS, so the I12 probe hung until timeout on rust-macos. Switch to 127.0.0.1:1: a privileged port both OSes refuse immediately if nothing is listening, and that a non-root wiremock 127.0.0.1:0 pool cannot occupy. The helper now asserts the port stays privileged so a bind-then-drop of an ephemeral port still fails. --- crates/bugwarden-core/tests/guard_wiremock.rs | 29 ++++++++++--------- crates/bugwarden/tests/common/mod.rs | 23 ++++++++------- crates/bugwarden/tests/preflight_wiremock.rs | 2 +- crates/bugwarden/tests/tools_wiremock.rs | 2 +- 4 files changed, 29 insertions(+), 27 deletions(-) diff --git a/crates/bugwarden-core/tests/guard_wiremock.rs b/crates/bugwarden-core/tests/guard_wiremock.rs index 536be68..f4b3438 100644 --- a/crates/bugwarden-core/tests/guard_wiremock.rs +++ b/crates/bugwarden-core/tests/guard_wiremock.rs @@ -34,22 +34,23 @@ fn client(server: &MockServer) -> BugzillaClient { /// Connect-time failure that wiremock's 127.0.0.1 pool cannot serve (#115). /// -/// 127.0.0.2 is still loopback (`127.0.0.0/8`); port 1 is a system port. -/// A bounded probe refuses to return an address that accepted or timed out, -/// so the 30s client cannot hang on an unroutable extra-loopback. The URL -/// is built from the probed socket so the two cannot drift, and 127.0.0.1 -/// is rejected so a bind-then-drop mutation fails this helper. +/// `127.0.0.1:1` is a privileged port: a non-root wiremock listener binds +/// `127.0.0.1:0` and cannot occupy it. Both Linux and macOS refuse +/// immediately if nothing is listening (unlike `127.0.0.2`, which is not +/// aliased on macOS and hangs). A bounded probe refuses to return an +/// address that accepted or timed out. The URL is built from the probed +/// socket so the two cannot drift, and the port must stay privileged so +/// a bind-then-drop of an ephemeral port fails this helper. fn refused_base_url() -> String { - let addr = std::net::SocketAddr::from(([127, 0, 0, 2], 1)); - assert_ne!( - addr.ip(), - std::net::IpAddr::V4(std::net::Ipv4Addr::LOCALHOST), - "I12 transport tests must not target 127.0.0.1; wiremock's pool binds there (#115)" + let addr = std::net::SocketAddr::from(([127, 0, 0, 1], 1)); + assert!( + addr.port() < 1024, + "I12 transport tests must use a privileged port; wiremock binds 127.0.0.1:0 (#115)" ); match std::net::TcpStream::connect_timeout(&addr, std::time::Duration::from_millis(500)) { Ok(_) => panic!( "{addr} accepted a connection; I12 tests need a refused address \ - that wiremock's 127.0.0.1 pool cannot occupy (#115)" + that wiremock's 127.0.0.1:0 pool cannot occupy (#115)" ), Err(e) if e.kind() == std::io::ErrorKind::TimedOut => panic!( "{addr} timed out; refusing to point the 30s client at an address \ @@ -461,7 +462,7 @@ async fn api_key_absent_from_transport_error_i12() { let err = tokio::time::timeout(REFUSED_CONNECT_BUDGET, bz.get_bugs(KEY, &[1], None)) .await - .expect("connect to the refused extra-loopback must not hang") + .expect("connect to the refused privileged port must not hang") .unwrap_err(); assert_key_absent_from_transport_error(&err); } @@ -1181,7 +1182,7 @@ async fn whoami_api_key_absent_from_transport_error_i12() { let err = tokio::time::timeout(REFUSED_CONNECT_BUDGET, bz.whoami(KEY)) .await - .expect("connect to the refused extra-loopback must not hang") + .expect("connect to the refused privileged port must not hang") .unwrap_err(); assert_key_absent_from_transport_error(&err); } @@ -1259,7 +1260,7 @@ async fn valid_login_api_key_absent_from_transport_error_i12() { bz.valid_login(KEY, "svc@example.com"), ) .await - .expect("connect to the refused extra-loopback must not hang") + .expect("connect to the refused privileged port must not hang") .unwrap_err(); assert_key_absent_from_transport_error(&err); } diff --git a/crates/bugwarden/tests/common/mod.rs b/crates/bugwarden/tests/common/mod.rs index 28d56dd..cb6b4d2 100644 --- a/crates/bugwarden/tests/common/mod.rs +++ b/crates/bugwarden/tests/common/mod.rs @@ -2,22 +2,23 @@ /// Connect-time failure that wiremock's 127.0.0.1 pool cannot serve (#115). /// -/// 127.0.0.2 is still loopback (`127.0.0.0/8`); port 1 is a system port. -/// A bounded probe refuses to return an address that accepted or timed out, -/// so the 30s client cannot hang on an unroutable extra-loopback. The URL -/// is built from the probed socket so the two cannot drift, and 127.0.0.1 -/// is rejected so a bind-then-drop mutation fails this helper. +/// `127.0.0.1:1` is a privileged port: a non-root wiremock listener binds +/// `127.0.0.1:0` and cannot occupy it. Both Linux and macOS refuse +/// immediately if nothing is listening (unlike `127.0.0.2`, which is not +/// aliased on macOS and hangs). A bounded probe refuses to return an +/// address that accepted or timed out. The URL is built from the probed +/// socket so the two cannot drift, and the port must stay privileged so +/// a bind-then-drop of an ephemeral port fails this helper. pub fn refused_base_url() -> String { - let addr = std::net::SocketAddr::from(([127, 0, 0, 2], 1)); - assert_ne!( - addr.ip(), - std::net::IpAddr::V4(std::net::Ipv4Addr::LOCALHOST), - "I12 transport tests must not target 127.0.0.1; wiremock's pool binds there (#115)" + let addr = std::net::SocketAddr::from(([127, 0, 0, 1], 1)); + assert!( + addr.port() < 1024, + "I12 transport tests must use a privileged port; wiremock binds 127.0.0.1:0 (#115)" ); match std::net::TcpStream::connect_timeout(&addr, std::time::Duration::from_millis(500)) { Ok(_) => panic!( "{addr} accepted a connection; I12 tests need a refused address \ - that wiremock's 127.0.0.1 pool cannot occupy (#115)" + that wiremock's 127.0.0.1:0 pool cannot occupy (#115)" ), Err(e) if e.kind() == std::io::ErrorKind::TimedOut => panic!( "{addr} timed out; refusing to point the 30s client at an address \ diff --git a/crates/bugwarden/tests/preflight_wiremock.rs b/crates/bugwarden/tests/preflight_wiremock.rs index d059003..9267198 100644 --- a/crates/bugwarden/tests/preflight_wiremock.rs +++ b/crates/bugwarden/tests/preflight_wiremock.rs @@ -289,7 +289,7 @@ async fn preflight_transport_error_does_not_leak_the_api_key_i12() { let err = tokio::time::timeout(common::REFUSED_CONNECT_BUDGET, server.preflight()) .await - .expect("connect to the refused extra-loopback must not hang") + .expect("connect to the refused privileged port must not hang") .expect_err("an unreachable whoami endpoint must fail preflight"); let msg = format!("{err:#} {err:?}"); assert!( diff --git a/crates/bugwarden/tests/tools_wiremock.rs b/crates/bugwarden/tests/tools_wiremock.rs index 3a89e57..74c3c2a 100644 --- a/crates/bugwarden/tests/tools_wiremock.rs +++ b/crates/bugwarden/tests/tools_wiremock.rs @@ -1682,7 +1682,7 @@ async fn whoami_transport_error_does_not_leak_the_api_key_i12() { call(&client, "bug_info", json!({ "bug_ids": [7] })), ) .await - .expect("connect to the refused extra-loopback must not hang"); + .expect("connect to the refused privileged port must not hang"); let text = serde_json::to_string(&result).unwrap(); assert!( !text.contains("SUPERSECRETKEY123"),