-
Notifications
You must be signed in to change notification settings - Fork 586
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 19 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 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 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 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 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 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>; | ||
} |
This file contains 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,5 @@ | ||
import crypto from "node:crypto"; | ||
|
||
export function generateToken(): string { | ||
return crypto.randomBytes(32).toString("hex"); | ||
} |
This file contains 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,93 @@ | ||
import { RequestHandler } from "express"; | ||
import { z } from "zod"; | ||
import { isValidUrl } from "../validation.js"; | ||
import { OAuthServerProvider } from "../provider.js"; | ||
|
||
export type AuthorizationHandlerOptions = { | ||
provider: OAuthServerProvider; | ||
}; | ||
|
||
// Parameters that must be validated in order to issue redirects. | ||
const ClientAuthorizationParamsSchema = z.object({ | ||
client_id: z.string(), | ||
redirect_uri: z.string().optional().refine((value) => value === undefined || isValidUrl(value), { message: "redirect_uri must be a valid URL" }), | ||
}); | ||
|
||
// Parameters that must be validated for a successful authorization request. Failure can be reported to the redirect URI. | ||
const RequestAuthorizationParamsSchema = z.object({ | ||
response_type: z.literal("code"), | ||
code_challenge: z.string(), | ||
code_challenge_method: z.literal("S256"), | ||
scope: z.string().optional(), | ||
state: z.string().optional(), | ||
}); | ||
|
||
export function authorizationHandler({ provider }: AuthorizationHandlerOptions): RequestHandler { | ||
return async (req, res) => { | ||
if (req.method !== "GET" && req.method !== "POST") { | ||
res.status(405).end("Method Not Allowed"); | ||
return; | ||
} | ||
|
||
let client_id, redirect_uri; | ||
try { | ||
({ client_id, redirect_uri } = ClientAuthorizationParamsSchema.parse(req.query)); | ||
} catch (error) { | ||
res.status(400).end(`Bad Request: ${error}`); | ||
return; | ||
} | ||
|
||
const client = await provider.clientsStore.getClient(client_id); | ||
if (!client) { | ||
res.status(400).end("Bad Request: invalid client_id"); | ||
return; | ||
} | ||
|
||
if (redirect_uri !== undefined) { | ||
if (!client.redirect_uris.includes(redirect_uri)) { | ||
res.status(400).end("Bad Request: unregistered redirect_uri"); | ||
return; | ||
} | ||
} else if (client.redirect_uris.length === 1) { | ||
redirect_uri = client.redirect_uris[0]; | ||
} else { | ||
res.status(400).end("Bad Request: missing redirect_uri"); | ||
return; | ||
} | ||
|
||
let params; | ||
try { | ||
params = RequestAuthorizationParamsSchema.parse(req.query); | ||
} catch (error) { | ||
const errorUrl = new URL(redirect_uri); | ||
errorUrl.searchParams.set("error", "invalid_request"); | ||
errorUrl.searchParams.set("error_description", String(error)); | ||
res.redirect(302, errorUrl.href); | ||
return; | ||
} | ||
|
||
let requestedScopes: string[] = []; | ||
if (params.scope !== undefined && client.scope !== undefined) { | ||
requestedScopes = params.scope.split(" "); | ||
const allowedScopes = new Set(client.scope.split(" ")); | ||
|
||
// If any requested scope is not in the client's registered scopes, error out | ||
for (const scope of requestedScopes) { | ||
if (!allowedScopes.has(scope)) { | ||
const errorUrl = new URL(redirect_uri); | ||
errorUrl.searchParams.set("error", "invalid_scope"); | ||
errorUrl.searchParams.set("error_description", `Client was not registered with scope ${scope}`); | ||
res.redirect(302, errorUrl.href); | ||
return; | ||
} | ||
} | ||
} | ||
|
||
await provider.authorize(client, { | ||
state: params.state, | ||
scopes: requestedScopes, | ||
redirectUri: redirect_uri, | ||
codeChallenge: params.code_challenge, | ||
}, res); | ||
}; | ||
} |
This file contains 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,66 @@ | ||
import express, { RequestHandler } from "express"; | ||
import { OAuthClientInformationFull, OAuthClientMetadataSchema, OAuthClientRegistrationError } from "../../../shared/auth.js"; | ||
import crypto from 'node:crypto'; | ||
import cors from 'cors'; | ||
import { OAuthRegisteredClientsStore } from "../clients.js"; | ||
|
||
export type ClientRegistrationHandlerOptions = { | ||
/** | ||
* A store used to save information about dynamically registered OAuth clients. | ||
*/ | ||
clientsStore: OAuthRegisteredClientsStore; | ||
|
||
/** | ||
* The number of seconds after which to expire issued client secrets, or 0 to prevent expiration of client secrets (not recommended). | ||
* | ||
* If not set, defaults to 30 days. | ||
*/ | ||
clientSecretExpirySeconds?: number; | ||
}; | ||
|
||
const DEFAULT_CLIENT_SECRET_EXPIRY_SECONDS = 30 * 24 * 60 * 60; // 30 days | ||
|
||
export function clientRegistrationHandler({ clientsStore, clientSecretExpirySeconds = DEFAULT_CLIENT_SECRET_EXPIRY_SECONDS }: ClientRegistrationHandlerOptions): RequestHandler { | ||
if (!clientsStore.registerClient) { | ||
throw new Error("Client registration store does not support registering clients"); | ||
} | ||
|
||
// Nested router so we can configure middleware and restrict HTTP method | ||
const router = express.Router(); | ||
router.use(express.json()); | ||
|
||
// Configure CORS to allow any origin, to make accessible to web-based MCP clients | ||
router.use(cors()); | ||
|
||
router.post("/", async (req, res) => { | ||
let clientMetadata; | ||
try { | ||
clientMetadata = OAuthClientMetadataSchema.parse(req.body); | ||
} catch (error) { | ||
res.status(400).json({ | ||
error: "invalid_client_metadata", | ||
error_description: String(error), | ||
}); | ||
return; | ||
} | ||
|
||
const clientId = crypto.randomUUID(); | ||
const clientSecret = clientMetadata.token_endpoint_auth_method !== 'none' | ||
? crypto.randomBytes(32).toString('hex') | ||
: undefined; | ||
const clientIdIssuedAt = Math.floor(Date.now() / 1000); | ||
|
||
let clientInfo: OAuthClientInformationFull = { | ||
...clientMetadata, | ||
client_id: clientId, | ||
client_secret: clientSecret, | ||
client_id_issued_at: clientIdIssuedAt, | ||
client_secret_expires_at: clientSecretExpirySeconds > 0 ? clientIdIssuedAt + clientSecretExpirySeconds : 0 | ||
}; | ||
|
||
clientInfo = await clientsStore.registerClient!(clientInfo); | ||
res.status(201).json(clientInfo); | ||
}); | ||
|
||
return router; | ||
} |
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