Marketplace auth: never follow redirects in token validation

Review fix round 1 for Task 5. reqwest's default redirect policy only
strips Authorization/Cookie/Proxy-Authorization/WWW-Authenticate on a
cross-host hop, so GitLab's PRIVATE-TOKEN header (and Authorization on
an https->http same-host downgrade) would have followed a redirect to
an attacker-controlled target. The client now disables redirects
outright, and a 3xx response is treated as "not this kind of host"
rather than an error. Also adds the missing N17 test for a malformed,
credential-bearing URL, and clarifies two doc comments.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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, String> {
     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::<String>();
+        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
+        }
+    }
 }