diff --git a/app/src-tauri/src/marketplace/auth.rs b/app/src-tauri/src/marketplace/auth.rs index d91e4da..2dd1d81 100644 --- a/app/src-tauri/src/marketplace/auth.rs +++ b/app/src-tauri/src/marketplace/auth.rs @@ -30,6 +30,11 @@ pub fn host_kind(host: &str) -> HostKind { /// `host[:port]` characters only — also what keeps a host safe as a `gh` argument. /// +/// This is a character-set check, not full `host:port` validation — it does +/// not bound a port to 0–65535 or otherwise parse the `:port` suffix. A +/// caller that needs that (e.g. a host validator layered on top of this one) +/// checks the port itself. +/// /// `pub(crate)` so other validators (the add-marketplace form, `gh_login`) use /// this same rule instead of a divergent copy (pre-flight F13). pub(crate) fn valid_host(host: &str) -> bool { @@ -182,6 +187,13 @@ fn http_client() -> Result { reqwest::Client::builder() .user_agent("Triple-C") .timeout(Duration::from_secs(15)) + // reqwest's default policy follows up to 10 redirects and only + // strips Authorization/Cookie/Proxy-Authorization/WWW-Authenticate + // on a cross-*host* hop — GitLab's PRIVATE-TOKEN header (and any + // header on a same-host https→http downgrade) would otherwise + // follow the token to wherever the response points. Never follow; + // `who_am_i` treats the resulting 3xx like an unrecognised API. + .redirect(reqwest::redirect::Policy::none()) .build() .map_err(|e| format!("Could not create an HTTP client: {}", e)) } @@ -237,6 +249,10 @@ async fn who_am_i( } 401 | 403 => Ok(Probe::Rejected(status)), 404 => Ok(Probe::NotThisKind), + // The client never follows redirects (see `http_client`); a 3xx here + // means this API would have sent the token onward, so treat it the + // same as a host that isn't this kind rather than as an error. + 300..=399 => Ok(Probe::NotThisKind), other => Err(format!( "{} answered HTTP {} when checking the token", base, other @@ -313,6 +329,10 @@ fn who(account: Option<&MarketplaceAccount>) -> String { /// User-facing message for a failed fetch, naming the account used and, for /// access problems, the usual organisation causes with the page that fixes each. +/// +/// `url` must be a marketplace URL already validated by [`host_of`] (as every +/// stored marketplace's URL is) — it is echoed into the message verbatim, so +/// passing unvalidated user input here would defeat the point of N17. pub fn describe_fetch_error( err: &FetchError, account: Option<&MarketplaceAccount>, @@ -382,6 +402,16 @@ mod tests { assert!(!err.contains("test-token-not-real")); } + /// Pre-flight N17, the parse-failure branch specifically: a URL that is + /// both malformed (port out of `u16` range) *and* carries credentials + /// must not have either the credentials or the raw URL echoed back. + #[test] + fn host_of_never_echoes_a_credential_bearing_url_that_fails_to_parse() { + let err = host_of("https://user:test-token-not-real@github.com:99999/a").unwrap_err(); + assert!(!err.contains("test-token-not-real"), "{}", err); + assert!(!err.contains("user:"), "{}", err); + } + #[test] fn fetch_username_per_host() { assert_eq!( @@ -552,4 +582,71 @@ mod tests { assert!(validate_token("-evil", FAKE).await.is_err()); assert!(validate_token("github.com", " ").await.is_err()); } + + /// Fix-round-1 security finding: reqwest's default redirect policy + /// follows up to 10 hops and only strips Authorization/Cookie/ + /// Proxy-Authorization/WWW-Authenticate on a cross-host hop — GitLab's + /// PRIVATE-TOKEN header is none of those, so an unfollowed-by-default + /// client is the only thing stopping a malicious/compromised "GitLab" + /// host from redirecting the probe (with the token still attached) to + /// an attacker-controlled target. Plain `std::net::TcpListener`s stand + /// in for the origin and the redirect target so the test can assert the + /// target is never even connected to, let alone handed the header. + #[tokio::test] + async fn redirect_is_never_followed_and_the_token_never_reaches_the_target() { + use std::io::{Read, Write}; + use std::net::TcpListener; + use std::sync::mpsc; + use std::time::Duration as StdDuration; + + // The redirect target. If the client ever followed the redirect, + // this listener would receive the request — token header included. + let target = TcpListener::bind("127.0.0.1:0").unwrap(); + let target_addr = target.local_addr().unwrap(); + let (tx, rx) = mpsc::channel::(); + std::thread::spawn(move || { + target.set_nonblocking(false).ok(); + if let Ok((mut stream, _)) = target.accept() { + let mut buf = [0u8; 4096]; + let n = stream.read(&mut buf).unwrap_or(0); + let request = String::from_utf8_lossy(&buf[..n]).to_string(); + let _ = stream.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n\r\n"); + let _ = tx.send(request); + } + }); + + // The origin the probe actually asks, which answers with a 3xx + // pointing at the target above. + let origin = TcpListener::bind("127.0.0.1:0").unwrap(); + let origin_addr = origin.local_addr().unwrap(); + std::thread::spawn(move || { + if let Ok((mut stream, _)) = origin.accept() { + let mut buf = [0u8; 4096]; + let _ = stream.read(&mut buf); + let body = format!( + "HTTP/1.1 302 Found\r\nLocation: http://{}/api/v4/user\r\nContent-Length: 0\r\n\r\n", + target_addr + ); + let _ = stream.write_all(body.as_bytes()); + } + }); + + let base = format!("http://{}", origin_addr); + let client = http_client().unwrap(); + let outcome = who_am_i(&client, HostKind::GitLab, &base, FAKE).await; + + // The redirect is reported as "not this kind of host", not an error + // and not a login — it must not be silently trusted either way. + assert_eq!(outcome.unwrap(), Probe::NotThisKind); + + // And the target must never see a connection carrying the token — + // ideally no connection at all, since the client never follows. + match rx.recv_timeout(StdDuration::from_millis(500)) { + Ok(request) => assert!( + !request.contains(FAKE) && !request.to_ascii_lowercase().contains("private-token"), + "the redirect target must never receive the token: {request}" + ), + Err(_) => {} // no connection at all — the expected outcome + } + } }