From 304644265e01431135a7e8a233c5d200ea73fa8d Mon Sep 17 00:00:00 2001 From: ForisKuang Date: Fri, 7 Aug 2026 12:34:37 -0400 Subject: [PATCH 1/3] fix(mcp): resolve {{LIBRECHAT_USER_ID}} in loadTools' MCP tool-loading path MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PRs #29-#31 fixed user-id propagation into MCP headers across the agent client, openai controller, and responses controller, but loadTools() (api/app/clients/tools/util/handleTools.js) builds the MCP `user` object from `options.req?.user` alone via createSafeUser. When req.user is a plain object deserialized from the passport session (no `id` virtual, no `_id`), that resolves to no usable id — even though loadTools is already given the caller's resolved id via its own `user` param (ToolService.js calls loadTools({ user: req.user.id, ... }), and that same `user` value is already used directly for auth lookups a few lines up). Confirmed live in production: mcp.tool/* spans on cbioportal-mcp still shipped enduser.id="{{LIBRECHAT_USER_ID}}" well after PR #31 deployed. Thread the existing `user` param through as createSafeUser's fallbackId, matching the pattern PR #31 established elsewhere. Adds regression tests exercising the MCP 'all'-tools branch (the one that calls createMCPTools), verifying the fallback fires when req.user has neither id nor _id, and that an explicit req.user.id still takes precedence. Co-Authored-By: Claude Sonnet 5 --- api/app/clients/tools/util/handleTools.js | 3 +- .../clients/tools/util/handleTools.test.js | 48 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/api/app/clients/tools/util/handleTools.js b/api/app/clients/tools/util/handleTools.js index adeb9f7ca99..d6244dfe0ab 100644 --- a/api/app/clients/tools/util/handleTools.js +++ b/api/app/clients/tools/util/handleTools.js @@ -451,10 +451,11 @@ const loadTools = async ({ /** MCP server tools are initialized sequentially by server */ let index = -1; const failedMCPServers = new Set(); - const safeUser = createSafeUser(options.req?.user); + const safeUser = createSafeUser(options.req?.user, user); const requestScopedConnections = options.requestScopedConnections ?? getMCPRequestContext(options.req, options.res); + for (const [serverName, toolConfigs] of Object.entries(requestedMCPTools)) { index++; /** @type {LCAvailableTools} */ diff --git a/api/app/clients/tools/util/handleTools.test.js b/api/app/clients/tools/util/handleTools.test.js index 697649e3bde..9870650772f 100644 --- a/api/app/clients/tools/util/handleTools.test.js +++ b/api/app/clients/tools/util/handleTools.test.js @@ -451,4 +451,52 @@ describe('Tool Handlers', () => { ); }); }); + + describe('loadTools MCP user id propagation', () => { + // Regression guard: MCP tool-call requests shipped the literal "{{LIBRECHAT_USER_ID}}" + // placeholder in x-user-id headers because `loadTools` built the MCP `user` object from + // `options.req?.user` alone. When `req.user` is a plain object deserialized from the + // passport session (no `id` virtual, no `_id`), that object resolves to no usable id even + // though `loadTools` is already given the caller's resolved id via its own `user` param + // (see ToolService.js: `loadTools({ user: req.user.id, ... })`). Assert that id makes it + // into the object handed to `createMCPTools`. + const mcpServerName = 'test-mcp-server'; + const mcpAllToolName = `${Constants.mcp_all}${Constants.mcp_delimiter}${mcpServerName}`; + + beforeEach(() => { + mockGetServerConfig.mockResolvedValue({ startup: true }); + mockCreateMCPTools.mockResolvedValue([]); + }); + + it('falls back to the loadTools `user` param when req.user has neither id nor _id', async () => { + const userId = fakeUser._id.toString(); + const sessionUser = { email: 'fakeuser@example.com', provider: 'local' }; + + await loadTools({ + user: userId, + tools: [mcpAllToolName], + options: { req: { user: sessionUser } }, + }); + + expect(mockCreateMCPTools).toHaveBeenCalledWith( + expect.objectContaining({ + user: expect.objectContaining({ id: userId }), + }), + ); + }); + + it('prefers a resolvable req.user id over the loadTools `user` param', async () => { + await loadTools({ + user: fakeUser._id.toString(), + tools: [mcpAllToolName], + options: { req: { user: { id: 'explicit-req-user-id' } } }, + }); + + expect(mockCreateMCPTools).toHaveBeenCalledWith( + expect.objectContaining({ + user: expect.objectContaining({ id: 'explicit-req-user-id' }), + }), + ); + }); + }); }); From b72fc3018a41a728a2dc3ed2f404b67cbe40c2fb Mon Sep 17 00:00:00 2001 From: Ino de Bruijn Date: Fri, 7 Aug 2026 21:19:21 +0000 Subject: [PATCH 2/3] Include conversation_starters in the VIEW safe-list test expectation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up to the v0.8.7 rebase: our fork intentionally exposes `conversation_starters` on the list-agents endpoint via commit b044fe8be ("Fix agent conversation starters not returned by list endpoint") so that the landing page can render them without a second per-agent fetch. Upstream added a "mass assignment protection" security test that asserts the exact safe-list of returned fields — that test doesn't know about our extension and fails on any PR against v0.8.7-custom-v1. Rather than undo b044fe8be's projection change (which would break the landing UI), extend the test's expected list to reflect what our fork intentionally returns. Distinct from the mcp user-id fix that opened this PR, but folded in so v0.8.7-custom-v1 gets a clean CI baseline. --- api/server/controllers/agents/v1.spec.js | 1 + 1 file changed, 1 insertion(+) diff --git a/api/server/controllers/agents/v1.spec.js b/api/server/controllers/agents/v1.spec.js index fda2bdd6167..7f9000c48a7 100644 --- a/api/server/controllers/agents/v1.spec.js +++ b/api/server/controllers/agents/v1.spec.js @@ -1345,6 +1345,7 @@ describe('Agent Controllers - Mass Assignment Protection', () => { 'author', 'avatar', 'category', + 'conversation_starters', 'description', 'id', 'is_promoted', From 6d837f5ce2c3556ad6f3d037efbe58d9a05f0b3c Mon Sep 17 00:00:00 2001 From: Ino de Bruijn Date: Fri, 7 Aug 2026 21:23:42 +0000 Subject: [PATCH 3/3] mcp.spec: expect _id to resolve LIBRECHAT_USER_ID (fork behavior) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second follow-up to the v0.8.7 rebase. This test asserted the pre-fork behavior ("we only check id") — but our fork intentionally treats _id as a fallback for id via the createSafeUser + loadTools chain from PRs #29/#30/#31 and #32 (the fix this PR ports). The whole point of that chain is that passport-deserialized session user objects carry _id but not id, and MCP tool calls previously shipped the literal placeholder {{LIBRECHAT_USER_ID}} instead of the resolved id. Update the "_id only" assertion to reflect what our fork actually does. The paired "id takes precedence when both are present" assertion below is already correct and unchanged. --- packages/api/src/mcp/__tests__/mcp.spec.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/api/src/mcp/__tests__/mcp.spec.ts b/packages/api/src/mcp/__tests__/mcp.spec.ts index 539c04f6b94..ef6d2a1a497 100644 --- a/packages/api/src/mcp/__tests__/mcp.spec.ts +++ b/packages/api/src/mcp/__tests__/mcp.spec.ts @@ -586,7 +586,9 @@ describe('Environment Variable Extraction (MCP)', () => { const result1 = processMCPEnv({ options: obj1, user: userWithId }); expect('headers' in result1 && result1.headers?.['User-Id']).toBe('user-123'); - // Test with '_id' property only (should not work since we only check 'id') + // Test with '_id' property only. Our fork accepts _id as a fallback for id + // (PRs #29/#30/#31/#32 threaded this through createSafeUser and loadTools) + // because passport-deserialized session user objects only carry _id. const userWithUnderscore = createTestUser({ id: undefined, // Remove default id to test _id _id: 'user-456', @@ -601,8 +603,7 @@ describe('Environment Variable Extraction (MCP)', () => { }; const result2 = processMCPEnv({ options: obj2, user: userWithUnderscore }); - // Since we don't check _id, the placeholder should remain unchanged - expect('headers' in result2 && result2.headers?.['User-Id']).toBe('{{LIBRECHAT_USER_ID}}'); + expect('headers' in result2 && result2.headers?.['User-Id']).toBe('user-456'); // Test with both properties (id takes precedence) const userWithBoth = createTestUser({