-
Notifications
You must be signed in to change notification settings - Fork 941
RFC 8707 Resource Indicators Implementation #638
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-Authored-By: Claude <[email protected]>
Co-Authored-By: Claude <[email protected]>
Co-Authored-By: Claude <[email protected]>
…emo provider - Clarify that core server handlers only pass through resource parameter - Emphasize that server URL validation is demonstrated in the demo provider - Update issue references to show #592 is fixed, #635 is related - Update examples to show DemoOAuthProviderConfig usage 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The plumbing looks good, but a few thoughts on the demo implementation:
The RFC 8707 is intending to make audience restricted tokens , so a key thing is that if we get a token with a different resource we reject it. So far, it looks like the demo is focused on preventing a token from ever being created with a bad resource, but doesn't exercise the token rejection flow that would happen during e.g. the bearerAuth.ts
middleware.
So, e.g. if I got a token for resource=https://foo.example.com/mcp
, and I used that on https://bar.example.com/mcp
we should reject it.
I think it's useful to prevent invalid tokens from being created, but I think it's important we demonstrate that the AS needs to either store the resource with the token (and check it), or it needs to encode the resource data into the token itself (and check it).
src/shared/auth-utils.ts
Outdated
* RFC 8707 section 2 states that resource URIs "MUST NOT include a fragment component". | ||
* Keeps everything else unchanged (scheme, domain, port, path, query). | ||
*/ | ||
export function resourceUrlFromServerUrl(url: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
optional: setting url.hash = ""
will remove the hash too, so you could keep it as a URL
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah cool, thanks!
@@ -28,18 +43,28 @@ export class DemoInMemoryClientsStore implements OAuthRegisteredClientsStore { | |||
* - Persistent token storage | |||
* - Rate limiting | |||
*/ | |||
interface ExtendedAuthInfo extends AuthInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it makes sense to add this to the base AuthInfo
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
if (resource) { | ||
await this.validateResource(client, resource); | ||
} | ||
throw new Error('Refresh tokens not implemented for example demo'); | ||
} | ||
|
||
async verifyAccessToken(token: string): Promise<AuthInfo> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think we want to change this method as well, and test it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementations that decode / parse the token probably would check the resource, but here we just lookup the tokenData we've validated in exchangeAuthorizationCode, and the resource expectations haven't changed since (same mcpServerUrl).
Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the code a bit :-)
@@ -28,18 +43,28 @@ export class DemoInMemoryClientsStore implements OAuthRegisteredClientsStore { | |||
* - Persistent token storage | |||
* - Rate limiting | |||
*/ | |||
interface ExtendedAuthInfo extends AuthInfo { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done, thanks
if (resource) { | ||
await this.validateResource(client, resource); | ||
} | ||
throw new Error('Refresh tokens not implemented for example demo'); | ||
} | ||
|
||
async verifyAccessToken(token: string): Promise<AuthInfo> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementations that decode / parse the token probably would check the resource, but here we just lookup the tokenData we've validated in exchangeAuthorizationCode, and the resource expectations haven't changed since (same mcpServerUrl).
src/shared/auth-utils.ts
Outdated
* RFC 8707 section 2 states that resource URIs "MUST NOT include a fragment component". | ||
* Keeps everything else unchanged (scheme, domain, port, path, query). | ||
*/ | ||
export function resourceUrlFromServerUrl(url: string): string { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hah cool, thanks!
…eware - Tests that when mcp-protocol-version header is missing, the middleware uses DEFAULT_NEGOTIATED_PROTOCOL_VERSION when calling verifyAccessToken - Ensures proper fallback behavior for protocol version negotiation 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
7b073bf
to
c2150f0
Compare
Fixed 5 instances of hardcoded "https://resource.example.com" in OAuth protected resource metadata mocks to use the actual resourceBaseUrl.href. This resolves test failures where the auth validation was rejecting requests because the resource URL in the metadata didn't match the actual test server URL. The failing tests were: - attempts auth flow on 401 during SSE connection - attempts auth flow on 401 during POST request - refreshes expired token during SSE connection - refreshes expired token during POST request - redirects to authorization if refresh token flow fails All SSE tests now pass (17/17). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Implements Resource Indicators validation for OAuth 2.0 (RFC 8707; spec change).
Fixes #592, Fixes #635
resource
parameter in OAuth authorization and token exchange flows to bind tokens to the MCP server.simpleStreamableHttp.ts
to show how to perform strict verification (disabled by default, run with--oauth --oauth-strict
to enable)Note: Inspector is being updated in pcarleton/auth-debugger-resource
Motivation and Context
Facilitate prevention of token theft/confusion attacks where a malicious MCP server steals tokens meant for other services by explicitly binding tokens to their intended resources and showing how a server can do strict checks of this binding.
This security vulnerability was identified in modelcontextprotocol/modelcontextprotocol#544.
How Has This Been Tested?
Test coverage has been added, and MCP inspector's auth flows (quick & guided) were tested manually (w/ pending modelcontextprotocol/inspector#526 changes applied)
Breaking Changes
While the change is breaking at a protocol level, it should not require code changes from SDK users (just SDK version bumping).
demoInMemoryOAuthProvider.ts
andsimpleStreamableHttp.ts
).simpleStreamableHttp.ts
only validates theresource
if--oauth-strict
is set.Types of changes
Checklist
Additional context
Implementation Approach
Resource URIs are used as-is with only fragment removal. This allows having different MCP servers under different subpaths (even w/ different query URLs) w/o sharing spilling their resource authorization to each other (to allow a variety of MCP server federation use cases).
Key Components Added
auth-utils.ts
): Resource URI handling and validationInvalidTargetError
for RFC 8707 complianceReferences