security: stop sending session token in SSE URL query params#1980
security: stop sending session token in SSE URL query params#1980sebastiondev wants to merge 1 commit intoMCPJam:mainfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
dosubot
Bot
added
the
size:M
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
WalkthroughThis pull request transitions authentication for Server-Sent Events (SSE) endpoints from URL-based query parameters ( 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. Review rate limit: 7/8 reviews remaining, refill in 7 minutes and 30 seconds.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@mcpjam-inspector/server/middleware/session-auth.ts`:
- Around line 117-122: The cookie extraction for the session token assigns to
token but doesn't URL-decode it; update the logic that builds token (the
expression that finds "mcp_session_auth=" and calls substring) to pass the found
value through decodeURIComponent (e.g., decodeURIComponent(foundValue)) and
handle potential decode errors (try/catch or a safe decode helper) so the token
used by the session validation code (the token variable used downstream) matches
the client-side encodeURIComponent() from session-token.ts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 152047cc-ce62-4a2c-a800-9755b3387064
📒 Files selected for processing (6)
mcpjam-inspector/client/src/lib/__tests__/session-token.test.tsmcpjam-inspector/client/src/lib/session-token.tsmcpjam-inspector/server/__tests__/auth-integration.test.tsmcpjam-inspector/server/middleware/__tests__/session-auth.test.tsmcpjam-inspector/server/middleware/session-auth.tsmcpjam-inspector/server/routes/mcp/__tests__/http-adapters.test.ts
| token = c.req | ||
| .header("Cookie") | ||
| ?.split(";") | ||
| .map((part) => part.trim()) | ||
| .find((part) => part.startsWith("mcp_session_auth=")) | ||
| ?.substring("mcp_session_auth=".length); |
There was a problem hiding this comment.
Missing URL decoding for cookie token value.
The client encodes the token with encodeURIComponent() (see session-token.ts:192-193), but this extraction does not decode it. While session tokens are typically alphanumeric and thus unaffected, this asymmetry could cause validation failures if tokens contain characters like + or =.
🛡️ Proposed fix to decode the cookie value
if (!token) {
- token = c.req
+ const rawCookie = c.req
.header("Cookie")
?.split(";")
.map((part) => part.trim())
- .find((part) => part.startsWith("mcp_session_auth="))
- ?.substring("mcp_session_auth=".length);
+ .find((part) => part.startsWith("mcp_session_auth="));
+ if (rawCookie) {
+ try {
+ token = decodeURIComponent(rawCookie.substring("mcp_session_auth=".length));
+ } catch {
+ // Malformed encoding - leave token undefined, will fail validation
+ }
+ }
}📝 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.
| token = c.req | |
| .header("Cookie") | |
| ?.split(";") | |
| .map((part) => part.trim()) | |
| .find((part) => part.startsWith("mcp_session_auth=")) | |
| ?.substring("mcp_session_auth=".length); | |
| if (!token) { | |
| const rawCookie = c.req | |
| .header("Cookie") | |
| ?.split(";") | |
| .map((part) => part.trim()) | |
| .find((part) => part.startsWith("mcp_session_auth=")); | |
| if (rawCookie) { | |
| try { | |
| token = decodeURIComponent(rawCookie.substring("mcp_session_auth=".length)); | |
| } catch { | |
| // Malformed encoding - leave token undefined, will fail validation | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mcpjam-inspector/server/middleware/session-auth.ts` around lines 117 - 122,
The cookie extraction for the session token assigns to token but doesn't
URL-decode it; update the logic that builds token (the expression that finds
"mcp_session_auth=" and calls substring) to pass the found value through
decodeURIComponent (e.g., decodeURIComponent(foundValue)) and handle potential
decode errors (try/catch or a safe decode helper) so the token used by the
session validation code (the token variable used downstream) matches the
client-side encodeURIComponent() from session-token.ts.
2 participants
Summary
The inspector's session token is currently appended to SSE/EventSource URLs as a
?_token=<token>query parameter. URL-embedded credentials are well known to leak into:Refererheaders if the SSE-loaded page navigates externallyThis is a CWE-200 / CWE-598 (Information Exposure Through Query Strings in URL) hardening issue. Severity is modest — the inspector binds to localhost by default and origin validation blocks cross-origin requests — but the leak path is real and the token has no visible expiry, so anyone with retrospective access to logs or browser history can replay it.
Affected files (before fix):
mcpjam-inspector/client/src/lib/session-token.ts—addTokenToUrl()appended?_token=<token>to every EventSource URLmcpjam-inspector/server/middleware/session-auth.ts—sessionAuthMiddlewareacceptedc.req.query("_token")as a credential sourceData flow:
__MCP_SESSION_TOKEN__→getSessionToken()→addTokenToUrl()→new EventSource(url)→ URL recorded in browser history / server logs.Fix
Replace the URL query parameter with a same-origin cookie scoped to the API:
setEventSourceAuthCookie(token)setsmcp_session_auth=<token>; Path=/api/; SameSite=Strict(andSecurewhen served over HTTPS). It is called frominitializeSessionToken()andgetSessionToken()so the cookie is in place before anyEventSourceconnection is opened.addTokenToUrl()no longer mutates the URL — it just ensures the cookie is set and returns the URL untouched.sessionAuthMiddlewarenow reads the token from theCookieheader instead of?_token=. TheX-MCP-Session-Auth: Bearer <token>header path used byfetch()callers is unchanged.scrubTokenFromUrl()regex is left in place to redact any pre-existing log lines (defense-in-depth for historical logs).Why a cookie and not just headers?
EventSourcedoes not allow custom headers, which is exactly why the original code used a URL parameter. A same-origin cookie is the standard alternative recommended for this case.SameSite=Strict+Path=/api/+ the existing origin-validation middleware closes the typical CSRF concern for cookie-based auth on a localhost developer tool.Tests
Updated tests reflect the new behavior:
client/src/lib/__tests__/session-token.test.ts—addTokenToUrlno longer adds?_token=, and a new test asserts the auth cookie is set with the expected attributes.server/middleware/__tests__/session-auth.test.ts— accepts cookie-based auth, rejects missing-token requests with the new hint string.server/__tests__/auth-integration.test.tsandserver/routes/mcp/__tests__/http-adapters.test.ts— SSE happy-path tests now sendCookie: mcp_session_auth=...instead of the query parameter.I ran the affected vitest suites locally and they pass. No production code path still consumes
_token(verified with grep acrossmcpjam-inspector/); the only remaining reference is the legacy log scrubber regex.Security analysis
http://127.0.0.1:6274). Any SSE connection made by the UI persists?_token=<token>in browser history. If the same machine has a browsing-history sync extension, screen-recording / observability agent, or a reverse proxy in front (some Docker users do this), the token is exposed at rest. The token alone is sufficient to impersonate the session against the API.SameSite=Strict(no cross-site delivery),Secureover HTTPS, and scoped toPath=/api/(not sent to static assets). TheX-MCP-Session-Authheader path is unchanged sofetchcallers are unaffected.Adversarial review
Before submitting, I tried to disprove this finding. The two most credible objections are: (1) "the inspector binds to localhost so logs aren't exposed," and (2) "origin validation already prevents abuse." Neither fully closes the gap — localhost binding doesn't prevent a local proxy, screen recorder, or browser-history sync from capturing the URL, and origin validation doesn't help once the token has already been read out of a log file. The fix is small, behavior-preserving for the header path, and removes a textbook URL-credential anti-pattern.
cc @lewiswigmore