Skip to content

Commit 80175ee

Browse files
mike-parkhillclaude
andcommitted
Fix findings from Maycon's AI review pass
- Redact Authorization/X-Admin-Secret/Cookie before the per-request debug log — it was writing tenant JWTs, the Truvera API key (passthrough mode), and the admin revoke secret to stderr/CloudWatch in plaintext on every request. The top-priority finding. - Extend the per-request session re-auth check (added in an earlier pass) from JWT-only to passthrough mode too. It was gated on authConfig?.mode === "jwt", so a reused Mcp-Session-Id in passthrough mode skipped auth entirely — a leaked session ID rode the original client's Truvera API key indefinitely, exactly the gap the code comment claimed was closed. Added a mirrored test suite for passthrough alongside the existing JWT one. - Drop the full tool name/description list from the unauthenticated /health response, keeping toolCount (which docs and an e2e test already depend on). /health has to stay unauthenticated for load balancers and uptime monitors, but it doesn't need to hand an unauthenticated caller the entire tool surface (delegation, credential issuance, AP2 payment tools) up front. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent eb8e0d7 commit 80175ee

2 files changed

Lines changed: 128 additions & 16 deletions

File tree

packages/mcp-shared/src/transport/http/index.test.ts

Lines changed: 91 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -266,4 +266,95 @@ describe("startHTTPTransport JWT session reuse", () => {
266266
expect(res.status).not.toBe(401);
267267
expect(res.status).not.toBe(403);
268268
});
269+
});
270+
271+
describe("startHTTPTransport passthrough session reuse", () => {
272+
let server: http.Server | undefined;
273+
let baseUrl: string;
274+
// Passthrough auth resolution has no crypto step, so these tests run back
275+
// to back within a few ms — fast enough that a stale keep-alive socket from
276+
// the previous test's (now-closed) server can still be sitting in fetch's
277+
// connection pool if consecutive tests reuse the exact same port. Starting
278+
// each test's port search well above the last one avoids that collision.
279+
let nextPort = 41300;
280+
281+
async function start() {
282+
const port = await findNextAvailablePort(nextPort);
283+
nextPort = port + 10;
284+
server = await startHTTPTransport({
285+
serverFactory: () => ({ server: new McpServer({ name: "test-service", version: "0.0.0" }) }),
286+
MCP_PORT: port,
287+
BUILD_INFO: { timestamp: "2026-01-01T00:00:00Z", buildNumber: 1, version: "0.0.0-test" },
288+
tools: [],
289+
serviceName: "test-service",
290+
authConfig: { mode: "passthrough" },
291+
});
292+
baseUrl = `http://127.0.0.1:${port}`;
293+
}
294+
295+
afterEach(async () => {
296+
if (!server) return;
297+
await new Promise<void>((resolve) => server!.close(() => resolve()));
298+
server = undefined;
299+
});
300+
301+
async function initialize(apiKey: string) {
302+
return fetch(`${baseUrl}/mcp`, {
303+
method: "POST",
304+
headers: {
305+
accept: "application/json, text/event-stream",
306+
"content-type": "application/json",
307+
authorization: `Bearer ${apiKey}`,
308+
},
309+
body: JSON.stringify({
310+
jsonrpc: "2.0",
311+
id: 1,
312+
method: "initialize",
313+
params: { protocolVersion: "2024-11-05", capabilities: {}, clientInfo: { name: "test", version: "0.0.0" } },
314+
}),
315+
});
316+
}
317+
318+
function reuseSession(sessionId: string, headers: Record<string, string> = {}) {
319+
return fetch(`${baseUrl}/mcp`, {
320+
method: "POST",
321+
headers: {
322+
accept: "application/json, text/event-stream",
323+
"content-type": "application/json",
324+
"mcp-session-id": sessionId,
325+
...headers,
326+
},
327+
body: JSON.stringify({ jsonrpc: "2.0", id: 2, method: "tools/list", params: {} }),
328+
});
329+
}
330+
331+
it("rejects reuse of a session with no Authorization header", async () => {
332+
await start();
333+
const initRes = await initialize("client-a-key");
334+
expect(initRes.status).toBe(200);
335+
const sessionId = initRes.headers.get("mcp-session-id")!;
336+
expect(sessionId).toBeTruthy();
337+
338+
const res = await reuseSession(sessionId);
339+
expect(res.status).toBe(401);
340+
});
341+
342+
it("rejects reuse of a session with a different client's API key", async () => {
343+
await start();
344+
const initRes = await initialize("client-a-key");
345+
const sessionId = initRes.headers.get("mcp-session-id")!;
346+
347+
const res = await reuseSession(sessionId, { authorization: "Bearer client-b-key" });
348+
expect(res.status).toBe(403);
349+
});
350+
351+
it("allows reuse of a session with the same client's API key", async () => {
352+
await start();
353+
const initRes = await initialize("client-a-key");
354+
const sessionId = initRes.headers.get("mcp-session-id")!;
355+
356+
const res = await reuseSession(sessionId, { authorization: "Bearer client-a-key" });
357+
expect(res.status).not.toBe(401);
358+
expect(res.status).not.toBe(403);
359+
});
269360
});

packages/mcp-shared/src/transport/http/index.ts

Lines changed: 37 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,16 @@ async function readJsonBody(req: IncomingMessage): Promise<unknown> {
4848
return data ? JSON.parse(data) : undefined;
4949
}
5050

51+
const SENSITIVE_HEADERS = new Set(["authorization", "x-admin-secret", "cookie"]);
52+
53+
function redactHeaders(headers: IncomingMessage["headers"]): Record<string, unknown> {
54+
const redacted: Record<string, unknown> = {};
55+
for (const [key, value] of Object.entries(headers)) {
56+
redacted[key] = SENSITIVE_HEADERS.has(key.toLowerCase()) ? "[redacted]" : value;
57+
}
58+
return redacted;
59+
}
60+
5161
function secretsMatch(provided: string, expected: string): boolean {
5262
const providedBuf = Buffer.from(provided);
5363
const expectedBuf = Buffer.from(expected);
@@ -203,11 +213,14 @@ export async function startHTTPTransport({
203213
}
204214

205215
const httpServer = http.createServer(async (req: IncomingMessage, res: ServerResponse) => {
206-
// Debug: Log all incoming requests
216+
// Debug: Log all incoming requests. Headers are redacted — Authorization
217+
// carries the tenant JWT or Truvera API key, X-Admin-Secret the revoke
218+
// secret; logging those in the clear hands out live credentials to
219+
// anyone with log access.
207220
console.error("[DEBUG] Incoming request:", {
208221
method: req.method,
209222
url: req.url,
210-
headers: req.headers
223+
headers: redactHeaders(req.headers)
211224
});
212225

213226
// Enable CORS
@@ -222,7 +235,11 @@ export async function startHTTPTransport({
222235
return;
223236
}
224237

225-
// Health check endpoint
238+
// Health check endpoint. Deliberately unauthenticated (load balancers and
239+
// uptime monitors hit this with no credentials) — so it must not leak
240+
// anything beyond "the service is up". Tool names/descriptions map the
241+
// whole attack surface (delegation, credential issuance, AP2 payment
242+
// tools) to anyone who can reach this port; toolCount alone doesn't.
226243
if (req.method === "GET" && req.url === "/health") {
227244
res.writeHead(200, { "Content-Type": "application/json" });
228245
res.end(JSON.stringify({
@@ -232,7 +249,6 @@ export async function startHTTPTransport({
232249
buildNumber: BUILD_INFO.buildNumber,
233250
buildTime: BUILD_INFO.timestamp,
234251
toolCount: tools.length,
235-
tools: tools.map((t) => ({ name: t.name, description: t.description ?? null })),
236252
}));
237253
return;
238254
}
@@ -317,13 +333,15 @@ export async function startHTTPTransport({
317333
let server: McpServer | undefined;
318334
let initializedSessionId: string | undefined;
319335
if (sessionId && typeof sessionId === "string" && transports[sessionId]) {
320-
// Existing session: reuse transport and server. In JWT mode, re-check
321-
// auth on every request rather than only at session creation — otherwise
322-
// a session outlives its token's revocation (POST /admin/revoke-tenant
323-
// would have no effect until the client reconnects) and a leaked/guessed
324-
// Mcp-Session-Id would grant access with no Authorization header at all.
336+
// Existing session: reuse transport and server. In jwt/passthrough mode,
337+
// re-check auth on every request rather than only at session creation —
338+
// otherwise a session outlives its token's revocation (POST
339+
// /admin/revoke-tenant would have no effect until the client
340+
// reconnects) and a leaked/guessed Mcp-Session-Id would grant access
341+
// (riding the original client's JWT or API key) with no Authorization
342+
// header at all.
325343
const session = transports[sessionId];
326-
if (authConfig?.mode === "jwt") {
344+
if (authConfig?.mode === "jwt" || authConfig?.mode === "passthrough") {
327345
let requestAuthContext;
328346
try {
329347
requestAuthContext = await resolveAuthContext(req, authConfig);
@@ -335,13 +353,16 @@ export async function startHTTPTransport({
335353
}
336354
throw err;
337355
}
338-
if (
339-
requestAuthContext.mode !== "jwt" ||
340-
session.authContext.mode !== "jwt" ||
341-
requestAuthContext.tenantId !== session.authContext.tenantId
342-
) {
356+
const credentialsMatch =
357+
(requestAuthContext.mode === "jwt" &&
358+
session.authContext.mode === "jwt" &&
359+
requestAuthContext.tenantId === session.authContext.tenantId) ||
360+
(requestAuthContext.mode === "passthrough" &&
361+
session.authContext.mode === "passthrough" &&
362+
requestAuthContext.apiKey === session.authContext.apiKey);
363+
if (!credentialsMatch) {
343364
res.writeHead(403, { "Content-Type": "application/json" });
344-
res.end(JSON.stringify({ error: "Token does not match this session's tenant" }));
365+
res.end(JSON.stringify({ error: "Credentials do not match this session" }));
345366
return;
346367
}
347368
}

0 commit comments

Comments
 (0)