Conversation
| `${process.env.API_URL}/.well-known/oauth-protected-resource/integration/mcp` | ||
| }); | ||
|
|
||
| export const useMCPAuth = (app: express.Application) => { |
There was a problem hiding this comment.
use named functions instead of anonymous. It's better for debugging.
| export const useMCPAuth = (app: express.Application) => { | |
| export function useMCPAuth(app: express.Application) { |
|
|
||
| export const useMCPAuth = (app: express.Application) => { | ||
| /** | ||
| * Dynamic client registration |
There was a problem hiding this comment.
Please, provide a little more descriptive docs.
| verifier: tokenVerifier, | ||
| resourceMetadataUrl: |
| }); | ||
|
|
||
| /** | ||
| * Protected resource metadata |
There was a problem hiding this comment.
same here. Explain why this method is needed and what is does.
| ); | ||
|
|
||
| /** | ||
| * Frontend callback |
| expiresAt: number; | ||
| }; | ||
|
|
||
| const authCodes = new Map<string, AuthCodeData>(); |
There was a problem hiding this comment.
this maps will be shared across all users, is it ok? Also, add docs please
| }); | ||
|
|
||
| /** | ||
| * MCP client callback |
| return payload as TokenData; | ||
| }; | ||
|
|
||
| const createTokenResponse = (userId: string, clientId: string) => ({ |
| const createTokenResponse = (userId: string, clientId: string) => ({ | ||
| access_token: jwt.sign( | ||
| { userId, clientId, tokenUse: "access" }, | ||
| process.env.JWT_SECRET_ACCESS_TOKEN as Secret, | ||
| { expiresIn: accessTokenLifetimeSeconds } | ||
| ), | ||
| refresh_token: jwt.sign( | ||
| { userId, clientId, tokenUse: "refresh" }, | ||
| process.env.JWT_SECRET_ACCESS_TOKEN as Secret, | ||
| { expiresIn: "30d" } | ||
| ), | ||
| token_type: "Bearer", | ||
| expires_in: accessTokenLifetimeSeconds, | ||
| scope: "mcp:tools mcp:resources" | ||
| }); |
There was a problem hiding this comment.
can we reuse generateTokensPair method here?
There was a problem hiding this comment.
🟡 Changes recommended
Token isolation, redirect validation, CORS, and distributed authorization-code storage must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds OAuth authorization and bearer-token protection to the MCP integration.
Changes:
- Adds OAuth discovery, registration, consent, token, PKCE, and refresh flows.
- Protects MCP routes and adds an authenticated user-ID tool.
- Adds the MCP Express dependency.
File summaries
| File | Description |
|---|---|
src/integrations/mcp/auth.ts |
Implements MCP OAuth flow. |
src/integrations/mcp/index.ts |
Applies authentication middleware. |
src/integrations/mcp/mcp.ts |
Adds an authenticated test tool. |
package.json |
Adds MCP Express dependency. |
yarn.lock |
Locks the new dependency. |
Review details
- Files reviewed: 4/5 changed files
- Comments generated: 9
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| refresh_token: jwt.sign( | ||
| { userId, clientId, tokenUse: "refresh" }, | ||
| process.env.JWT_SECRET_ACCESS_TOKEN as Secret, |
| authCodes.set(code, { | ||
| userId: user.userId, | ||
| clientId: client_id, | ||
| redirectUri: redirect_uri, | ||
| codeChallenge: code_challenge, |
| expiresAt: number; | ||
| }; | ||
|
|
||
| const authCodes = new Map<string, AuthCodeData>(); |
| { expiresIn: "30d" } | ||
| ), | ||
| token_type: "Bearer", | ||
| expires_in: accessTokenLifetimeSeconds, |
| /** | ||
| * Frontend callback | ||
| */ | ||
| app.post("/concent/integration/mcp", (req, res) => { |
| const user = jwt.verify( | ||
| loginToken, | ||
| process.env.JWT_SECRET_ACCESS_TOKEN as Secret | ||
| ) as UserJWTData; |
| const calculatedChallenge = crypto | ||
| .createHash("sha256") | ||
| .update(code_verifier) | ||
| .digest("base64url"); |
| `${process.env.API_URL}/.well-known/oauth-protected-resource/integration/mcp` | ||
| }); | ||
|
|
||
| export const useMCPAuth = (app: express.Application) => { |
| server.registerTool( | ||
| "print_userId", | ||
| { | ||
| description: "A test tool that pritn userId from auth token" |
| /** | ||
| * MCP | ||
| */ | ||
| app.use("/integration/mcp", authMiddleware, router); |
There was a problem hiding this comment.
Under WHATWG Fetch §3.2.1, browser CORS preflights (OPTIONS) do not carry credentials (Authorization). Because OPTIONS currently cascades into authMiddleware (requireBearerAuth), it responds with 401 Unauthorized per RFC 6750 §3.1, causing browsers to abort cross-origin MCP requests.
Consider either:
- intercepting
OPTIONSbeforeauthMiddlewareto respond with204to keep changes local to this route - terminating
OPTIONSpreflights with204and addingAuthorizationtoAccess-Control-Allow-Headers
For discovery endpoints (/.well-known/oauth-*), RFC 8414 §3 also recommends Access-Control-Allow-Origin: * for external clients.
| }); | ||
| } | ||
|
|
||
| const auth = authCodes.get(code); |
There was a problem hiding this comment.
Per RFC 6749 §10.5 and §4.1.2, authorization codes should be single-use. Currently authCodes.delete(code) is only reached after PKCE verification succeeds, so on parameter or PKCE failure the code remains active for 5 minutes.
Consider consuming/invalidating the code upon initial lookup (or ensuring deletion in all error branches) to prevent replay or brute-force verifier probing against an intercepted code.
| description: "A test tool that pritn userId from auth token" | ||
| }, | ||
| async (ctx: ServerContext) => { | ||
| const token = ctx.http?.req?.headers.get("authorization")?.slice(7)!; |
There was a problem hiding this comment.
Accessing ctx.http?.req?.headers.get("authorization")?.slice(7)! directly and passing it to jwt.decode can throw an unhandled TypeError if the header is missing, malformed, or formatted differently, and reads unverified claims instead of the token already verified by requireBearerAuth.
You can use modified frontend version to test mcp and authorization flow