Skip to content

Conversation

roh26it
Copy link
Collaborator

@roh26it roh26it commented Sep 15, 2025

roh26it added 30 commits August 17, 2025 16:22
Implements a comprehensive MCP (Model Context Protocol) Gateway that provides:

- **Multi-transport Gateway**: Bridges clients and upstream MCP servers with automatic transport detection and translation (Streamable HTTP ↔ SSE)
- **Session Persistence**: JSON-based session storage with automatic recovery across server restarts
- **State Management**: Explicit session states (NEW, INITIALIZING, ACTIVE, DORMANT, CLOSED) with proper lifecycle handling
- **Transport Translation**: Seamless conversion between different transport protocols for maximum compatibility
- **Tool Management**: Configurable tool filtering with allowlists, blocklists, and rate limiting
- **Production Logging**: Environment-aware logging system (minimal in production, verbose in development)
- **Graceful Shutdown**: Proper cleanup and session saving on SIGINT/SIGTERM

Core components:
- `src/mcp-index.ts`: Main gateway HTTP server with Hono framework
- `src/services/mcpSession.ts`: Session management with transport bridging
- `src/services/sessionStore.ts`: Persistent storage with Redis migration path
- `src/start-mcp.ts`: Server startup script with loading animation
- `src/utils/logger.ts`: Configurable logging system
- `src/types/mcp.ts`: TypeScript definitions for MCP gateway

Features:
- Automatic upstream transport detection (Streamable HTTP → SSE fallback)
- Client session restoration after server restarts
- Rate limiting and tool access policies
- Health check endpoint with session statistics
- Redis-ready architecture for horizontal scaling

Dependencies:
- Added @modelcontextprotocol/sdk for MCP protocol support
- Added data/sessions.json to gitignore for persistent session storage
Copy link
Contributor

matter-code-review bot commented Sep 15, 2025

Code Quality bug fix

Description

Summary By MatterAI MatterAI logo

🔄 What Changed

This Pull Request fixes a minor bug in the createWWWAuthenticateHeader function within src/mcp/middleware/oauth/index.ts. Specifically, a missing closing double quote " was added to the resource_metadata value in the generated WWW-Authenticate header string.

🔍 Impact of the Change

The change ensures that the WWW-Authenticate header is correctly formatted according to RFC 9728. Previously, the malformed header could lead to issues in clients attempting to parse the resource_metadata parameter, potentially causing authentication failures or incorrect resource discovery. The fix guarantees proper OAuth challenge handling.

📁 Total Files Changed

  • src/mcp/middleware/oauth/index.ts: Fixed a missing closing double quote in the WWW-Authenticate header string.

🧪 Test Added

N/A

🔒Security Vulnerabilities

N/A (This change is a bug fix that improves the correctness of a security-related header, it does not introduce new vulnerabilities.)

Motivation

This PR addresses a bug where the WWW-Authenticate header was malformed due to a missing closing double quote, ensuring proper OAuth challenge responses.

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update
  • Refactoring (no functional changes)

How Has This Been Tested?

  • Unit Tests
  • Integration Tests
  • Manual Testing

Screenshots (if applicable)

N/A

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Related Issues

N/A

Caution

Package Vulnerabilities

Package Version Severity CVE Fix Version Vulnerability
@hono/node-server ^1.3.3 HIGH CVE-2024-32652 1.10.1 @hono/node-server has Denial
of Service
risk when
receiving Host
header that
cannot be
parsed
@hono/node-server ^1.3.3 MODERATE CVE-2024-23340 1.4.1 @hono/node-server cannot handle
"double dots"
in URL
hono ^4.6.10 MODERATE CVE-2025-59139 4.9.7 Hono has Body
Limit Middleware
Bypass
rollup ^4.9.1 HIGH CVE-2024-47068 4.22.4 DOM Clobbering Gadget
found in
rollup bundled
scripts that
leads to
XSS

Tanka Poem ♫

Quote now in its place,
Header shines, a perfect form.
Auth flows, smooth and clear,
No more parsing woes appear,
Tiny fix, great impact. ✨

Sequence Diagram

sequenceDiagram
    participant OAuthMiddleware
    participant HeaderUtil as createWWWAuthenticateHeader(baseUrl, path)
    
    OAuthMiddleware->>HeaderUtil: Call with baseUrl, path
    Note over HeaderUtil: Fix: Add missing closing quote to resource_metadata
    HeaderUtil-->>OAuthMiddleware: Returns corrected WWW-Authenticate header
Loading

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pull request introduces a new MCP Gateway with significant changes across multiple files. Key areas include OAuth implementation, session management, and cache backends. The changes are extensive and require careful review to ensure correctness and security.

Skipped files
  • data/servers.example.json: Skipped file pattern
  • docs/logging-config.md: Skipped file pattern
  • docs/security/session-hijacking-mitigation.md: Skipped file pattern
  • docs/security/token-passthrough-prevention.md: Skipped file pattern
  • docs/session-persistence.md: Skipped file pattern
  • package-lock.json: Skipped file pattern
  • wrangler-mcp.toml: Skipped file pattern
  • wrangler.toml: Skipped file pattern

"async-retry": "^1.3.3",
"avsc": "^5.7.7",
"hono": "^4.6.10",
"ioredis": "^5.7.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Duplicate dependency ioredis and missing cleanup.
Fix: Remove duplicate ioredis and ensure proper cleanup.
Impact: Reduces package size and avoids potential conflicts.

Suggested change
"ioredis": "^5.7.0",
"@modelcontextprotocol/sdk": "^1.17.3",
"@portkey-ai/mustache": "^2.1.3",
"@smithy/signature-v4": "^2.1.1",
"@types/mustache": "^4.2.5",
"async-retry": "^1.3.3",
"avsc": "^5.7.7",
"hono": "^4.6.10",
"jose": "^6.0.11",
"minimist": "^1.2.8",
"openid-client": "^6.7.1",
"patch-package": "^8.0.0",
"ws": "^8.18.0",
"zod": "^3.22.4"

Comment on lines +106 to +108
if (!tokenInfo?.client_id) {
logger.debug('No client_id in tokenInfo, skipping OAuth token purge');
return;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Potential null pointer exception if tokenInfo is null or undefined.
Fix: Add null check before accessing tokenInfo.client_id.
Impact: Prevents runtime errors.

Suggested change
if (!tokenInfo?.client_id) {
logger.debug('No client_id in tokenInfo, skipping OAuth token purge');
return;
if (!tokenInfo?.client_id) {
logger.debug('No client_id in tokenInfo, skipping OAuth token purge');
return;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved: Null check for tokenInfo.client_id is no longer needed as tokenInfo is not used in this function

Comment on lines +332 to +335
logger.error(
`Failed to prepare session ${sessionId} for SSE messages`,
error
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Inconsistent error handling for session restoration.
Fix: Ensure consistent error handling and logging.
Impact: Improves debugging and error tracking.

Suggested change
logger.error(
`Failed to prepare session ${sessionId} for SSE messages`,
error
);
logger.error(
`Failed to prepare session ${session.id} for SSE messages`,
error
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved: Error handling for session restoration is now consistent with other error handling in the file

Comment on lines +58 to +69
'*',
cors({
origin: '*', // Configure appropriately for production
allowHeaders: [
'Content-Type',
'Authorization',
'mcp-session-id',
'mcp-protocol-version',
],
exposeHeaders: ['mcp-session-id', 'WWW-Authenticate'],
credentials: true, // Allow cookies and authorization headers
})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Potential security risk with wildcard CORS origin.
Fix: Configure CORS origin appropriately for production.
Impact: Enhances security by restricting allowed origins.

Suggested change
'*',
cors({
origin: '*', // Configure appropriately for production
allowHeaders: [
'Content-Type',
'Authorization',
'mcp-session-id',
'mcp-protocol-version',
],
exposeHeaders: ['mcp-session-id', 'WWW-Authenticate'],
credentials: true, // Allow cookies and authorization headers
})
cors({
origin: process.env.CORS_ORIGIN || '*', // Configure appropriately for production
allowHeaders: [
'Content-Type',
'Authorization',
'mcp-session-id',
'mcp-protocol-version',
],
exposeHeaders: ['mcp-session-id', 'WWW-Authenticate'],
credentials: true, // Allow cookies and authorization headers
})

Comment on lines +6 to +11
export const cacheBackendMiddleware = createMiddleware(
async (c: Context, next) => {
createCacheBackendsCF(env(c));
return next();
}
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Missing error handling for cache backend creation.
Fix: Add error handling to catch and log potential issues.
Impact: Improves robustness of cache backend initialization.

Suggested change
export const cacheBackendMiddleware = createMiddleware(
async (c: Context, next) => {
createCacheBackendsCF(env(c));
return next();
}
);
export const cacheBackendMiddleware = createMiddleware(
async (c: Context, next) => {
try {
createCacheBackendsCF(env(c));
return next();
} catch (error) {
console.error('Failed to create cache backends:', error);
return next(); // Continue without cache
}
}
);

Comment on lines 93 to 95
if (tokenInfo.refresh_token) {
// Try control plane first if available
if (controlPlane) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Potential null pointer exception if tokenInfo is null.
Fix: Add null check before accessing tokenInfo.refresh_token.
Impact: Prevents runtime errors.

Suggested change
if (tokenInfo.refresh_token) {
// Try control plane first if available
if (controlPlane) {
if (tokenInfo?.refresh_token) {
// Try control plane first if available
if (controlPlane) {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved: Null check for tokenInfo.refresh_token is no longer needed as tokenInfo is accessed safely

Comment on lines +955 to +956
<div class="card-subtitle">Gateway configs allow you to route requests to different providers and models. You
can load balance, set fallbacks, and configure automatic retries & timeouts. <a href="#">Learn more</a></div>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Potential XSS vulnerability due to direct insertion of user input into HTML.
Fix: Sanitize user input before inserting into HTML.
Impact: Prevents XSS attacks.

Suggested change
<div class="card-subtitle">Gateway configs allow you to route requests to different providers and models. You
can load balance, set fallbacks, and configure automatic retries & timeouts. <a href="#">Learn more</a></div>
<div class="card-subtitle">Gateway configs allow you to route requests to different providers and models. You
can load balance, set fallbacks, and configure automatic retries & timeouts. <a href="#">Learn more</a></div>

Comment on lines +186 to +188
get = async (key: string): Promise<string | null> => {
return await this.KV.get(key);
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Potential null pointer exception if this.KV is null.
Fix: Add null check before accessing this.KV.
Impact: Prevents runtime errors.

Suggested change
get = async (key: string): Promise<string | null> => {
return await this.KV.get(key);
};
get = async (key: string): Promise<string | null> => {
if (!this.KV) return null;
return await this.KV.get(key);
};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved: Null check for this.KV is no longer needed as KV is initialized before use

Comment on lines +55 to +59
private getFullKey(key: string, namespace?: string): string {
return namespace
? `${this.dbName}:${namespace}:${key}`
: `${this.dbName}:default:${key}`;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Potential null pointer exception if this.client is null.
Fix: Add null check before accessing this.client.
Impact: Prevents runtime errors.

Suggested change
private getFullKey(key: string, namespace?: string): string {
return namespace
? `${this.dbName}:${namespace}:${key}`
: `${this.dbName}:default:${key}`;
}
private getFullKey(key: string, namespace?: string): string {
if (!this.client) return '';
return namespace
? `${this.dbName}:${namespace}:${key}`
: `${this.dbName}:default:${key}`;
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Resolved: Null check for this.client is no longer needed as client is initialized before use

Comment on lines 15 to 45
// Extract the port number and flags from command line arguments using minimist
const defaultPort = 8787;
const args = process.argv.slice(2);
const portArg = args.find((arg) => arg.startsWith('--port='));
const port = portArg ? parseInt(portArg.split('=')[1]) : defaultPort;
const defaultMCPPort = process.env.PORT || 8788;

const argv = minimist(process.argv.slice(2), {
default: {
port: defaultPort,
'mcp-port': defaultMCPPort,
},
boolean: ['llm-node', 'mcp-node', 'llm-grpc', 'headless'],
});

const port = argv.port;
const mcpPort = argv['mcp-port'];

const isHeadless = args.includes('--headless');
// Add flags to choose what all to start (llm-node, llm-grpc, mcp-node)
// Default starts both llm-node and mcp-node

let llmNode = argv['llm-node'];
let mcpNode = argv['mcp-node'];
let llmGrpc = argv['llm-grpc'];

if (!llmNode && !mcpNode && !llmGrpc) {
llmNode = true;
mcpNode = true;
}

const isHeadless = argv.headless;

// Setup static file serving only if not in headless mode
if (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Potential port conflict if default ports are already in use.
Fix: Add error handling for port conflicts.
Impact: Prevents crashes due to port conflicts.

Suggested change
// Extract the port number and flags from command line arguments using minimist
const defaultPort = 8787;
const args = process.argv.slice(2);
const portArg = args.find((arg) => arg.startsWith('--port='));
const port = portArg ? parseInt(portArg.split('=')[1]) : defaultPort;
const defaultMCPPort = process.env.PORT || 8788;
const argv = minimist(process.argv.slice(2), {
default: {
port: defaultPort,
'mcp-port': defaultMCPPort,
},
boolean: ['llm-node', 'mcp-node', 'llm-grpc', 'headless'],
});
const port = argv.port;
const mcpPort = argv['mcp-port'];
const isHeadless = args.includes('--headless');
// Add flags to choose what all to start (llm-node, llm-grpc, mcp-node)
// Default starts both llm-node and mcp-node
let llmNode = argv['llm-node'];
let mcpNode = argv['mcp-node'];
let llmGrpc = argv['llm-grpc'];
if (!llmNode && !mcpNode && !llmGrpc) {
llmNode = true;
mcpNode = true;
}
const isHeadless = argv.headless;
// Setup static file serving only if not in headless mode
if (
const port = argv.port;
const mcpPort = argv['mcp-port'];
// Add flags to choose what all to start (llm-node, llm-grpc, mcp-node)
// Default starts both llm-node and mcp-node
let llmNode = argv['llm-node'];
let mcpNode = argv['mcp-node'];
let llmGrpc = argv['llm-grpc'];
if (!llmNode && !mcpNode && !llmGrpc) {
llmNode = true;
mcpNode = true;
}
const isHeadless = argv.headless;
// Setup static file serving only if not in headless mode
if (!isHeadless) {
const serveIndex = serveStatic({ root: './src/public' });
// Set up routes
app.get('/public/logs', serveIndex);
app.get('/public/mcp', serveIndex);
app.get('/public/', serveIndex);
// Redirect `/public` to `/public/`
app.get('/public', (c) => {
return c.redirect('/public/');
});
}
// Loading animation function
async function showLoadingAnimation() {
const frames = ['⠋', '⠙', '⠹', '⠸', '⠼', '⠴', '⠦', '⠧', '⠇', '⠏'];
let i = 0;
return new Promise((resolve) => {
const interval = setInterval(() => {
process.stdout.write(`\
${frames[i]} Starting...`);
i = (i + 1) % frames.length;
}, 80);
setTimeout(() => {
clearInterval(interval);
resolve(null);
}, 2000);
});
}
// Clear the console and show animation before main output
await showLoadingAnimation();
console.clear();
if (mcpNode) {
const mcpUrl = `http://localhost:${mcpPort}`;
const mcpServer = serve({
fetch: mcpApp.fetch,
port: mcpPort,
});
console.log('\x1b[1m%s\x1b[0m', '🤯 MCP Gateway is running at:');
console.log(' ' + '\x1b[1;4;32m%s\x1b[0m', `${mcpUrl}`);
}
const url = `http://localhost:${port}`;
if (llmNode) {
const { injectWebSocket, upgradeWebSocket } = createNodeWebSocket({ app });
app.get(
'/v1/realtime',
requestValidator,
upgradeWebSocket(realTimeHandlerNode)
);
const server = serve({
fetch: app.fetch,
port: port,
});
injectWebSocket(server);
console.log('\x1b[1m%s\x1b[0m', '🚀 AI Gateway is running at:');
console.log(' ' + '\x1b[1;4;32m%s\x1b[0m', `${url}`);
}
// Secondary information on single lines
if (!isHeadless) {
console.log('\
\
');
console.log(' ' + '\x1b[1m%s\x1b[0m', 'Public APIs:');
console.log(' ' + '\x1b[1;4m%s\x1b[0m', `${url}/public`);
console.log(' ' + '\x1b[1m%s\x1b[0m', 'Documentation:');
console.log(' ' + '\x1b[1;4m%s\x1b[0m', `${url}/docs`);
console.log(' ' + '\x1b[1m%s\x1b[0m', 'Health Check:');
console.log(' ' + '\x1b[1;4m%s\x1b[0m', `${url}/health`);
}
// Graceful shutdown
process.on('SIGINT', async () => {
console.log('\
\
Shutting down gracefully...');
if (llmNode) {
server.close();
}
if (mcpNode) {
mcpServer.close();
}
process.exit(0);
});

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored MCP session handling and improved cache backend implementations. Removed unused imports and streamlined session initialization.

Skipped files
  • src/mcp/utils/emitLog.ts: File hunk diff too large

Comment on lines +13 to +18
import { MCPSession, TransportType } from '../services/mcpSession';
import { getSessionStore } from '../services/sessionStore';
import { createLogger } from '../../shared/utils/logger';
import { ControlPlane } from '../middleware/controlPlane';
import { revokeAllClientTokens } from '../utils/oauthTokenRevocation';

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Unused Transport import.
Fix: Remove unused import.
Impact: Reduces bundle size and improves clarity.

Suggested change
import { MCPSession, TransportType } from '../services/mcpSession';
import { getSessionStore } from '../services/sessionStore';
import { createLogger } from '../../shared/utils/logger';
import { ControlPlane } from '../middleware/controlPlane';
import { revokeAllClientTokens } from '../utils/oauthTokenRevocation';
import { MCPSession, TransportType } from '../services/mcpSession';
import { getSessionStore } from '../services/sessionStore';
import { createLogger } from '../../shared/utils/logger';
import { ControlPlane } from '../middleware/controlPlane';
import { revokeAllClientTokens } from '../utils/oauthTokenRevocation';

Comment on lines +194 to +202
c: Context<Env>,
session: MCPSession
): Promise<any> {
// Ensure session is active or can be restored
try {
await session.initializeOrRestore();
logger.debug(`Session ${session.id} ready`);
} catch (error: any) {
logger.error(`Failed to prepare session ${session.id}`, error);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Unnecessary variable declaration for transport.
Fix: Remove unused variable.
Impact: Simplifies code and reduces memory usage.

Suggested change
c: Context<Env>,
session: MCPSession
): Promise<any> {
// Ensure session is active or can be restored
try {
await session.initializeOrRestore();
logger.debug(`Session ${session.id} ready`);
} catch (error: any) {
logger.error(`Failed to prepare session ${session.id}`, error);
): Promise<any> {
// Ensure session is active or can be restored
try {
await session.initializeOrRestore();
logger.debug(`Session ${session.id} ready`);

Comment on lines +207 to +215
return c.json(ErrorResponse.sessionRestoreFailed(), 500);
}

// Route based on transport type
if (session.getClientTransportType() === 'sse') {
const transport = session.initializeSSETransport();
await setSession(transport.sessionId, session);
await transport.start();
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Unused req and res destructuring.
Fix: Remove unused destructuring.
Impact: Simplifies code and improves readability.

Suggested change
return c.json(ErrorResponse.sessionRestoreFailed(), 500);
}
// Route based on transport type
if (session.getClientTransportType() === 'sse') {
const transport = session.initializeSSETransport();
await setSession(transport.sessionId, session);
await transport.start();
} else {
// Route based on transport type
if (session.getClientTransportType() === 'sse') {
const transport = session.initializeSSETransport();

Comment on lines +253 to +259
* This is the optimized entry point that delegates to specific handlers
*/
export async function handleMCPRequest(c: Context<Env>) {
const { serverConfig } = c.var;
if (!serverConfig) return c.json(ErrorResponse.serverConfigNotFound(), 500);

let session: MCPSession | undefined = c.var.session;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Unused tokenInfo variable.
Fix: Remove unused variable.
Impact: Simplifies code and reduces memory usage.

Suggested change
* This is the optimized entry point that delegates to specific handlers
*/
export async function handleMCPRequest(c: Context<Env>) {
const { serverConfig } = c.var;
if (!serverConfig) return c.json(ErrorResponse.serverConfigNotFound(), 500);
let session: MCPSession | undefined = c.var.session;
export async function handleMCPRequest(c: Context<Env>) {
const { serverConfig } = c.var;
if (!serverConfig) return c.json(ErrorResponse.serverConfigNotFound(), 500);

Comment on lines +268 to +277
}

export async function handleSSERequest(c: Context<Env>) {
const { serverConfig } = c.var;
if (!serverConfig) return c.json(ErrorResponse.serverConfigNotFound(), 500);

let session: MCPSession | undefined = c.var.session;
const isSSE = c.req.header('Accept') === 'text/event-stream';

if (!isSSE) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Unused tokenInfo and method variables.
Fix: Remove unused variables.
Impact: Simplifies code and reduces memory usage.

Suggested change
}
export async function handleSSERequest(c: Context<Env>) {
const { serverConfig } = c.var;
if (!serverConfig) return c.json(ErrorResponse.serverConfigNotFound(), 500);
let session: MCPSession | undefined = c.var.session;
const isSSE = c.req.header('Accept') === 'text/event-stream';
if (!isSSE) {
export async function handleSSERequest(c: Context<Env>) {
const { serverConfig } = c.var;
if (!serverConfig) return c.json(ErrorResponse.serverConfigNotFound(), 500);
let session: MCPSession | undefined = c.var.session;
const isSSE = c.req.header('Accept') === 'text/event-stream';

Comment on lines +282 to +288
let expiredCount = 0;
let hasChanges = false;

for (const [, namespaceData] of Object.entries(this.data)) {
for (const [key, entry] of Object.entries(namespaceData)) {
if (this.isExpired(entry)) {
delete namespaceData[key];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Unused namespace variable in loop.
Fix: Remove unused variable.
Impact: Simplifies code.

Suggested change
let expiredCount = 0;
let hasChanges = false;
for (const [, namespaceData] of Object.entries(this.data)) {
for (const [key, entry] of Object.entries(namespaceData)) {
if (this.isExpired(entry)) {
delete namespaceData[key];
let hasChanges = false;
for (const [, namespaceData] of Object.entries(this.data)) {
for (const [key, entry] of Object.entries(namespaceData)) {
if (this.isExpired(entry)) {

Comment on lines +193 to +198
}

async cleanup(): Promise<void> {
let expiredCount = 0;

for (const [key, entry] of this.cache.entries()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Unused now variable.
Fix: Remove unused variable.
Impact: Simplifies code.

Suggested change
}
async cleanup(): Promise<void> {
let expiredCount = 0;
for (const [key, entry] of this.cache.entries()) {
async cleanup(): Promise<void> {
let expiredCount = 0;
for (const [key, entry] of this.cache.entries()) {

Comment on lines +387 to +393
}

export function createCacheBackendsRedis(redisUrl: string): void {
logger.info('Creating cache backends with Redis', redisUrl);
let commonOptions: CacheConfig = {
backend: 'redis',
redisUrl: redisUrl,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Using console.log instead of logger.
Fix: Use logger for consistency.
Impact: Improves logging consistency.

Suggested change
}
export function createCacheBackendsRedis(redisUrl: string): void {
logger.info('Creating cache backends with Redis', redisUrl);
let commonOptions: CacheConfig = {
backend: 'redis',
redisUrl: redisUrl,
export function createCacheBackendsRedis(redisUrl: string): void {
logger.info('Creating cache backends with Redis', redisUrl);
let commonOptions: CacheConfig = {

Comment on lines +37 to +43
};
}

private formatMessage(level: string, message: string): string {
const parts: string[] = [];

if (this.config.timestamp) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Unused data parameter in formatMessage.
Fix: Remove unused parameter.
Impact: Simplifies function signature.

Suggested change
};
}
private formatMessage(level: string, message: string): string {
const parts: string[] = [];
if (this.config.timestamp) {
private formatMessage(level: string, message: string): string {
const parts: string[] = [];
if (this.config.timestamp) {

Comment on lines +57 to +63
private log(level: LogLevel, levelName: string, message: string, data?: any) {
if (level > this.config.level) return;

const formattedMessage = this.formatMessage(levelName, message);
const color = this.config.colors
? this.colors[levelName as keyof typeof this.colors]
: '';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Unused data parameter in formatMessage call.
Fix: Remove unused parameter.
Impact: Simplifies function call.

Suggested change
private log(level: LogLevel, levelName: string, message: string, data?: any) {
if (level > this.config.level) return;
const formattedMessage = this.formatMessage(levelName, message);
const color = this.config.colors
? this.colors[levelName as keyof typeof this.colors]
: '';
private log(level: LogLevel, levelName: string, message: string, data?: any) {
if (level > this.config.level) return;
const formattedMessage = this.formatMessage(levelName, message);
const color = this.config.colors

…om 7 days to 30 minutes and adjust save interval to 1 second
Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Low complexity changes with minor cleanup and configuration adjustments.

private upstreamSessionId?: string,
private controlPlane?: ControlPlane
) {
this.client = new Client(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Documentation Gap

Issue: TODO comment indicates incomplete implementation.
Fix: Either implement capability advertisement or remove TODO with explanation.
Impact: Improves code clarity and completeness.

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Session middleware removal and cache TTL/save interval adjustments reviewed.

Comment on lines +20 to +25
} from './handlers/mcpHandler';
import { oauthMiddleware } from './middleware/oauth';
import { hydrateContext } from './middleware/hydrateContext';
import { oauthRoutes } from './routes/oauth';
import { wellKnownRoutes } from './routes/wellknown';
import { adminRoutes } from './routes/admin';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: sessionMiddleware import and usage removed but not commented.
Fix: Add comment explaining removal rationale for future maintainers.
Impact: Improves code clarity and intent communication.

Comment on lines +138 to +143
skipPaths: ['/oauth', '/.well-known'],
}),
hydrateContext,
async (c) => {
return handleMCPRequest(c);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: sessionMiddleware removed from middleware chain without explanation.
Fix: Add inline comment clarifying intentional removal.
Impact: Prevents confusion during future debugging/maintenance.

Comment on lines +154 to +159
skipPaths: ['/oauth', '/.well-known'],
}),
hydrateContext,
async (c) => {
return handleSSERequest(c);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: sessionMiddleware removed from SSE request chain.
Fix: Document removal reason in code.
Impact: Maintains clear audit trail of middleware changes.

Comment on lines +171 to +176
skipPaths: ['/oauth', '/.well-known'],
}),
hydrateContext,
async (c) => {
return handleSSEMessages(c);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: sessionMiddleware removed from SSE messages chain.
Fix: Add explanatory comment for middleware removal.
Impact: Ensures future developers understand architectural decisions.

Comment on lines 17 to 18
// REMOVED FROM FLOW FOR LATER!!!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 Documentation Gap

Issue: Comment indicates session middleware was intentionally removed from flow.
Fix: Either restore middleware or remove file if permanently deprecated.
Impact: Reduces dead code and improves repository hygiene.

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug fixes and refactoring in OAuth token revocation logic

if (controlPlane) {
try {
await controlPlane.revoke(
refreshToken!,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: Unsafe non-null assertion on refreshToken which could be null
Fix: Add proper null check before using refreshToken
Impact: Prevents potential runtime error

Suggested change
refreshToken!,
await controlPlane.revoke(
refreshToken ?? '',
'refresh_token',
tokenInfo.client_id
);

let refreshToken: string | null = null;

if (tokenInfo.refresh_token) {
refreshToken = tokenInfo.refresh_token;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Code Refactor

Issue: Variable refreshToken declared twice in different scopes
Fix: Use single declaration and assignment
Impact: Improves code readability and reduces scope confusion

Suggested change
refreshToken = tokenInfo.refresh_token;
if (tokenInfo.refresh_token) {
refreshToken = tokenInfo.refresh_token;

Comment on lines +127 to +132
// Always revoke locally (for cache cleanup)
await revokeOAuthToken(
tokenInfo.refresh_token,
tokenInfo.client_id,
'refresh_token'
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚡️ Performance Improvement

Issue: Unnecessary local token revocation when refresh token is missing
Fix: Skip local revocation if no refresh token exists
Impact: Avoids redundant function calls

Suggested change
// Always revoke locally (for cache cleanup)
await revokeOAuthToken(
tokenInfo.refresh_token,
tokenInfo.client_id,
'refresh_token'
);
// Always revoke locally (for cache cleanup)
if (tokenInfo.refresh_token) {
await revokeOAuthToken(
tokenInfo.refresh_token,
tokenInfo.client_id,
'refresh_token'
);
}

Copy link
Contributor

@matter-code-review matter-code-review bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added control plane handling in OAuth routes with early return for unimplemented paths.

};
Variables: {
gateway: OAuthGateway;
controlPlane?: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🐛 Bug Fix

Issue: controlPlane property added without type definition
Fix: Add proper type instead of any
Impact: Improves type safety and maintainability

Suggested change
controlPlane?: any;
controlPlane?: ControlPlaneType;

Copy link
Contributor

Fix missing closing quote in WWW-Authenticate header string literal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants