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

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')
                 }
 

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 }

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

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',

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/<token>` 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))
+    })
+})

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',