feat(auth): implement generalized authentication layer and scope-based authorization#1729
feat(auth): implement generalized authentication layer and scope-based authorization#1729sairajm wants to merge 4 commits intomodelcontextprotocol:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 2f985be The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@modelcontextprotocol/client
@modelcontextprotocol/server
@modelcontextprotocol/express
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| this._onerror(new Error(`Unknown message type: ${JSON.stringify(message)}`)); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
🔴 Auth errors are detected by string-matching error.message.includes("Unauthorized") / error.message.includes("Forbidden") in 5+ locations (protocol.ts, mcp.ts, streamableHttp.ts) instead of using dedicated error codes. Any ProtocolError whose message coincidentally contains these words (e.g., "Forbidden characters in input") would be incorrectly escalated to HTTP 401/403. The ProtocolErrorCode enum already supports custom codes (e.g., UrlElicitationRequired = -32_042) -- consider adding Unauthorized/Forbidden codes instead of string matching.
Extended reasoning...
What the bug is
This PR introduces authentication and authorization error handling by throwing ProtocolError instances with ProtocolErrorCode.InvalidRequest and messages like "Unauthorized" or "Forbidden". These errors are then identified downstream via error.message.includes("Unauthorized") or error.message.includes("Forbidden") in at least 5 locations:
protocol.tsonmessagecatch block (~line 718)protocol.ts_onrequesterror handler (~line 887)protocol.ts.catchhandler (~line 915)mcp.tstools/callcatch block (~line 228-233)streamableHttp.tsin multiple catch blocks that map to HTTP 401/403
Why this is fragile
The string matching operates on the error message, not on a structured error code. This means any ProtocolError whose message happens to contain the substring "Unauthorized" or "Forbidden" will be incorrectly treated as an auth error. For example, consider a tool handler that throws:
throw new ProtocolError(ProtocolErrorCode.InvalidParams, "Forbidden characters in input");Step-by-step proof of false positive
- A user registers a tool via
McpServer.registerTool()that validates input and throwsnew ProtocolError(ProtocolErrorCode.InvalidParams, "Forbidden characters in input"). - The
tools/callhandler inmcp.tscatches this error. In the PR’s version, the catch block checkserror.message.includes("Forbidden")— this matches, so the error is re-thrown instead of being wrapped in aCallToolResulterror. - The re-thrown error propagates up to
protocol.ts_onrequest, where the error handler again checkserror.message.includes("Forbidden")and re-throws it instead of sending a normal JSON-RPC error response. - In
streamableHttp.ts, the outer catch block maps anyProtocolErrorcontaining"Forbidden"to an HTTP 403 response. - The client receives a 403 Forbidden HTTP response for what was actually a simple input validation error.
Why existing code doesn’t prevent this
The ProtocolErrorCode used for auth errors is InvalidRequest — the same generic code used for many other error conditions. The only distinguishing factor is the message string. There is no structural difference between an auth error and any other ProtocolError that happens to use a message containing these words.
Impact
This is a public SDK where users write custom tool/resource/prompt handlers that can throw ProtocolError with arbitrary messages. A false positive would cause:
- Normal validation errors being mapped to HTTP 401/403 instead of proper JSON-RPC error responses
- Client-side auth retry logic being triggered unnecessarily
- Confusing error behavior that would be very difficult to debug
Additionally, the mcp.ts catch block at lines ~228-233 is dead code: both branches of the if statement do the same thing (throw error), suggesting this pattern was applied mechanically without careful consideration.
How to fix
Add dedicated error codes to ProtocolErrorCode, e.g.:
Unauthorized = -32_043,
Forbidden = -32_044,Then check error.code instead of error.message in all catch blocks. This follows the existing pattern used by UrlElicitationRequired and ResourceNotFound.
There was a problem hiding this comment.
The dedicated error codes were added (Unauthorized = 401, Forbidden = 403 in ProtocolErrorCode) and some locations were updated to use code-based checking (mcp.ts catch block and streamableHttp.ts SSE/outer catch blocks now check error.code). However, protocol.ts itself — the file where this comment was posted — still uses error.message.includes("Unauthorized") / error.message.includes("Forbidden") in all 3 catch blocks (lines ~720, ~890, ~918). The JSON response mode in streamableHttp.ts (lines ~735-740) also still uses string matching.
These remaining string-match locations are still vulnerable to the false-positive scenario described in the original comment. They should be updated to check error.code === ProtocolErrorCode.Unauthorized / error.code === ProtocolErrorCode.Forbidden to match the pattern already applied in mcp.ts and the SSE mode of streamableHttp.ts.
| const server = new McpServer({ | ||
| name: "my-authenticated-server", | ||
| version: "1.0.0", | ||
| }, { | ||
| authenticator: new BearerTokenAuthenticator({ | ||
| validate: async (token) => { | ||
| // Validate the token (e.g., verify with an OAuth provider) | ||
| if (token === "valid-token") { | ||
| return { | ||
| name: "john_doe", | ||
| scopes: ["read:resources", "execute:tools"] | ||
| }; | ||
| } | ||
| return undefined; // Invalid token | ||
| } |
There was a problem hiding this comment.
🔴 Documentation in docs/auth.md and docs/server.md shows new BearerTokenAuthenticator({ validate: async (token) => { ... } }), but the actual constructor in bearer.ts takes a plain function: constructor(private readonly _validator: BearerTokenValidator). Any developer following the docs will get a runtime error. The correct usage is new BearerTokenAuthenticator(async (token) => { ... }) as shown in the integration test.
Extended reasoning...
Bug Analysis
The BearerTokenAuthenticator class in packages/server/src/server/auth/bearer.ts defines its constructor as:
constructor(private readonly _validator: BearerTokenValidator) {}where BearerTokenValidator is typed as (token: string) => Promise<AuthInfo | undefined> — a plain async function.
Documentation Mismatch
However, the documentation in two locations shows an object-based API:
docs/auth.md(line 27):new BearerTokenAuthenticator({ validate: async (token) => { ... } })docs/server.md(line 455): Same object-wrapping pattern
Additionally, the JSDoc examples in packages/middleware/express/src/middleware/auth.ts and packages/middleware/hono/src/middleware/auth.ts repeat the same incorrect pattern.
Step-by-Step Proof
- A developer reads
docs/auth.mdand writes:new BearerTokenAuthenticator({ validate: async (token) => { ... } }) - The constructor stores
{ validate: fn }(an object) asthis._validator - When
authenticate()is called, it extracts the Bearer token and callsthis._validator(token) - Since
this._validatoris actually{ validate: fn }(an object, not a function), calling it as a function throwsTypeError: this._validator is not a function
Confirmation
The integration test in auth_integration.test.ts (line 41) and the unit test in auth.test.ts (line 7) both correctly use the function-based API: new BearerTokenAuthenticator(async (token) => { ... }), confirming the docs are wrong.
Impact
Any developer following the official documentation to set up authentication will encounter a runtime TypeError when the first request with a Bearer token arrives. This is the primary documentation for a new security feature, so incorrect examples will affect most adopters.
Fix
Update all four locations to pass a plain function instead of an object:
// Before (wrong)
new BearerTokenAuthenticator({ validate: async (token) => { ... } })
// After (correct)
new BearerTokenAuthenticator(async (token) => { ... })There was a problem hiding this comment.
The docs (docs/auth.md and docs/server.md) and the Express middleware JSDoc now correctly show the function-based API — nice fix. However, the Hono middleware JSDoc example in packages/middleware/hono/src/middleware/auth.ts (lines 27-29) still uses the incorrect object-based pattern:
const authenticator = new BearerTokenAuthenticator({
validate: async (token) => ({ name: 'user', scopes: ['read'] })
});This should be:
const authenticator = new BearerTokenAuthenticator(
async (token) => ({ token, clientId: 'user', scopes: ['read'] })
);
| this._transport.onmessage = async (message, extra) => { | ||
| try { | ||
| if (isJSONRPCResultResponse(message) || isJSONRPCErrorResponse(message)) { | ||
| await _onmessage?.(message, extra); | ||
| this._onresponse(message); | ||
| } else if (isJSONRPCRequest(message)) { | ||
| await this._onrequest(message, extra); | ||
| } else if (isJSONRPCNotification(message)) { | ||
| await this._onnotification(message); | ||
| } else { | ||
| await _onmessage?.(message, extra); | ||
| this._onerror(new Error(`Unknown message type: ${JSON.stringify(message)}`)); | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
🟡 The PR moves the _onmessage?.(message, extra) call from being invoked unconditionally for all message types to only being called in the response and unknown-message branches. Code that set transport.onmessage before protocol.connect() to observe all incoming messages will silently stop seeing JSON-RPC requests and notifications. Restore await _onmessage?.(message, extra) at the top of the handler, before the if/else chain, to maintain the original behavior consistent with how onclose and onerror preserve their pre-existing handlers.
Extended reasoning...
What the bug is
In the original code, _onmessage (the previously-registered transport onmessage handler) was called unconditionally at the top of the new onmessage wrapper, before the if/else chain that dispatches by message type:
this._transport.onmessage = (message, extra) => {
_onmessage?.(message, extra); // Called for ALL messages
if (isJSONRPCResultResponse(message) || isJSONRPCErrorResponse(message)) {
this._onresponse(message);
} else if (isJSONRPCRequest(message)) {
...The PR restructures this so _onmessage is only called inside the response branch and the unknown-message else branch. It is completely skipped for isJSONRPCRequest and isJSONRPCNotification messages.
Why this is an inconsistency
The connect() method preserves pre-existing handlers for all three transport callbacks: onclose, onerror, and onmessage. For onclose and onerror, the old handler is still called unconditionally. But with this PR, onmessage only calls the old handler for some message types. This inconsistency suggests the omission is unintentional rather than a deliberate design choice.
Step-by-step proof
- External code sets
transport.onmessage = (msg) => console.log(msg)before callingprotocol.connect(transport). connect()captures this as_onmessage.- A client sends a JSON-RPC request (e.g.,
tools/call). The transport receives it and calls the newonmessagewrapper. - The wrapper enters the
isJSONRPCRequestbranch and callsawait this._onrequest(message, extra)— but never calls_onmessage. - The pre-existing handler never sees the request message. The same applies to notifications.
- Only response messages and unknown messages trigger
_onmessage.
Existing test coverage
There is a test at protocol.test.ts:212 ("should not overwrite existing hooks when connecting transports") that verifies callback preservation. However, it passes an empty string as the message, which falls into the unknown-message else branch where _onmessage IS still called — so the test does not catch this regression.
Impact
The practical impact is low: no production code in the repository sets transport.onmessage before connect(), and the Protocol docs state it assumes ownership of the transport. However, this is a behavioral contract change that could silently break external code relying on pre-connect message observation, and the inconsistency with onclose/onerror handling makes the code confusing.
How to fix
Restore await _onmessage?.(message, extra) at the top of the handler, before the if/else chain:
this._transport.onmessage = async (message, extra) => {
try {
await _onmessage?.(message, extra); // Restore: called for ALL messages
if (isJSONRPCResultResponse(message) || isJSONRPCErrorResponse(message)) {
this._onresponse(message);
} else if (isJSONRPCRequest(message)) {
await this._onrequest(message, extra);
} ...…Forbidden error codes
| scopes: string[] | undefined, | ||
| readCallback: ReadResourceCallback | ||
| ): RegisteredResource { | ||
| const registeredResource: RegisteredResource = { |
There was a problem hiding this comment.
🔴 _createRegisteredResource accepts a scopes parameter but never assigns it to the registeredResource object initializer, so resource.scopes is always undefined. This silently bypasses the authorization check in the resources/read handler (if (resource.scopes && resource.scopes.length > 0)), leaving static resources with scopes completely unprotected. Fix: add scopes, to the object initializer, matching the pattern in _createRegisteredResourceTemplate.
Extended reasoning...
The Bug
The _createRegisteredResource method (line ~687) was updated by this PR to accept a scopes: string[] | undefined parameter, but the registeredResource object initializer only includes name, title, metadata, readCallback, and enabled — it omits scopes. The parameter is accepted and silently discarded.
Comparison with Sibling Methods
_createRegisteredResourceTemplate (line ~719) correctly includes scopes, in its object initializer. Similarly, _createRegisteredPrompt and _createRegisteredTool both correctly store scopes in their respective registered objects. _createRegisteredResource is the only one that drops it.
Step-by-Step Proof
- A developer registers a static resource with scopes:
server.registerResource("secure", "secure://data", { scopes: ["admin"] }, callback) registerResourceextractsscopesfrom config and passes it to_createRegisteredResource(name, title, uri, metadata, scopes, readCallback)- Inside
_createRegisteredResource, the object initializer createsregisteredResourcewithoutscopes— it is{ name, title, metadata, readCallback, enabled: true } registeredResource.scopesisundefined- When a client calls
resources/read, the authorization check evaluatesresource.scopes && resource.scopes.length > 0— this isfalsebecauseresource.scopesisundefined - The authorization check is entirely skipped, and the resource callback executes without any scope verification
Why Existing Code Does Not Prevent It
The update() handler correctly handles updates.scopes (line ~704 in the diff), but it only applies to subsequent updates — the initial value from registration is never stored. TypeScript does not catch this because the RegisteredResource type has scopes as an optional field (scopes?: string[]), so the compiler allows the initializer to omit it.
Impact
This is a security-relevant bug. Any static resource registered with scopes will be silently left unprotected. The developer believes the resource is secured (they passed scopes: ["admin"]), but any authenticated user can read it regardless of their scopes. Resource templates are correctly protected, making this inconsistency particularly insidious — testing with templates would pass while static resources remain exposed.
Fix
Add scopes, to the registeredResource object initializer in _createRegisteredResource, between metadata and readCallback:
const registeredResource: RegisteredResource = {
name,
title,
metadata,
scopes,
readCallback,
enabled: true,
};1 participant
This PR introduces a robust, generalized authentication and authorization framework for the MCP TypeScript SDK. It enables servers to verify identities (e.g., via Bearer tokens) and enforce granular scope-based access control for tools, resources, and prompts.
Additionally, this PR includes critical fixes to the core protocol's asynchronous message loop to ensure that authentication and authorization errors (401/403) propagate correctly through the stack without being masked as generic internal errors.
Motivation and Context
The MCP TypeScript SDK lacked a standardized, built-in mechanism for handling authentication and authorization at the server level. This resulted in developers having to implement ad-hoc security logic that often conflicted with the internal protocol state, leading to inconsistent error reporting (e.g., returning 500 Internal Server Error instead of 401/403).
This change introduces a unified
Authenticator
layer and scope-based authorization, while also fixing a critical asynchronous propagation bug in the core
Protocol
class that previously prevented security errors from correctly surfacing to the transport layer.
How Has This Been Tested?
I implemented a comprehensive integration test suite in
packages/server/test/server/auth_integration.test.tsthat validates the following scenarios using theWebStandardStreamableHTTPServerTransport:401 Unauthorized: Verified that requests without a token or with an invalid token return a 401 status with appropriate headers.
403 Forbidden: Verified that validly authenticated users are blocked with a 403 status when attempting to access tools/resources for which they lack the required scopes.
Success Cases: Verified that public tools remain accessible and that private tools are accessible when the correct scopes are provided.
Transport Modes: Confirmed these behaviors work across both standard JSON responses and SSE streaming modes.
Breaking Changes
Yes, this PR introduces internal breaking changes to the
Protocolbase class:Protocol._onrequestsignature has been changed fromvoidtoPromise<void>. Subclasses overriding this method must now be updated to be async.Protocol.onmessageis now an asynchronous handler.Types of changes
Checklist
Additional context
Authenticatorinterface, allowing developers to bring their own authentication logic (JWT, API Keys, etc.).McpServerregistration methods, making it simple to secure specific tools or resources by simply passing a scopes array._onrequestpromise chain improves the overall reliability of the SDK's error handling by ensuring all request-processing errors are properly awaited and propagated.