From bff9bfea87db063f8b8434434f26e73a310734b3 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 4 May 2026 23:19:50 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix:=20Propagate=20User=20Identi?= =?UTF-8?q?ty=20to=20Subagent=20MCP=20Tool=20Calls=20(#12950)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🐛 fix: Propagate User Identity to Subagent MCP Tool Calls The `@librechat/agents` SDK's `SubagentExecutor` invokes the child workflow with a fresh configurable of `{ thread_id }` only — the parent's `user` / `user_id` are dropped on the way into the child graph. The child's `ToolNode` then dispatches `ON_TOOL_EXECUTE` to the parent's handler, which merges `{ ...configurable, ...toolConfigurable }`, but neither side carries user identity for subagents. Downstream MCP tools read `config.configurable.user?.id || user_id` and got `undefined`, so `MCPManager.getConnection` fell through to the "No connection found for server X" error path — it can't reach the user-connection lookup without a userId. Re-inject `user` (via `createSafeUser`) and `user_id` from `req.user` into the configurable returned by `loadToolsForExecution`. This is the single point all controllers (chat, Responses API, OpenAI-compat) flow through. For the parent agent it's a no-op (outer config already carries the same values); for subagents it fills the gap so MCP connection lookup, user-placeholder substitution, and tools that read configurable.user all work correctly. * 🐛 fix: Preserve `api-user` Fallback When Injecting Subagent Identity Codex review pointed out that the prior commit unconditionally wrote `user_id: req.user?.id` (and `user`) into `toolConfigurable`. The handler merges via `{ ...configurable, ...toolConfigurable }` — `toolConfigurable` wins — so when `req.user` is absent, this overwrote the outer config's `'api-user'` fallback (set by `responses.js` / `openai.js` for the unauthenticated API-key path) with `undefined`, breaking MCP connection lookup for that path. Only inject the keys when `req.user.id` is truthy. Omitting them lets the merge preserve whatever the outer configurable already had. Tests updated to assert key omission for `req.user` undefined / null / present without `id`. * 🩹 fix: Narrow `IUser.id` to required string `IUser` extends mongoose `Document`, which types `id?: any` (the optional virtual). At runtime `id` is always `_id.toString()` for a hydrated doc, so narrow the type to a required string. Closes two `@rollup/plugin-typescript` TS2322 warnings introduced by PR #12450 (OIDC Bearer Token Authentication for Remote Agent API) where `req.user = userResolution.user` and the `(req: Request, res: Response, next: NextFunction)` signature both failed against the project's local `Express.User` augmentation (`{ [key: string]: any; id: string; }`) because `IUser.id` was `any`/optional. Narrowing here fixes both at the source rather than casting at every assignment site. * 🩹 fix: Resolve TS Build Warnings Surfaced by `IUser.id` Narrowing Three rollup TS plugin warnings surfaced after narrowing `IUser.id` from `any` to `string`: - `utils/env.ts:95` — `safeUser[field] = user[field]` failed strict checking because indexed write through a union-typed key collapses the LHS to the intersection of all field write types (i.e., `undefined` when fields have mixed types). The previous `id?: any` on IUser had been masking this. Switch to `Object.assign(safeUser, { [field]: user[field] })` which widens the assignment. - `endpoints/google/initialize.ts:35` — `getUserKey({ userId: req.user?.id, ... })` failed because `req.user?.id` is now `string | undefined` (no longer `any`). Match the pattern already used in `endpoints/openAI/initialize.ts:49`: `req.user?.id ?? ''`. - `middleware/remoteAgentAuth.ts:465` — pre-existing, unrelated to the IUser change. The local (gitignored) `express.d.ts` augments `express.Request` but not `express-serve-static-core.Request`, so the explicit `(req: Request, ...)` annotation imported from `'express'` resolves to a Request whose `req.user` differs from the one `RequestHandler` expects internally. Type the closure as `RequestHandler` directly so TS infers params from the augmented type. * 🩹 fix: Cast `RemoteAgentAuth` Closure to `RequestHandler` My previous attempt removed the explicit `req: Request` annotation on the closure to side-step the outer `RequestHandler` mismatch. That shifted the error to every helper call site inside the closure (`getConfigOptions(req)`, `runApiKeyAuth(req, ...)` at 467/474/493/ 512/531), because the helpers annotate their params with `express.Request` (which has the local `Request.user` augmentation), while the unannotated closure inferred `req` as `express-serve-static-core.Request` (no augmentation). Reproduced locally by stubbing the gitignored `src/types/express.d.ts`. Right approach: keep the explicit `req: Request` annotation so the closure body matches the helpers' types, then cast at the return — `RequestHandler`'s internal `Request` resolves through `express-serve-static-core` and lacks the augmentation, so the cast is the boundary that bridges the two views of `req.user`. Verified against a build with the local express.d.ts stub: zero warnings on `remoteAgentAuth.ts`, `env.ts`, and `google/initialize.ts`. --- api/server/services/ToolService.js | 29 +++++++ .../services/__tests__/ToolService.spec.js | 82 +++++++++++++++++++ .../api/src/endpoints/google/initialize.ts | 2 +- .../api/src/middleware/remoteAgentAuth.ts | 11 ++- packages/api/src/utils/env.ts | 9 +- packages/data-schemas/src/types/user.ts | 8 ++ 6 files changed, 138 insertions(+), 3 deletions(-) diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index e15d802973..747e2401bb 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -12,6 +12,7 @@ const { const { sendEvent, getToolkitKey, + createSafeUser, getUserMCPAuthMap, loadToolDefinitions, GenerationJobManager, @@ -1252,7 +1253,35 @@ async function loadToolsForExecution({ }) { const appConfig = req.config; const allLoadedTools = []; + /** + * Re-inject user identity into the configurable returned to the + * `ON_TOOL_EXECUTE` handler. The handler merges this onto whatever + * configurable arrived with the dispatched event: + * `{ ...configurable, ...toolConfigurable }` + * For the parent agent the outer stream config (set in AgentClient / + * Responses / OpenAI controllers) already carries `user` and `user_id`; + * re-injecting the same values from `req.user` is a no-op there. + * + * For SUBAGENT tool calls the SDK's `SubagentExecutor` invokes the child + * workflow with a fresh configurable of `{ thread_id }` only — the + * parent's `user`, `user_id`, and `userMCPAuthMap` are dropped on the way + * into the child graph. Without this re-injection, downstream MCP tools + * read `config.configurable.user?.id || user_id === undefined` and + * `MCPManager.getConnection` throws "No connection found for server X" + * (it can't fall through to the user-connection path without a userId). + * + * Only set the keys when `req.user.id` is present. Because + * `toolConfigurable` wins the merge, writing `user_id: undefined` + * unconditionally would clobber the outer config's `'api-user'` + * fallback (set by Responses/OpenAI controllers when `req.user` is + * absent). Omitting the keys lets the merge preserve whatever the + * outer configurable already had. + */ const configurable = { userMCPAuthMap }; + if (req.user?.id) { + configurable.user = createSafeUser(req.user); + configurable.user_id = req.user.id; + } if (actionsEnabled === undefined) { const enabledCapabilities = await resolveAgentCapabilities(req, appConfig, agent?.id); diff --git a/api/server/services/__tests__/ToolService.spec.js b/api/server/services/__tests__/ToolService.spec.js index 63b2287684..86f00f7562 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -363,6 +363,88 @@ describe('ToolService - Action Capability Gating', () => { }); }); + describe('loadToolsForExecution — configurable user injection', () => { + /** + * Subagents invoked via `@librechat/agents`' SubagentExecutor receive + * a fresh `configurable` of `{ thread_id }` only — the parent's + * `user`/`user_id` are dropped on the way into the child workflow. + * The handler in `packages/api/src/agents/handlers.ts` merges + * `{ ...configurable, ...toolConfigurable }`, so the configurable + * returned here MUST carry the user identity for MCP connection + * lookup to succeed in subagent tool calls. + * + * Conversely, when `req.user` is absent (API-key path) we must NOT + * write `user`/`user_id` keys, otherwise the merge would clobber the + * outer config's `'api-user'` fallback set by Responses/OpenAI + * controllers. + */ + it('returns configurable with user and user_id from req.user', async () => { + const req = createMockReq([]); + req.config = {}; + req.user = { id: 'user_abc', email: 'a@b.c', role: 'USER' }; + + const result = await loadToolsForExecution({ + req, + res: {}, + agent: { id: 'agent_123' }, + toolNames: [], + }); + + expect(result.configurable.user_id).toBe('user_abc'); + expect(result.configurable.user).toEqual( + expect.objectContaining({ id: 'user_abc', email: 'a@b.c', role: 'USER' }), + ); + }); + + it('omits user keys when req.user is undefined so outer config fallback is preserved', async () => { + const req = createMockReq([]); + req.config = {}; + req.user = undefined; + + const result = await loadToolsForExecution({ + req, + res: {}, + agent: { id: 'agent_123' }, + toolNames: [], + }); + + expect('user_id' in result.configurable).toBe(false); + expect('user' in result.configurable).toBe(false); + }); + + it('omits user keys when req.user is null so outer config fallback is preserved', async () => { + const req = createMockReq([]); + req.config = {}; + req.user = null; + + const result = await loadToolsForExecution({ + req, + res: {}, + agent: { id: 'agent_123' }, + toolNames: [], + }); + + expect('user_id' in result.configurable).toBe(false); + expect('user' in result.configurable).toBe(false); + }); + + it('omits user keys when req.user is present but has no id', async () => { + const req = createMockReq([]); + req.config = {}; + req.user = { email: 'a@b.c' }; + + const result = await loadToolsForExecution({ + req, + res: {}, + agent: { id: 'agent_123' }, + toolNames: [], + }); + + expect('user_id' in result.configurable).toBe(false); + expect('user' in result.configurable).toBe(false); + }); + }); + describe('checkCapability logic', () => { const createCheckCapability = (enabledCapabilities, logger = { warn: jest.fn() }) => { return (capability) => { diff --git a/packages/api/src/endpoints/google/initialize.ts b/packages/api/src/endpoints/google/initialize.ts index 812769e030..dcf750bb87 100644 --- a/packages/api/src/endpoints/google/initialize.ts +++ b/packages/api/src/endpoints/google/initialize.ts @@ -32,7 +32,7 @@ export async function initializeGoogle({ let userKey = null; if (expiresAt && isUserProvided) { checkUserKeyExpiry(expiresAt, EModelEndpoint.google); - userKey = await db.getUserKey({ userId: req.user?.id, name: EModelEndpoint.google }); + userKey = await db.getUserKey({ userId: req.user?.id ?? '', name: EModelEndpoint.google }); } let serviceKey: Record = {}; diff --git a/packages/api/src/middleware/remoteAgentAuth.ts b/packages/api/src/middleware/remoteAgentAuth.ts index ab06583c41..0471352744 100644 --- a/packages/api/src/middleware/remoteAgentAuth.ts +++ b/packages/api/src/middleware/remoteAgentAuth.ts @@ -462,7 +462,15 @@ export function createRemoteAgentAuth({ updateUser, getAppConfig, }: RemoteAgentAuthDeps): RequestHandler { - return async (req: Request, res: Response, next: NextFunction) => { + /** + * Annotated as `express.Request` (and helpers below take the same type) + * so the local `Request.user` augmentation in `src/types/express.d.ts` + * applies inside the closure. The closure is then cast to + * `RequestHandler` at the return — `RequestHandler`'s internal + * `Request` resolves through `express-serve-static-core` and lacks the + * augmentation, so a direct return would mismatch on `user`. + */ + const handler = async (req: Request, res: Response, next: NextFunction) => { try { const initialConfigOptions = getConfigOptions(req); const config = await getAppConfig(initialConfigOptions); @@ -559,4 +567,5 @@ export function createRemoteAgentAuth({ return; } }; + return handler as RequestHandler; } diff --git a/packages/api/src/utils/env.ts b/packages/api/src/utils/env.ts index f71a131c09..b5220f1ae8 100644 --- a/packages/api/src/utils/env.ts +++ b/packages/api/src/utils/env.ts @@ -92,7 +92,14 @@ export function createSafeUser( const safeUser: Partial & { federatedTokens?: IUser['federatedTokens'] } = {}; for (const field of ALLOWED_USER_FIELDS) { if (field in user) { - safeUser[field] = user[field]; + /** + * Indexed write through a union-typed key would otherwise fail strict + * checking — TS computes the LHS type as the *intersection* of all + * field write types (which collapses to `undefined` when fields have + * mixed types). `Object.assign` widens the assignment so each field + * preserves its concrete type at runtime. + */ + Object.assign(safeUser, { [field]: user[field] }); } } diff --git a/packages/data-schemas/src/types/user.ts b/packages/data-schemas/src/types/user.ts index d4ab3356f9..86d382a711 100644 --- a/packages/data-schemas/src/types/user.ts +++ b/packages/data-schemas/src/types/user.ts @@ -4,6 +4,14 @@ import { CursorPaginationParams } from '~/common'; export interface IUser extends Document { _id: Types.ObjectId; + /** + * Mongoose's `Document.id` virtual is typed `id?: any`. At runtime it's + * always `_id.toString()` for a hydrated doc, so narrow to a required + * string. This also lets `IUser` satisfy Express.User augmentations + * (the OIDC remote-agent middleware assigns `req.user = IUser` where + * the project's local `Express.User` requires `id: string`). + */ + id: string; name?: string; username?: string; email: string;