-
Notifications
You must be signed in to change notification settings - Fork 483
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
Server implementation of MCP auth #151
Conversation
db51706
to
76f367b
Compare
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.
This looks really good - I can't wait to cook with it.
One question regarding expired servers vs servers without secrets, the rest are non-blocking qns/comments
"eventsource": "^3.0.2", | ||
"express": "^5.0.1", |
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
standardHeaders: true, | ||
legacyHeaders: false, | ||
message: new TooManyRequestsError('You have exceeded the rate limit for authorization requests').toResponseObject(), | ||
...rateLimitConfig |
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 qn] explicitly disabled by setting configuration opens here?
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.
Sorry, I don't understand the question. Can you rephrase?
|
||
// Validate scopes | ||
let requestedScopes: string[] = []; | ||
if (scope !== undefined && client.scope !== undefined) { |
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] if scopes are requested, but the client doesn't have any scopes (i.e. it's undefined), should we also throw an error? ig the client isn't concerned with scopes so it doesn't really matter what the client provides?
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.
oh also is "Invalid client_secret" correct for this error?
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.
Yes, we should throw an error. Good catch.
Where do you see "invalid client_secret?"
* | ||
* This will validate the token with the auth provider and add the resulting auth info to the request object. | ||
*/ | ||
export function requireBearerAuth({ provider, requiredScopes = [] }: BearerAuthMiddlewareOptions): RequestHandler { |
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] is there not something pre-built for this?
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.
Most of the logic is custom here. Only the header parsing bit conceivably might exist already, but it's so simple that it's probably not worth it.
/** | ||
* Verifies an access token and returns information about it. | ||
*/ | ||
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.
[non-blocking thought] is there room to do more for the server creator here? i.e. can we just have them make the "getToken"/"setToken" and we do the verification/expiry checks here?
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.
Claude thinks this could work:
interface SimplifiedOAuthProvider {
// Token storage operations
storeAuthorizationCode(userId: string, clientId: string, code: string, metadata: CodeMetadata): Promise<void>;
fetchAuthorizationCode(code: string): Promise<CodeRecord | null>;
deleteAuthorizationCode(code: string): Promise<void>;
// Token operations
storeToken(userId: string, clientId: string, tokenType: 'access'|'refresh', token: string, metadata: TokenMetadata): Promise<void>;
fetchToken(token: string, tokenType: 'access'|'refresh'): Promise<TokenRecord | null>;
deleteToken(token: string, tokenType: 'access'|'refresh'): Promise<void>;
// User operations
authenticateUser(req: Request, res: Response): Promise<string | null>; // Returns userId on success
// Optional: Permission checking
checkUserPermissions?(userId: string, clientId: string, scopes: string[]): Promise<boolean>;
}
But tbh we can add this later if needed
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.
Yeah, let's tackle this later. I'm worried that this will actually be quite complex to make fully pluggable (e.g., for the auth proxying case too).
Thanks for the review! Will tackle the follow-ups in one or more other PRs, as this one has gotten big enough as it is. |
…/justin/server-auth Server implementation of MCP auth
This implements the server side only of the MCP auth draft spec.
To do
Allow
header/authorize
endpoint to check body for parameters in POST requestsWWW-Authenticate
header when bearer token auth failsCache-Control
headerRemove(this is done at the application level)X-Powered-By
headerFollow-ups
iss
parameter, per https://www.rfc-editor.org/rfc/rfc9207.htmllocalhost
?authenticateClient
torequireAuthedClient
or similar