fix(http): refuse HTTPS→HTTP redirect downgrades

crates/tinymcp/src/transport/http/mod.rs

@@ -12,10 +12,13 @@
 //! retried, so a server that answers 404 for some other reason costs one extra
 //! round trip rather than an unbounded loop.
 //!
-//! **Redirects are followed, up to five.** Servers are commonly published
-//! behind a vanity URL that redirects to the real endpoint. `reqwest` strips
-//! `Authorization` and `Cookie` on a cross-origin redirect, so a bearer token
-//! does not follow the request to another host.
+//! **Redirects are followed, up to five, but an HTTPS→HTTP downgrade is
+//! refused.** Servers are commonly published behind a vanity URL that redirects
+//! to the real endpoint. `reqwest` strips `Authorization` and `Cookie` on a
+//! cross-origin redirect, so a bearer token does not follow the request to
+//! another host — but a same-origin downgrade, and any custom header or
+//! query-param credential on any hop, are not stripped, so the policy itself
+//! refuses a hop that would move the request from HTTPS to plaintext.
 //!
 //! **The SSE body is read incrementally.** See the `sse` module for why that is
 //! load-bearing rather than an optimization.
@@ -171,10 +174,14 @@ impl McpHttpClientBuilder {
             .connect_timeout(CONNECT_TIMEOUT)
             // Servers are commonly published behind a vanity URL that redirects
             // to the real endpoint; refusing to follow it surfaces as a bare
-            // "MCP HTTP 301". `reqwest` strips `Authorization` and `Cookie` on
-            // a cross-origin redirect, so a bearer token does not follow the
-            // request to another host.
-            .redirect(reqwest::redirect::Policy::limited(MAX_REDIRECTS));
+            // "MCP HTTP 301". A custom policy follows up to `MAX_REDIRECTS`
+            // hops but refuses an HTTPS→HTTP downgrade, which would carry any
+            // attached credential over plaintext. `reqwest` strips
+            // `Authorization` and `Cookie` on a cross-origin redirect, so a
+            // bearer token does not follow the request to another host, but
+            // custom headers, query-param credentials and same-origin
+            // downgrades are not stripped — the policy closes that gap.
+            .redirect(redirect_policy());
 
         if let Some(proxy) = self.proxy.as_ref() {
             builder = apply_proxy(builder, proxy);
@@ -197,6 +204,61 @@ impl McpHttpClientBuilder {
     }
 }
 
+/// The redirect policy every HTTP client uses.
+///
+/// Follows vanity-URL redirects (servers are commonly published behind one)
+/// but caps the chain at [`MAX_REDIRECTS`] and refuses an HTTPS→HTTP downgrade:
+/// a redirect that moves the request to plaintext after it has been over TLS
+/// would carry any attached credential in the clear. The same-origin case is
+/// the gap `reqwest` leaves open — it strips `Authorization` and `Cookie` only
+/// on a cross-origin hop, so a bearer on a same-host downgrade and any custom
+/// header or query-param credential on any hop would otherwise follow. A
+/// refused downgrade surfaces as a redirect error rather than a silent leak.
+fn redirect_policy() -> reqwest::redirect::Policy {
+    reqwest::redirect::Policy::custom(|attempt| {
+        // `previous[0]` is the initial URL (reqwest counts it, not a redirect),
+        // so it is the scheme the host configured — and, for a credentialed
+        // non-loopback endpoint, the one already required to be HTTPS.
+        let origin_scheme = attempt.previous().first().map(reqwest::Url::scheme);
+        match redirect_decision(
+            origin_scheme,
+            attempt.url().scheme(),
+            attempt.previous().len(),
+        ) {
+            RedirectDecision::Follow => attempt.follow(),
+            RedirectDecision::Error(msg) => attempt.error(msg),
+        }
+    })
+}
+
+/// What [`redirect_policy`] decides for one hop, as a pure function of its
+/// inputs so the rule is unit-testable without standing up a redirect server.
+///
+/// `hops` is `previous.len()`, matching reqwest's `Limit` accounting: the
+/// initial URL is counted, so a chain that has followed `MAX_REDIRECTS`
+/// redirects reports `MAX_REDIRECTS + 1`.
+#[derive(Debug, PartialEq, Eq)]
+enum RedirectDecision {
+    Follow,
+    Error(&'static str),
+}
+
+fn redirect_decision(
+    origin_scheme: Option<&str>,
+    target_scheme: &str,
+    hops: usize,
+) -> RedirectDecision {
+    if origin_scheme == Some("https") && target_scheme == "http" {
+        return RedirectDecision::Error(
+            "refusing an https→http redirect that would expose credentials in cleartext",
+        );
+    }
+    if hops > MAX_REDIRECTS {
+        return RedirectDecision::Error("too many redirects");
+    }
+    RedirectDecision::Follow
+}
+
 /// Applies a resolved proxy to a client builder.
 ///
 /// An unusable proxy URL is logged and skipped rather than failing the build:

crates/tinymcp/src/transport/http/test.rs

@@ -1291,3 +1291,52 @@ fn a_payload_split_over_several_data_lines_is_joined() {
 
     assert_eq!(value, json!({ "ok": true }));
 }
+
+// ---------------------------------------------------------------------------
+// Redirect policy
+// ---------------------------------------------------------------------------
+
+#[test]
+fn follows_an_https_to_https_redirect() {
+    use super::{RedirectDecision, redirect_decision};
+    assert_eq!(
+        redirect_decision(Some("https"), "https", 1),
+        RedirectDecision::Follow
+    );
+}
+
+#[test]
+fn refuses_an_https_to_http_downgrade() {
+    use super::{RedirectDecision, redirect_decision};
+    // A same-origin downgrade is the gap reqwest leaves open: a bearer on a
+    // same-host hop, and any custom header or query-param credential on any
+    // hop, are not stripped. The policy refuses the hop instead.
+    let decision = redirect_decision(Some("https"), "http", 1);
+    assert!(matches!(decision, RedirectDecision::Error(_)));
+}
+
+#[test]
+fn does_not_refuse_a_plain_http_redirect_that_started_on_http() {
+    // An endpoint the host already allowed over HTTP (loopback, or an
+    // unauthenticated server) is not downgraded by staying on HTTP.
+    use super::{RedirectDecision, redirect_decision};
+    assert_eq!(
+        redirect_decision(Some("http"), "http", 1),
+        RedirectDecision::Follow
+    );
+}
+
+#[test]
+fn caps_the_redirect_chain_at_max_redirects() {
+    // `hops` counts the initial URL (reqwest's accounting), so the cap fires
+    // one past `MAX_REDIRECTS` — matching `Policy::limited`.
+    use super::{MAX_REDIRECTS, RedirectDecision, redirect_decision};
+    assert_eq!(
+        redirect_decision(Some("https"), "https", MAX_REDIRECTS),
+        RedirectDecision::Follow
+    );
+    assert!(matches!(
+        redirect_decision(Some("https"), "https", MAX_REDIRECTS + 1),
+        RedirectDecision::Error(_)
+    ));
+}