-
Notifications
You must be signed in to change notification settings - Fork 641
Server implementation of MCP auth #151
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
Merged
Merged
Changes from 39 commits
Commits
Show all changes
45 commits
Select commit
Hold shift + click to select a range
afae421
Move OAuth definitions into shared, add more
jspahrsummers a0069e2
Split full/partial client information
jspahrsummers 9ba6ce0
Add `express` as a production dependency
jspahrsummers 76f367b
WIP client registration handler for Express
jspahrsummers 6bfbd20
Client registration using a "store" interface
jspahrsummers e27907d
Add `cors` package
jspahrsummers a776f5f
Build a router to chain middleware better
jspahrsummers 6d33a7c
Rename handler file to register.ts to make path more obvious
jspahrsummers 9dc4a4f
WIP /authorize endpoint
jspahrsummers 54f643c
Validate redirect URIs _as_ URIs
jspahrsummers 62cd952
Authorization flow handoff
jspahrsummers 5738f0c
WIP /token endpoint
jspahrsummers 26ffe00
Auth code and refresh token flows
jspahrsummers f4002e5
Add type definition for token revocation requests
jspahrsummers 19f2ac7
Extract client authentication into reusable middleware
jspahrsummers 9abcccf
Token revocation endpoint
jspahrsummers bff65e6
Add client information to all OAuth provider methods
jspahrsummers 6830c92
Remove redundant comment
jspahrsummers 43f433c
Match style for registration request handler
jspahrsummers a6e78b7
Add handler for auth server metadata
jspahrsummers bb49189
Router for all MCP auth endpoints + behaviors
jspahrsummers f292b12
Remove `generateToken` until needed
jspahrsummers 76b9781
Use `URL.canParse` instead of custom `isValidUrl`
jspahrsummers 6a6dcba
Install `supertest` and `@jest-mock/express`
jspahrsummers 76938c7
Claude-authored tests
jspahrsummers 36f9c6b
Get tests passing
jspahrsummers 4d02ed9
Install `express-rate-limit`
jspahrsummers 1415c2d
Rate limiting on individual handlers
jspahrsummers 48cddd9
Pass through rate limits and other options cleanly from router
jspahrsummers 07e8ce9
Fix router test after `metadata` key removed
jspahrsummers 8d62612
Extract middleware for 405 Method Not Allowed
jspahrsummers 5d7bc7a
Fix unused variable lint
jspahrsummers 85e3cad
Fix `Request` type extension to not use namespacing
jspahrsummers 7b81b4f
Handle POST bodies in /authorize
jspahrsummers 63db322
Set Cache-Control: no-store
jspahrsummers f6e47c4
Add missing `name` to `McpError`
jspahrsummers 1dfb9aa
Classes for standard OAuth error types
jspahrsummers 57fe3b3
Use standard error types in middleware
jspahrsummers 7fb8e4a
Catch and format standard error types in responses
jspahrsummers c8f4d62
Render /authorize errors as JSON
jspahrsummers b8b816c
Bearer auth middleware
jspahrsummers 529d017
Merge branch 'main' into justin/server-auth
jspahrsummers fadcee3
Remove tests that are just testing a mock
jspahrsummers 094b81f
Throw error if scopes are requested and client has none
jspahrsummers f0777d2
Client secrets should not be removed, but expiry checked in middleware
jspahrsummers File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,144 @@ | ||
import { OAuthRegisteredClientsStore } from './clients.js'; | ||
import { OAuthClientInformationFull } from '../../shared/auth.js'; | ||
|
||
describe('OAuthRegisteredClientsStore', () => { | ||
jspahrsummers marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// Create a mock implementation class for testing | ||
class MockClientStore implements OAuthRegisteredClientsStore { | ||
private clients: Record<string, OAuthClientInformationFull> = {}; | ||
|
||
async getClient(clientId: string): Promise<OAuthClientInformationFull | undefined> { | ||
const client = this.clients[clientId]; | ||
|
||
// Return undefined for non-existent client | ||
if (!client) return undefined; | ||
|
||
// Check if client secret has expired | ||
if (client.client_secret && | ||
client.client_secret_expires_at && | ||
client.client_secret_expires_at < Math.floor(Date.now() / 1000)) { | ||
// If expired, retain client but remove the secret | ||
// eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
const { client_secret, ...clientWithoutSecret } = client; | ||
return clientWithoutSecret as OAuthClientInformationFull; | ||
} | ||
|
||
return client; | ||
} | ||
|
||
async registerClient(client: OAuthClientInformationFull): Promise<OAuthClientInformationFull> { | ||
this.clients[client.client_id] = { ...client }; | ||
return client; | ||
} | ||
} | ||
|
||
let mockStore: MockClientStore; | ||
|
||
beforeEach(() => { | ||
mockStore = new MockClientStore(); | ||
}); | ||
|
||
describe('getClient', () => { | ||
it('returns undefined for non-existent client', async () => { | ||
const result = await mockStore.getClient('non-existent-id'); | ||
expect(result).toBeUndefined(); | ||
}); | ||
|
||
it('returns client information for existing client', async () => { | ||
const mockClient: OAuthClientInformationFull = { | ||
client_id: 'test-client-123', | ||
client_secret: 'secret456', | ||
redirect_uris: ['https://example.com/callback'] | ||
}; | ||
|
||
await mockStore.registerClient(mockClient); | ||
const result = await mockStore.getClient('test-client-123'); | ||
|
||
expect(result).toEqual(mockClient); | ||
}); | ||
|
||
it('handles expired client secrets correctly', async () => { | ||
const now = Math.floor(Date.now() / 1000); | ||
|
||
// Client with expired secret (one hour in the past) | ||
const expiredClient: OAuthClientInformationFull = { | ||
client_id: 'expired-client', | ||
client_secret: 'expired-secret', | ||
client_secret_expires_at: now - 3600, | ||
redirect_uris: ['https://example.com/callback'] | ||
}; | ||
|
||
await mockStore.registerClient(expiredClient); | ||
const result = await mockStore.getClient('expired-client'); | ||
|
||
// Expect client to be returned but without the secret | ||
expect(result).toBeDefined(); | ||
expect(result!.client_id).toBe('expired-client'); | ||
expect(result!.client_secret).toBeUndefined(); | ||
}); | ||
|
||
it('keeps valid client secrets', async () => { | ||
const now = Math.floor(Date.now() / 1000); | ||
|
||
// Client with valid secret (expires one hour in the future) | ||
const validClient: OAuthClientInformationFull = { | ||
client_id: 'valid-client', | ||
client_secret: 'valid-secret', | ||
client_secret_expires_at: now + 3600, | ||
redirect_uris: ['https://example.com/callback'] | ||
}; | ||
|
||
await mockStore.registerClient(validClient); | ||
const result = await mockStore.getClient('valid-client'); | ||
|
||
// Secret should still be present | ||
expect(result?.client_secret).toBe('valid-secret'); | ||
}); | ||
}); | ||
|
||
describe('registerClient', () => { | ||
it('successfully registers a new client', async () => { | ||
const newClient: OAuthClientInformationFull = { | ||
client_id: 'new-client-id', | ||
client_secret: 'new-client-secret', | ||
redirect_uris: ['https://example.com/callback'] | ||
}; | ||
|
||
const result = await mockStore.registerClient(newClient); | ||
|
||
// Verify registration returns the client | ||
expect(result).toEqual(newClient); | ||
|
||
// Verify the client is retrievable | ||
const storedClient = await mockStore.getClient('new-client-id'); | ||
expect(storedClient).toEqual(newClient); | ||
}); | ||
|
||
it('handles clients with all metadata fields', async () => { | ||
const fullClient: OAuthClientInformationFull = { | ||
client_id: 'full-client', | ||
client_secret: 'full-secret', | ||
client_id_issued_at: Math.floor(Date.now() / 1000), | ||
client_secret_expires_at: Math.floor(Date.now() / 1000) + 86400, // 24 hours | ||
redirect_uris: ['https://example.com/callback'], | ||
token_endpoint_auth_method: 'client_secret_basic', | ||
grant_types: ['authorization_code', 'refresh_token'], | ||
response_types: ['code'], | ||
client_name: 'Test Client', | ||
client_uri: 'https://example.com', | ||
logo_uri: 'https://example.com/logo.png', | ||
scope: 'profile email', | ||
contacts: ['[email protected]'], | ||
tos_uri: 'https://example.com/tos', | ||
policy_uri: 'https://example.com/privacy', | ||
jwks_uri: 'https://example.com/jwks', | ||
software_id: 'test-software', | ||
software_version: '1.0.0' | ||
}; | ||
|
||
await mockStore.registerClient(fullClient); | ||
const result = await mockStore.getClient('full-client'); | ||
|
||
expect(result).toEqual(fullClient); | ||
}); | ||
}); | ||
}); |
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
import { OAuthClientInformationFull } from "../../shared/auth.js"; | ||
|
||
/** | ||
* Stores information about registered OAuth clients for this server. | ||
*/ | ||
export interface OAuthRegisteredClientsStore { | ||
/** | ||
* Returns information about a registered client, based on its ID. | ||
*/ | ||
getClient(clientId: string): OAuthClientInformationFull | undefined | Promise<OAuthClientInformationFull | undefined>; | ||
|
||
/** | ||
* Registers a new client with the server. The client ID and secret will be automatically generated by the library. A modified version of the client information can be returned to reflect specific values enforced by the server. | ||
* | ||
* NOTE: Implementations must ensure that client secrets, if present, are expired in accordance with the `client_secret_expires_at` field. | ||
* | ||
* If unimplemented, dynamic client registration is unsupported. | ||
*/ | ||
registerClient?(client: OAuthClientInformationFull): OAuthClientInformationFull | Promise<OAuthClientInformationFull>; | ||
} |
Oops, something went wrong.
Oops, something went wrong.
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.
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.
[non-blocking] do we want this to be optional
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.
Possibly… my main concern is really browser-based apps, but I think tree-shaking will ensure that none of Express makes it into those (it wouldn't be compatible anyway). I think it's pretty low-concern to have here, but willing to change it later if it becomes a problem.
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.
We should probably include CORS, or browser based clients won't be able to connect