Conversation
akulistus
added this pull request to stack #682
September 16, 2026 00:43
neSpecc
reviewed
Sep 16, 2026
| `${process.env.API_URL}/.well-known/oauth-protected-resource/integration/mcp` | ||
| }); | ||
|
|
||
| export const useMCPAuth = (app: express.Application) => { |
Member
There was a problem hiding this comment.
use named functions instead of anonymous. It's better for debugging.
Suggested change
| export const useMCPAuth = (app: express.Application) => { | |
| export function useMCPAuth(app: express.Application) { |
|
|
||
| export const useMCPAuth = (app: express.Application) => { | ||
| /** | ||
| * Dynamic client registration |
Member
There was a problem hiding this comment.
Please, provide a little more descriptive docs.
Comment on lines
+83
to
+84
| verifier: tokenVerifier, | ||
| resourceMetadataUrl: |
| }); | ||
|
|
||
| /** | ||
| * Protected resource metadata |
Member
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>(); |
Member
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) => ({ |
Comment on lines
+47
to
+61
| 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" | ||
| }); |
Member
There was a problem hiding this comment.
can we reuse generateTokensPair method here?
Contributor
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.
Comment on lines
+53
to
+55
| refresh_token: jwt.sign( | ||
| { userId, clientId, tokenUse: "refresh" }, | ||
| process.env.JWT_SECRET_ACCESS_TOKEN as Secret, |
Comment on lines
+188
to
+192
| 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) => { |
Comment on lines
+179
to
+182
| const user = jwt.verify( | ||
| loginToken, | ||
| process.env.JWT_SECRET_ACCESS_TOKEN as Secret | ||
| ) as UserJWTData; |
Comment on lines
+291
to
+294
| 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" |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
You can use modified frontend version to test mcp and authorization flow