feat: Client ID Metadata Documents with SSRF-guarded resolution + consent interstitial (#166)

[heskew]

Summary

Implements Client ID Metadata Documents (#166) — the modern MCP client-registration path (the draft spec marks DCR deprecated): an HTTPS URL is accepted as a client_id, resolved by fetching the JSON metadata document it points to, validated, cached, and taken through a consent interstitial before the upstream redirect.

• Resolution (src/lib/mcp/cimd.ts): URL-shape detection (https + non-root path, no userinfo/fragment/IP-literals/dot-segments) → SSRF-guarded fetch → validation → per-process cache. resolveClient() routes URL-shaped ids to CIMD and everything else to the existing DCR store; DCR is unchanged as the fallback.
• SSRF guards: DNS gate allowing only globally routable addresses, classified table-driven against the full IANA IPv4/IPv6 special-purpose registries; IPv6 allows only global unicast (2000::/3) with v4-mapped and 6to4/ISATAP forms classified by embedded IPv4 (real parser: :: compression, dotted-quad tails, zone indexes; fail-closed on anything unparseable) and in-2000::/3 special-use (Teredo/ORCHID/documentation) rejected. Pinned-connect: the fetch (https.request + custom lookup) connects to the exact address the gate validated while keeping the hostname for TLS SNI/cert — closing the rebind TOCTOU. No redirect-follow, only 200 OK accepted, one deadline (default 5 s) spanning DNS + connect + body, 64 KB cap, JSON-only, config values coerced fail-closed. Concurrent DNS resolutions are globally bounded (uncancellable getaddrinfo) with per-client_id dedup. Every gate rejection returns one generic invalid_client message (no internal-DNS probing); detail is server-side log only.
• Validation: document client_id must equal the fetched URL exactly; required fields (client_id, client_name, redirect_uris); redirect URIs validated with the same structural rules and the same redirect-host policy as DCR (dynamicClientRegistration.allowedRedirectUriHosts) — clientIdMetadataDocuments.allowedHosts governs the document host trust policy only.
• Caching: LRU-bounded (1000 entries, keys are attacker-chosen); TTL from Cache-Control: max-age clamped to [60 s, 24 h] (no-store/no-cache floor at 60 s as deliberate DoS protection); failures never cached (draft requirement); cached records revalidated against the live redirect-host policy on every hit.
• Consent interstitial (CIMD clients only): instead of an immediate 302 to the upstream IdP, a self-contained HTML page shows the authoritative client_id host, the client name (unverified), and the redirect URI hostname (prominent loopback-impersonation warning when the redirect is localhost-only — spec MUST). Continue is a POST with a single-use, short-TTL token binding the full validated authorize params; the confirm handler resumes the existing upstream-redirect logic. Served with X-Frame-Options: DENY / frame-ancestors 'none' / Cache-Control: no-store.
• Consent browser binding (src/lib/mcp/consentBinding.ts): the interstitial sets a per-flow __Host--prefixed, Secure/HttpOnly/SameSite=Lax nonce cookie; sha256(nonce) travels inside the confirm token and the upstream OAuth state, and both /oauth/mcp/confirm and the upstream callback require a constant-time match before an MCP authorization code is issued. The callback checks it before the upstream code exchange and onLogin, so a mismatched flow triggers no side effects. __Host- blocks sibling-origin cookie injection; the per-flow name supports concurrent tabs. The callback also hard-rejects confirm tokens presented as upstream state (token purpose enforcement). DCR flows carry no binding and are untouched.
• Config: mcp.clientIdMetadataDocuments (enabled — default ON when mcp.enabled; allowedHosts; fetch timeout/size). AS metadata advertises client_id_metadata_document_supported: true when enabled.
• Add RFC 7523 client_credentials grant (private_key_jwt, EdDSA) for headless agent auth #159 handoff: jwks / jwks_uri / token_endpoint_auth_method are carried through on resolved records; v1 accepts token_endpoint_auth_method: none only — private_key_jwt activates with Add RFC 7523 client_credentials grant (private_key_jwt, EdDSA) for headless agent auth #159's assertion verification.

Where to focus review

• Consent binding (consentBinding.ts + its use in authorize.ts / handlers.ts) — newest surface; verify the cookie→hash→state chain has no unbound leg. Deliberate consequence: CIMD authorization now requires cookies in the user's browser.
• SSRF surface (cimd.ts guards) — the AS fetches an attacker-supplied URL; the DNS gate, deadline, size caps, and no-redirect posture are the containment. The IPv6 policy is deny-by-default (2000::/3 only).
• Interstitial escaping (authorize.ts escapeHtml) — client_name is attacker-controlled and rendered; every interpolation is escaped and tested.
• Deliberate deviations to sanity-check: DNS resolution failure now returns 400 invalid_client (generic) instead of 500, so status codes don't reopen the internal-DNS probe channel; no-store is floored at 60 s rather than honored literally (DoS floor).
• Coordination with Add RFC 7523 client_credentials grant (private_key_jwt, EdDSA) for headless agent auth #159 (in flight separately): grant logic and tokenIssuer.ts deliberately untouched; only the token endpoint's client-record lookup is routed through resolveClient.

Review history

Two rounds of external review, all findings addressed:

• Round 1 (f0da8a1): Claude + Codex + a security pass + two gemini bot rounds — 12 findings incl. two flow-level blockers (consent not browser-bound; confirm token accepted as upstream state). Fixed in 195cf21; per-finding mapping.
• Round 2 (f0da8a1 Codex pass): 3 High + 4 Medium — sibling-origin cookie injection, dns.lookup threadpool DoS, preflight-not-connection-bound, RFC 6890 special-use ranges, binding-after-login-side-effects, config fail-open, concurrent-flow cookie collision. Fixed in 7f07d87/6fb7dc8; per-finding mapping.

Tests

938 total / 936 pass / 2 pre-existing skips: URL-shape + SSRF rejections (incl. ::, non-canonical loopback, v4-mapped hex, 6to4/ISATAP/Teredo embedded-v4, IANA special-use, dot-segments), 200-only, full-fetch deadline, config coercion, DNS concurrency-cap + dedup + pinned-address wiring, exact-match and required-field validation, cache TTL clamps + LRU bound + no-failure-caching + live policy revalidation, XSS escaping, interstitial flow (HTML → POST confirm → 302; single-use/expired/tampered tokens; anti-framing headers; per-flow __Host- cookie), consent-binding attack regressions on both legs (incl. no exchange/onLogin on mismatch), token-purpose rejection at the callback, config normalization, DCR-client flow unchanged, metadata flag gating.

Docs updated: docs/mcp-oauth.md + docs/configuration.md.

Closes #166

🤖 Generated with Claude Code (Opus 4.8; review fixes by Fable 5)

[github-actions]

Reviewed; no blockers found.

[claude]

Reviewed; no blockers found.

[heskew]

/gemini review

[gemini-code-assist]

Code Review

This pull request implements Client ID Metadata Document (CIMD) resolution for the Model Context Protocol (MCP) authorization flow, allowing clients to identify themselves using HTTPS URLs. It introduces a DNS-gated fetch mechanism with SSRF guards, process-level caching, and an interstitial confirmation page requiring explicit user confirmation before redirecting to the upstream IdP. The review feedback focuses on strengthening the SSRF filters for both IPv4 and IPv6 addresses, respecting 'no-store'/'no-cache' directives in Cache-Control headers, defensively handling potential runtime exceptions from malformed redirect URIs, and improving error observability by logging caught exceptions and preserving original error stacks using the 'cause' option.

[heskew]

Reviewed the security-critical surfaces (cimd.ts SSRF gate, the interstitial in authorize.ts, resolveClient wiring in token.ts, shared clientValidator). Two blockers, both in the areas this PR flagged for review. Neither is a design problem — both are contained fixes.

Blockers

1. SSRF DNS gate misses 0.0.0.0 / 0.0.0.0/8 and IPv6 :: — src/lib/mcp/cimd.ts (isPrivateIpv4 / isPrivateIpv6, ~L111–137)

isPrivateIpv4('0.0.0.0') returns false (a=0 matches none of the ranges), and 0.0.0.0 routes to loopback on Linux. Because the client controls the DNS record for their client_id URL host, pointing it at 0.0.0.0 passes checkHostSsrf and the subsequent fetch connects to the AS's own localhost services — a reachable SSRF-to-loopback bypass of the primary containment. IPv6 :: (unspecified) slips through the same way (firstGroup is empty → returns false).

Fix: reject a === 0 (0.0.0.0/8) in isPrivateIpv4, add 100.64.0.0/10 (CGNAT) while there, and reject the all-zeros :: in isPrivateIpv6. Add 0.0.0.0 and :: cases to cimd.test.js.

2. Consent interstitial is frameable (clickjacking) — src/lib/mcp/authorize.ts (HtmlResponse, ~L450–454)

The interstitial exists specifically to satisfy the spec's "clearly display the redirect URI hostname" consent requirement, but it's served with only Content-Type. A malicious CIMD client can frame it and clickjack the "Continue to sign in" POST for a victim already authenticated upstream, defeating the consent gate.

Fix: add X-Frame-Options: DENY and Content-Security-Policy: frame-ancestors 'none' (and Cache-Control: no-store, since the body carries the single-use confirm token) to the response headers.

Held up under scrutiny

DNS-rebinding TOCTOU is disclosed and accepted; the lying-content-length case is covered by the streaming byte cap; redirect: 'error' closes redirect-SSRF; escapeHtml covers every client-controlled interpolation; the shared clientValidator holds CIMD and DCR to identical redirect/grant rules; and the #159 handoff (jwks carried through, private_key_jwt gated off in v1) is wired correctly.

Flagging with extra weight because #161/#162 (the client_credentials grant for headless agents) now resolve clients through resolveClient() and will lean on this exact SSRF gate to fetch agent JWKS documents — the 0.0.0.0 gap should close before that builds on top.

🤖 Review by Claude (Fable 5)

[heskew]

Fresh pass on f0da8a1. I treated the existing SSRF range, bounded-cache, no-store/no-cache, error-observability, and clickjacking comments as already raised. I found three additional issues:

1. src/lib/mcp/cimd.ts:371 accepts non-2xx metadata responses. The resolver never checks response.ok/status, so a 404 or 500 with Content-Type: application/json and a syntactically valid CIMD body is accepted and cached as a client. The current CIMD draft requires the document to be served with 200 OK and treats other status codes as discovery errors. Please reject anything other than 200 before content-type/body parsing, and add a regression test for 404/500 JSON.

2. src/lib/mcp/cimd.ts:429 negative-caches invalid/error metadata documents. That is separate from the already-raised “Map is unbounded” issue: the current CIMD draft says the AS MUST NOT cache error responses or invalid/malformed documents. Today a bad document, unsupported auth method, oversized body, or non-JSON response is cached for 60s, and the test suite codifies that behavior. Please remove negative caching for CIMD discovery failures, or at least do not cache the classes the draft forbids.

3. src/lib/mcp/cimd.ts:354 uses fetchTimeoutMs and maxDocumentBytes directly from config. maxDocumentBytes: NaN or Infinity disables both size checks (contentLength > maxBytes and total > maxBytes are false), so the 64 KB cap fails open. This config path also supports env-expanded values, and nearby MCP config already coerces TTL strings defensively. Please coerce/validate these options to finite positive numbers before use, falling back to defaults or failing closed, and cover NaN/Infinity/numeric strings in tests.

Verified locally: npm run build; node --import ./test/helpers/harper-mock.mjs --test test/lib/mcp/cimd.test.js test/lib/mcp/authorize.test.js test/lib/mcp/wellKnown.test.js test/lib/mcp/token.test.js.

---

🤖 Posted by Codex (gpt-5.5) on Nathan's behalf

[heskew]

Fresh security pass on f0da8a1. I treated the already-raised SSRF-range, cache-bound, clickjacking/cache-control, non-200, negative-cache, and numeric-config findings as open and did not repeat them. I found the following additional issues.

1. Blocker: the CIMD consent is not bound to the resource owner's browser - src/lib/mcp/authorize.ts:432-525

   /authorize returns a bearer confirm_token, and /confirm validates only that token. A malicious CIMD client can retrieve and submit the interstitial itself, then send the resulting upstream IdP URL to a victim. The victim never sees the client identity or redirect-host disclosure, but an existing IdP session can still produce an authorization code at the attacker's redirect URI, with the attacker's PKCE verifier. This defeats the purpose of the interstitial.

   Bind the confirmation and subsequent upstream callback to a Secure, HttpOnly, SameSite browser nonce, or authenticate upstream first and render consent afterward. The callback must verify that same binding.

2. Blocker: a confirm token is accepted as upstream OAuth callback state - src/lib/mcp/authorize.ts:434, src/lib/handlers.ts:176-265

   Confirm tokens use the shared CSRF store and carry both mcp and _confirm. The callback treats any verified state carrying mcp as upstream state and never rejects _confirm. Supplying a confirm token as the upstream state therefore exchanges the upstream code, persists an MCP authorization code, and redirects it to the malicious client without calling /confirm.

   Add an enforced token purpose, for example cimd_confirm versus upstream_oauth, and reject each purpose on the wrong endpoint. Separate stores would be stronger. Add a callback regression test using a _confirm token.

3. High: fetchTimeoutMs ends at response headers, not document completion - src/lib/mcp/cimd.ts:354-406

   The abort timer is cleared as soon as fetch() returns a response. DNS resolution and the streamed body are outside that deadline, so a hostile CIMD server can send JSON headers and never finish the body, tying up unauthenticated requests and outbound connections indefinitely.

   Apply one deadline across DNS, connection, headers, and body parsing; keep the abort active through body consumption and cancel the reader on expiry.

4. Medium: cached clients survive a live redirect-host-policy tightening - src/lib/mcp/cimd.ts:338-344

   Positive cache entries are keyed only by client_id, even though validation depends on the live dynamicClientRegistration.allowedRedirectUriHosts setting. A redirect accepted before an operator restricts that setting remains accepted for the cached document lifetime, up to 24 hours.

   Revalidate cached redirects on every hit, include a policy fingerprint in the cache key, or clear CIMD cache entries when relevant MCP configuration changes.

5. Medium: the interstitial omits the authoritative client-ID hostname - src/lib/mcp/authorize.ts:218-229

   The page displays the redirect hostname and an optional attacker-controlled client_uri, but never displays the hostname of client_id, which is the domain that served the metadata document. A client can claim a trusted client_uri while using its own client-ID domain. The CIMD phishing guidance recommends displaying the client-ID hostname.

   Prominently render the escaped hostname from client.client_id; either omit client_uri or label it as unverified metadata.

6. Medium: discovery errors expose Harper's internal DNS view - src/lib/mcp/cimd.ts:160-164, src/lib/mcp/authorize.ts:352-356, src/lib/mcp/token.ts:129-131

   A private DNS result is embedded in CimdClientError and returned to unauthenticated callers. Requests for guessed internal names can reveal whether Harper resolves them and disclose the exact private address.

   Return a generic invalid_client description and log the hostname/address only server-side.

7. Low: dot-segment client IDs are accepted after URL normalization - src/lib/mcp/cimd.ts:182-195

   WHATWG URL parsing normalizes both literal and percent-encoded dot segments before this function validates the path. Inputs such as https://example.com/a/../client.json are accepted even though the CIMD draft prohibits dot path components, weakening simple-string client identity.

   Reject raw and percent-encoded single/double-dot path components before URL normalization, with regression coverage.

Verified locally: 202 targeted authorize, CIMD, callback, token, and discovery tests pass on this head.

[heskew]

All findings from the three review passes on f0da8a1 are addressed in 195cf21. Mapping:

Claude review (2 blockers)

1. SSRF gate 0.0.0.0/:: bypass → fixed. IPv4 gate now rejects 0/8, 100.64/10, 198.18/15, 224/4+ and fails closed on malformed input; IPv6 is a real parser with a fail-closed "global unicast (2000::/3) only" policy (v4-mapped classified by embedded IPv4), which also kills non-canonical loopback (::01, expanded form) and hex-form v4-mapped bypasses.
2. Frameable interstitial → fixed: X-Frame-Options: DENY, Content-Security-Policy: frame-ancestors 'none', Cache-Control: no-store.

Codex review (3 findings)

1. Non-200 accepted → fixed: only 200 OK resolves; 404/500-with-JSON regression tests added.
2. Negative caching forbidden by the draft → removed entirely; failures are never cached (repeat-fetch amplification is client_credentials (4/4): rate-limit token issuance (defense-in-depth) #163 rate limiting's job; the tests that codified negative caching now assert the opposite).
3. NaN/Infinity config fail-open → fixed: fetchTimeoutMs/maxDocumentBytes coerce to finite positive numbers (numeric strings honored) and fall back to defaults; covered for NaN/Infinity/strings/negatives.

Security pass (7 findings)

1. Consent not browser-bound (blocker) → fixed with a nonce cookie (HttpOnly, Secure, SameSite=Lax, 15 min): the interstitial response sets it, sha256(nonce) rides inside the confirm token and upstream state, and BOTH /oauth/mcp/confirm and the OAuth callback require a constant-time hash match before proceeding. New module src/lib/mcp/consentBinding.ts; attack-path regression tests for the self-approved interstitial in both legs. DCR flows carry no hash and are untouched.
2. Confirm token accepted as upstream state (blocker) → fixed: the callback rejects any verified state carrying _confirm exactly like an invalid token (no field of a mis-purposed token is trusted, including its redirect_uri). Regression test asserts no code minted and no upstream exchange.
3. Timeout ends at headers (high) → fixed: one deadline spans the DNS pre-flight (raced against the same abort signal), connect, headers, and full body read; trickling-body regression test.
4. Cache outlives policy tightening (medium) → fixed: cached records revalidate against the live allowedRedirectUriHosts on every hit and are evicted on failure.
5. Interstitial omits client_id host (medium) → fixed: the page now leads with "Client identity: <client_id host>" and labels client_uri as "Claimed application domain (unverified)".
6. DNS error detail leak (medium) → fixed: every gate rejection (didn't resolve / no addresses / blocked address) returns one generic invalid_client message; hostname/address detail is logged server-side only.
7. Dot-segment client_ids (low) → fixed: raw and percent-encoded dot path segments are rejected before URL normalization; non-lowercase scheme spellings rejected too (the document's exact-match client_id could never validate them anyway).

Also folded in the gemini bot rounds (all inline threads replied + resolved): bounded LRU cache (1000 entries), no-store/no-cache floored at the 60 s DoS floor (documented deviation), error-cause chaining, defensive redirect_uri rendering, and logging on the swallowed catch paths.

Docs (docs/mcp-oauth.md, docs/configuration.md) updated to match. Suite: 923 tests, 921 pass, 2 pre-existing skips (52 new/updated).

One deliberate deviation to sanity-check: DNS resolution failure is now reported as 400 invalid_client (generic) rather than 500 — making it distinguishable from the blocked-address case would have kept the internal-DNS probing channel open via status codes.

🤖 Response by Claude (Fable 5) on Nathan's behalf

[heskew]

Security review — request changes

Reviewed current head 6fb7dc84 with two independent passes and targeted validation. I found three high-severity issues and four medium-severity issues. The full suite passes locally (926 tests, 924 pass, 2 existing skips), but these need resolution before merge.

1. Sibling-domain cookie injection bypasses CIMD consent — High

File: src/lib/mcp/consentBinding.ts:28

mcp_consent is a normal cookie name. If the AS is auth.example.com, an attacker controlling evil.example.com can fetch its own interstitial server-side to obtain a nonce and confirm_token, set mcp_consent=<nonce>; Domain=example.com in the victim's browser, and auto-submit the confirm form. Since sibling origins are same-site, SameSite=Lax does not block this. Both /confirm and the callback accept the planted cookie, so the victim can complete upstream login and the attacker receives a code at its redirect URI with the attacker's PKCE verifier, without the victim seeing the interstitial.

Rename this to a per-flow __Host-... cookie (the existing Secure; Path=/ and no Domain are compatible), and bind the flow ID in state. Also require the configured issuer as the Origin on /oauth/mcp/confirm as defense in depth. __Host- prevents a sibling from planting a parent-domain cookie; see RFC6265bis §4.1.3.2.

2. DNS timeout does not terminate dns.lookup() work — High

File: src/lib/mcp/cimd.ts:351

withAbort() only rejects the caller; it cannot cancel the underlying dns.lookup(). An attacker can send many client IDs under DNS zones that drop queries. Each request returns after five seconds, but the synchronous getaddrinfo() calls keep occupying libuv's fixed thread pool, starving unrelated DNS/filesystem work. Node documents this exact dns.lookup() behavior in its DNS implementation notes.

Use cancellable DNS resolution and a global in-flight CIMD resolution bound. Do not release a concurrency permit until the underlying lookup has actually stopped.

3. The SSRF preflight is not connection-bound — High

File: src/lib/mcp/cimd.ts:542

checkHostSsrf() validates one DNS answer, then fetch(clientId) resolves the hostname again. An attacker-controlled authority can return a public address to the preflight and an internal/special-use address to the actual connection. TLS narrows the exploitable targets but does not eliminate internal TLS probing or services with compatible SNI/certificates. The PR documents this residual risk, but the current CIMD draft requires authorization servers not to fetch URLs that resolve to special-use addresses.

Pin the HTTP connection to the validated address using a custom lookup/agent while preserving the original hostname for Host, SNI, and certificate verification. Revalidate every retry address. CIMD draft §8.6

4. The IP filter permits RFC 6890 special-use space — Medium

File: src/lib/mcp/cimd.ts:127

The filter allows 192.0.0.0/24, TEST-NET ranges, 2001:2::/48, 2001:db8::/32, and other special-use prefixes. It also intentionally allows public-target 2002::/16 addresses, although 6to4 is special-use. These answers reach _fetch(); I reproduced this with 192.0.2.1.

Classify against the complete IANA IPv4 and IPv6 special-purpose registries and add table-driven tests for every denied prefix. IANA IPv4 registry, IANA IPv6 registry

5. Browser binding runs after upstream login side effects — Medium

File: src/lib/handlers.ts:241

On the self-approved-interstitial path, the cookie mismatch is checked only at line 283. Before that, the callback exchanges the upstream code, loads identity data, and invokes onLogin, which can provision users, synchronize roles, or cause external mutations. The fix prevents MCP code issuance but not attacker-initiated hook effects.

Validate browserNonceHash immediately after state-purpose and provider validation, before upstream errors, code exchange, userinfo, or hooks. Add a regression that a mismatch invokes neither exchangeCodeForToken nor onLogin.

6. CIMD security configuration can fail open — Medium

Files: src/lib/mcp/cimd.ts:485, src/lib/mcp/cimd.ts:649

Environment expansion preserves strings, so enabled: ${CIMD_ENABLED} with CIMD_ENABLED=false becomes "false"; enabled !== false still enables CIMD. A scalar allowedHosts becomes a string and uses substring matching, and an empty list skips the restriction completely.

Normalize/validate configuration at load time: explicitly coerce documented boolean strings, require an array of normalized exact hostnames, and reject invalid or ambiguous security-policy values instead of treating them as unrestricted.

7. One global consent cookie breaks concurrent CIMD flows — Medium

File: src/lib/mcp/consentBinding.ts:28

Every interstitial overwrites mcp_consent. Opening a second authorization flow causes the first to fail either at /confirm or after upstream authentication. This occurs with ordinary parallel tabs and permits deliberate disruption of a pending authorization.

Use bounded per-flow __Host- cookies keyed by a random flow identifier carried in confirmation and upstream state, and clear only the completed flow.

[heskew]

Thanks — strong pass. All seven addressed on 7f07d87 (with 6fb7dc8). Per-finding:

1. Sibling-domain cookie injection (High) — fixed. The consent cookie is now a per-flow __Host--prefixed name (__Host-mcp_consent_<flowId>). __Host- forbids a Domain attribute, so a sibling origin can't plant a parent-domain cookie to forge the binding. I did not add the issuer-Origin check on /confirm — the __Host- prefix closes the attack on its own; happy to add the Origin check too if you'd rather have the belt-and-suspenders.

2. dns.lookup() not cancellable (High) — fixed. Concurrent CIMD DNS resolutions are bounded by a global permit (MAX_CONCURRENT_DNS) that is released only when the underlying getaddrinfo settles, not when the caller's deadline fires — so a flood of blackholed-DNS client_ids can't pin the libuv pool. Over the bound, resolution fast-rejects (temporarily_unavailable); concurrent resolutions of the same client_id are also deduped to one fetch. Test asserts peak concurrency ≤ cap + saturation fast-reject.

3. Preflight not connection-bound (High) — fixed with pinned-connect. The fetch now goes through https.request with a custom lookup that returns the exact address the gate validated; the hostname is kept for TLS SNI + cert verification. The socket therefore connects to a validated IP with no second resolution, closing the rebind TOCTOU. (No new dep: undici's Agent isn't importable here, and https.request also gives no-redirect-follow for free.) The gate resolves once and hands the addresses to the fetch, so there's no double lookup.

4. RFC 6890 special-use space permitted (Medium) — fixed. isPrivateIpv4/isPrivateIpv6 are now table-driven against the IANA IPv4/IPv6 special-purpose registries: added 192.0.0/24, 192.0.2/24 (TEST-NET-1), 192.88.99/24, 198.51.100/24 (TEST-NET-2), 203.0.113/24 (TEST-NET-3), AS112/AMT, and the in-2000::/3 IPv6 special-use (2001:db8::/32, 2001:2::/48, ORCHID, 3fff::/20), with per-prefix tests. 192.0.2.1 is now rejected.

5. Binding runs after login side effects (Medium) — fixed. The browserNonceHash check moved to before exchangeCodeForToken and onLogin (right after state-purpose + provider validation). A mismatched self-approved flow now triggers no upstream exchange and no hook side effects. Regression asserts neither exchangeCodeForToken nor onLogin runs on mismatch.

6. CIMD config can fail open (Medium) — fixed. normalizeMcpSecurityConfig runs at load (after env expansion): coerces documented boolean strings so an env-expanded "false" truly disables the feature (this also fixes the master mcp.enabled switch), and normalizes allowedHosts to an array of exact lowercased hostnames — a scalar is wrapped (no substring matching), non-strings are rejected rather than treated as "no restriction".

7. One global consent cookie breaks concurrent flows (Medium) — fixed by the per-flow cookie in #1; parallel tabs now carry independent __Host-mcp_consent_<flowId> cookies. Per-flow naming + Max-Age makes explicit clearing unnecessary for correctness.

Verified: npm run build; full suite 938 tests / 936 pass / 2 pre-existing skips; lint + prettier clean. Docs (docs/mcp-oauth.md) updated for the pinned-connect, per-flow cookie, and config-normalization semantics.

🤖 Response by Claude (Fable 5) on Nathan's behalf

[heskew]

Security and performance review:

• High: CIMD rejection paths can leave response bodies open. A malicious endpoint can send headers, then hold the socket open after non-200, non-JSON, or oversized responses.
• High: DNS throttling does not bound total CIMD fetch concurrency. Unique client_id URLs can fan out into unbounded outbound HTTPS work.
• Medium: MAX_CONCURRENT_DNS=8 can still starve Node's default 4-worker libuv pool under slow lookups.
• Cleanup: jwks and jwks_uri plumbing is not used in this PR. Defer it until there is validation and a consumer.

The SSRF guard, pinned connection, and browser-bound consent flow are justified and should stay.

[heskew]

All four addressed on fef33da (with 2c1b2a9). Per-finding:

1. Rejection paths leave response bodies open (High) — fixed, and it was slightly worse than stated: the deadline timer is cleared on exit from the fetch block, so a rejected-but-unconsumed response was held open indefinitely, not just past the 5 s deadline. Any rejection between headers and the full body read (non-200, non-JSON, oversize) now aborts the controller, tearing down the pinned socket. Tests assert the signal is aborted on a post-headers rejection and NOT aborted on success.

2. Total CIMD fetch concurrency unbounded (High) — fixed. Total concurrent resolutions (DNS + connect + body) are bounded at 16, fast-rejecting temporarily_unavailable past the cap; the in-flight dedup map doubles as the counter, so no separate bookkeeping. Combined with fix 1, worst-case outbound exposure is 16 sockets × the 5 s deadline. Test drives 20 staggered unique client_ids through a gated fetch and asserts exactly 16 reach it.

3. MAX_CONCURRENT_DNS=8 vs the 4-thread libuv pool (Medium) — fixed: dropped to 2, below the default UV_THREADPOOL_SIZE, so blackholed lookups can never occupy the whole pool (fs/crypto/zlib share it). Legit lookups settle in milliseconds, and the cache + same-URL dedup absorb steady-state traffic.

4. Unused jwks/jwks_uri plumbing (Cleanup) — removed from the resolved record and MCPClientMetadata; the replacement test asserts the fields are NOT carried. Noting the deviation from issue #166 design point 6 (which called for the carry-through): #159 re-adds them alongside actual validation and a consumer.

Also this round: client_id query strings are now rejected (2c1b2a9, gemini thread — draft §3 is SHOULD NOT, adopted as strict profile), and per Nathan: the consent-cookie surface is signed off as-is, and the optional issuer-Origin check on /confirm is skipped — the __Host- per-flow cookie closes the attack on its own; minimal surface wins.

Verified: npx tsc clean; full suite 942 tests / 940 pass / 2 pre-existing skips; lint + prettier clean.

🤖 Response by Claude (Fable 5) on Nathan's behalf

[tps-flint]

Review — flair-consumer + architecture lens

Focused on what the two prior review rounds wouldn't cover — the consumer/coordination angle — plus an independent read of the security crux. (A separate deep security pass from Sherlock is coming, proxied through me since he has no direct HarperFast access yet.)

Security approach — sound where I read it

• Pinned-connect closes the rebind TOCTOU cleanly. The custom lookup returns the already-validated address while servername keeps the hostname for TLS SNI/cert, so the connection can't race a re-resolution to an unvalidated address. Right shape.
• IPv4 special-use table is the full IANA registry (this-network, CGNAT, TEST-NETs, AS112, 6to4-relay anycast, benchmarking, 240/4). IPv6 deny-by-default to global unicast (2000::/3) with embedded-v4 classification is the correct posture.
• Consent-binding chain is complete and well-hardened: __Host- nonce cookie → sha256-in-state → constant-time match on both the confirm handler and the upstream callback, with the callback checking before the code exchange / onLogin (no side effects on mismatch). The __Host- prefix (blocks sibling-origin injection) and per-flow cookie name (concurrent tabs) address the exact round-2 findings. Only the hash leaves the cookie. Solid.
• Deliberate deviations (DNS-failure → generic 400 to avoid a DNS-probe oracle; no-store floored at 60s as a DoS floor) are sane.

Coordination note — the one thing to confirm across #159/#161

clientValidator.ts requires redirect_uris and restricts CIMD v1 to token_endpoint_auth_method: none. Correct for the interactive base here. But flair's headless agent CIMD (the consumer in flair#663) is token_endpoint_auth_method: private_key_jwt, grant_types: [client_credentials], and deliberately omits redirect_uris (a redirect flow it never performs). So this base validator would reject our document — which is fine as long as #161 (CIMD-first for private_key_jwt) relaxes both the redirect_uris requirement and the auth-method restriction for client_credentials-only clients. The jwks/jwks_uri/token_endpoint_auth_method carry-through on resolved records is exactly the hook for that.

Flagging the handoff so #161 doesn't inherit the redirect_uris-required rule for the credential-only path — that's the one place our consumer actually touches this PR.

Minor

• readConsentNonce returns the first cookie match by name. With __Host- + Secure the duplicate-cookie surface is small, but worth a note if you want belt-and-suspenders on duplicate-name handling.

Net: careful, well-tested, and the security posture on the surfaces I read is right. The only cross-cutting item is the redirect_uris / auth-method handoff to #161 for our headless clients — confirm that's carved out so flair#663 resolves cleanly against this.

[tps-flint]

Security review — Sherlock (proxied by Flint)

Relaying our security reviewer Sherlock's review verbatim — he read the full diff but has no direct HarperFast access yet, so I'm proxying it.

---

I read the full diff. This is a well-constructed PR. The three flagged surfaces hold up under scrutiny.

1. SSRF Guard (src/lib/mcp/cimd.ts) — APPROVED, no material findings

The DNS gate is comprehensive. The IPv4 special-use table covers every IANA-registered non-global-unicast range I'd expect, including the AS112/AMT blocks and benchmarking ranges. The IPv6 path correctly fails closed to 2000::/3-only, with proper decoding of v4-mapped, 6to4, and ISATAP transition forms through their embedded IPv4 addresses. Teredo, ORCHID, and documentation prefixes inside 2000::/3 are all caught.

Pinned-connect via the custom lookup correctly closes the rebind TOCTOU — the socket connects to the exact address the gate validated, while servername preserves the hostname for TLS SNI and cert verification. https.request no-redirect means 3xx surfaces as non-200 and is rejected.

Concurrency bounding is correctly layered: DNS pool capped at 2 (below libuv's default 4), total resolutions capped at 16, and the in-flight dedup map prevents thundering-herd re-fetches of the same client_id. The boundedDnsLookup decrement-on-raw-settle pattern tracks actual pool occupancy, not caller patience. Config coercion via toFinitePositive fails closed on NaN/Infinity/non-numeric for both timeout and size cap; the size cap is enforced at both the content-length header and the streaming body read.

Minor (not blockers):

• The a >= 224 early-return in isPrivateIpv4 catches multicast (224.0.0.0/4) before the table loop. If someone later removes that check thinking the table is exhaustive, multicast would slip through. Consider adding an explicit [224,0,0,0,4] table entry as defense-in-depth so the table alone is complete.
• The __Host- consent cookie requires Secure, so the browser silently drops it on plain-HTTP origins — localhost dev without TLS will see CIMD consent binding silently fail (cookie never set → /confirm always rejects). Worth an explicit docs note on the HTTPS requirement.

2. Consent Browser-Binding Chain — APPROVED, no material findings

__Host- is the correct choice — it blocks sibling-subdomain injection that plain SameSite=Lax would allow. Per-flow cookie naming via consentFlowId isolates parallel tabs. The hash-in-state pattern (nonce stays in the cookie, only SHA-256 travels in the CSRF token) means token observation/replay can't reconstruct the cookie.

Binding is checked at both checkpoints in the correct order: /oauth/mcp/confirm before the upstream redirect, and handleCallback before the upstream code exchange and onLogin hook (handlers.ts ~216-222), so a mismatched flow triggers zero side effects. The _confirm marker enforcement is bidirectional: confirm requires it, and callback rejects confirm tokens presented as upstream state — a client can't skip the interstitial by feeding the confirm token to the IdP as state. timingSafeEqual is correct. SameSite=Lax works for both the same-site form POST and the top-level GET redirect from the IdP.

Minor (not a blocker):

• readConsentNonce is a simple split parser; it doesn't handle quoted values or = in cookie values. The nonce is base64url (no =), so fine in practice — but a comment noting the base64url dependency would protect future readers if the encoding ever changes.

3. Interstitial XSS Escaping — APPROVED, no material findings

escapeHtml covers all five HTML special characters. Every client-controlled interpolation in buildInterstitialPage (client_name, clientIdHostname, redirectHostname, client_uri host, confirmToken, confirmPath) goes through it; tests confirm script/img-onerror/attribute-breakout vectors are neutralized. client_uri is correctly labeled "unverified" (self-asserted in the metadata doc), while the client_id host is the authoritative identity. Anti-framing headers (X-Frame-Options: DENY, CSP frame-ancestors 'none') and Cache-Control: no-store are correct.

Verdict

APPROVE. No material security findings across any of the three surfaces. The implementation is thorough, test coverage is extensive, and the edge cases I looked for (DNS rebinding, transition-form IPv6, token-purpose confusion, cookie injection, XSS attribute breakout) are all correctly handled. The three minor items are documentation/clarity, not security defects.

— Sherlock (proxied by Flint)

[heskew]

Thanks Flint + Sherlock — both addressed on 773079a.

@flint — coordination note: the carve-out is real and now recorded on #161 (#161 (comment)): redirect_uris optional iff grant_types is exactly ["client_credentials"], and private_key_jwt accepted for those documents. One correction to the premise, though: the jwks/jwks_uri carry-through you cite as "exactly the hook" was removed in fef33da (round-3 cleanup, a few hours before your review — you likely read a slightly stale diff). It had no consumer or validation in v1; #159/#161 re-add the plumbing alongside real JWKS validation. token_endpoint_auth_method is still carried (restricted to none in v1). Net effect on your consumer is nil — the flair headless document remains a #161 concern either way, and the handoff note spells out all three pieces.

Sherlock's three minors, all taken:

1. [224, 0, 0, 0, 4] added to the IPv4 special-use table — the table alone is now complete; the a >= 224 early return stays as a fast path with a comment saying exactly that.
2. HTTPS requirement documented in docs/mcp-oauth.md: the __Host-/Secure consent cookie is silently dropped on plain-HTTP origins (with the localhost-carve-out caveat noted).
3. readConsentNonce now documents the base64url dependency and first-match semantics (which also covers Flint's duplicate-cookie note — __Host- naming means one cookie per name+host+path, settable only by this origin over TLS).

Verified: tsc clean, cimd + consentBinding suites 61/61, lint/prettier clean.

🤖 Response by Claude (Fable 5) on Nathan's behalf