feat: add Streamable HTTP transport with OAuth 2.1 for remote MCP access#363
Closed
yusufaverroes wants to merge 4 commits into
Closed
feat: add Streamable HTTP transport with OAuth 2.1 for remote MCP access#363yusufaverroes wants to merge 4 commits into
yusufaverroes wants to merge 4 commits into
Conversation
- Add src/http-server.ts: Express-based HTTP server with OAuth 2.1 (authorization code + PKCE), per-session MCP server instances, pre-registered client store (no DCR), and Bearer token auth on /mcp - Refactor src/server.ts: extract createServer() factory function so HTTP transport can create per-session server instances while keeping backward-compatible singleton for stdio mode - Add "http" subcommand in src/index.ts for dynamic import - Add "start:http" npm script in package.json Usage: PUBLIC_URL=https://example.com OAUTH_CLIENT_ID=... OAUTH_CLIENT_SECRET=... node dist/index.js http Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
CodeAnt AI is reviewing your PR. Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
Contributor
📝 WalkthroughWalkthroughAdds an Express-based HTTP transport with OAuth 2.1, session-managed Streamable MCP transports and endpoints (authorize, token, /.well-known, /mcp, health), a CLI Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Express as "Express Server"
participant OAuth as "OAuth Provider"
participant SessionMgr as "Session Manager"
participant Transport as "MCP Transport"
participant MCP as "MCP Server"
Client->>Express: POST /authorize (client_id, redirect_uri)
Express->>OAuth: create authorization code
OAuth->>Express: return code
Express->>Client: redirect with code
Client->>Express: POST /token (code, client_id, client_secret)
Express->>OAuth: exchange code -> access token
OAuth->>Express: return token
Express->>Client: return bearer token
Client->>Express: POST /mcp/initialize (Bearer token)
Express->>SessionMgr: create session id & transport
SessionMgr->>Transport: instantiate StreamableHTTPServerTransport
Transport->>MCP: call createServer() to create per-session server
MCP->>Transport: send capabilities/state
Express->>Client: return session id & server info
Client->>Express: POST /mcp (streamed request)
Express->>SessionMgr: route request by session id
SessionMgr->>Transport: deliver request
Transport->>MCP: process request
MCP->>Transport: respond
Transport->>Express: stream response to Client
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
Contributor
Nitpicks 🔍
|
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| 'https://claude.ai/api/mcp/auth_callback', | ||
| 'https://claude.com/api/mcp/auth_callback', | ||
| ], | ||
| grant_types: ['authorization_code', 'refresh_token'], |
Contributor
There was a problem hiding this comment.
Suggestion: The client metadata advertises support for the "refresh_token" grant type even though the OAuth provider explicitly throws for refresh token exchanges, so clients that rely on the metadata may legitimately attempt a refresh and encounter unexpected failures or 5xx errors instead of a supported flow. [logic error]
Severity Level: Major ⚠️
- ❌ OAuth refresh_token grant fails despite advertised support.
- ⚠️ Remote MCP clients cannot refresh access tokens.
- ⚠️ Users may see unexpected token-refresh errors.
Suggested change
| grant_types: ['authorization_code', 'refresh_token'], | |
| grant_types: ['authorization_code'], |
Steps of Reproduction ✅
1. Start the HTTP MCP server via `node dist/index.js http` so that `main()` in
`src/http-server.ts:179-305` registers OAuth routes using `mcpAuthRouter(...)` (lines
~196-206).
2. As an OAuth client, fetch authorization server metadata from `GET
${PUBLIC_URL}/.well-known/oauth-authorization-server`, whose URL is logged in
`src/http-server.ts:285` and served by `mcpAuthRouter`.
3. Observe that the pre-registered client configuration created in
`PreRegisteredClientsStore` at `src/http-server.ts:57-70` includes `grant_types:
['authorization_code', 'refresh_token']`, advertising refresh token support.
4. After performing an authorization code flow (handled by
`AutoApproveOAuthProvider.exchangeAuthorizationCode()` at `src/http-server.ts:114-139`),
attempt to refresh using `POST ${PUBLIC_URL}/token` with `grant_type=refresh_token`;
`mcpAuthRouter` calls `AutoApproveOAuthProvider.exchangeRefreshToken()` at
`src/http-server.ts:141-147`, which throws `Error('Refresh tokens not supported')`,
resulting in a failed refresh despite the advertised grant type.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/http-server.ts
**Line:** 66:66
**Comment:**
*Logic Error: The client metadata advertises support for the "refresh_token" grant type even though the OAuth provider explicitly throws for refresh token exchanges, so clients that rely on the metadata may legitimately attempt a refresh and encounter unexpected failures or 5xx errors instead of a supported flow.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Comment on lines
+150
to
+162
| _scopes?: string[], | ||
| _resource?: URL, | ||
| ): Promise<OAuthTokens> { | ||
| throw new Error('Refresh tokens not supported'); | ||
| } | ||
|
|
||
| async verifyAccessToken(token: string): Promise<AuthInfo> { | ||
| const tokenData = this.tokens.get(token); | ||
| if (!tokenData || tokenData.expiresAt < Date.now()) { | ||
| throw new Error('Invalid or expired token'); | ||
| } | ||
| return { | ||
| token, |
Contributor
There was a problem hiding this comment.
Suggestion: Access tokens are stored in an in-memory Map and never removed when they expire or are rejected, causing unbounded growth of the token store over time in a long-running process and risking memory exhaustion. [resource leak]
Severity Level: Major ⚠️
- ⚠️ In-memory token Map grows with every authorization flow.
- ⚠️ Long-running HTTP MCP server risks gradual memory exhaustion.
- ⚠️ Increased GC pressure may degrade MCP request latency.
Suggested change
| _scopes?: string[], | |
| _resource?: URL, | |
| ): Promise<OAuthTokens> { | |
| throw new Error('Refresh tokens not supported'); | |
| } | |
| async verifyAccessToken(token: string): Promise<AuthInfo> { | |
| const tokenData = this.tokens.get(token); | |
| if (!tokenData || tokenData.expiresAt < Date.now()) { | |
| throw new Error('Invalid or expired token'); | |
| } | |
| return { | |
| token, | |
| async verifyAccessToken(token: string): Promise<AuthInfo> { | |
| const tokenData = this.tokens.get(token); | |
| if (!tokenData) { | |
| throw new Error('Invalid or expired token'); | |
| } | |
| if (tokenData.expiresAt < Date.now()) { | |
| this.tokens.delete(token); | |
| throw new Error('Invalid or expired token'); | |
| } | |
| return { | |
| token, | |
| clientId: tokenData.clientId, | |
| scopes: tokenData.scopes, | |
| expiresAt: Math.floor(tokenData.expiresAt / 1000), | |
| resource: tokenData.resource, | |
| }; | |
| } |
Steps of Reproduction ✅
1. Start the HTTP MCP server via `node dist/index.js http` so that `main()` in
`src/http-server.ts:179-305` initializes `AutoApproveOAuthProvider` and its in-memory
`tokens` Map (defined at `src/http-server.ts:83-87`).
2. From a client, perform the OAuth authorization code flow multiple times, causing
`AutoApproveOAuthProvider.exchangeAuthorizationCode()` at `src/http-server.ts:114-139` to
execute and call `this.tokens.set(token, {...})` for each new access token.
3. Keep the server running for hours or days; no code anywhere in the repository calls
`this.tokens.delete(...)` on these entries (the only references are `set` at line 124 and
`get` at line 151), so expired tokens remain stored indefinitely.
4. When an expired token is presented on any `/mcp` request (routes at
`src/http-server.ts:216-271` protected by `authMiddleware` created at lines 209-213),
`requireBearerAuth` invokes `provider.verifyAccessToken()` at
`src/http-server.ts:150-162`, which throws for expired tokens but never removes them from
`this.tokens`, allowing unbounded growth visible in heap snapshots or instrumentation.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/http-server.ts
**Line:** 150:162
**Comment:**
*Resource Leak: Access tokens are stored in an in-memory Map and never removed when they expire or are rejected, causing unbounded growth of the token store over time in a long-running process and risking memory exhaustion.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/server.ts (1)
91-131:⚠️ Potential issue | 🟠 Major
currentClientmodule-level state causes race conditions with concurrent HTTP sessions.Each HTTP session creates its own server instance via
createServer(), but the shared module-levelcurrentClientvariable is mutated byupdateCurrentClient()called within request handlers. When multiple clients connect concurrently, theirinitializeandcall_toolrequests race to update the global state, causing:
shouldIncludeTool()to filter tools based on the wrong client (line 124)- Client-specific onboarding logic to apply to incorrect sessions (line 188)
- Exported
currentClientaccessed by other modules to reflect stale/incorrect dataMove
currentClientfrom module-level intocreateServer()and pass it through the server instance, or store it in the session/transport context rather than as shared module state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 91 - 131, currentClient is global and races across concurrent sessions; move it into createServer() as server-scoped state and stop using the module-level currentClient, updateCurrentClient, and shouldIncludeTool to reference that scoped state. Specifically: remove the module-level let currentClient and export, create a server.currentClient object inside createServer(), turn updateCurrentClient into a method (or helper) that accepts the server/session context (still named updateCurrentClient) and uses server.currentClient to update name/version and call (global as any).mcpTransport.configureForClient via the transport tied to that server/session, and change shouldIncludeTool to accept or read the server/session-scoped currentClient rather than the module variable; update all call sites (initialize, call_tool handlers, and any modules that read currentClient) to use the server-scoped accessor or passed context instead of the old exported currentClient.
🧹 Nitpick comments (3)
src/http-server.ts (3)
248-248: Consider wrappingtransport.handleRequestin try-catch.If
transport.handleRequestthrows an unexpected error, Express's default error handler may leak stack traces or return inconsistent error responses. Adding explicit error handling improves robustness.♻️ Suggested error handling
- await transport.handleRequest(req, res, req.body); + try { + await transport.handleRequest(req, res, req.body); + } catch (err) { + console.error(`[http] Error handling request:`, err); + if (!res.headersSent) { + res.status(500).json({ error: 'Internal server error' }); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http-server.ts` at line 248, Wrap the call to transport.handleRequest(req, res, req.body) in a try-catch to prevent unhandled exceptions from bubbling to Express; inside the catch call next(err) or send a standardized 5xx JSON response using res.status(500).json(...) so errors are logged and a consistent response is returned, and ensure you log the error with your logger before responding; update the surrounding code where transport.handleRequest is invoked to perform this explicit error handling.
233-240: Consider callingtransport.close()explicitly in addition toserver.close().The
onclosehandler callsserver.close()but doesn't explicitly calltransport.close(). Based on learnings from this repository, the recommended pattern for MCP server implementations is to call bothtransport.close()andserver.close()to ensure complete isolation and proper cleanup.The current implementation may work if
transport.oncloseis triggered by the transport itself closing, but explicit closure ensures cleanup in all scenarios.♻️ Suggested change
transport.onclose = () => { const sid = transport.sessionId; if (sid) { console.log(`[http] Session closed: ${sid}`); sessions.delete(sid); } - server.close().catch(() => {}); + Promise.all([ + transport.close?.().catch(() => {}), + server.close().catch(() => {}) + ]); };Based on learnings: "In stateless MCP server implementations using the modelcontextprotocol/typescript-sdk, the correct pattern is to call both transport.close() and server.close() in the request close event handler."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http-server.ts` around lines 233 - 240, The onclose handler currently only calls server.close() and deletes the session; update the transport.onclose callback to explicitly call transport.close() before (or alongside) server.close() to ensure the transport is properly shut down; locate the transport.onclose closure that references transport.sessionId, sessions.delete(sid), and server.close() and add a safe transport.close() call (wrapped to ignore or log errors) so both transport.close() and server.close() run for complete cleanup.
62-65: Hardcoded redirect URIs limit client flexibility.The redirect URIs are hardcoded for
claude.aiandclaude.com. This is acceptable for the current pre-registered client design, but consider making these configurable via environment variables if other clients need to connect in the future.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http-server.ts` around lines 62 - 65, The redirect_uris array is hardcoded which prevents flexibility; change the code that sets redirect_uris (the redirect_uris property) to read from an environment variable (e.g. AUTH_REDIRECT_URIS or REDIRECT_URIS) and fall back to the existing two-URL list if the env var is empty; parse the env var by splitting on commas, trim entries, and validate non-empty values before assigning to redirect_uris so other clients can be supported without code changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/http-server.ts`:
- Around line 83-86: The codes and tokens Maps in AutoApproveOAuthProvider are
never pruned and will leak memory; add expiry cleanup by (1) ensuring any place
that consumes a code or token (e.g., the code-issuing and token-exchange
methods) deletes the entry after use, (2) making verifyAccessToken remove the
token from this.tokens when it finds an expired entry, and (3) adding a periodic
background cleaner (e.g., a setInterval started in the AutoApproveOAuthProvider
constructor and cleared on shutdown) that scans this.tokens and this.codes to
remove items whose expiresAt (or derived TTL for codes) is past Date.now();
reference the this.codes and this.tokens maps, the verifyAccessToken method, and
the provider constructor to implement these changes.
- Around line 114-145: The exchangeAuthorizationCode method currently ignores
the provided _codeVerifier; update it to enforce PKCE: when the retrieved
codeData.params contains a codeChallenge (and optionally codeChallengeMethod),
require a non-empty _codeVerifier and validate it by computing SHA-256 of the
provided _codeVerifier, base64url-encoding the digest, and comparing that value
to the stored codeChallenge (supporting codeChallengeMethod 'S256' only); if
they don't match (or verifier missing when a challenge exists) throw an Error
like 'Invalid PKCE code verifier'. Implement this logic inside
exchangeAuthorizationCode before deleting the code from this.codes so you
reference codeData.params.codeChallenge / codeData.params.codeChallengeMethod
and the _codeVerifier argument.
---
Outside diff comments:
In `@src/server.ts`:
- Around line 91-131: currentClient is global and races across concurrent
sessions; move it into createServer() as server-scoped state and stop using the
module-level currentClient, updateCurrentClient, and shouldIncludeTool to
reference that scoped state. Specifically: remove the module-level let
currentClient and export, create a server.currentClient object inside
createServer(), turn updateCurrentClient into a method (or helper) that accepts
the server/session context (still named updateCurrentClient) and uses
server.currentClient to update name/version and call (global as
any).mcpTransport.configureForClient via the transport tied to that
server/session, and change shouldIncludeTool to accept or read the
server/session-scoped currentClient rather than the module variable; update all
call sites (initialize, call_tool handlers, and any modules that read
currentClient) to use the server-scoped accessor or passed context instead of
the old exported currentClient.
---
Nitpick comments:
In `@src/http-server.ts`:
- Line 248: Wrap the call to transport.handleRequest(req, res, req.body) in a
try-catch to prevent unhandled exceptions from bubbling to Express; inside the
catch call next(err) or send a standardized 5xx JSON response using
res.status(500).json(...) so errors are logged and a consistent response is
returned, and ensure you log the error with your logger before responding;
update the surrounding code where transport.handleRequest is invoked to perform
this explicit error handling.
- Around line 233-240: The onclose handler currently only calls server.close()
and deletes the session; update the transport.onclose callback to explicitly
call transport.close() before (or alongside) server.close() to ensure the
transport is properly shut down; locate the transport.onclose closure that
references transport.sessionId, sessions.delete(sid), and server.close() and add
a safe transport.close() call (wrapped to ignore or log errors) so both
transport.close() and server.close() run for complete cleanup.
- Around line 62-65: The redirect_uris array is hardcoded which prevents
flexibility; change the code that sets redirect_uris (the redirect_uris
property) to read from an environment variable (e.g. AUTH_REDIRECT_URIS or
REDIRECT_URIS) and fall back to the existing two-URL list if the env var is
empty; parse the env var by splitting on commas, trim entries, and validate
non-empty values before assigning to redirect_uris so other clients can be
supported without code changes.
Comment on lines
+114
to
+145
| async exchangeAuthorizationCode( | ||
| client: OAuthClientInformationFull, | ||
| authorizationCode: string, | ||
| _codeVerifier?: string, | ||
| _redirectUri?: string, | ||
| _resource?: URL, | ||
| ): Promise<OAuthTokens> { | ||
| const codeData = this.codes.get(authorizationCode); | ||
| if (!codeData) throw new Error('Invalid authorization code'); | ||
| if (codeData.client.client_id !== client.client_id) { | ||
| throw new Error('Authorization code was not issued to this client'); | ||
| } | ||
|
|
||
| this.codes.delete(authorizationCode); | ||
|
|
||
| const token = randomUUID(); | ||
| this.tokens.set(token, { | ||
| token, | ||
| clientId: client.client_id, | ||
| scopes: codeData.params.scopes || [], | ||
| expiresAt: Date.now() + 3600000, // 1 hour | ||
| resource: codeData.params.resource, | ||
| }); | ||
|
|
||
| console.log(`[oauth] Issued access token for client ${client.client_id}`); | ||
| return { | ||
| access_token: token, | ||
| token_type: 'bearer', | ||
| expires_in: 3600, | ||
| scope: (codeData.params.scopes || []).join(' '), | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
PKCE code verifier is not validated.
The exchangeAuthorizationCode method accepts _codeVerifier but doesn't verify it against the stored codeChallenge. PKCE (Proof Key for Code Exchange) requires validating that SHA256(codeVerifier) === codeChallenge to prevent authorization code interception attacks.
🔒 Proposed fix to validate PKCE code verifier
+import { createHash } from 'node:crypto';
+
+function verifyPkce(codeVerifier: string, codeChallenge: string): boolean {
+ const hash = createHash('sha256').update(codeVerifier).digest('base64url');
+ return hash === codeChallenge;
+}
+
async exchangeAuthorizationCode(
client: OAuthClientInformationFull,
authorizationCode: string,
- _codeVerifier?: string,
+ codeVerifier?: string,
_redirectUri?: string,
_resource?: URL,
): Promise<OAuthTokens> {
const codeData = this.codes.get(authorizationCode);
if (!codeData) throw new Error('Invalid authorization code');
if (codeData.client.client_id !== client.client_id) {
throw new Error('Authorization code was not issued to this client');
}
+ if (codeData.params.codeChallenge && codeVerifier) {
+ if (!verifyPkce(codeVerifier, codeData.params.codeChallenge)) {
+ throw new Error('Invalid code verifier');
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/http-server.ts` around lines 114 - 145, The exchangeAuthorizationCode
method currently ignores the provided _codeVerifier; update it to enforce PKCE:
when the retrieved codeData.params contains a codeChallenge (and optionally
codeChallengeMethod), require a non-empty _codeVerifier and validate it by
computing SHA-256 of the provided _codeVerifier, base64url-encoding the digest,
and comparing that value to the stored codeChallenge (supporting
codeChallengeMethod 'S256' only); if they don't match (or verifier missing when
a challenge exists) throw an Error like 'Invalid PKCE code verifier'. Implement
this logic inside exchangeAuthorizationCode before deleting the code from
this.codes so you reference codeData.params.codeChallenge /
codeData.params.codeChallengeMethod and the _codeVerifier argument.
Owner
|
Hey hey @yusufaverroes |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/server.ts (2)
92-133:⚠️ Potential issue | 🟠 MajorPer-session isolation is broken by module-level
currentClient.Line 93 stores client identity globally, but
createServer()is now per-session. This lets one session overwrite another session’s client context (tool filtering, onboarding behavior, telemetry metadata).🔧 Proposed fix (instance-scoped client state)
-// Store current client info (simple variable) - module-level for external access -let currentClient = { name: 'uninitialized', version: 'uninitialized' }; - -async function updateCurrentClient(clientInfo: { name?: string, version?: string }) { - ... -} - -function shouldIncludeTool(toolName: string): boolean { - ... -} - -export { currentClient }; - export function createServer(): Server { + let currentClient = { name: 'uninitialized', version: 'uninitialized' }; + + async function updateCurrentClient(clientInfo: { name?: string, version?: string }) { + ... + } + + function shouldIncludeTool(toolName: string): boolean { + ... + } + const server = new Server(...)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 92 - 133, The module-level currentClient breaks per-session isolation; make client state instance-scoped by moving currentClient (and the functions updateCurrentClient and shouldIncludeTool) off the module top-level into the per-session createServer context (or attach them to the server/session instance), update any callers to use the instance-bound methods rather than the exported module values, and stop exporting the module-level currentClient; also ensure the transport lookup uses the session's transport (not (global as any).mcpTransport) so configureForClient is invoked on the session-specific transport.
138-1553:⚠️ Potential issue | 🔴 CriticalAdd
transport.close()to thetransport.onclosehandler for proper session isolation.The current teardown in
http-server.ts:233–240only callsserver.close()but should call bothtransport.close()andserver.close()in thetransport.onclosehandler. This is the recommended MCP TypeScript SDK pattern for per-session servers to ensure complete resource cleanup and prevent request ID collisions in concurrent scenarios.Update the handler at line 233:
transport.onclose = () => { const sid = transport.sessionId; if (sid) { console.log(`[http] Session closed: ${sid}`); sessions.delete(sid); } transport.close().catch(() => {}); server.close().catch(() => {}); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` around lines 138 - 1553, The transport.onclose handler currently only calls server.close() and removes the session but doesn't call transport.close(), which can leave per-session resources open and cause request ID collisions; update the transport.onclose handler (the transport.onclose callback where sessionId is read and sessions.delete is called) to also call transport.close() (and still call server.close()), handling promise rejections (e.g., .catch(() => {})) so both transport and server are closed on session teardown.
🧹 Nitpick comments (1)
src/server.ts (1)
66-66: Useimport typeforServerResultat Line 66.
ServerResultis only used as a type annotation (Lines 1167 and 1211). Withmodule: "Node16"in tsconfig, using a type-only import aligns with TypeScript best practices and prevents unnecessary runtime imports.Suggested change
-import { ServerResult } from './types.js'; +import type { ServerResult } from './types.js';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/server.ts` at line 66, Change the regular import of ServerResult to a type-only import since ServerResult is only used for type annotations; locate the top-level import statement that currently says "import { ServerResult } from './types.js'" and replace it with a type-only import (using the "import type" syntax) so TypeScript with module: "Node16" won't emit a runtime import for ServerResult; keep the same symbol name "ServerResult" so references at its usage sites (lines where ServerResult is used as a type) remain unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/server.ts`:
- Around line 92-133: The module-level currentClient breaks per-session
isolation; make client state instance-scoped by moving currentClient (and the
functions updateCurrentClient and shouldIncludeTool) off the module top-level
into the per-session createServer context (or attach them to the server/session
instance), update any callers to use the instance-bound methods rather than the
exported module values, and stop exporting the module-level currentClient; also
ensure the transport lookup uses the session's transport (not (global as
any).mcpTransport) so configureForClient is invoked on the session-specific
transport.
- Around line 138-1553: The transport.onclose handler currently only calls
server.close() and removes the session but doesn't call transport.close(), which
can leave per-session resources open and cause request ID collisions; update the
transport.onclose handler (the transport.onclose callback where sessionId is
read and sessions.delete is called) to also call transport.close() (and still
call server.close()), handling promise rejections (e.g., .catch(() => {})) so
both transport and server are closed on session teardown.
---
Nitpick comments:
In `@src/server.ts`:
- Line 66: Change the regular import of ServerResult to a type-only import since
ServerResult is only used for type annotations; locate the top-level import
statement that currently says "import { ServerResult } from './types.js'" and
replace it with a type-only import (using the "import type" syntax) so
TypeScript with module: "Node16" won't emit a runtime import for ServerResult;
keep the same symbol name "ServerResult" so references at its usage sites (lines
where ServerResult is used as a type) remain unchanged.
Author
Hi thanks, sure you can email me yusuf.averoes@gmail.com regards, |
- Move currentClient state inside createServer() for per-session isolation - Add transport.close() to onclose handler to prevent resource leaks - Use import type for ServerResult (type-only import) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
|
CodeAnt AI is running Incremental review Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
Comment on lines
+136
to
+148
| }); | ||
|
|
||
| console.log(`[oauth] Issued access token for client ${client.client_id}`); | ||
| return { | ||
| access_token: token, | ||
| token_type: 'bearer', | ||
| expires_in: 3600, | ||
| scope: (codeData.params.scopes || []).join(' '), | ||
| }; | ||
| } | ||
|
|
||
| async exchangeRefreshToken( | ||
| _client: OAuthClientInformationFull, |
Contributor
There was a problem hiding this comment.
Suggestion: Access tokens are stored indefinitely in the in-memory tokens map and are never removed when expired, causing unbounded growth over time and a memory leak under repeated authentications. [resource leak]
Severity Level: Major ⚠️
- ⚠️ OAuth provider memory usage grows with each token issued.
- ⚠️ Long-lived servers may accumulate many expired token entries.
Suggested change
| }); | |
| console.log(`[oauth] Issued access token for client ${client.client_id}`); | |
| return { | |
| access_token: token, | |
| token_type: 'bearer', | |
| expires_in: 3600, | |
| scope: (codeData.params.scopes || []).join(' '), | |
| }; | |
| } | |
| async exchangeRefreshToken( | |
| _client: OAuthClientInformationFull, | |
| async verifyAccessToken(token: string): Promise<AuthInfo> { | |
| const tokenData = this.tokens.get(token); | |
| if (!tokenData) { | |
| throw new Error('Invalid or expired token'); | |
| } | |
| if (tokenData.expiresAt < Date.now()) { | |
| this.tokens.delete(token); | |
| throw new Error('Invalid or expired token'); | |
| } | |
| return { | |
| token, | |
| clientId: tokenData.clientId, | |
| scopes: tokenData.scopes, | |
| expiresAt: Math.floor(tokenData.expiresAt / 1000), | |
| resource: tokenData.resource, | |
| }; | |
| } |
Steps of Reproduction ✅
1. Start the HTTP MCP server via `main()` in `src/http-server.ts:157-167`; this creates an
`AutoApproveOAuthProvider` instance at line 167 with an internal `tokens` map defined at
line 77.
2. Complete the OAuth authorization code flow at least once via the `/authorize` and
`/token` routes wired by `mcpAuthRouter` at `src/http-server.ts:173-179`; the `/token`
handler calls `AutoApproveOAuthProvider.exchangeAuthorizationCode()` at lines 99-127.
3. Inside `exchangeAuthorizationCode()`, a new access token is generated and stored in
`this.tokens` at `src/http-server.ts:112-119`, increasing `this.tokens.size` by one for
each completed flow.
4. Repeat the auth flow many times over the lifetime of the process; expired tokens (older
than one hour per `expiresAt` set at line 117) are never removed anywhere in the file,
since `verifyAccessToken()` at `src/http-server.ts:136-148` only checks `expiresAt <
Date.now()` and throws but does not delete entries. Observing `this.tokens.size` via
instrumentation or a debugger shows it monotonically increasing, indicating unbounded
in-memory growth tied to the number of issued tokens.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/http-server.ts
**Line:** 136:148
**Comment:**
*Resource Leak: Access tokens are stored indefinitely in the in-memory `tokens` map and are never removed when expired, causing unbounded growth over time and a memory leak under repeated authentications.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Contributor
|
CodeAnt AI Incremental review completed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/http-server.ts (1)
85-86:⚠️ Potential issue | 🟠 MajorAuthorization codes/tokens are retained indefinitely in memory.
Codes have no TTL, and expired tokens are never pruned from
this.tokens. Over long runtimes this can cause unbounded growth.🧹 Suggested pruning baseline
- private codes = new Map<string, { client: OAuthClientInformationFull; params: AuthorizationParams }>(); + private codes = new Map<string, { client: OAuthClientInformationFull; params: AuthorizationParams; expiresAt: number }>(); async authorize(client: OAuthClientInformationFull, params: AuthorizationParams, res: Response): Promise<void> { const code = randomUUID(); - this.codes.set(code, { client, params }); + this.codes.set(code, { client, params, expiresAt: Date.now() + 5 * 60_000 }); } async exchangeAuthorizationCode(...) { const codeData = this.codes.get(authorizationCode); - if (!codeData) throw new Error('Invalid authorization code'); + if (!codeData || codeData.expiresAt < Date.now()) { + this.codes.delete(authorizationCode); + throw new Error('Invalid or expired authorization code'); + } ... } async verifyAccessToken(token: string): Promise<AuthInfo> { const tokenData = this.tokens.get(token); - if (!tokenData || tokenData.expiresAt < Date.now()) { + if (!tokenData || tokenData.expiresAt < Date.now()) { + this.tokens.delete(token); throw new Error('Invalid or expired token'); }Also applies to: 92-95, 121-128, 156-159
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http-server.ts` around lines 85 - 86, The in-memory Maps codes and tokens retain entries forever causing unbounded growth; add expiration handling by (1) storing a TTL/expiry timestamp with each authorization code entry (similar to tokens' expiresAt) and enforcing removal when reading/validating, (2) updating any creators (where codes or tokens are inserted) to set an expiresAt for codes and ensure tokens have correct expiresAt, and (3) adding a periodic cleanup routine (e.g., pruneExpiredEntries) that iterates this.codes and this.tokens and deletes entries whose expiresAt < Date.now(), invoked on server start and/or via setInterval; reference the properties this.codes, this.tokens, token.expiresAt and the code validation/lookup functions to locate where to check-and-delete on access and where to wire the background prune.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/http-server.ts`:
- Around line 66-67: The grant_types advertised in the OAuth metadata includes
'refresh_token' but the token exchange path never issues refresh tokens and the
token endpoint rejects refresh grant requests, causing inconsistent behavior;
update the grant_types array to only list supported grants (remove
'refresh_token') or implement refresh support consistently by (a) issuing a
refresh token during the authorization code exchange (the function/path that
currently issues tokens after code exchange) and (b) accepting and handling the
'refresh_token' grant in the token endpoint handler (the function that currently
rejects refresh requests). Ensure the same symbols are changed: the grant_types
declaration and the token exchange and token endpoint handlers so advertised
capabilities match actual behavior.
In `@src/server.ts`:
- Around line 92-97: The module-level mutable currentClient is being overwritten
per HTTP request (written from sessionClient) and leaks cross-session state;
stop mutating the exported currentClient in HTTP/async paths (only keep the
stdio/synchronous behavior for backward-compatibility) and instead thread the
client context explicitly through callsites (pass the sessionClient object into
functions in capture.ts, config.ts, usageTracker.ts and any handlers that
currently read currentClient); as an immediate mitigation, guard the assignment
to currentClient so it only happens in the stdio sync path and remove any writes
to currentClient from HTTP request handling, and then refactor helpers and
consumers to accept a client parameter (or use a request-scoped context) so no
per-session state is stored in the module global currentClient.
---
Duplicate comments:
In `@src/http-server.ts`:
- Around line 85-86: The in-memory Maps codes and tokens retain entries forever
causing unbounded growth; add expiration handling by (1) storing a TTL/expiry
timestamp with each authorization code entry (similar to tokens' expiresAt) and
enforcing removal when reading/validating, (2) updating any creators (where
codes or tokens are inserted) to set an expiresAt for codes and ensure tokens
have correct expiresAt, and (3) adding a periodic cleanup routine (e.g.,
pruneExpiredEntries) that iterates this.codes and this.tokens and deletes
entries whose expiresAt < Date.now(), invoked on server start and/or via
setInterval; reference the properties this.codes, this.tokens, token.expiresAt
and the code validation/lookup functions to locate where to check-and-delete on
access and where to wire the background prune.
Comment on lines
+92
to
+97
| // Module-level currentClient kept for backward compatibility with stdio mode | ||
| // (used by capture.ts, config.ts, usageTracker.ts) | ||
| let currentClient = { name: 'uninitialized', version: 'uninitialized' }; | ||
|
|
||
| // Export current client info for access by other modules | ||
| export { currentClient }; |
Contributor
There was a problem hiding this comment.
Per-session client state is still leaking through shared module state.
Line 117 writes each session’s sessionClient into module-level currentClient. In HTTP mode, sessions can overwrite each other and downstream readers of currentClient can observe the wrong client context.
🛡️ Immediate mitigation (stdio-only sync)
- currentClient = sessionClient;
+ // Keep global client only for stdio/single-transport mode
+ if ((global as any).mcpTransport) {
+ currentClient = sessionClient;
+ }Longer-term, pass client context explicitly through handlers/utilities instead of relying on mutable module-global state.
Also applies to: 104-118
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/server.ts` around lines 92 - 97, The module-level mutable currentClient
is being overwritten per HTTP request (written from sessionClient) and leaks
cross-session state; stop mutating the exported currentClient in HTTP/async
paths (only keep the stdio/synchronous behavior for backward-compatibility) and
instead thread the client context explicitly through callsites (pass the
sessionClient object into functions in capture.ts, config.ts, usageTracker.ts
and any handlers that currently read currentClient); as an immediate mitigation,
guard the assignment to currentClient so it only happens in the stdio sync path
and remove any writes to currentClient from HTTP request handling, and then
refactor helpers and consumers to accept a client parameter (or use a
request-scoped context) so no per-session state is stored in the module global
currentClient.
- Remove 'refresh_token' from advertised grant_types since exchangeRefreshToken() is not implemented - Purge expired tokens on each verifyAccessToken() call to prevent unbounded memory growth in long-running processes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/http-server.ts (1)
92-95:⚠️ Potential issue | 🟠 MajorAdd TTL enforcement for authorization codes.
this.codesentries created at Line 94 never expire, so stale codes can accumulate and remain redeemable indefinitely until used (Line 121). This is both a security window and a memory-growth risk.🔧 Suggested fix
class AutoApproveOAuthProvider implements OAuthServerProvider { readonly clientsStore: PreRegisteredClientsStore; - private codes = new Map<string, { client: OAuthClientInformationFull; params: AuthorizationParams }>(); + private codes = new Map<string, { + client: OAuthClientInformationFull; + params: AuthorizationParams; + expiresAt: number; + }>(); private tokens = new Map<string, { token: string; clientId: string; scopes: string[]; expiresAt: number; resource?: URL }>(); @@ async authorize(client: OAuthClientInformationFull, params: AuthorizationParams, res: Response): Promise<void> { const code = randomUUID(); - this.codes.set(code, { client, params }); + this.codes.set(code, { client, params, expiresAt: Date.now() + 5 * 60 * 1000 }); @@ async challengeForAuthorizationCode(_client: OAuthClientInformationFull, authorizationCode: string): Promise<string> { const codeData = this.codes.get(authorizationCode); - if (!codeData) throw new Error('Invalid authorization code'); + if (!codeData || codeData.expiresAt < Date.now()) { + this.codes.delete(authorizationCode); + throw new Error('Invalid or expired authorization code'); + } return codeData.params.codeChallenge; } @@ ): Promise<OAuthTokens> { const codeData = this.codes.get(authorizationCode); - if (!codeData) throw new Error('Invalid authorization code'); + if (!codeData || codeData.expiresAt < Date.now()) { + this.codes.delete(authorizationCode); + throw new Error('Invalid or expired authorization code'); + } if (codeData.client.client_id !== client.client_id) { throw new Error('Authorization code was not issued to this client'); }Also applies to: 108-112, 121-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http-server.ts` around lines 92 - 95, The authorize flow stores authorization codes in this.codes without expiry; modify the authorize method to store a timestamp (e.g., createdAt) alongside { client, params } and set a TTL constant (e.g., AUTH_CODE_TTL_MS). Update the code redemption path (the function that reads and consumes codes) to check the timestamp and reject/delete expired codes before redeeming, and optionally add a periodic cleanup to prune expired entries from this.codes to prevent memory growth; reference the authorize method, the this.codes Map, and the code-redemption handler when implementing these checks.
🧹 Nitpick comments (1)
src/http-server.ts (1)
39-41: ValidatePORTbefore server startup.If
PORTis non-numeric,parseIntyieldsNaNand failure happens later inlisten(). Fail fast with a clear error.🛠️ Suggested hardening
-const PORT = parseInt(process.env.PORT || '3100', 10); +const rawPort = process.env.PORT ?? '3100'; +const PORT = Number.parseInt(rawPort, 10); +if (!Number.isInteger(PORT) || PORT < 1 || PORT > 65535) { + console.error(`[http] Invalid PORT value: "${rawPort}"`); + process.exit(1); +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/http-server.ts` around lines 39 - 41, Validate the PORT environment value immediately after computing PORT (the PORT constant) and fail fast with a clear error if it's not a valid integer port (e.g., NaN or out of 1–65535 range) before using it in server.listen(); update PUBLIC_URL construction to only use PORT when validated; raise or log a descriptive error and exit (or throw) so startup never proceeds with an invalid PORT value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/http-server.ts`:
- Around line 211-219: The bearer auth is advertising mcp:tools via
resourceName/metadata but auth middleware uses requiredScopes: [] so tokens with
no scope pass; update the requireBearerAuth call (authMiddleware) to enforce the
advertised scope by setting requiredScopes: ['mcp:tools'] (or derive it from the
resource metadata/resourceName) while keeping verifier: provider and
resourceMetadataUrl: getOAuthProtectedResourceMetadataUrl(mcpServerUrl)
unchanged so /mcp actually requires the mcp:tools scope.
---
Duplicate comments:
In `@src/http-server.ts`:
- Around line 92-95: The authorize flow stores authorization codes in this.codes
without expiry; modify the authorize method to store a timestamp (e.g.,
createdAt) alongside { client, params } and set a TTL constant (e.g.,
AUTH_CODE_TTL_MS). Update the code redemption path (the function that reads and
consumes codes) to check the timestamp and reject/delete expired codes before
redeeming, and optionally add a periodic cleanup to prune expired entries from
this.codes to prevent memory growth; reference the authorize method, the
this.codes Map, and the code-redemption handler when implementing these checks.
---
Nitpick comments:
In `@src/http-server.ts`:
- Around line 39-41: Validate the PORT environment value immediately after
computing PORT (the PORT constant) and fail fast with a clear error if it's not
a valid integer port (e.g., NaN or out of 1–65535 range) before using it in
server.listen(); update PUBLIC_URL construction to only use PORT when validated;
raise or log a descriptive error and exit (or throw) so startup never proceeds
with an invalid PORT value.
Comment on lines
+211
to
+219
| scopesSupported: ['mcp:tools'], | ||
| resourceName: 'DesktopCommanderMCP', | ||
| })); | ||
|
|
||
| // ---------- Bearer auth middleware for /mcp ---------- | ||
| const authMiddleware = requireBearerAuth({ | ||
| verifier: provider, | ||
| requiredScopes: [], | ||
| resourceMetadataUrl: getOAuthProtectedResourceMetadataUrl(mcpServerUrl), |
Contributor
There was a problem hiding this comment.
Enforce the advertised scope on /mcp bearer auth.
Line 211 advertises mcp:tools, but Line 218 accepts any token (requiredScopes: []). This weakens authorization checks.
🔒 Suggested fix
const authMiddleware = requireBearerAuth({
verifier: provider,
- requiredScopes: [],
+ requiredScopes: ['mcp:tools'],
resourceMetadataUrl: getOAuthProtectedResourceMetadataUrl(mcpServerUrl),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| scopesSupported: ['mcp:tools'], | |
| resourceName: 'DesktopCommanderMCP', | |
| })); | |
| // ---------- Bearer auth middleware for /mcp ---------- | |
| const authMiddleware = requireBearerAuth({ | |
| verifier: provider, | |
| requiredScopes: [], | |
| resourceMetadataUrl: getOAuthProtectedResourceMetadataUrl(mcpServerUrl), | |
| scopesSupported: ['mcp:tools'], | |
| resourceName: 'DesktopCommanderMCP', | |
| })); | |
| // ---------- Bearer auth middleware for /mcp ---------- | |
| const authMiddleware = requireBearerAuth({ | |
| verifier: provider, | |
| requiredScopes: ['mcp:tools'], | |
| resourceMetadataUrl: getOAuthProtectedResourceMetadataUrl(mcpServerUrl), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/http-server.ts` around lines 211 - 219, The bearer auth is advertising
mcp:tools via resourceName/metadata but auth middleware uses requiredScopes: []
so tokens with no scope pass; update the requireBearerAuth call (authMiddleware)
to enforce the advertised scope by setting requiredScopes: ['mcp:tools'] (or
derive it from the resource metadata/resourceName) while keeping verifier:
provider and resourceMetadataUrl:
getOAuthProtectedResourceMetadataUrl(mcpServerUrl) unchanged so /mcp actually
requires the mcp:tools scope.
Owner
|
After discussion closed in favor of https://mcp.desktopcommander.app/ But https://mcp.desktopcommander.app/ requires some fixes too. |
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
2 participants
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.
User description
Usage: PUBLIC_URL=https://example.com OAUTH_CLIENT_ID=... OAUTH_CLIENT_SECRET=... node dist/index.js http
Summary by CodeRabbit
New Features
Refactor
CodeAnt-AI Description
Add HTTP MCP transport with OAuth 2.1 and per-session isolation
What Changed
Impact
✅ Remote MCP access over HTTP✅ OAuth-protected MCP connections✅ Per-session isolation prevents session cross-talk💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.