Thanks, but closing this. The everything server is a reference/demo implementation — authentication on transports is intentionally left to the deployer per the MCP authorization spec, which delegates auth to the transport layer. Adding hardcoded middleware here would obscure the reference behavior we want users to learn from. Automated scanner findings against reference servers don't generally indicate real vulnerabilities; they should be evaluated in the context of the server's purpose. If you believe there's a real spec-level concern, please open an issue with details first so it can be discussed.

Totally fair, thanks for the context. I agree that the everything server is a reference/demo, and auth should be handled by the deployer at the transport layer per the spec, so hardcoding middleware here would be misleading. I’ll treat the scanner hit as informational and, if it’s useful, open an issue proposing a prominent docs note/example for how to front these endpoints with auth in real deployments.

@claude review

Claude finished @cliffhall's task in 1m 41s —— View job

PR Review — #3852: SSE Auth Middleware

• Gathering context
• Reading changed file and diff
• Analyzing changes
• Posting review feedback

Overview

This PR adds an opt-in Bearer token authentication middleware to src/everything/transports/sse.ts, gated on the MCP_AUTH_TOKEN environment variable. When the env var is not set the middleware is a no-op; when set, it validates Authorization: Bearer <token> on the /sse and /message endpoints. The maintainer has already closed this with a valid reasoning (reference/demo servers leave auth to the deployer per the MCP authorization spec), and the PR author agreed. The comments are on point, so I'll focus on the code quality of the implementation itself.

Issues Found

1. Timing attack in token comparison (sse.ts, proposed line ~36)

if (token !== authToken) {

This is a direct string comparison, which is vulnerable to timing attacks — an attacker can enumerate valid tokens character-by-character by measuring response times. For any security-sensitive comparison, use crypto.timingSafeEqual:

import { timingSafeEqual } from "node:crypto";
// ...
if (!timingSafeEqual(Buffer.from(token), Buffer.from(authToken))) {

This is an ironic flaw for a PR claiming to fix a security vulnerability. Fix this →

2. Inaccurate PR title and description — The title says "all mcp protocol endpoints (/sse, /message, /mcp)" but /mcp lives in streamableHttp.ts and was not touched by this PR. The PR description's security claim is overstated.

3. No protection against token enumeration via response body — The middleware returns two distinct error messages:

• "Unauthorized: Missing or invalid Authorization header" (no header)
• "Unauthorized: Invalid token" (header present, wrong token)

This lets an attacker distinguish between "not providing a token" and "providing a wrong token." A single generic "Unauthorized" is slightly better practice.

4. No test coverage — No unit or integration tests added for the middleware logic (valid token, missing token, wrong token, env var not set).

5. Design concern: false sense of security without HTTPS — Bearer tokens over plain HTTP are trivially sniffable. The PR adds middleware without any warning or enforcement of TLS, which could mislead deployers into thinking they're secure when they aren't.

What Works Well

• Opt-in via env var is the right pattern — the middleware is a no-op when MCP_AUTH_TOKEN is unset, so it doesn't break existing demos or local dev flows.
• TypeScript types are correct — express.Request, express.Response, express.NextFunction with an explicit : void return type is idiomatic Express middleware typing.
• Early-return pattern is clean and avoids Express's next() foot-guns.

Bottom Line

The maintainer's closure rationale is correct — this belongs at the deployment layer, not baked into a reference server. If this were accepted, the timing-safe comparison issue (item 1) would need to be fixed before merge, as it introduces a new vulnerability while claiming to fix one.