Reject unsafe redirects in CIMD documents#470
Merged
Conversation
A client that identifies by a vendor-hosted URL (Client ID Metadata Document) supplies its own redirect_uris in that document, and MCP_OAUTH_ALLOWED_REDIRECTS does not gate CIMD clients — the curated CIMD host allowlist is their trust anchor. validateCimdDocument checked only that redirect_uris was a non-empty array of strings, so an allowlisted host whose document listed a cleartext http redirect to a non-loopback host would have that redirect honoured, where a network attacker on the path could intercept the authorization code. Require each redirect_uri to be an https URL, a loopback http URL (RFC 8252: 127.0.0.1 / localhost / [::1], any port, reusing isLoopbackHost), or a custom app scheme (e.g. vscode:// or com.example.app:/…). Reject everything else: cleartext http to a non-loopback host, and any string that is not a parseable absolute URL (fail closed — every legitimate redirect form parses). No first-party client is affected. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
alistair3149
force-pushed
the
cimd-reject-cleartext-redirects
branch
from
July 23, 2026 05:29
6bdc657 to
375a929
Compare
alistair3149
marked this pull request as ready for review
July 23, 2026 05:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A client that identifies by a vendor-hosted URL (a Client ID Metadata Document, CIMD) supplies its own
redirect_urisin that document.MCP_OAUTH_ALLOWED_REDIRECTSdoes not gate CIMD clients — the curated CIMD host allowlist (MCP_OAUTH_CIMD_ALLOWED_HOSTS+ shipped first-party hosts) is their trust anchor.validateCimdDocumentchecked only thatredirect_uriswas a non-empty array of strings, so an allowlisted host whose document listed a cleartexthttpredirect to a non-loopback host would have it honored — a target a network attacker on that path could intercept the authorization code at.What changed
validateCimdDocumentnow requires eachredirect_urito be one of:httpsURL,httpURL (RFC 8252:127.0.0.1/localhost/[::1], any port — reusingisLoopbackHost), orvscode://,com.example.app:/…).Everything else is rejected: cleartext
httpto a non-loopback host, and any string that is not a parseable absolute URL (fail-closed — every legitimate redirect form parses). No shipped first-party client (vscode.dev, chatgpt.com, claude.ai, zed.dev) is affected.deployment.mddocuments that the CIMD host set grants redirect authority independent of the redirect allowlist.Testing
http://evil.example, uppercase scheme, userinfo, the deceptivehttp://127.0.0.1.evil.example(starts with a loopback literal but is not loopback), protocol-relative//evil.example, and unparseable/empty. Accept cases cover https (incl. nonstandard port), all three loopback hosts, and custom schemes (incl. the RFC 8252 private-use form)./authorizeroute: a bad-redirect CIMD doc yields a 400 auth-error page with no code minted.URL; full gate (fmt:check,lint,tsc, tests) green.Review
Reviewed by a code-review agent (verdict: merge). Folded in its recommendations: pinned the deceptive-subdomain regression test, and moved from "leave unparseable alone" (which produced a 500 for a trusted host's garbage redirect) to rejecting unparseable at validation (clean 400, fail-closed) — safe because every legitimate redirect form parses.
🤖 Generated with Claude Code