fix: mcp adapter service exposes three critical http... in mcp-adapte...#1522
fix: mcp adapter service exposes three critical http... in mcp-adapte...#1522orbisai0security wants to merge 3 commits intomoeru-ai:mainfrom
Conversation
MCP adapter service exposes three critical HTTP endpoints (/sse for Server-Sent Events, /messages for message handling, and / for root access) without any authentication checks Resolves V-003
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 89c29cbc0e
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| const authToken = process.env.MCP_AUTH_TOKEN | ||
| if (authToken) { | ||
| const authHeader = event.node.req.headers['authorization'] | ||
| if (!authHeader || authHeader !== `Bearer ${authToken}`) { |
There was a problem hiding this comment.
When MCP_AUTH_TOKEN is set, these handlers require an Authorization header, but the same router still sets Access-Control-Allow-Headers to only Content-Type in the CORS middleware (services/twitter-services/src/adapters/mcp-adapter.ts:427-431). In browser clients, preflight requests for authenticated /messages (and any authenticated fetch-based /sse calls) will be rejected before reaching this auth check, so valid clients cannot connect. Add Authorization (at minimum) to allowed CORS headers whenever token auth is enabled.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces authentication checks for the SSE, messages, and root endpoints within the MCP adapter, utilizing a Bearer token mechanism. A critical security review identified a timing attack vulnerability due to the use of standard string comparison for tokens. The feedback recommends implementing constant-time comparison using Node.js's crypto.timingSafeEqual, properly parsing the 'Bearer ' prefix, and validating environment variables upon service startup to ensure robustness.
| const authHeader = event.node.req.headers['authorization'] | ||
| if (!authHeader || authHeader !== `Bearer ${authToken}`) { | ||
| logger.mcp.warn('Unauthorized MCP SSE connection attempt rejected') | ||
| event.node.res.statusCode = 401 | ||
| event.node.res.end(JSON.stringify({ error: 'Unauthorized' })) | ||
| return | ||
| } |
There was a problem hiding this comment.
Using !== for comparing authentication tokens is vulnerable to timing attacks, which could allow an attacker to guess the token. Please use a constant-time comparison function like timingSafeEqual from Node's crypto module. Additionally, ensure that the MCP_AUTH_TOKEN environment variable is validated on startup to prevent runtime errors or undefined behavior, rather than relying on checks within the request handler. The current check should also be updated to handle the Bearer prefix correctly.
This vulnerability is present in all three authentication checks added in this PR. Please apply a similar fix to all of them.
You will need to add import { timingSafeEqual } from 'node:crypto'; at the top of the file.
const authHeader = event.node.req.headers['authorization']
const prefix = 'Bearer '
let authorized = false
if (authHeader?.startsWith(prefix)) {
const providedToken = authHeader.substring(prefix.length)
if (authToken.length === providedToken.length) {
try {
const expectedTokenBuf = Buffer.from(authToken)
const providedTokenBuf = Buffer.from(providedToken)
authorized = timingSafeEqual(expectedTokenBuf, providedTokenBuf)
} catch {
// Buffer.from can throw on invalid input. Treat as unauthorized.
authorized = false
}
}
}
if (!authorized) {
logger.mcp.warn('Unauthorized MCP SSE connection attempt rejected')
event.node.res.statusCode = 401
event.node.res.end(JSON.stringify({ error: 'Unauthorized' }))
return
}References
- Validate environment variables on startup to ensure all required variables are present, preventing runtime errors from undefined values.
Zazzik1
left a comment
Zazzik1
left a comment
There was a problem hiding this comment.
Hi, what do you think about this idea?
| router.get('/sse', defineEventHandler(async (event) => { | ||
| const authToken = process.env.MCP_AUTH_TOKEN | ||
| if (authToken) { | ||
| const authHeader = event.node.req.headers['authorization'] | ||
| if (!authHeader || authHeader !== `Bearer ${authToken}`) { | ||
| logger.mcp.warn('Unauthorized MCP SSE connection attempt rejected') | ||
| event.node.res.statusCode = 401 | ||
| event.node.res.end(JSON.stringify({ error: 'Unauthorized' })) | ||
| return | ||
| } |
There was a problem hiding this comment.
Since the token validation logic is nearly identical across all three cases, what do you think about extracting it into a shared “guard”? This could improve maintainability and reduce the risk of someone updating it in one place while forgetting the others.
It could look like this (plus, we would need to address the part vulnerable to timing attacks):
| router.get('/sse', defineEventHandler(async (event) => { | |
| const authToken = process.env.MCP_AUTH_TOKEN | |
| if (authToken) { | |
| const authHeader = event.node.req.headers['authorization'] | |
| if (!authHeader || authHeader !== `Bearer ${authToken}`) { | |
| logger.mcp.warn('Unauthorized MCP SSE connection attempt rejected') | |
| event.node.res.statusCode = 401 | |
| event.node.res.end(JSON.stringify({ error: 'Unauthorized' })) | |
| return | |
| } | |
| function defineProtectedEventHandler(label: string, handler: EventHandler<EventHandlerRequest, void>) { | |
| return defineEventHandler((event) => { | |
| const authToken = process.env.MCP_AUTH_TOKEN | |
| if (authToken) { | |
| const authHeader = event.node.req.headers.authorization | |
| if (!authHeader || authHeader !== `Bearer ${authToken}`) { | |
| logger.mcp.warn(`Unauthorized ${label} rejected`) | |
| event.node.res.statusCode = 401 | |
| return { error: 'Unauthorized' } | |
| } | |
| } | |
| return handler(event) | |
| }) | |
| } | |
| // SSE endpoint | |
| router.get('/sse', defineProtectedEventHandler('MCP SSE connection attempt', async (event) => { |
and on top of the file
import type { EventHandler, EventHandlerRequest } from 'h3'| router.post('/messages', defineEventHandler(async (event) => { | ||
| const authToken = process.env.MCP_AUTH_TOKEN | ||
| if (authToken) { | ||
| const authHeader = event.node.req.headers['authorization'] | ||
| if (!authHeader || authHeader !== `Bearer ${authToken}`) { | ||
| logger.mcp.warn('Unauthorized MCP message request rejected') | ||
| event.node.res.statusCode = 401 | ||
| return { error: 'Unauthorized' } | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
this would be simplified to
| router.post('/messages', defineEventHandler(async (event) => { | |
| const authToken = process.env.MCP_AUTH_TOKEN | |
| if (authToken) { | |
| const authHeader = event.node.req.headers['authorization'] | |
| if (!authHeader || authHeader !== `Bearer ${authToken}`) { | |
| logger.mcp.warn('Unauthorized MCP message request rejected') | |
| event.node.res.statusCode = 401 | |
| return { error: 'Unauthorized' } | |
| } | |
| } | |
| router.post('/messages', defineProtectedEventHandler('MCP message request', async (event) => { |
| router.get('/', defineEventHandler((event) => { | ||
| const authToken = process.env.MCP_AUTH_TOKEN | ||
| if (authToken) { | ||
| const authHeader = event.node.req.headers['authorization'] | ||
| if (!authHeader || authHeader !== `Bearer ${authToken}`) { | ||
| logger.mcp.warn('Unauthorized MCP root request rejected') | ||
| event.node.res.statusCode = 401 | ||
| return { error: 'Unauthorized' } | ||
| } | ||
| } |
There was a problem hiding this comment.
and this would simplify to
| router.get('/', defineEventHandler((event) => { | |
| const authToken = process.env.MCP_AUTH_TOKEN | |
| if (authToken) { | |
| const authHeader = event.node.req.headers['authorization'] | |
| if (!authHeader || authHeader !== `Bearer ${authToken}`) { | |
| logger.mcp.warn('Unauthorized MCP root request rejected') | |
| event.node.res.statusCode = 401 | |
| return { error: 'Unauthorized' } | |
| } | |
| } | |
| router.get('/', defineProtectedEventHandler('MCP root request', async (event) => { |
| import type { Context } from '../core/browser/context' | ||
| import type { Tweet } from '../core/services/tweet' | ||
| import type { TwitterServices } from '../types/services' | ||
|
|
There was a problem hiding this comment.
for completeness - if we decide to implement the shared guard
| import type { EventHandler, EventHandlerRequest } from 'h3' | |
|
@orbisai0security can you address code review comments? |
|
✅ Changes Applied I've updated the code based on your feedback: Address all code review feedback on the MCP adapter security PR:
Files modified:
The changes have been pushed to this PR branch. Please review! |
nekomeowww
added
scope/agent
| if (authToken) { | ||
| const authHeader = event.node.req.headers['authorization'] | ||
| const providedToken | ||
| = typeof authHeader === 'string' && authHeader.startsWith('Bearer ') |
There was a problem hiding this comment.
LGTM, I'd just update this line, as the Bearer part ideally should be case-insensitive ("Bearer" is correct, but "bearer" and "BEARER" are correct as well)
https://www.rfc-editor.org/rfc/rfc7235.html#section-2.1
| = typeof authHeader === 'string' && authHeader.startsWith('Bearer ') | |
| = typeof authHeader === 'string' && authHeader.toLowerCase().startsWith("bearer ") |
There was a problem hiding this comment.
@orbisai0security can you address the code review comments?
|
✅ Changes Applied I've updated the code based on your feedback: The only remaining unaddressed review comment is from @Zazzik1: the Files modified:
The changes have been pushed to this PR branch. Please review! |
4 participants
Summary
Fix high severity security issue in
services/twitter-services/src/adapters/mcp-adapter.ts.Vulnerability
V-003services/twitter-services/src/adapters/mcp-adapter.ts:439Description: MCP adapter service exposes three critical HTTP endpoints (/sse for Server-Sent Events, /messages for message handling, and / for root access) without any authentication checks. The code at lines 4...
Changes
services/twitter-services/src/adapters/mcp-adapter.tsVerification
Automated security fix by OrbisAI Security