feat: add OAuth example for providers without DCR support#1695
feat: add OAuth example for providers without DCR support#1695travisbreaks wants to merge 1 commit intomodelcontextprotocol:mainfrom
Conversation
Adds an example MCP server demonstrating how to authenticate users via an upstream OAuth provider (GitHub, Google, etc.) that does not support RFC 7591 Dynamic Client Registration. The pattern: the MCP server runs a lightweight OAuth Authorization Server proxy that accepts DCR from MCP clients, then proxies the actual authentication to the upstream provider using pre-registered credentials from environment variables. Includes: - OAuth proxy server with /register, /authorize, /callback, /token - Protected resource metadata (RFC 9728) on the MCP server - Bearer auth middleware for MCP endpoints - README with setup instructions and security considerations - Configuration for any OAuth 2.0 provider via environment variables Closes modelcontextprotocol#707 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
|
@claude review |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
No serious bugs — the inline comments are all minor nits. However, this is a substantial new example (~650 lines) implementing an OAuth proxy pattern, so a human should review the overall approach and whether this is the pattern the project wants to recommend.
Extended reasoning...
Overview
This PR adds three files: a new examples/server/src/oauthWithoutDcr.ts example (~645 lines), a dedicated README, and a one-line update to the examples index. All changes are confined to examples/server/ — no library or production code is modified.
Security risks
The example implements an OAuth Authorization Server proxy with DCR, token issuance, and bearer auth middleware. While it is demo code with explicit security disclaimers, the inline comments flag a minor client authentication logic gap where omitting client_secret bypasses validation. This is low-risk given the context (demo code, in-memory stores, localhost-only), but the pattern could be copied into production code. No injection, XSS, or other OWASP risks were identified.
Level of scrutiny
This warrants human review because: (1) it is a large new file implementing a non-trivial OAuth proxy pattern that serves as an architectural recommendation for the community, (2) the OAuth/auth domain means even example code should demonstrate correct security practices, and (3) no human reviewer has looked at it yet. The filename case mismatch (uppercase DCR vs lowercase) in the README run commands would cause failures on Linux — a straightforward fix but worth catching before merge.
Other factors
No automated tests are included; the test plan is entirely manual. No changeset was added (flagged by the changeset bot). The code is typecheck and lint clean per the PR description. The overall architecture (proxy pattern for non-DCR providers) is sound and well-documented, but the design decision of recommending this pattern deserves human sign-off.
| } else if (client_id) { | ||
| // Support client_secret_post | ||
| const client = registeredClients.get(client_id); | ||
| if (!client || (client.client_secret !== client_secret && client_secret !== undefined)) { | ||
| res.status(401).json({ error: 'invalid_client', error_description: 'Invalid client credentials' }); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 The client authentication check on line 337 allows requests with no client_secret to bypass secret validation: when client_secret is undefined, the condition client.client_secret \!== client_secret && client_secret \!== undefined evaluates to true && false = false, skipping the rejection. Since this is demo code with security disclaimers and the AS metadata already advertises "none" as a supported auth method, this is a minor inconsistency rather than a critical flaw — but it could teach bad habits if copied into production code. Consider requiring client_secret unconditionally or at least checking the client's registered token_endpoint_auth_method.
Extended reasoning...
Bug Analysis
The token endpoint at line 337 has a logic flaw in client authentication. The condition used to reject invalid credentials is:
if (\!client || (client.client_secret \!== client_secret && client_secret \!== undefined)) {When a request omits client_secret entirely (so it is undefined), the evaluation proceeds as follows:
client.client_secret \!== client_secret→"actual_secret" \!== undefined→trueclient_secret \!== undefined→undefined \!== undefined→falsetrue && false→false\!client || false→ only rejects if the client is unregistered
This means any caller who provides a valid client_id but omits client_secret will pass the authentication check, effectively allowing unauthenticated authorization code exchange.
Concrete example
Suppose client mcp-client-abc123 was registered via DCR and received secret s3cr3t. An attacker who discovers this client_id can POST to /token with { grant_type: "authorization_code", code: "<valid_code>", client_id: "mcp-client-abc123" } — no client_secret field at all. The code enters the else if (client_id) branch, looks up the client (found), then evaluates the condition: ("s3cr3t" \!== undefined && undefined \!== undefined) = (true && false) = false. Authentication passes. The attacker receives an access token mapped to the upstream token.
Mitigating factors
Several factors reduce the practical severity: (1) the AS metadata on line 136 explicitly lists "none" as a token_endpoint_auth_methods_supported value, which could suggest semi-intentional support for public clients; (2) exploiting this still requires a valid UUID-based client_id and a valid single-use authorization code bound to that client; (3) this is explicitly a demo/example file with security disclaimers in the README.
The real problem
The inconsistency is the issue worth highlighting. DCR (line 174) tells all clients to use client_secret_post and issues them secrets, but the token endpoint does not enforce that the client actually provides its secret. A proper implementation would track whether a client was registered as public vs. confidential and enforce accordingly.
Suggested fix
The simplest fix is to always require the secret when the client has one:
const client = registeredClients.get(client_id);
if (\!client || client.client_secret \!== client_secret) {This removes the client_secret \!== undefined escape hatch. If public client support is truly intended, the server should track each client's token_endpoint_auth_method and only skip the secret check for clients explicitly registered with method "none".
| if (state) { | ||
| pendingAuthorizations.get(proxyState)!.scope = `${pendingAuthorizations.get(proxyState)!.scope ?? ''}`; |
There was a problem hiding this comment.
🟡 Line 223 is a no-op: pendingAuthorizations.get(proxyState)\!.scope = \${pendingAuthorizations.get(proxyState)!.scope ?? ''}`;` just reassigns the scope string to itself via a template literal. This looks like leftover from an incomplete implementation (perhaps something was meant to be appended to the scope). The line should be removed as dead code.
Extended reasoning...
What the bug is
Line 223 in the /authorize handler performs this assignment:
pendingAuthorizations.get(proxyState)\!.scope = `${pendingAuthorizations.get(proxyState)\!.scope ?? ''}`;This is a no-op. The template literal ${value ?? '\} evaluates to the string value of scope (or the empty string if nullish). Since scope was already set to scope ?? OAUTH_SCOPES on line 211, it is guaranteed to be a non-nullish string at this point. The assignment just reads the scope, converts it to the same string, and writes it back.
Why existing code does not prevent it
There is no lint rule or type check that catches "assign a value to itself via template literal." TypeScript sees a valid string assignment. The code compiles and runs without error — it just does nothing useful.
How it manifests
This line appears inside the if (state) block (line 222) whose comment says "Store the original state from the MCP client so we can forward it back." The scope reassignment seems unrelated to that purpose. It looks like the author may have intended to append the original state to the scope, or perform some other transformation, but the implementation was never completed. As written, the line is dead code that could confuse future readers.
Step-by-step proof
- On line 211,
scopeis set:scope: scope ?? OAUTH_SCOPES. This is always a string (either the query param or the default). - On line 223, the code reads that scope:
pendingAuthorizations.get(proxyState)\!.scope→ e.g."read:user user:email". - The
?? ''fallback is unreachable since scope is already a string. - The template literal
`${"read:user user:email"}`evaluates to"read:user user:email". - The assignment writes
"read:user user:email"back to.scope— identical to what was already there. No-op.
Impact
No functional impact — this is dead code in an example file. However, it is confusing and suggests incomplete logic that a reader might try to "fix" or build upon incorrectly.
How to fix
Simply remove line 223. The scope is already correctly set on line 211 and does not need to be reassigned.
|
|
||
| ```bash | ||
| pnpm --filter @modelcontextprotocol/examples-server exec tsx src/oauthWithoutDCR.ts | ||
| ``` | ||
|
|
||
| Or from within this package: | ||
|
|
There was a problem hiding this comment.
🟡 The README run commands on lines 51 and 57 reference oauthWithoutDCR.ts (uppercase DCR), but the actual file is oauthWithoutDcr.ts (lowercase). On case-sensitive filesystems (Linux), these commands will fail with file-not-found. The same typo appears in the .ts file's own usage comment (line 37).
Extended reasoning...
Bug: Filename case mismatch in run commands
The file added by this PR is named oauthWithoutDcr.ts (lowercase dcr), as confirmed by the diff header and the filesystem. However, the accompanying README (README-oauthWithoutDcr.md) references the file as oauthWithoutDCR.ts (uppercase DCR) in two run commands:
- Line 51:
pnpm --filter @modelcontextprotocol/examples-server exec tsx src/oauthWithoutDCR.ts - Line 57:
pnpm tsx src/oauthWithoutDCR.ts
Additionally, the TypeScript file itself contains the wrong case in its own usage docstring comment at line 37: OAUTH_CLIENT_ID=xxx OAUTH_CLIENT_SECRET=yyy tsx src/oauthWithoutDCR.ts.
Why this matters
On case-sensitive filesystems (Linux, which is the standard deployment target and CI environment), oauthWithoutDCR.ts and oauthWithoutDcr.ts are different filenames. Running the documented commands will produce a Cannot find module / file-not-found error. This would only work silently on case-insensitive filesystems like macOS (default) or Windows.
Step-by-step proof
- User clones the repo on a Linux machine
- Follows the README instructions and runs:
pnpm tsx src/oauthWithoutDCR.ts - Node/tsx looks for
src/oauthWithoutDCR.tson the filesystem - The actual file is
src/oauthWithoutDcr.ts— these are different paths on Linux - The command fails with a file-not-found error
Note: the parent examples/server/README.md correctly references src/oauthWithoutDcr.ts (lowercase), so only the dedicated README and the .ts file's own comment are affected.
Fix
Replace oauthWithoutDCR.ts with oauthWithoutDcr.ts on lines 51 and 57 of the README, and on line 37 of the .ts file.
|
Apologies, first misread this as a solving DCR client-side. Still, we're moving towards this SDK not providing Authorization Server functionality, which a proxy server like this provides. It adds a lot of complexity to the SDK, and we generally recommend against the "all-in-one" pattern in favor of separate RS and AS. Going to close this. |
3 participants
Summary
Closes #707.
Adds an example demonstrating how to connect MCP clients to OAuth providers that don't support Dynamic Client Registration (DCR), like GitHub, Google, etc. Uses a proxy/adapter pattern where the MCP server acts as its own authorization server, proxying to the upstream provider.
Architecture:
What's included:
examples/server/src/oauthWithoutDcr.ts(~500 lines, fully documented)examples/server/src/README-oauthWithoutDcr.mdwith setup guide, architecture diagram, and adaptation instructions for Google/other providersTypecheck and lint clean.
Test plan