From a333f10b808274c72843b6668775a558ac548af1 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 11:38:24 +0000 Subject: [PATCH 1/4] auth: reject redirect URIs carrying a query or fragment (MCP05) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The redirect-URI allow-list validated scheme/host/path/port but accepted a query or fragment component. /oauth/authorize appends `?code=…&state=…` to the redirect (build_redirect), so a redirect that already carried a query produced `code=123&code=…` parameter pollution that a lax downstream parser could resolve to an attacker-seeded value. PKCE + the exact-registered-redirect match make it non-exploitable today, but disallowing query/fragment removes the ambiguity as defense in depth (MCP05, per the security review). - redirect_uri_permitted: parse first, reject any URI with a query or fragment, on loopback and hosted alike, before the existing checks. Enforced at both /oauth/register and /oauth/authorize (both route through this function). - is_wellformed_hosted_redirect: also exclude query/fragment, so a query-bearing hosted redirect is classified `invalid_request` (client error) rather than routed to the "request access" page. - Tests: hosted_redirect_allow_list now asserts `?code=`, `?x=1`, and `#frag` are refused on both a hosted vendor callback and loopback, and that the same URI without a query still passes. - README: the hosted-redirect allow-list note documents the query/fragment rule. Every seeded vendor callback and every registered loopback URI is already query/fragment-free, so this rejects nothing legitimate. cargo test: 141 lib + 7 router + 1 doctest passing; clippy clean. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- README.md | 5 ++++- src/auth.rs | 41 ++++++++++++++++++++++++++++++++++------- 2 files changed, 38 insertions(+), 8 deletions(-) diff --git a/README.md b/README.md index d314906..a9a0500 100644 --- a/README.md +++ b/README.md @@ -534,7 +534,10 @@ the callback page. > MCP connector vendors' callback paths and widened per deployment with > `OAUTH_ALLOWED_REDIRECT_PREFIXES` (additive; each entry a full `https://host/path` > URL prefix, a bare domain is refused); loopback/native clients are exempt (the code -> resolves on the consenter's own machine). Enforced at both `…/oauth/register` and `…/oauth/authorize`. A client +> resolves on the consenter's own machine). No redirect (loopback or hosted) may +> carry a **query or fragment**: the authorization endpoint appends `?code=…&state=…`, +> so a pre-existing query would risk `code=…&code=…` parameter pollution, and a +> fragment is meaningless on a redirect target. Enforced at both `…/oauth/register` and `…/oauth/authorize`. A client > turned away is pointed at a contact address to request approval: > `…/oauth/register` says so in its JSON `error_description`, and a browser that > reaches `…/oauth/authorize` gets an on-brand "not allowed" page (not a raw error) diff --git a/src/auth.rs b/src/auth.rs index 52ec00a..e4b4a7d 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -286,8 +286,12 @@ fn has_dot_segment(redirect_uri: &str) -> bool { } /// Whether a `redirect_uri` may be registered or receive an authorization code. -/// Loopback (`http://localhost` / `127.0.0.1` / `[::1]`, any port) is always -/// allowed. A hosted redirect must be `https`, carry no userinfo, name no +/// No redirect (loopback or hosted) may carry a query or fragment component: the +/// authorization endpoint appends `?code=…&state=…`, so a pre-existing query would +/// risk `code=…&code=…` parameter pollution (MCP05), and a fragment is meaningless +/// on a redirect target. Loopback (`http://localhost` / `127.0.0.1` / `[::1]`, any +/// port) is otherwise always allowed. A hosted redirect must be `https`, carry no +/// userinfo, name no /// off-origin port (only the implicit/`:443` default is allowed), its host must /// equal (or be a subdomain of) an allow-listed registrable domain, AND its path /// must fall within that entry's pinned callback prefix (see @@ -299,12 +303,22 @@ fn has_dot_segment(redirect_uri: &str) -> bool { /// `https://user@claude.ai` is refused too (userinfo serves no purpose in a /// redirect target, only muddies which host is addressed, and loopback rejects it). fn redirect_uri_permitted(redirect_uri: &str) -> bool { - if is_loopback_redirect(redirect_uri) { - return true; - } let Ok(url) = url::Url::parse(redirect_uri) else { return false; }; + // Reject any query or fragment component (MCP05), on loopback and hosted alike. + // `/oauth/authorize` appends `?code=…&state=…` to the redirect, so a redirect + // that already carries a query invites `code=123&code=456` parameter pollution + // a lax downstream parser could resolve to an attacker-seeded value; a fragment + // serves no purpose in a redirect target. Every seeded vendor callback and every + // registered loopback URI is already query/fragment-free, so this rejects nothing + // legitimate while keeping the code-appending redirect unambiguous. + if url.query().is_some() || url.fragment().is_some() { + return false; + } + if is_loopback_redirect(redirect_uri) { + return true; + } if url.scheme() != "https" || !url.username().is_empty() || url.password().is_some() { return false; } @@ -341,8 +355,8 @@ fn redirect_uri_permitted(redirect_uri: &str) -> bool { /// because its host isn't on the list. It lets `/oauth/authorize` tell "a real /// client whose redirect just isn't approved yet" (→ the [`not_allowlisted_page`]) /// apart from a genuinely **malformed or ineligible** `redirect_uri` -/// (unparseable, non-`https`, or userinfo-bearing), which is a client-side request -/// error, not an approval gap. Loopback is intentionally NOT counted here: a +/// (unparseable, non-`https`, userinfo-bearing, or query/fragment-carrying), which +/// is a client-side request error, not an approval gap. Loopback is intentionally NOT counted here: a /// loopback redirect is always permitted, so it never reaches the not-permitted /// branch this distinction guards. fn is_wellformed_hosted_redirect(redirect_uri: &str) -> bool { @@ -352,6 +366,11 @@ fn is_wellformed_hosted_redirect(redirect_uri: &str) -> bool { && url.username().is_empty() && url.password().is_none() && url.host_str().is_some_and(|h| !h.is_empty()) + // A query or fragment makes it ineligible (MCP05), not merely + // unapproved, so it is classified `invalid_request` rather than + // routed to the "request access" page. + && url.query().is_none() + && url.fragment().is_none() } Err(_) => false, } @@ -1828,6 +1847,14 @@ mod tests { assert!(redirect_uri_permitted("https://claude.ai:443/api/mcp/auth_callback")); // A hosted redirect must be https even to an allowed domain + path. assert!(!redirect_uri_permitted("http://claude.ai/api/mcp/auth_callback")); + // A query or fragment is refused (MCP05: `?code=…` appended by the AS would + // pollute a redirect that already carries one), on hosted AND loopback, even + // when the host + path are otherwise valid. The same URI with no query passes. + assert!(!redirect_uri_permitted("https://claude.ai/api/mcp/auth_callback?code=123")); + assert!(!redirect_uri_permitted("https://claude.ai/api/mcp/auth_callback?x=1")); + assert!(!redirect_uri_permitted("https://claude.ai/api/mcp/auth_callback#frag")); + assert!(!redirect_uri_permitted("http://127.0.0.1:6112/cb?code=123")); + assert!(!redirect_uri_permitted("http://localhost/callback#frag")); // Defense in depth: a client registered before the allow-list (or via a // now-removed domain) still can't receive a code at /oauth/authorize. let junk = ClientReg { redirect_uris: vec!["https://example.com/cb".to_string()] }; From d60a2e94e20440b401f319c9191e2124151c6c13 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 12:45:12 +0000 Subject: [PATCH 2/4] Address review: avoid double-parse for loopback; fix doc summary Two Copilot review nits on the MCP05 change: - redirect_uri_permitted parsed the URL, then called is_loopback_redirect, which re-parsed the same string (twice-parsed on the /oauth/register and /oauth/authorize path). Split out is_loopback_url(&Url) and call it on the already-parsed url; is_loopback_redirect(&str) now delegates to it (kept for loopback_match and the tests). - is_wellformed_hosted_redirect's doc summary now mentions the query/fragment exclusion, matching the implementation. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- src/auth.rs | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/auth.rs b/src/auth.rs index e4b4a7d..fca3b8a 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -316,7 +316,9 @@ fn redirect_uri_permitted(redirect_uri: &str) -> bool { if url.query().is_some() || url.fragment().is_some() { return false; } - if is_loopback_redirect(redirect_uri) { + // Loopback is checked on the already-parsed `url` (not a re-parse of the raw + // string), since this runs on the `/oauth/register` and `/oauth/authorize` path. + if is_loopback_url(&url) { return true; } if url.scheme() != "https" || !url.username().is_empty() || url.password().is_some() { @@ -350,7 +352,7 @@ fn redirect_uri_permitted(redirect_uri: &str) -> bool { } /// Whether `redirect_uri` is a **well-formed hosted** redirect: it parses, is -/// `https`, carries no userinfo, and has a host. This is the shape that could +/// `https`, carries no userinfo, has a host, and has no query or fragment. This is the shape that could /// legitimately be allow-listed and that [`redirect_uri_permitted`] rejects only /// because its host isn't on the list. It lets `/oauth/authorize` tell "a real /// client whose redirect just isn't approved yet" (→ the [`not_allowlisted_page`]) @@ -1744,9 +1746,12 @@ fn bearer_challenge(had_token: bool, resource_metadata_url: &str) -> String { /// and the userinfo-with-port form `http://localhost:1234@evil.com` all parse to /// host `evil.com` (or carry userinfo) and are rejected. fn is_loopback_redirect(redirect_uri: &str) -> bool { - let Ok(url) = url::Url::parse(redirect_uri) else { - return false; - }; + url::Url::parse(redirect_uri).map(|u| is_loopback_url(&u)).unwrap_or(false) +} + +/// Loopback test on an already-parsed URL, so a caller that has parsed the +/// `redirect_uri` (e.g. [`redirect_uri_permitted`]) need not parse it again. +fn is_loopback_url(url: &url::Url) -> bool { url.scheme() == "http" && url.username().is_empty() && url.password().is_none() From f20962a40dbe48f03fd0616f069a3293d2627f29 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 13:56:17 +0000 Subject: [PATCH 3/4] auth: allowlist the redirect's shape via canonical-URL equality (review) Per review (sea-snake): prefer allow-listing the URL's permitted shape over denylisting each disallowed component. Replaced the separate scheme/userinfo/ port "is-not-present" checks in redirect_uri_permitted with a single positive check: rebuild the canonical `https://` from the parts we allow and require the parsed input to equal it. Any userinfo, off-origin port, http scheme, or other unexpected component now makes the two differ and is refused in one comparison. Kept separate (they can't be a single equality): the raw-string dot-segment check (url::Url normalises `.`/`..` away on parse, so the parsed path wouldn't reveal them) and the host/path-prefix allow-list match (the path is a prefix, not a fixed value). Query/fragment are still rejected up front for loopback and hosted alike (MCP05). Behavior-equivalent: hosted_redirect_allow_list (userinfo, :443/:444, query, fragment, dot-segments, wrong-path, look-alikes, http) all pass. cargo test: 141 lib + 7 router + 1 doctest green; clippy clean. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- src/auth.rs | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/src/auth.rs b/src/auth.rs index fca3b8a..8c3077e 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -321,24 +321,28 @@ fn redirect_uri_permitted(redirect_uri: &str) -> bool { if is_loopback_url(&url) { return true; } - if url.scheme() != "https" || !url.username().is_empty() || url.password().is_some() { - return false; - } - // Refuse an explicit non-default port: `https://claude.ai:444/…` is a DIFFERENT - // origin than the pinned `https://claude.ai` and may serve different content, so - // the path pin wouldn't hold. `url::Url::port()` returns `None` for the scheme's - // default (443), so `:443` and no-port both pass; only an off-origin port fails. - if url.port().is_some() { - return false; - } - // Refuse `.`/`..` (raw or `%2e`) path segments: they normalize in the browser - // and could escape the pinned callback prefix (`/connector/oauth/../../g/evil`). + // Refuse `.`/`..` (raw or `%2e`) path segments before url::Url normalises them + // away: they normalize in the browser and could escape the pinned callback prefix + // (`/connector/oauth/../../g/evil`), and the parsed path wouldn't reveal them. if has_dot_segment(redirect_uri) { return false; } let Some(host) = url.host_str() else { return false; }; + // Allowlist the redirect's SHAPE rather than denylisting each disallowed part: a + // permitted hosted redirect serialises to exactly `https://` (https, + // no userinfo, no port, no query, no fragment). Rebuild that canonical form from + // the parts we allow and require an exact match, so any unexpected component is + // refused in one comparison. url::Url drops the https-default `:443` (so it still + // matches), while an off-origin `:444`, a userinfo prefix, or an `http` scheme + // makes the two differ; query and fragment were already refused above. + let Ok(canonical) = url::Url::parse(&format!("https://{host}{}", url.path())) else { + return false; + }; + if url != canonical { + return false; + } let host = host.trim_end_matches('.').to_ascii_lowercase(); let path = url.path(); // host == domain (or a dot-boundary subdomain) AND the path is within the From 9ccee1d3edfdb4d0b5468192a58bb1653bd2bf3c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 29 Jul 2026 15:01:10 +0000 Subject: [PATCH 4/4] auth: drop the redundant dot-segment check; rely on url::Url normalization (review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Per review (sea-snake): the explicit has_dot_segment guard duplicates what url::Url::parse already does. Verified against this crate's url version that parse collapses every dot-segment form to the normalized path — raw `..`, `%2e%2e`, `%2E%2E`, and mixed `.%2e` all yield `/g/evil` — following the WHATWG URL Standard. So `.../connector/oauth/../../g/evil` reaches the matcher as `/g/evil` and is already rejected by the pinned-prefix check; the raw-string guard caught nothing extra. - Removed has_dot_segment and its unit test. - The pinned-prefix comment now notes that url.path() is already dot-segment-normalized, so traversal is caught by the prefix check. - hosted_redirect_allow_list keeps the traversal payloads as end-to-end regression assertions (raw + `%2e%2e` + `%2E%2E`, all rejected) — these now double as a guard that the parser keeps normalizing — and adds a case showing a dot-segment that normalizes to WITHIN the prefix is (correctly) allowed, which the old blanket guard wrongly rejected. cargo test: 140 lib + 7 router + 1 doctest green; clippy clean. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym --- src/auth.rs | 47 ++++++++++------------------------------------- 1 file changed, 10 insertions(+), 37 deletions(-) diff --git a/src/auth.rs b/src/auth.rs index 8c3077e..748d7be 100644 --- a/src/auth.rs +++ b/src/auth.rs @@ -271,20 +271,6 @@ fn path_within_prefix(path: &str, prefix: &str) -> bool { } } -/// Whether the redirect carries a `.`/`..` path segment (raw or `%2e`-encoded). A -/// user agent normalizes these away before requesting, so a redirect like -/// `/connector/oauth/../../g/evil` could satisfy the pinned-prefix check here yet -/// land OUTSIDE the prefix in the browser; such redirects are refused outright. -/// (`url::Url` already normalizes special-scheme paths, so this is a belt-and- -/// suspenders check on the raw input, independent of that normalization.) -fn has_dot_segment(redirect_uri: &str) -> bool { - let normalized = redirect_uri.to_ascii_lowercase().replace("%2e", "."); - normalized.split('/').any(|seg| { - let seg = seg.split(['?', '#']).next().unwrap_or(""); - seg == "." || seg == ".." - }) -} - /// Whether a `redirect_uri` may be registered or receive an authorization code. /// No redirect (loopback or hosted) may carry a query or fragment component: the /// authorization endpoint appends `?code=…&state=…`, so a pre-existing query would @@ -321,12 +307,6 @@ fn redirect_uri_permitted(redirect_uri: &str) -> bool { if is_loopback_url(&url) { return true; } - // Refuse `.`/`..` (raw or `%2e`) path segments before url::Url normalises them - // away: they normalize in the browser and could escape the pinned callback prefix - // (`/connector/oauth/../../g/evil`), and the parsed path wouldn't reveal them. - if has_dot_segment(redirect_uri) { - return false; - } let Some(host) = url.host_str() else { return false; }; @@ -344,6 +324,10 @@ fn redirect_uri_permitted(redirect_uri: &str) -> bool { return false; } let host = host.trim_end_matches('.').to_ascii_lowercase(); + // `url.path()` is already dot-segment-normalized: url::Url follows the WHATWG URL + // Standard, collapsing `.`/`..` (raw or `%2e`/`%2E`-encoded) on parse, so a + // traversal like `/connector/oauth/../../g/evil` arrives here as `/g/evil` and is + // caught by the prefix check below (no separate raw-string dot-segment guard needed). let path = url.path(); // host == domain (or a dot-boundary subdomain) AND the path is within the // vendor's pinned callback prefix. The path pin is what keeps a registration @@ -1827,10 +1811,14 @@ mod tests { // Right domain, wrong path, plus a non-segment-boundary near-miss of the pin. assert!(!redirect_uri_permitted("https://claude.ai/foo")); assert!(!redirect_uri_permitted("https://claude.ai/api/mcp/auth_callbackEVIL")); - // Dot-segment traversal that would normalize (in the browser) outside the - // pinned prefix, raw and percent-encoded (`%2e`) forms. + // Dot-segment traversal (raw and percent-encoded): url::Url normalizes these + // to `/g/evil` on parse (WHATWG), which then fails the pinned-prefix check. assert!(!redirect_uri_permitted("https://chatgpt.com/connector/oauth/../../g/evil")); assert!(!redirect_uri_permitted("https://chatgpt.com/connector/oauth/%2e%2e/%2e%2e/g/evil")); + assert!(!redirect_uri_permitted("https://chatgpt.com/connector/oauth/%2E%2E/%2E%2E/g/evil")); + // A dot-segment that normalizes to WITHIN the vendor's pinned prefix is fine + // (it lands in the vendor's own callback space, not an escape). + assert!(redirect_uri_permitted("https://chatgpt.com/connector/oauth/x/../y")); // Cursor: allowed at its real hosted callback path (registered as // www.cursor.com), refused on any other path. assert!(redirect_uri_permitted("https://www.cursor.com/agents/mcp/oauth/callback")); @@ -1870,21 +1858,6 @@ mod tests { assert!(!redirect_allowed(Some(&junk), "https://example.com/cb")); } - /// Dot-segment detection, independent of `url::Url` normalization: whole-segment - /// `.`/`..` (raw or `%2e`) is caught; dots inside a segment and in the query are not. - #[test] - fn detects_dot_segments() { - use super::has_dot_segment; - assert!(has_dot_segment("https://x.example/a/../b")); - assert!(has_dot_segment("https://x.example/a/./b")); - assert!(has_dot_segment("https://x.example/a/%2e%2e/b")); - assert!(has_dot_segment("https://x.example/a/%2E%2E/b")); - assert!(has_dot_segment("https://x.example/foo/..")); - assert!(!has_dot_segment("https://x.example/api/mcp/auth_callback")); - assert!(!has_dot_segment("https://x.example/a.b/c..d")); // dots inside a segment - assert!(!has_dot_segment("https://x.example/cb?next=../x")); // query, not path - } - /// `OAUTH_ALLOWED_REDIRECT_PREFIXES` entries parse to `(host, path)` only for a /// bare `https://host/path`; a port, query, fragment, or userinfo is refused /// (dropped) rather than silently discarded, and so is a non-https or root-path