diff --git a/crates/gitlawb-node/src/main.rs b/crates/gitlawb-node/src/main.rs index 6f48c3f..236e910 100644 --- a/crates/gitlawb-node/src/main.rs +++ b/crates/gitlawb-node/src/main.rs @@ -145,16 +145,8 @@ async fn main() -> Result<()> { None }; - // Do not follow redirects: this client fans out to peer-supplied URLs - // (sync trigger, profile/repo fetches). A peer answering `302 Location: - // http://127.0.0.1/` would otherwise bypass the announce-time public-URL - // check and turn the redirect into an SSRF. - let http_client = Arc::new( - reqwest::Client::builder() - .timeout(std::time::Duration::from_secs(10)) - .redirect(reqwest::redirect::Policy::none()) - .build()?, - ); + // Shared no-redirect HTTP client. See build_http_client for the SSRF rationale. + let http_client = Arc::new(build_http_client()?); let (ref_update_tx, _) = tokio::sync::broadcast::channel::(256); let (task_event_tx, _) = tokio::sync::broadcast::channel::(256); @@ -515,7 +507,12 @@ async fn gossip_task( } } - let client = reqwest::Client::new(); + // Reuse the shared no-redirect client for every gossip outbound call (the + // bootstrap announce POST and the periodic peer /health ping). Peer URLs are + // attacker-influenceable, so a 3xx to a private address must not be followed. + // Do NOT fall back to reqwest::Client::new(): its default follows redirects + // and would reintroduce the SSRF closed here (#93). + let client = state.http_client.clone(); let my_did = state.node_did.to_string(); let my_url = state.config.public_url.clone().unwrap_or_default(); @@ -590,13 +587,7 @@ async fn gossip_task( Err(_) => continue, }; for peer in peers { - let url = format!("{}/health", peer.http_url.trim_end_matches('/')); - let ok = client - .get(&url) - .send() - .await - .map(|r| r.status().is_success()) - .unwrap_or(false); + let ok = ping_peer_health(&client, &peer.http_url).await; let _ = state.db.mark_peer_ping(&peer.did, ok).await; } } @@ -610,6 +601,38 @@ async fn gossip_task( } } +/// Build the shared node HTTP client used for every outbound fan-out (sync +/// trigger, profile/repo fetches, gossip announce + peer pings). +/// +/// No redirects: peer URLs are attacker-influenceable, so a `3xx` to a private +/// address must not be followed (SSRF guard, #78/#93). Do NOT replace with +/// `reqwest::Client::new()` — its default follows redirects. Kept as a named +/// builder so tests bind the redirect guarantee to the real client the node +/// runs, not a hand-rolled equivalent. +fn build_http_client() -> reqwest::Result { + reqwest::Client::builder() + .timeout(std::time::Duration::from_secs(10)) + .redirect(reqwest::redirect::Policy::none()) + .build() +} + +/// Ping a peer's `/health` endpoint and report whether it answered 2xx. +/// +/// Takes the client by reference so callers supply the shared, no-redirect +/// `state.http_client`. Peer URLs are attacker-influenceable, so a `3xx` to a +/// private address must not be followed. Do NOT call this with a bare +/// `reqwest::Client::new()`: its default follows redirects and would +/// reintroduce the SSRF this guards against (#93). +async fn ping_peer_health(client: &reqwest::Client, http_url: &str) -> bool { + let url = format!("{}/health", http_url.trim_end_matches('/')); + client + .get(&url) + .send() + .await + .map(|r| r.status().is_success()) + .unwrap_or(false) +} + fn load_or_create_keypair(config: &Config) -> Result { let key_path = config.resolved_key_path(); @@ -642,3 +665,64 @@ fn load_or_create_keypair(config: &Config) -> Result { Ok(kp) } } + +#[cfg(test)] +mod gossip_ssrf_tests { + use super::ping_peer_health; + + // Build the client exactly as production does (super::build_http_client) so + // these tests bind the redirect guarantee to the real shared client the + // node runs. A regression that makes build_http_client follow redirects + // fails ping_peer_health_does_not_follow_redirect. + fn production_http_client() -> reqwest::Client { + super::build_http_client().expect("failed to build production http client") + } + + // A peer answering `/health` with a 302 toward an internal address must not + // be followed: the redirect target must never be requested (#93). + #[tokio::test] + async fn ping_peer_health_does_not_follow_redirect() { + let mut server = mockito::Server::new_async().await; + let internal = server + .mock("GET", "/internal-metadata") + .with_status(200) + .expect(0) + .create_async() + .await; + let _health = server + .mock("GET", "/health") + .with_status(302) + .with_header("location", &format!("{}/internal-metadata", server.url())) + .create_async() + .await; + + let ok = ping_peer_health(&production_http_client(), &server.url()).await; + + assert!(!ok, "a 302 must not count as a healthy peer"); + // expect(0) is enforced only at assert time; this fails if the redirect + // was followed to the internal target. + internal.assert_async().await; + } + + #[tokio::test] + async fn ping_peer_health_reports_success_on_200() { + let mut server = mockito::Server::new_async().await; + let _health = server + .mock("GET", "/health") + .with_status(200) + .create_async() + .await; + + let ok = ping_peer_health(&production_http_client(), &server.url()).await; + + assert!(ok, "a 200 /health must count as a healthy peer"); + } + + // A transport error (nothing listening) must map to unhealthy, never a + // spurious healthy — the .unwrap_or(false) arm. + #[tokio::test] + async fn ping_peer_health_reports_unhealthy_on_connection_error() { + let ok = ping_peer_health(&production_http_client(), "http://127.0.0.1:1").await; + assert!(!ok, "a connection error must count as an unhealthy peer"); + } +}