From 3a3d5aaa26a924722599fee8479e8819f87dd583 Mon Sep 17 00:00:00 2001 From: cstns Date: Fri, 18 Sep 2026 14:26:18 +0300 Subject: [PATCH] fix: make a third-party MCP session id safe for the gateway topic The session id becomes one level of the gateway's MQTT topic. openai/session is shaped `v1/`, 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. --- forge/comms/aclManager.js | 4 +- forge/comms/utils/mcpSessionId.js | 37 ++++++++++++++ forge/ee/routes/mcp/server.js | 4 +- test/unit/forge/comms/authRoutesV2_spec.js | 16 ++++++ .../forge/comms/utils/mcpSessionId_spec.js | 51 +++++++++++++++++++ test/unit/forge/ee/routes/mcp/server_spec.js | 38 ++++++++++++++ 6 files changed, 148 insertions(+), 2 deletions(-) create mode 100644 forge/comms/utils/mcpSessionId.js create mode 100644 test/unit/forge/comms/utils/mcpSessionId_spec.js diff --git a/forge/comms/aclManager.js b/forge/comms/aclManager.js index 40c0bbb481..5b54eab137 100644 --- a/forge/comms/aclManager.js +++ b/forge/comms/aclManager.js @@ -6,6 +6,8 @@ * * Other components (ie EE-specific features) can register their own additional ACLs */ +const { TOPIC_SAFE_SESSION_ID } = require('./utils/mcpSessionId') + module.exports = function (app) { const expertRbacToolCheck = async (teamMembership, toolName, application) => { const applicationCheck = typeof application !== 'undefined' @@ -666,7 +668,7 @@ module.exports = function (app) { if (!acl.allowWildcard?.session) { throw ValidationError('invalid session wildcard') } - } else if (mcpSessionId.length < 8) { + } else if (!TOPIC_SAFE_SESSION_ID.test(mcpSessionId)) { throw ValidationError('invalid mcp session id') } diff --git a/forge/comms/utils/mcpSessionId.js b/forge/comms/utils/mcpSessionId.js new file mode 100644 index 0000000000..071897143f --- /dev/null +++ b/forge/comms/utils/mcpSessionId.js @@ -0,0 +1,37 @@ +const crypto = require('node:crypto') + +/** + * The shape an MCP session id must have to be usable as a single MQTT topic level. + * + * An allow-list rather than a list of characters to strip: the value arrives from a + * third-party client, and an allow-list cannot be surprised by a separator nobody + * thought of. '/' splits the topic into extra levels, '+' and '#' are wildcards, and + * any of the three silently reshapes the topic so it matches no ACL pattern at all - + * the publish is then denied without an error, which is indistinguishable from the + * gateway simply never answering. + */ +const TOPIC_SAFE_SESSION_ID = /^[A-Za-z0-9_-]{8,128}$/ + +/** + * Returns a session id safe to embed as one level of an MQTT topic. + * + * Ids that already have the shape are passed through untouched, so a well-behaved + * client's session id stays readable in logs and on the wire. Anything else is hashed + * rather than stripped: stripping would collapse two distinct sessions onto one topic, + * and the mapping has to stay stable or a client's pinned tab is lost between its own + * requests. + * + * @param {string} sessionId The raw session id from the client + * @returns {string|null} A topic-safe id, or null if there was nothing usable + */ +function toTopicSafeSessionId (sessionId) { + if (typeof sessionId !== 'string' || sessionId.length === 0) { + return null + } + if (TOPIC_SAFE_SESSION_ID.test(sessionId)) { + return sessionId + } + return crypto.createHash('sha256').update(sessionId).digest('hex') +} + +module.exports = { TOPIC_SAFE_SESSION_ID, toTopicSafeSessionId } diff --git a/forge/ee/routes/mcp/server.js b/forge/ee/routes/mcp/server.js index 83cacba2f8..b6c78cafb1 100644 --- a/forge/ee/routes/mcp/server.js +++ b/forge/ee/routes/mcp/server.js @@ -1,5 +1,7 @@ const { randomUUID } = require('node:crypto') +const { toTopicSafeSessionId } = require('../../../comms/utils/mcpSessionId') + // Maps mcpSessionId to the third-party caller's PAT, consumed by the comms layer. const MCP_SESSION_TOKEN_CACHE = 'mcp-session-token' const MCP_SESSION_TOKEN_CACHE_TTL = 1000 * 60 * 60 // 1 hour @@ -86,7 +88,7 @@ module.exports = async function (app) { } const mcpSessionId = request.headers['mcp-session-id'] || - mcpBody.params?._meta?.['openai/session'] || + toTopicSafeSessionId(mcpBody.params?._meta?.['openai/session']) || randomUUID() const authHeader = request.headers.authorization || '' const token = authHeader.startsWith('Bearer ') ? authHeader.slice('Bearer '.length) : null diff --git a/test/unit/forge/comms/authRoutesV2_spec.js b/test/unit/forge/comms/authRoutesV2_spec.js index 50bf0c81fa..1b1ff5a4f7 100644 --- a/test/unit/forge/comms/authRoutesV2_spec.js +++ b/test/unit/forge/comms/authRoutesV2_spec.js @@ -1880,6 +1880,22 @@ describe('Broker Auth v2 API', async function () { topic: `ff/v1/mcp/${OTHER_PLATFORM_ID}/${TestObjects.alice.hashid}/short/request` }) }) + it('denies an mcp request with a wildcard character in the session id', async function () { + await denyWrite({ + username: 'forge_platform', + topic: `ff/v1/mcp/${OTHER_PLATFORM_ID}/${TestObjects.alice.hashid}/sess+ion12345/request` + }) + await denyWrite({ + username: 'forge_platform', + topic: `ff/v1/mcp/${OTHER_PLATFORM_ID}/${TestObjects.alice.hashid}/sess#ion12345/request` + }) + }) + it('allows an mcp request with a hashed session id', async function () { + await allowWrite({ + username: 'forge_platform', + topic: `ff/v1/mcp/${OTHER_PLATFORM_ID}/${TestObjects.alice.hashid}/${'a1b2c3d4'.repeat(8)}/request` + }) + }) it('denies an mcp request for an unknown user', async function () { await denyWrite({ username: 'forge_platform', diff --git a/test/unit/forge/comms/utils/mcpSessionId_spec.js b/test/unit/forge/comms/utils/mcpSessionId_spec.js new file mode 100644 index 0000000000..22dbb24041 --- /dev/null +++ b/test/unit/forge/comms/utils/mcpSessionId_spec.js @@ -0,0 +1,51 @@ +const should = require('should') + +const FF_UTIL = require('flowforge-test-utils') + +const { TOPIC_SAFE_SESSION_ID, toTopicSafeSessionId } = FF_UTIL.require('forge/comms/utils/mcpSessionId') + +describe('MCP session id topic safety', function () { + it('passes an already-safe id through untouched', function () { + const uuid = '3f702f9d-47d1-4800-a92f-a86772c63559' + toTopicSafeSessionId(uuid).should.equal(uuid) + toTopicSafeSessionId('abc_123-XYZ').should.equal('abc_123-XYZ') + }) + + // OpenAI's clients send `v1/` in _meta['openai/session']. Embedded raw it adds a + // topic level, which matches no ACL pattern and is denied without an error. + it('rewrites an id carrying an MQTT separator', function () { + const openai = 'v1/3bjqKQlGRjpIMC9JfN8ZOLOI6XvwTstDuqZYmPAjNvBd9ZNRmU3NmyD4iT8CSJsVbFrSDHk0sSgz' + const safe = toTopicSafeSessionId(openai) + safe.should.not.containEql('/') + safe.should.match(TOPIC_SAFE_SESSION_ID) + }) + + it('rewrites ids carrying MQTT wildcards', function () { + toTopicSafeSessionId('abc+def123').should.match(TOPIC_SAFE_SESSION_ID) + toTopicSafeSessionId('abc#def123').should.match(TOPIC_SAFE_SESSION_ID) + }) + + // A pinned tab is keyed by session id, so the same client must map to the same topic + // on every request or its pin is unreachable by the next call. + it('is stable for the same input', function () { + const openai = 'v1/some-session-token' + toTopicSafeSessionId(openai).should.equal(toTopicSafeSessionId(openai)) + }) + + it('keeps distinct ids distinct', function () { + toTopicSafeSessionId('v1/aaa').should.not.equal(toTopicSafeSessionId('v1/bbb')) + }) + + // The rewritten value is returned to the client, which may send it back to us + it('is idempotent', function () { + const once = toTopicSafeSessionId('v1/some-session-token') + toTopicSafeSessionId(once).should.equal(once) + }) + + it('rejects a too-short or unusable id', function () { + toTopicSafeSessionId('short').should.match(TOPIC_SAFE_SESSION_ID) + should.not.exist(toTopicSafeSessionId('')) + should.not.exist(toTopicSafeSessionId(null)) + should.not.exist(toTopicSafeSessionId(undefined)) + }) +}) diff --git a/test/unit/forge/ee/routes/mcp/server_spec.js b/test/unit/forge/ee/routes/mcp/server_spec.js index b97b01ae74..b784026c29 100644 --- a/test/unit/forge/ee/routes/mcp/server_spec.js +++ b/test/unit/forge/ee/routes/mcp/server_spec.js @@ -176,6 +176,44 @@ describe('MCP Platform Tools Server', function () { second.should.equal(first) }) + it('should make an openai/session carrying a separator safe for the topic', async function () { + const openaiSession = 'v1/3bjqKQlGRjpIMC9JfN8ZOLOI6XvwTstDuqZYmPAjNvBd9ZNRmU3NmyD4iT8CSJsVbFrSDHk0sSgz' + const response = await app.inject({ + method: 'POST', + url: '/mcp', + headers: { authorization: `Bearer ${TestObjects.alicePAT.token}` }, + payload: { + jsonrpc: '2.0', + method: 'tools/call', + id: 1, + params: { name: 'a-tool', _meta: { 'openai/session': openaiSession } } + } + }) + response.statusCode.should.equal(200) + const routed = proxyRequest.firstCall.args[0].mcpSessionId + routed.should.not.containEql('/') + routed.should.match(/^[A-Za-z0-9_-]{8,128}$/) + response.headers['mcp-session-id'].should.equal(routed) + }) + + it('should route the same openai/session to the same topic id every time', async function () { + const call = async () => app.inject({ + method: 'POST', + url: '/mcp', + headers: { authorization: `Bearer ${TestObjects.alicePAT.token}` }, + payload: { + jsonrpc: '2.0', + method: 'tools/call', + id: 1, + params: { name: 'a-tool', _meta: { 'openai/session': 'v1/stable-token' } } + } + }) + await call() + await call() + proxyRequest.secondCall.args[0].mcpSessionId + .should.equal(proxyRequest.firstCall.args[0].mcpSessionId) + }) + it('should prefer an explicit mcp-session-id over the openai/session meta', async function () { const response = await app.inject({ method: 'POST',