fix(security): port fastmcp v4 CWE-601 checks to multi-instance OAuth proxy#38
Merged
Conversation
- `ignoreDeprecations: "6.0"` silences the TS5101 baseUrl deprecation error so `npm test` and `npm run build` exit cleanly. TypeScript 6.0 aborts further checks on this error, which was masking pre-existing type issues in the test suite. - `types: ["node", "mocha"]` explicitly includes the mocha globals that Node16 moduleResolution no longer auto-picks up, so `describe`/`it`/ `beforeEach` resolve without each test file importing them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…h proxy
fastmcp 4.0.0 hardened `OAuthProxy` against CWE-601 open-redirect /
authorization-code theft (see commit 5478753 in the upstream repo). Four
public methods now enforce checks against the in-memory `registeredClients`
Map populated by DCR. Because `RedisOAuthProxy` overrides those methods and
never delegates to `super`, the new security guarantees were being
silently bypassed on every deployment of this server.
The Map-based check also doesn't work in multi-instance deployments: DCR on
instance A leaves instance B's Map empty, so a callback load-balanced to B
would be rejected as unregistered — or, as in our case, the check is skipped
entirely and every redirect_uri is accepted.
Changes
-------
- `RedisOAuthProxy.registerClient` override mirrors every accepted
redirect_uri to Redis under `mcp:oauth:client:<uri>` with a TTL so other
instances can honor the v4 check.
- `isClientRegistered(uri)` helper falls back to Redis when the local Map
misses, giving single-instance and multi-instance deployments a consistent
answer.
- `authorize()` override now enforces:
- `client_id === upstreamClientId` (invalid_client) — RFC 6749 §5.2
- `redirect_uri` is registered (invalid_request) — RFC 6749 §3.1.2.3
- `exchangeAuthorizationCode()` override now enforces the client_id check
on the non-PKCE branch too, so a stolen code can't be redeemed.
- `handleCallback()` override adds the defense-in-depth check on
`transaction.clientCallbackUrl` and purges the transaction on failure.
- `PassthroughOAuthProvider` no longer defaults `allowedRedirectUriPatterns`
to `https://*` — the exact pattern fastmcp v4 removed as unsafe because it
lets an attacker DCR `https://evil.example.com/*`. New
`OAUTH_ALLOWED_REDIRECT_URI_PATTERNS` env var is comma-separated;
defaults are `http://localhost:*` and `http://127.0.0.1:*`.
- Fix `consentRequired: true` passed to PassthroughOAuthProvider while
`createProxy` hardcoded `false` — harmless but misleading; both `false`
now, matching the passthrough token-swap intent.
- Update the stale "registeredClients Map is write-only" design comment.
Tests
-----
New `test/lib/redis-oauth-proxy.spec.ts` covers:
- DCR mirrors every URI to Redis with TTL, rejects patterns that fail DCR.
- authorize: unknown client_id, unregistered redirect_uri, attacker URI
with matching client_id, cross-instance DCR via shared Redis, tx persist.
- exchangeAuthorizationCode: unknown client_id on non-PKCE path.
- handleCallback defense-in-depth: callback on instance B after DCR
revocation, transaction purge, upstream token endpoint not called.
- Happy path: full authorize → callback → code stored in Redis.
Uses `ioredis-mock` as a dev dep. Shared in-memory state across instances
simulates real Redis; a `flushall` between tests resets the registry while
still allowing cross-instance behavior within a single test.
Updates 10 existing test fixtures to add the new `oauthAllowedRedirectUriPatterns`
field on the `McpConfig` literal.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded configurable OAuth redirect allow-list and persisted Dynamic Client Registration (DCR) in the Redis-backed OAuth proxy; enforced stricter client_id/redirect_uri checks and atomic code consumption; added ioredis-mock dev deps, comprehensive tests for cross-instance behavior, and updated test fixtures and tsconfig. Changes
Sequence DiagramsequenceDiagram
participant Client as Client App
participant Proxy as Redis OAuth Proxy
participant Redis as Redis
participant Auth as OAuth Server
participant CB as Callback Handler
Client->>Proxy: registerClient(DCR request)
Proxy->>Redis: store redirect_uri entries with TTL
Proxy-->>Client: respond with client_id
Client->>Proxy: authorize(client_id, redirect_uri)
Proxy->>Redis: isClientRegistered(redirect_uri)?
alt registered
Proxy->>Redis: persist transaction
Proxy-->>Client: redirect to Auth server (302)
else not registered
Proxy-->>Client: error invalid_request
end
Auth->>CB: callback(code, state)
CB->>Proxy: handleCallback(code, state)
Proxy->>Redis: getdel transaction
Proxy->>Redis: isClientRegistered(transaction.callback_url)?
alt still registered
Proxy->>Auth: exchange code for token
Auth-->>Proxy: token
Proxy-->>CB: redirect to registered callback URL (302)
else not registered
Proxy->>Redis: delete transaction
Proxy-->>CB: error invalid_request
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Probing the prod deployment (https://mcp.browserless.io) with the narrow localhost-only defaults introduced in the previous commit would have broken two real clients that DCR against this server today: - https://claude.ai/api/mcp/auth_callback (Claude.ai web custom connectors) - https://chatgpt.com/connector_platform_oauth_redirect (ChatGPT MCP connector) Encode the verified set in the repo so the server works out-of-the-box for the hosted deployment without any extra env wiring, and rename the extension knob from `OAUTH_ALLOWED_REDIRECT_URI_PATTERNS` (replace) to `OAUTH_ADDITIONAL_REDIRECT_URI_PATTERNS` (append) to match the new semantics — the env var now adds to the baseline rather than replacing it. Prod vulnerability context: with the pre-v4 `https://*` default, DCR on prod currently accepts any HTTPS URI including attacker-controlled hosts, and /oauth/authorize returns 302 for arbitrary client_id + evil redirect (verified live). Narrowing to this explicit list closes CWE-601 without disrupting known clients. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/config.ts (2)
30-46: Consider deployment-scoped defaults for redirect allow-list.These defaults now trust hosted-specific callbacks (
claude.ai,chatgpt.com) for every deployment. For least-privilege in self-hosted installs, keep loopback-only defaults and gate hosted callbacks behind an explicit env/profile toggle.Proposed refactor
const DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS = [ 'http://localhost:*', 'http://127.0.0.1:*', - 'https://claude.ai/api/mcp/auth_callback', - 'https://chatgpt.com/connector_platform_oauth_redirect', ]; + +const HOSTED_ALLOWED_REDIRECT_URI_PATTERNS = [ + 'https://claude.ai/api/mcp/auth_callback', + 'https://chatgpt.com/connector_platform_oauth_redirect', +]; function parseRedirectUriPatterns(raw: string | undefined): string[] { + const includeHosted = process.env.OAUTH_INCLUDE_HOSTED_REDIRECTS === 'true'; const additional = (raw ?? '') .split(',') .map((s) => s.trim()) .filter((s) => s.length > 0); - return [...DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS, ...additional]; + return [ + ...DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS, + ...(includeHosted ? HOSTED_ALLOWED_REDIRECT_URI_PATTERNS : []), + ...additional, + ]; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 30 - 46, DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS currently includes hosted-specific callbacks which should not be trusted by default in self-hosted installs; change the constant to only include loopback entries ('http://localhost:*', 'http://127.0.0.1:*') and remove the claude.ai/chatgpt.com entries, then add a gated mechanism (new env var like ENABLE_HOSTED_REDIRECTS or a deployment profile check) in the config initialization or parseRedirectUriPatterns to append the hosted patterns when that toggle is explicitly enabled; reference DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS, OAUTH_ADDITIONAL_REDIRECT_URI_PATTERNS and parseRedirectUriPatterns so you update the constant and extend/merge hosted patterns only when the new env/profile flag is true.
48-54: Optional: de-duplicate merged redirect patterns.If additional patterns repeat defaults, config will carry duplicates. A stable de-dup keeps behavior deterministic and avoids redundant checks.
Small cleanup
function parseRedirectUriPatterns(raw: string | undefined): string[] { const additional = (raw ?? '') .split(',') .map((s) => s.trim()) .filter((s) => s.length > 0); - return [...DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS, ...additional]; + return Array.from( + new Set([...DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS, ...additional]), + ); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/config.ts` around lines 48 - 54, The merged redirect patterns in parseRedirectUriPatterns may contain duplicates when additional patterns repeat defaults; update parseRedirectUriPatterns to return a stable de-duplicated array (preserving order) by combining DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS and additional and removing duplicates—e.g. use a Set or filter to keep the first occurrence of each pattern so DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS remain first and repeated entries from additional are dropped.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/config.ts`:
- Around line 30-46: DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS currently includes
hosted-specific callbacks which should not be trusted by default in self-hosted
installs; change the constant to only include loopback entries
('http://localhost:*', 'http://127.0.0.1:*') and remove the
claude.ai/chatgpt.com entries, then add a gated mechanism (new env var like
ENABLE_HOSTED_REDIRECTS or a deployment profile check) in the config
initialization or parseRedirectUriPatterns to append the hosted patterns when
that toggle is explicitly enabled; reference
DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS, OAUTH_ADDITIONAL_REDIRECT_URI_PATTERNS
and parseRedirectUriPatterns so you update the constant and extend/merge hosted
patterns only when the new env/profile flag is true.
- Around line 48-54: The merged redirect patterns in parseRedirectUriPatterns
may contain duplicates when additional patterns repeat defaults; update
parseRedirectUriPatterns to return a stable de-duplicated array (preserving
order) by combining DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS and additional and
removing duplicates—e.g. use a Set or filter to keep the first occurrence of
each pattern so DEFAULT_ALLOWED_REDIRECT_URI_PATTERNS remain first and repeated
entries from additional are dropped.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Previous commit (483a22e) added `ignoreDeprecations: "6.0"` to work around the TS 6.0.3 `baseUrl` deprecation error. Proper fix: just delete `baseUrl`. It was `"."` with no `paths` entries and no non-relative imports in src/ or test/, so under `moduleResolution: Node16` it was dead weight. Removing it makes `tsc` exit 0 without a suppression, so future deprecation changes won't be silently absorbed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per review feedback from Devin. Today the two are identical (fastmcp v4 registerClient writes `request.redirect_uris` to the Map and echoes them back in the response), but using `response.redirect_uris` is the authoritative output of the parent's validation, so Redis stays in sync with the local Map even if upstream starts normalizing URIs during registration. DCRResponse.redirect_uris is a required string[] per the fastmcp type, so no nullish fallback is needed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Review triageApplied one fix, declined the rest with rationale. Applied (55d2f1e):
Declined / resolved:
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
… rollback Three issues raised in review, all cross-instance correctness bugs in the security path this PR is meant to patch. Atomic one-time redemption (code-reviewer, test-analyzer) --------------------------------------------------------- `exchangeAuthorizationCode` now uses Redis GETDEL to consume the code atomically. Previously, two concurrent redemptions on different instances could both GET the code, both pass validation, and both issue tokens — the parent's `used` flag is in-memory and wasn't being persisted to Redis, so the non-PKCE `clientCode.used` check was dead code. GETDEL guarantees only one caller pulls a value regardless of instance, so the `used` check is removed and the redundant `redis.del` after success is dropped (GETDEL already consumed the key). DCR mirror rollback on Redis failure (silent-failure-hunter) ------------------------------------------------------------ `registerClient` used `Promise.all`, so a Redis write failure would leave the parent's in-memory registeredClients Map populated while Redis was partial or empty — the exact cross-instance inconsistency this class exists to prevent. Switched to `Promise.allSettled`; on any rejection, roll back the parent's Map entries and best-effort delete any Redis keys that succeeded before surfacing the error. Tests ----- - Non-PKCE happy path: verifies upstream access/refresh tokens flow through - Non-PKCE one-time use: second redemption of the same code → invalid_grant - Cross-instance atomic race: two proxies share Redis and race on the same code; exactly one wins (proves GETDEL vs GET+DEL) - registerClient rollback: stubs redis.set to reject, asserts authorize then rejects the URI as unregistered (both layers cleaned up) - parseRedirectUriPatterns: dedicated spec for append semantics, whitespace/comma parsing, empty-input → baseline fallback 171 → 180 tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Review triage round 2 — post code-review-toolkit agentsRan the four pr-review-toolkit agents (code-reviewer, pr-test-analyzer, silent-failure-hunter, comment-analyzer) against the branch. Applied the critical + important findings in 0ec4890; deferring the rest with rationale below. Applied (0ec4890)
Deferred (follow-up, not this PR)
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Three CodeRabbit findings on top of 0ec4890. 1. `registerClient` rollback only clears entries this call introduced --------------------------------------------------------------- Prior rollback unconditionally deleted every URI in the response when a Redis write failed, which could deregister a valid prior registration if two DCR calls overlapped on a redirect_uri (same client re-registering, or two clients colliding on a localhost port). Snapshot local-Map + Redis pre-existence before mutating state; on failure, only roll back entries not in either snapshot. New test exercises the overlap case. 2. Document Valkey / Redis 6.2+ requirement for GETDEL --------------------------------------------------- Header comment now calls out that the token-exchange atomicity relies on GETDEL (Redis 6.2+, all Valkey releases). No runtime version probe — the deployment target (DO managed Valkey) predates the code, so failing at startup on an incompatible store would be a configuration error, not a runtime mystery. The command-not-supported error from the store would be loud enough. 3. `config.spec.ts` asserts full baseline membership ------------------------------------------------ Replaced partial `include` + length checks with `have.members` against a `BASELINE_PATTERNS` constant so substituting any baseline entry with an unintended value would fail the test. Matters because the baseline is the security-sensitive allowlist shipped with the repo. 180 → 181 tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Review triage round 3 — 02068beAll three open CodeRabbit findings addressed. Context on Valkey since we run on it. Applied
Test totals180 → 181 passing (+ 1 new rollback-preserves-prior-registration test). |
Contributor
Author
|
@coderabbitai please re-review — all previous actionable findings have been addressed across commits 55d2f1e, 0ec4890, and 02068be. PR head is now 02068be with 181 tests passing. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me fetch the current state of the key files for a thorough re-review. 🏁 Script executed: Length of output: 43457
Remaining finding:
|
Caught by CodeRabbit on the re-review of 02068be. The pre-existence probe used Promise.all, so a Redis connection failure during exists() would propagate before reaching the Promise.allSettled writes block — which hosts the rollback branch. Net effect: super.registerClient had populated the parent's in-memory registeredClients Map, and the error escaped registerClient without cleanup, leaving this instance accepting the URI via Map while every other instance rejected it (the exact cross-instance inconsistency this class prevents). Switched the probe to Promise.allSettled. On any probe rejection, roll back the Map synchronously (no writes have been attempted, so no Redis cleanup is needed) and surface the originating error. Extracted `rollbackLocalMap` so both failure paths share it. New test stubs redis.exists to reject, asserts registerClient throws, and then asserts authorize() rejects the URI as unregistered — proving both layers are clean after the fault. 181 → 182 tests passing. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Round 4 — 1f090cfThanks @coderabbitai, legit bug. Applied. Root cause: the pre-existence probe I added in 02068be used Fix: probe with New test stubs 181 → 182 tests passing. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainLet me verify the fix at 1f090cf before confirming. 🏁 Script executed: Length of output: 14155
The new test (stub 🧠 Learnings used |
The parent's `registeredClients` Map was a same-instance read-cache layered
over Redis. It saved ~1ms per authorize/callback round-trip on the hot path
but was the root of every bug we iterated on across rounds 3–5: over-deletion
on rollback, `localPreExisting` snapshot logic, probe-failure leaking the
parent Map populated. The cache's value (partial same-instance resilience
during Redis blips) was weak — other instances were rejected anyway.
`isClientRegistered` now queries Redis directly. `registerClient` still calls
`super` (the parent populates the Map as a side effect — we just don't read
from it) but drops all Map-rollback plumbing; on failure we only clean up
Redis. Redis pre-existence probe stays to avoid DEL'ing a concurrent valid
registration. Net: -60 lines of coordination logic, one clearer invariant
("Redis is the single source of truth for DCR state").
Added a constructor guard: throws if `consentRequired` is true. The parent's
`handleConsent` reads transactions from a process-local Map which would
reintroduce the exact multi-instance bug this class prevents, and our
`authorize` override short-circuits the consent branch in the parent's
`authorize`, so enabling consent would either be silently ignored or
partially-broken. Fail fast at construction time.
Tests renamed to drop "parent Map" framing (no longer implementation-
relevant), new test asserts the constructor guard.
182 → 183 tests passing.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
Round 5 — 39956c6: drop parent Map cachePer the "why do we need per-instance cache" discussion. The parent's `registeredClients` Map was a same-instance read-cache over Redis — saving ~1ms per authorize/callback on the hot path, but producing every bug we iterated on across rounds 3–5 (over-delete on rollback, Changes
Consent guardOn the side question about flipping `consentRequired: true` — that doesn't fix the `handleConsent` gap: our `authorize` override short-circuits the parent's consent branch, and `handleConsent` itself reads transactions from a process-local Map (not Redis) which is exactly the multi-instance bug this class prevents. Enabling consent would either be silently ignored (current) or partially broken (if we made `authorize` honor it). Added a constructor guard that throws if `consentRequired` is true, so a future caller who tries to enable it fails immediately at construction rather than running into the half-broken flow at runtime. New test covers it. Tradeoff accepted
182 → 183 tests passing. |
Contributor
Author
|
@coderabbitai please re-review — PR head is now 39956c6. Since your last pass on 1f090cf, I dropped the parent Map cache and switched |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 30600
The pre-existing overlap is handled correctly: Constructor guard — the
Test suite covers every new invariant introduced across all five rounds: constructor rejection, probe failure, write failure, overlap preservation, cross-instance DCR→authorize, GETDEL race, No remaining issues. This PR is in good shape. [approve] |
…i-instance # Conflicts: # tsconfig.json
andyMrtnzP
pushed a commit
that referenced
this pull request
May 20, 2026
… proxy (#38) ## Summary fastmcp 4.0.0 ([upstream commit](punkpeye/fastmcp@5478753)) hardened `OAuthProxy` against CWE-601 open-redirect / authorization-code theft. The fix added validation in four public methods — all of which `RedisOAuthProxy` overrides without delegating to `super`. As a result, the v4.0.0 security guarantees were silently bypassed on every deployment of this server after the Dependabot bump (#28) landed. **This has been verified live against `https://mcp.browserless.io`:** - `POST /oauth/register` with `redirect_uris: ["https://evil.attacker.com/steal"]` → returns `200` with a client_id/secret. - `GET /oauth/authorize?client_id=arbitrary-attacker-id&redirect_uri=https%3A%2F%2Fevil.attacker.com%2Fsteal&response_type=code&state=probe` → returns `302` to the upstream Supabase authorize page. The attacker-controlled URL is persisted on the transaction; once a victim authenticates at Supabase, the MCP server's callback would forward the fresh authorization code to the attacker's host. This PR ports the v4 checks, extends them to work in the multi-instance deployment, and narrows `allowedRedirectUriPatterns` to the known-good list instead of the removed `https://*` fallback. ### Why the bypass happens The v4 checks read from `OAuthProxy.registeredClients` — a process-local `Map` populated by DCR. The `RedisOAuthProxy` overrides (`authorize`, `exchangeAuthorizationCode`, `handleCallback`) re-implement their logic from scratch against Redis and never call `super`, so: 1. **The CWE-601 check is skipped.** Attacker-controlled `redirect_uri` is accepted at `/oauth/authorize`. 2. **Multi-instance is architecturally broken if the check were applied.** DCR on instance A leaves instance B's Map empty — B would reject every legitimate `redirect_uri` under the v4 check. The stale comment in `redis-oauth-proxy.ts:24-27` ("registeredClients Map is write-only (never read by other methods)") was true in v3.33 and no longer is. ### What changed - **Redis-backed DCR registry.** `registerClient` override mirrors every accepted `redirect_uri` to Redis under `mcp:oauth:client:<uri>` with a TTL. New `isClientRegistered(uri)` helper falls back to Redis when the local Map misses — consistent answers across single- and multi-instance deployments. - **`authorize()`** enforces `client_id === upstreamClientId` (RFC 6749 §5.2) and `isClientRegistered(redirect_uri)` (RFC 6749 §3.1.2.3). - **`exchangeAuthorizationCode()`** enforces the client_id check on the non-PKCE branch as well, so a stolen code can't be redeemed by an arbitrary caller. - **`handleCallback()`** adds the defense-in-depth registry check on `transaction.clientCallbackUrl` and purges the transaction on failure. - **Allowlist default replaced** — `https://*` is gone. New baseline encodes the real clients that DCR against prod today (verified by probing the live service): - `http://localhost:*`, `http://127.0.0.1:*` — Claude Desktop, Cursor, VS Code, Windsurf, any loopback-callback client - `https://claude.ai/api/mcp/auth_callback` — Claude.ai web custom connectors - `https://chatgpt.com/connector_platform_oauth_redirect` — ChatGPT MCP connector - **`OAUTH_ADDITIONAL_REDIRECT_URI_PATTERNS`** env var (comma-separated) lets deployments append patterns on top of the baseline without forking the code. New MCP hosts / staging domains go here. - **`consentRequired: true` passed to PassthroughOAuthProvider but the proxy config hardcoded `false`.** Harmless today (proxy config wins) but misleading — both `false` now, matching the passthrough token-swap intent. - **Dependency housekeeping.** `tsconfig.json` gets `types: ["node", "mocha"]` so `@types/mocha` is picked up under `moduleResolution: Node16`, and the now-vestigial `baseUrl` entry is removed so `tsc` stops warning about the TS 6.0 deprecation without a suppression flag. ### Tests New `test/lib/redis-oauth-proxy.spec.ts` (uses `ioredis-mock`): - DCR mirrors every URI to Redis with TTL; rejects patterns that fail DCR validation. - `authorize`: unknown `client_id`, unregistered `redirect_uri`, attacker URI with matching `client_id`, cross-instance DCR via shared Redis, transaction persisted. - `exchangeAuthorizationCode`: unknown `client_id` on non-PKCE path. - `handleCallback` defense-in-depth: callback on instance B after DCR revocation, transaction purged, upstream token endpoint never called. - Happy path: full `authorize → callback`, code stored in Redis, 302 back to registered callback. All 171 tests pass. 10 new tests added. ## Test plan - [x] `npm test` — 171 passing, 0 failing - [x] `npx tsc` — clean exit - [x] Probed prod `https://mcp.browserless.io` → confirmed current vulnerability (DCR accepts evil, authorize returns 302 for arbitrary client_id) - [x] Verified prod known clients against the new default allowlist — claude.ai, chatgpt.com, and localhost all match - [ ] Deploy to test env (`https://dev-mcp.browserless.io`), verify: - [ ] Claude.ai web + ChatGPT MCP connector can still complete the OAuth flow end-to-end - [ ] Claude Desktop / Cursor / VS Code can still complete the OAuth flow (localhost) - [ ] Multi-instance: DCR on one instance, authorize+callback+exchange across different instances via the LB - [ ] Attempt the CWE-601 PoC — confirm `invalid_client` for arbitrary `client_id`, `invalid_request` for unregistered `redirect_uri` - [ ] Promote to prod ## Deployment notes for operators No terraform change required for the current prod deployment — the known-good list is baked in. If a future MCP client appears on a new host (staging, new integration, etc.), extend at runtime without shipping a code change: ``` OAUTH_ADDITIONAL_REDIRECT_URI_PATTERNS=https://new-host.example.com/* ``` Deployments outside of Browserless that embed this server can override the baseline by setting `OAUTH_ADDITIONAL_REDIRECT_URI_PATTERNS` to their own client list — the baseline does not prevent anything, it just guarantees the Browserless-hosted path works without extra config. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Configurable OAuth allowed redirect URI patterns via environment variable (defaults preserved). * Dynamic Client Registration persisted in Redis for cross-instance consistency. * **Security Improvements** * Enforces client_id and registered redirect URI checks across auth and token flows. * Rejects and purges requests using unregistered or attacker-controlled callbacks. * **Changes** * OAuth consent prompt disabled by default. * **Chores** * Added Redis mock dev dependencies for testing. * **Tests** * New and expanded tests covering DCR, shared registry, validation, rollback, and atomic code exchange. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Merged
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.
Summary
fastmcp 4.0.0 (upstream commit) hardened
OAuthProxyagainst CWE-601 open-redirect / authorization-code theft. The fix added validation in four public methods — all of whichRedisOAuthProxyoverrides without delegating tosuper. As a result, the v4.0.0 security guarantees were silently bypassed on every deployment of this server after the Dependabot bump (#28) landed.This has been verified live against
https://mcp.browserless.io:POST /oauth/registerwithredirect_uris: ["https://evil.attacker.com/steal"]→ returns200with a client_id/secret.GET /oauth/authorize?client_id=arbitrary-attacker-id&redirect_uri=https%3A%2F%2Fevil.attacker.com%2Fsteal&response_type=code&state=probe→ returns302to the upstream Supabase authorize page. The attacker-controlled URL is persisted on the transaction; once a victim authenticates at Supabase, the MCP server's callback would forward the fresh authorization code to the attacker's host.This PR ports the v4 checks, extends them to work in the multi-instance deployment, and narrows
allowedRedirectUriPatternsto the known-good list instead of the removedhttps://*fallback.Why the bypass happens
The v4 checks read from
OAuthProxy.registeredClients— a process-localMappopulated by DCR. TheRedisOAuthProxyoverrides (authorize,exchangeAuthorizationCode,handleCallback) re-implement their logic from scratch against Redis and never callsuper, so:redirect_uriis accepted at/oauth/authorize.redirect_uriunder the v4 check.The stale comment in
redis-oauth-proxy.ts:24-27("registeredClients Map is write-only (never read by other methods)") was true in v3.33 and no longer is.What changed
registerClientoverride mirrors every acceptedredirect_urito Redis undermcp:oauth:client:<uri>with a TTL. NewisClientRegistered(uri)helper falls back to Redis when the local Map misses — consistent answers across single- and multi-instance deployments.authorize()enforcesclient_id === upstreamClientId(RFC 6749 §5.2) andisClientRegistered(redirect_uri)(RFC 6749 §3.1.2.3).exchangeAuthorizationCode()enforces the client_id check on the non-PKCE branch as well, so a stolen code can't be redeemed by an arbitrary caller.handleCallback()adds the defense-in-depth registry check ontransaction.clientCallbackUrland purges the transaction on failure.https://*is gone. New baseline encodes the real clients that DCR against prod today (verified by probing the live service):http://localhost:*,http://127.0.0.1:*— Claude Desktop, Cursor, VS Code, Windsurf, any loopback-callback clienthttps://claude.ai/api/mcp/auth_callback— Claude.ai web custom connectorshttps://chatgpt.com/connector_platform_oauth_redirect— ChatGPT MCP connectorOAUTH_ADDITIONAL_REDIRECT_URI_PATTERNSenv var (comma-separated) lets deployments append patterns on top of the baseline without forking the code. New MCP hosts / staging domains go here.consentRequired: truepassed to PassthroughOAuthProvider but the proxy config hardcodedfalse. Harmless today (proxy config wins) but misleading — bothfalsenow, matching the passthrough token-swap intent.tsconfig.jsongetstypes: ["node", "mocha"]so@types/mochais picked up undermoduleResolution: Node16, and the now-vestigialbaseUrlentry is removed sotscstops warning about the TS 6.0 deprecation without a suppression flag.Tests
New
test/lib/redis-oauth-proxy.spec.ts(usesioredis-mock):authorize: unknownclient_id, unregisteredredirect_uri, attacker URI with matchingclient_id, cross-instance DCR via shared Redis, transaction persisted.exchangeAuthorizationCode: unknownclient_idon non-PKCE path.handleCallbackdefense-in-depth: callback on instance B after DCR revocation, transaction purged, upstream token endpoint never called.authorize → callback, code stored in Redis, 302 back to registered callback.All 171 tests pass. 10 new tests added.
Test plan
npm test— 171 passing, 0 failingnpx tsc— clean exithttps://mcp.browserless.io→ confirmed current vulnerability (DCR accepts evil, authorize returns 302 for arbitrary client_id)https://dev-mcp.browserless.io), verify:invalid_clientfor arbitraryclient_id,invalid_requestfor unregisteredredirect_uriDeployment notes for operators
No terraform change required for the current prod deployment — the known-good list is baked in. If a future MCP client appears on a new host (staging, new integration, etc.), extend at runtime without shipping a code change:
Deployments outside of Browserless that embed this server can override the baseline by setting
OAUTH_ADDITIONAL_REDIRECT_URI_PATTERNSto their own client list — the baseline does not prevent anything, it just guarantees the Browserless-hosted path works without extra config.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Security Improvements
Changes
Chores
Tests