Conversation
cb1kenobi
approved these changes
Oct 30, 2025
heskew
added a commit
that referenced
this pull request
Jul 10, 2026
…flow consent cookie, DNS bounding, config hardening Second external-review batch on top of the consent-binding/SSRF work. Consent cookie (Codex #1 sibling-injection High, #7 concurrent-flows Medium): - Switch the consent cookie to a per-flow __Host--prefixed name (__Host-mcp_consent_<flowId>). __Host- forbids a Domain attribute, so a sibling origin can no longer plant a parent-domain cookie to forge the binding (SameSite=Lax doesn't stop siblings — they're same-site). The per-flow id (carried in confirm + upstream state) lets parallel tabs run concurrent flows without clobbering each other's binding. Callback ordering (Codex #5 Medium): - Verify the browser binding BEFORE exchangeCodeForToken and the onLogin hook, so a mismatched (self-approved) flow triggers no upstream exchange and no provisioning side-effects. Regression asserts neither runs on mismatch. SSRF (Codex #3 rebind High, #2 threadpool High, #4 special-use Medium): - Pinned-connect: fetch via https.request with a custom lookup that connects to the exact address the gate validated, while keeping the hostname for TLS SNI + cert verification — closes the DNS-rebinding TOCTOU. (No undici dep: undici isn't importable here; https.request also gives no-redirect-follow for free.) - Bound concurrent DNS resolutions with a permit released only when the raw (uncancellable) getaddrinfo settles, so a flood of blackholed-DNS client_ids can't pin the libuv pool; fast-reject when saturated; dedup concurrent resolutions of the same client_id into one fetch. - Classify against the full IANA IPv4/IPv6 special-purpose registries (192.0.2/24, 198.51.100/24, 203.0.113/24, 192.0.0/24, 192.88.99/24, AS112/AMT, and in-2000::/3 Teredo/ORCHID/documentation), table-driven. Config (Codex #6 Medium): - Normalize the mcp block at load: coerce documented boolean strings (env-expanded "false" no longer leaves a security switch truthy) and require allowedHosts to be an array of exact lowercased hostnames (a scalar is wrapped, not substring-matched; non-strings rejected). Deferred (agreed): the pinnedHttpsFetch path is production-only (tests stub the fetch seam and exercise SSRF via the DNS seam); its lookup is a thin pass-through of the pre-validated addresses. Tests 938/936 pass (2 pre-existing skips); docs updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
heskew
added a commit
that referenced
this pull request
Jul 10, 2026
…sent interstitial (#166) (#167) * feat: Client ID Metadata Documents (CIMD) with SSRF-guarded resolution and consent interstitial (#166) Implements the MCP authorization spec's CIMD support: when a client_id is an HTTPS URL with a non-root path, the AS fetches it as a JSON metadata document instead of doing a DCR lookup. Core changes: - New `src/lib/mcp/cimd.ts`: `isCimdClientId`, `resolveCimdClient`, `resolveClient`. SSRF guards via DNS pre-flight (all A/AAAA records checked against private/loopback ranges), IP-literal rejection, no-redirect fetch, 5 s timeout, 64 KB cap. Per-process cache with `Cache-Control: max-age`-based TTL clamped to [60 s, 86400 s]; negative-cache on client errors (60 s), not on server errors. - New `src/lib/mcp/clientValidator.ts`: shared validators extracted from dcr.ts (`validateRedirectUri`, `validateStringArray`, `validateGrantTypes`, etc.). - `src/lib/mcp/authorize.ts`: `resolveClient` replaces direct `MCPClientStore` lookup. `escapeHtml` and `buildInterstitialPage` for the CIMD consent page. `handleAuthorize` returns 200 HTML for CIMD clients; 302 for stored/DCR clients. New `handleAuthorizeConfirm` for `POST /oauth/mcp/confirm`: verify + consume one-time token, validate `_confirm` marker, redirect to upstream IdP. - `src/lib/mcp/wellKnown.ts`: advertises `client_id_metadata_document_supported: true` when CIMD is enabled (default on). - `src/lib/mcp/token.ts`: `authenticateClient` uses `resolveClient`; handles `CimdClientError` as `invalid_client`. - `src/lib/mcp/index.ts` + `src/lib/resource.ts`: route `POST /oauth/mcp/confirm` to `handleAuthorizeConfirm`; thread `providers` registry into `handleMCPPost`. - `src/types.ts`: `MCPClientIdMetadataDocumentsConfig`, `MCPConfig.clientIdMetadataDocuments`, `MCPClientRecord._cimd`, `MCPClientMetadata.jwks/jwks_uri`. Tests (50 new passing): - `test/lib/mcp/cimd.test.js`: isCimdClientId shape checks, SSRF DNS gate, allowedHosts policy, document validation, cache TTL, resolveClient routing. - `test/lib/mcp/authorize.test.js`: escapeHtml (XSS vectors), buildInterstitialPage (loopback warning, token binding, XSS), handleAuthorize CIMD path (200 HTML), handleAuthorizeConfirm (valid token → 302, single-use, expired, missing _confirm). - `test/lib/mcp/wellKnown.test.js`: CIMD flag present when enabled, absent when disabled. Docs: CIMD section in mcp-oauth.md, four config rows in configuration.md. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(cimd): separate document-host policy from redirect-host policy clientIdMetadataDocuments.allowedHosts governs which hosts may SERVE metadata documents; redirect URIs are a different policy and now validate against dynamicClientRegistration.allowedRedirectUriHosts (same rules as DCR clients). A trusted vendor whose document declares redirect targets on another host is no longer wrongly rejected. Regression tests added. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(cimd): address external security review — consent browser binding, token purpose, SSRF gate, cache and fetch hardening Consent flow (the two flow-level blockers): - Bind the CIMD consent to the approving browser: the interstitial sets an HttpOnly/Secure/SameSite=Lax nonce cookie whose SHA-256 travels inside the confirm token and upstream state; POST /oauth/mcp/confirm and the OAuth callback both require a hash match before proceeding (new src/lib/mcp/consentBinding.ts). A malicious client can no longer self-approve the interstitial and hand the victim the upstream IdP URL. - Enforce token purpose at the callback: a confirm token presented as upstream OAuth state is rejected like an invalid token (it previously passed the mcp/providerName checks and skipped consent entirely). SSRF DNS gate: - IPv4: also reject 0/8, 100.64/10 (CGNAT), 198.18/15, 224/4+; malformed input fails closed. - IPv6: real parser (:: compression, embedded dotted-quad, zone index); allow only global unicast 2000::/3, with v4-mapped addresses classified by their embedded IPv4 address. Closes the ::, non-canonical loopback, and hex-form v4-mapped bypasses. - All gate rejections (didn't resolve / blocked address) return one generic invalid_client message; details are logged server-side only, so callers can't probe the server's internal DNS view. - Reject dot path segments (raw or percent-encoded) and non-lowercase scheme spellings in CIMD client_ids before URL normalization erases them. Fetch path: - Only 200 OK is accepted (404/500 JSON no longer resolves as a client). - One deadline across DNS, connect, headers, and body read — a trickling body can no longer hold connections open past the timeout. - fetchTimeoutMs/maxDocumentBytes are coerced to finite positive numbers; NaN/Infinity/garbage fall back to defaults instead of failing open. Cache: - LRU-bounded to 1000 entries (keys are attacker-chosen input). - Failures are never cached (CIMD draft forbids caching errors/invalid documents); negative caching removed. - Cache-Control no-store/no-cache floor at the 60 s DoS floor. - Cached records revalidate against the live allowedRedirectUriHosts on every hit, so tightening the policy takes effect immediately. Interstitial: - Served with X-Frame-Options: DENY, CSP frame-ancestors 'none', and Cache-Control: no-store (page carries the single-use confirm token). - Displays the authoritative client_id hostname; client_uri is labelled unverified; unparseable redirect_uri degrades instead of throwing. Plus error-cause chaining and server-side logging on swallowed catch paths (confirm verify, token client lookup). 52 new/updated tests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cimd): decode IPv4-in-IPv6 transition forms and fail closed on unknown DNS family Follow-up from the cross-model review pass on 195cf21. - isPrivateIpv6 now decodes the IPv4 embedded in 6to4 (2002::/16) and ISATAP and classifies it via isPrivateIpv4, and rejects Teredo (2001:0000::/32) outright. These transition forms sit inside the 2000::/3 global-unicast allow but can target a private IPv4 — previously a 6to4/ISATAP address wrapping 10/8 or 127/8 was allowed. - checkHostSsrf fails closed on any DNS address family other than 4/6 rather than skipping both range checks (defense-in-depth; dns.lookup only returns 4/6 today). - Tests for 6to4/ISATAP/Teredo private targets, a public 6to4 pass-through, and the unknown-family reject. In-flight fetch dedup (thundering herd on an uncached attacker URL) was raised as a suggestion and is deferred to rate limiting (#163), bounded meanwhile by the 64 KB / 5 s caps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cimd): address second Codex review — pinned-connect, __Host- per-flow consent cookie, DNS bounding, config hardening Second external-review batch on top of the consent-binding/SSRF work. Consent cookie (Codex #1 sibling-injection High, #7 concurrent-flows Medium): - Switch the consent cookie to a per-flow __Host--prefixed name (__Host-mcp_consent_<flowId>). __Host- forbids a Domain attribute, so a sibling origin can no longer plant a parent-domain cookie to forge the binding (SameSite=Lax doesn't stop siblings — they're same-site). The per-flow id (carried in confirm + upstream state) lets parallel tabs run concurrent flows without clobbering each other's binding. Callback ordering (Codex #5 Medium): - Verify the browser binding BEFORE exchangeCodeForToken and the onLogin hook, so a mismatched (self-approved) flow triggers no upstream exchange and no provisioning side-effects. Regression asserts neither runs on mismatch. SSRF (Codex #3 rebind High, #2 threadpool High, #4 special-use Medium): - Pinned-connect: fetch via https.request with a custom lookup that connects to the exact address the gate validated, while keeping the hostname for TLS SNI + cert verification — closes the DNS-rebinding TOCTOU. (No undici dep: undici isn't importable here; https.request also gives no-redirect-follow for free.) - Bound concurrent DNS resolutions with a permit released only when the raw (uncancellable) getaddrinfo settles, so a flood of blackholed-DNS client_ids can't pin the libuv pool; fast-reject when saturated; dedup concurrent resolutions of the same client_id into one fetch. - Classify against the full IANA IPv4/IPv6 special-purpose registries (192.0.2/24, 198.51.100/24, 203.0.113/24, 192.0.0/24, 192.88.99/24, AS112/AMT, and in-2000::/3 Teredo/ORCHID/documentation), table-driven. Config (Codex #6 Medium): - Normalize the mcp block at load: coerce documented boolean strings (env-expanded "false" no longer leaves a security switch truthy) and require allowedHosts to be an array of exact lowercased hostnames (a scalar is wrapped, not substring-matched; non-strings rejected). Deferred (agreed): the pinnedHttpsFetch path is production-only (tests stub the fetch seam and exercise SSRF via the DNS seam); its lookup is a thin pass-through of the pre-validated addresses. Tests 938/936 pass (2 pre-existing skips); docs updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(cimd): drop redundant Buffer.from in Buffer.concat Buffer.concat accepts Uint8Array[] directly (gemini review nit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(cimd): reject query strings in CIMD client_ids The draft (§3) says client_id URLs SHOULD NOT include a query string; enforce it as part of the strict URL-shape profile — a dynamic server could otherwise mint unlimited exact-match client_id aliases of one document by echoing query variants. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 * fix(cimd): tear down rejected connections, bound total resolution concurrency Third external review batch: - Abort the pinned connection when a response is rejected after headers (non-200, non-JSON, oversize) — the deadline timer is cleared on exit, so a hostile endpoint could otherwise hold rejected sockets open indefinitely. - Bound TOTAL concurrent CIMD resolutions (DNS + connect + body) at 16, fast-rejecting past the cap; the DNS permit alone released too early to stop unique client_ids fanning out into unbounded HTTPS work. - Drop MAX_CONCURRENT_DNS from 8 to 2 — below the default 4-thread libuv pool, so blackholed lookups can never starve fs/crypto users. - Remove the jwks/jwks_uri carry-through: no consumer or validation until #159, which will add the plumbing alongside both. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 * chore(cimd): defense-in-depth minors from external agent review - Add 224/4 multicast to the IPv4 special-use table so the table alone is complete (the a >= 224 early return remains as a fast path). - Document that CIMD interactive authorization requires HTTPS — the __Host-/Secure consent cookie is silently dropped on plain-HTTP origins. - Note readConsentNonce's base64url dependency and first-match semantics for future readers. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 * docs(cimd): spell out 224/4 multicast + 240/4 reserved in the SSRF header Replace the cryptic 224/4+ shorthand in the module-header range list (gemini nit). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SerGP6Am3xz2CKyPgbRc73 --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
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
2 participants
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.
No description provided.