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..f4b3438 100644 --- a/crates/bugwarden-core/tests/guard_wiremock.rs +++ b/crates/bugwarden-core/tests/guard_wiremock.rs @@ -32,6 +32,53 @@ 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.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, 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: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 \ + 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 +455,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 privileged port must not hang") + .unwrap_err(); + assert_key_absent_from_transport_error(&err); } #[tokio::test] @@ -475,11 +518,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 +1178,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 privileged port must not hang") + .unwrap_err(); + assert_key_absent_from_transport_error(&err); } // --------------------------------------------------------------------------- @@ -1218,18 +1253,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 privileged port 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..cb6b4d2 --- /dev/null +++ b/crates/bugwarden/tests/common/mod.rs @@ -0,0 +1,33 @@ +//! Shared helpers for bugwarden integration tests. + +/// Connect-time failure that wiremock's 127.0.0.1 pool cannot serve (#115). +/// +/// `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, 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: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 \ + 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..9267198 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 privileged port 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..74c3c2a 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 privileged port must not hang"); let text = serde_json::to_string(&result).unwrap(); assert!( !text.contains("SUPERSECRETKEY123"),