Skip to content

Make a third-party MCP session id safe for the gateway topic - #8565

Merged
cstns merged 1 commit into
mainfrom
fix-mcp-topic-safe-session-id
Sep 18, 2026
Merged

cstns merged 1 commit into
mainfrom
fix-mcp-topic-safe-session-id

Conversation

@cstns

@cstns cstns commented Sep 18, 2026

Copy link
Copy Markdown
Contributor

Closes #8564

_meta["openai/session"] is shaped v1/<token>. The session id becomes one level of the gateway's MQTT topic, so embedding that raw produced a topic with an extra level, which matched no ACL pattern and was denied without raising a validation error. The request never reached the broker or the gateway, and the caller waited out the full 30s proxy timeout before getting a 504. Production showed result: deny in 0.226 ms for those topics while neighbouring UUID-carrying requests from the same user were allowed and completed in ~150 ms.

The fix sanitises that value before it is used. Ids already safe as a topic level pass through untouched, so a well-behaved client's session id stays readable in logs and on the wire. Anything else is replaced with a SHA-256 digest. Hashed rather than stripped for two reasons: stripping would collapse two distinct sessions onto the same topic, and the mapping has to be stable or a pinned browser tab is unreachable between a client's own requests.

Only the _meta value is treated as untrusted. The mcp-session-id header is an id we minted coming back to us, and randomUUID() is ours, so neither is touched. The helper returns null when there is nothing usable, which lets a missing openai/session fall through to a fresh id without an extra branch.

The ACL's session check moves from length (mcpSessionId.length < 8) to format. Worth being precise about what that adds: a slashed id never matched the topic pattern ([^/]+) in the first place, so it was already denied, just silently. The case the format check genuinely catches is something like sess+ion12345, long enough for the old check and a clean match for the topic pattern, but an MQTT wildcard sitting inside a topic level. It is a backstop, not the fix.

Tests: seven on the helper (passthrough, separator, wildcards, stability, distinctness, idempotence, unusable input), two on the route (a slashed openai/session yields a topic-safe id echoed in the response header, and the same value routes identically across two calls), two on the ACL.

One known gap, left deliberately: mcp-session-id is still a client-supplied header, so a client sending mcp-session-id: a/b would rebuild the same broken topic and eat the timeout. That is self-inflicted rather than a security issue, the ACL denies it either way, and the clients actually affected here do not send the header at all.

The session id becomes one level of the gateway's MQTT topic. openai/session
is shaped `v1/<token>`, so embedding it raw added an extra level, the topic
then matched no ACL pattern, and the publish was denied without raising a
validation error. The request never reached the gateway and the caller waited
out the full 30s proxy timeout.

Sanitise that value before use: ids already safe for a topic level pass
through, anything else is hashed. Hashed rather than stripped so two sessions
cannot collapse onto one topic, and stably so a pinned browser tab survives
between a client's own requests.

Only the _meta value is treated as untrusted. The header is an id we minted
coming back to us and randomUUID is ours.

Also swap the ACL's session length check for a format check, as a backstop for
anything that reaches the topic by another route.
@cstns cstns self-assigned this Sep 18, 2026
@cstns
cstns enabled auto-merge (squash) September 18, 2026 11:29
@codecov

codecov Bot commented Sep 18, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 77.03%. Comparing base (addb6bf) to head (3a3d5aa).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8565      +/-   ##
==========================================
+ Coverage   77.02%   77.03%   +0.01%     
==========================================
  Files         465      466       +1     
  Lines       24923    24934      +11     
  Branches     6636     6638       +2     
==========================================
+ Hits        19197    19208      +11     
  Misses       5726     5726              
Flag Coverage Δ
backend 77.03% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@cstns
cstns merged commit 743ae9a into main Sep 18, 2026
36 of 38 checks passed
@cstns
cstns deleted the fix-mcp-topic-safe-session-id branch September 18, 2026 12:10

This branch was successfully deployed

1 active deployment
staging 3a3d5aaa Deployed Sep 18, 2026 by cstns via Remove application #11748
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.

openai/session breaks the MCP gateway topic, denying every request from OpenAI clients

2 participants