fix(mcp-oauth): disable redirect following on OAuth HTTP clients (SSRF + credential leak)#6315
Conversation
The four MCP OAuth outbound clients (metadata discovery, token exchange, dynamic client registration, refresh) followed HTTP redirects: reqwest's default policy follows up to 10 hops and the bundled-CA client used Policy::limited(5). The per-URL SSRF guard validates only the initial URL, never the redirect Location, so a malicious or compromised authorization server could 307/308 a credential-bearing POST to replay client_secret / code_verifier / refresh_token to an attacker-controlled host, or 302 metadata discovery into a blind SSRF / cloud-metadata pivot. Add proxy-aware oauth_client_builder() / oauth_client() pinned to redirect::Policy::none() in librefang-http and route all four call sites through it. refresh and DCR also gain the proxy support discovery and token exchange already had (#3577); with no proxy configured their behavior is unchanged. This mirrors librefang_runtime::web_fetch's pinned client, which disables redirect following for the same reason. The shared build_http_client default is left untouched so LLM-streaming callers keep following redirects. Adds a loopback regression test asserting oauth_client() returns a 302 verbatim and never requests the redirect target.
github-actions
Bot
added
size/M
houko
left a comment
houko
left a comment
There was a problem hiding this comment.
Pushed one mechanical fix (missing (#6315) PR reference in the CHANGELOG security entry — every other entry in the file has a PR number before the attribution). Five inline comments for CLAUDE.md prose-wrapping violations in the new comment blocks; no logic issues found. Security reasoning in the PR is sound.
Generated by Claude Code
| /// Proxy-aware [`reqwest::ClientBuilder`] that NEVER follows HTTP redirects, for OAuth machine-to-machine endpoints (metadata discovery, token exchange, dynamic client registration, token refresh). | ||
| /// | ||
| /// OAuth endpoints return JSON directly and have no legitimate reason to emit a 3xx mid-flow. | ||
| /// Following a redirect on these calls is a security hole the per-URL SSRF guard cannot catch, because that guard validates only the initial URL — never the redirect `Location`. | ||
| /// A `307`/`308` on a credential-bearing POST replays the request body (`client_secret`, `code_verifier`, `refresh_token`) to an attacker-controlled redirect target; a `302` on discovery becomes a blind SSRF / cloud-metadata pivot. | ||
| /// | ||
| /// This mirrors `librefang_runtime::web_fetch`'s pinned client, which disables reqwest's automatic redirect following for the same reason. | ||
| /// Use this for every outbound OAuth request; do not reach for [`proxied_client_builder`] there, whose default policy follows up to 10 hops. |
There was a problem hiding this comment.
CLAUDE.md: "never write multi-paragraph docstrings or multi-line comment blocks — one short line max."
This doc comment spans two paragraphs (8 lines). The security rationale is genuinely important, but the rule caps doc comments at a single short line. Consider condensing to something like:
/// Proxy-aware OAuth [`reqwest::ClientBuilder`] with `redirect::Policy::none()` — a 3xx redirect on a credential POST replays the body to the redirect target, bypassing the SSRF guard that validates only the initial URL.
Generated by Claude Code
| // Exchange authorization code for tokens. | ||
| // Use the proxy-aware client so token endpoint requests respect proxy config | ||
| // and inherit default connect/read timeouts (prevents hung token exchanges). | ||
| let http_client = librefang_kernel::http_client::proxied_client(); | ||
| // Use the proxy-aware OAuth client so token endpoint requests respect proxy | ||
| // config and inherit default connect/read timeouts (prevents hung token | ||
| // exchanges). Redirects are disabled: a 307/308 here would replay the PKCE | ||
| // code_verifier and any client_secret to an attacker-controlled target. |
There was a problem hiding this comment.
CLAUDE.md prose-wrapping rule: "Break only at sentence boundaries (after ., ?, !, or other terminal punctuation). One sentence = one line, regardless of length."
This block is two sentences wrapped at a column limit. Should be:
// Use the proxy-aware OAuth client so token endpoint requests respect proxy config and inherit default connect/read timeouts (prevents hung token exchanges).
// Redirects are disabled: a 307/308 here would replay the PKCE code_verifier and any client_secret to an attacker-controlled target.Generated by Claude Code
| // Proxy-aware OAuth client with redirects disabled: a 307/308 on the | ||
| // refresh POST would replay refresh_token / client_secret to the | ||
| // redirect target, and the SSRF guard above validates only the initial | ||
| // token_endpoint, not redirect hops. `oauth_client` also routes through | ||
| // [proxy] config (#3577), which the previous bundled-CA client skipped. |
There was a problem hiding this comment.
CLAUDE.md prose-wrapping rule: sentences should not be broken at a column limit. This block is two sentences wrapped at ~80 columns. Should be:
// Proxy-aware OAuth client with redirects disabled: a 307/308 on the refresh POST would replay refresh_token / client_secret to the redirect target, and the SSRF guard above validates only the initial token_endpoint, not redirect hops.
// `oauth_client` also routes through [proxy] config (#3577), which the previous bundled-CA client skipped.Generated by Claude Code
| // Proxy-aware OAuth client with redirects disabled — same rationale as | ||
| // try_refresh / token exchange: an OAuth endpoint emitting a 3xx is | ||
| // never legitimate, and following it would pivot the registration POST | ||
| // (and the client_secret the AS echoes back) to an attacker target. |
There was a problem hiding this comment.
Same prose-wrapping violation as the try_refresh block above: column-wrapping mid-sentence. Should be one sentence per line:
// Proxy-aware OAuth client with redirects disabled — same rationale as try_refresh / token exchange: an OAuth endpoint emitting a 3xx is never legitimate, and following it would pivot the registration POST (and the client_secret the AS echoes back) to an attacker target.Generated by Claude Code
| // OAuth metadata discovery must respect [proxy] config (#3577) — corporate | ||
| // networks routinely require a proxy and OAuth was a primary failure case. | ||
| let client = librefang_http::proxied_client_builder() | ||
| // Redirects are disabled: a malicious/compromised MCP server returning a | ||
| // 302 would otherwise drive a blind SSRF / cloud-metadata pivot, since the | ||
| // SSRF guard validates only the initial URL, not redirect hops. |
There was a problem hiding this comment.
Column-wrapping mid-sentence. The two added sentences should each be on their own line:
// OAuth metadata discovery must respect [proxy] config (#3577) — corporate networks routinely require a proxy and OAuth was a primary failure case.
// Redirects are disabled: a malicious/compromised MCP server returning a 302 would otherwise drive a blind SSRF / cloud-metadata pivot, since the SSRF guard validates only the initial URL, not redirect hops.Generated by Claude Code
| // --------------------------------------------------------------------------- | ||
| // Regression: OAuth HTTP clients must NOT follow redirects. | ||
| // | ||
| // A 307/308 on a credential-bearing OAuth POST (token exchange, refresh, DCR) | ||
| // would replay client_secret / code_verifier / refresh_token to the redirect | ||
| // target, and a 302 on metadata discovery is a blind SSRF / cloud-metadata | ||
| // pivot. The per-URL SSRF guard validates only the initial URL, so the client | ||
| // built by `oauth_client()` must itself refuse to follow any redirect. | ||
| // --------------------------------------------------------------------------- |
There was a problem hiding this comment.
CLAUDE.md: "never write multi-paragraph docstrings or multi-line comment blocks — one short line max."
The separator-line banner (// -----...) is the pre-existing test file's convention, but the new block adds a multi-paragraph prose comment inside the separators. If the function name test_oauth_client_does_not_follow_redirects is clear enough on its own (it is), the entire block can be dropped. If the WHY needs capturing, the existing separator pattern in this file already carries the first line; the multi-sentence paragraph inside it (lines 370–374) should be reduced to the one non-obvious constraint: that Policy::none() is the mechanism being tested.
Generated by Claude Code
2 participants
Summary
The four MCP OAuth outbound HTTP call sites built their reqwest clients with redirect following enabled — reqwest's default policy follows up to 10 hops, and the kernel/extensions bundled-CA client used
redirect::Policy::limited(5).The per-URL SSRF guard (
is_ssrf_blocked_url) validates only the initial request URL; it never re-checks the redirectLocation.A malicious or compromised MCP authorization server could therefore:
307/308on a credential-bearing POST (authorization-code exchange, token refresh, dynamic client registration) to replay the request body —client_secret, the PKCEcode_verifier, therefresh_token— to an attacker-controlled redirect target; or302on metadata discovery to drive a blind SSRF / cloud-metadata (IMDS) pivot from the long-running daemon.This brings the OAuth call paths in line with
librefang_runtime::web_fetch's pinned client, which already disables reqwest's automatic redirect following for exactly this reason.Changes
librefang-http: addoauth_client_builder()/oauth_client()— proxy-aware client builders that pinredirect::Policy::none(). The sharedbuild_http_clientdefault is left untouched (LLM-streaming callers legitimately need redirects).discover_oauth_metadata(librefang-runtime-mcp): build the discovery client viaoauth_client_builder().librefang-api,routes/mcp_auth.rs): build vialibrefang_kernel::http_client::oauth_client().try_refreshandregister_client/ DCR (librefang-kernel,mcp_oauth_provider.rs): switch fromlibrefang_extensions::http_client::new_client()(bundled-CA,Policy::limited(5), proxy-unaware) tolibrefang_runtime::http_client::oauth_client(). This disables redirects and also gives refresh/DCR the same proxy support discovery and token exchange already had (arch: six independent reqwest::Client constructions bypass librefang-http proxy/TLS #3577); with no proxy configured the behavior is unchanged.Otherwise the per-site handling is unchanged: each call site already treats a non-
is_success()response as a failure (discovery falls through to the next tier / config fallback; token/refresh/DCR log a redacted digest and surface a generic error).With redirects disabled the credential body is never re-sent to a redirect target.
Verification
cargo check -p librefang-http -p librefang-runtime-mcp -p librefang-api -p librefang-kernel -p librefang-runtime --lib— passed, clean.cargo clippy -p librefang-http -p librefang-runtime-mcp -p librefang-api -p librefang-kernel -p librefang-runtime --all-targets -- -D warnings— passed, zero warnings.cargo test -p librefang-runtime --test mcp_oauth_integration— 10 passed, 0 failed. The newtest_oauth_client_does_not_follow_redirectsstands up a loopback mock that answers every non-/followedrequest with a302 Location: /followed; it assertsoauth_client()returns the302verbatim (status302) and never requests the redirect target. Existing OAuth discovery/lifecycle tests still pass.(Run in the repo's
Dockerfile.rust-devimage against an isolated target volume, per the project's no-local-cargo rule.)Out of scope (intentionally separate PRs)
is_ssrf_blocked_urlmatches host strings literally, so a hostname whose A-record points at169.254.169.254/127.0.0.1still passes the initial-URL guard. Closing it requires resolving DNS before the IP-tier check (and pinning the connecting client's resolution, asweb_fetchdoes);librefang-channelscurrently documents DNS-resolution SSRF as out of scope by design, so this is a deliberate, larger follow-up rather than something to bolt on here.