🛂 fix: Enforce MCP Permissions for Agent Tools (#13174)

* fix: Enforce MCP Permissions for Agent Tools

* fix: Measure MCP Image Limit by Decoded Size

* fix: gate cached MCP tools and tighten remote image URL detection

Addresses Codex review findings on the MCP permissions PR:

- filterAuthorizedTools previously fast-accepted any tool present in the
  global tool cache before reaching the MCP-use permission gate. App-level
  MCP tools (keyed `name_mcp_server` by MCPServerInspector and merged into
  the cache via mergeAppTools) therefore bypassed the canUseMCP check,
  letting a user without MCP_SERVERS.USE persist/bind them. Route all
  MCP-delimited tools through the permission + server-access gate
  regardless of cache presence.

- assertImageDataWithinLimit / image formatter used startsWith("http")
  to skip the size cap, which also matched base64 payloads that happen to
  begin with those chars. Require http:// or https:// via a shared
  isRemoteImageUrl helper so oversized inline base64 can no longer bypass
  MCP_IMAGE_DATA_MAX_BYTES.

Adds regression tests for both paths.

* fix: address Codex round-2 findings on MCP permissions PR

- parsers.ts: parseAsString dropped the image payload for unrecognized
  providers, returning only `Image result: <mimeType>`. Pre-PR these
  items survived via JSON.stringify(item). Keep the size guard but fall
  through to JSON.stringify so the data/URL is preserved.

- MCP.js: the runtime MCP-use check only read `configurable.user`, so
  paths that propagate `user_id` only (e.g. the OpenAI-compatible API in
  agents/openai/service.ts) rejected every MCP tool call for an
  authenticated user. Add resolveMCPPermissionUser: use the safe user
  directly when it already carries a role (no extra DB call), otherwise
  fall back to loading the role by user_id. Update fail-closed tests to
  the resolved behavior.

- v1.js: the update path only re-filtered newly added MCP tools, so a
  user who lost MCP_SERVERS.USE kept existing MCP bindings on edit while
  create/duplicate/revert stripped them. Strip all MCP tools on update
  when the permission is revoked; keep the narrower new-tool gating (and
  disconnect/registry preservation) when it is intact.

Updates and adds regression tests for all three paths.

* fix: populate safe user at producer instead of resolving in runtime MCP check

Corrects the Finding B approach from the previous commit. Rather than
loading the user by id inside the runtime MCP permission check, populate
`configurable.user` (and createRun's `user`) with the full safe user at
the producer, matching the in-repo agent controllers
(responses.js / openai.js) which already pass `createSafeUser(req.user)`.

- service.ts: derive `safeUser` via createSafeUser(req.user) and pass it
  to both createRun and processStream's configurable, so the role-bearing
  identity reaches the runtime `userCanUseMCPServers(configurable.user)`
  check. Falls back to a bare id when the host app attached no user,
  which correctly leaves MCP gated (fail closed).
- MCP.js: revert the resolveMCPPermissionUser DB-load fallback; the
  runtime check again reads configurable.user directly and fails closed
  when absent (defense in depth).
- MCP.spec.js: revert to the matching runtime test expectations.

* test: cover safe-user propagation in createAgentChatCompletion

Adds a focused spec for the OpenAI-compatible chat completion service
(the producer fixed for Codex Finding B). Injects mocked deps and asserts
that createRun and processStream's configurable.user carry the role from
req.user (with sensitive fields stripped by createSafeUser), and that an
unauthenticated request falls back to a bare { id: 'api-user' } so the
runtime MCP check fails closed.

* fix: address Codex round-3 findings + TS6133

- MCP.js (P1): the assistants required-action path invokes tool._call(
  toolInput) with no LangChain config, so the runtime check saw no
  configurable.user and rejected authorized users. createToolInstance now
  captures the creation-time user (req.user via createMCPTool) and _call
  falls back to it for both the permission check and userId. Still fails
  closed when neither config nor captured user carries a role.

- v1.js (P2): the update-path isMCPTool used a bare mcp_delimiter substring
  check, misclassifying action tools whose operationId contains "_mcp_"
  (e.g. sync_mcp_state_action_...) as MCP and dropping them on a
  permission-revoked edit. Delegate to the canonical isActionTool so only
  real MCP tools are gated. Regression test added.

- service.ts: drop the now-unused IUser import (TS6133); derive reqUser's
  type from createSafeUser's own parameter instead.

* fix: resolve TS7022 self-reference in service.spec mock res

The mock response object referenced `res` inside its own `status`/`json`
initializers without a type annotation, so tsc inferred `res` as `any`
(TS7022). Annotate the object and assign the self-referencing chainable
methods after declaration.

* fix: correct round-4 findings (isActionTool import, captured user, partial-update)

- v1.js: import isActionTool from librechat-data-provider (its real export;
  @librechat/api does not export it, so the prior import was undefined and
  threw TypeError). Exclude action tools from MCP classification in both the
  main filterAuthorizedTools loop and the update path, so action tools whose
  operationId contains _mcp_ (e.g. sync_mcp_state_action_...) are preserved
  regardless of MCP permission.
- v1.js: evaluate the effective tool set (updateData.tools ?? existingAgent.tools)
  so a tools-less PATCH by a user who lost MCP_SERVERS.USE still strips stale
  MCP bindings, matching create/duplicate/revert.
- MCP.js: createToolInstance now receives the construction-time user and _call
  falls back to it (permissionUser) when configurable.user is absent, fixing the
  assistants required-action path that invokes _call without a config and
  resolving the capturedUser no-undef/ReferenceError.
- Tests: action-tool preservation (authorized + denied), tools-less revocation
  PATCH, updated revocation test to expect all MCP tools stripped.

Affected specs pass locally: MCP 49/49, filterAuthorizedTools 49/49.

* fix: guard isActionTool against non-string tools; correct actionDelimiter import

Two test regressions from the prior commit:
- The main filterAuthorizedTools loop called isActionTool(tool) directly,
  but isActionTool does toolName.indexOf(...) and throws on null/undefined.
  Compute isActionToolName = typeof tool === 'string' && isActionTool(tool)
  once and reuse it, restoring graceful null/undefined handling.
- The action-tool test referenced Constants.actionDelimiter (undefined);
  actionDelimiter is a standalone librechat-data-provider export. Import and
  use it directly.

filterAuthorizedTools 36/36 and MCP 40/40 pass locally.

* fix: address MCP permission review follow-ups

* fix: preserve shared agent MCP tools

packages/api/src/agents/openai/service.spec.ts

@@ -0,0 +1,107 @@
+import { createAgentChatCompletion } from './service';
+import type { ChatCompletionDependencies } from './service';
+
+jest.mock('@librechat/data-schemas', () => ({
+  logger: {
+    debug: jest.fn(),
+    error: jest.fn(),
+    info: jest.fn(),
+    warn: jest.fn(),
+  },
+}));
+
+type CreateRunArgs = { user?: Record<string, unknown> };
+type ProcessStreamConfig = { configurable?: Record<string, unknown> };
+
+function createMockReq(user?: Record<string, unknown>) {
+  return {
+    body: {
+      model: 'agent_test',
+      messages: [{ role: 'user', content: 'hi' }],
+      stream: false,
+    },
+    user,
+    on: jest.fn(),
+  } as unknown as Parameters<typeof createAgentChatCompletion>[0];
+}
+
+function createMockRes() {
+  const res: Record<string, unknown> = {
+    setHeader: jest.fn(),
+    flushHeaders: jest.fn(),
+    write: jest.fn(),
+    end: jest.fn(),
+    headersSent: false,
+  };
+  res.status = jest.fn(() => res);
+  res.json = jest.fn(() => res);
+  return res as unknown as Parameters<typeof createAgentChatCompletion>[1];
+}
+
+describe('createAgentChatCompletion - MCP permission user propagation', () => {
+  let createRun: jest.Mock;
+  let processStream: jest.Mock;
+  let deps: ChatCompletionDependencies;
+
+  beforeEach(() => {
+    processStream = jest.fn().mockResolvedValue(undefined);
+    createRun = jest.fn().mockResolvedValue({ processStream });
+
+    deps = {
+      getAgent: jest.fn().mockResolvedValue({
+        id: 'agent_test',
+        provider: 'openai',
+        model: 'gpt-4o-mini',
+        tools: [],
+      }),
+      initializeAgent: jest.fn().mockResolvedValue({
+        id: 'agent_test',
+        provider: 'openai',
+        model: 'gpt-4o-mini',
+        tools: [],
+        attachments: [],
+        toolContextMap: {},
+        maxContextTokens: 1000,
+        model_parameters: {},
+      }),
+      createRun: createRun as unknown as ChatCompletionDependencies['createRun'],
+    };
+  });
+
+  it('forwards the role-bearing safe user to createRun and configurable.user', async () => {
+    const req = createMockReq({
+      id: 'user-123',
+      role: 'ADMIN',
+      email: 'admin@example.com',
+      password: 'secret',
+    });
+
+    await createAgentChatCompletion(req, createMockRes(), deps);
+
+    expect(createRun).toHaveBeenCalledTimes(1);
+    const runArgs = createRun.mock.calls[0][0] as CreateRunArgs;
+    expect(runArgs.user).toMatchObject({ id: 'user-123', role: 'ADMIN' });
+    // createSafeUser must strip sensitive fields.
+    expect(runArgs.user).not.toHaveProperty('password');
+
+    expect(processStream).toHaveBeenCalledTimes(1);
+    const streamConfig = processStream.mock.calls[0][1] as ProcessStreamConfig;
+    expect(streamConfig.configurable?.user).toMatchObject({ id: 'user-123', role: 'ADMIN' });
+    expect(streamConfig.configurable?.user_id).toBe('user-123');
+  });
+
+  it('falls back to a bare id when no authenticated user is attached', async () => {
+    const req = createMockReq(undefined);
+
+    await createAgentChatCompletion(req, createMockRes(), deps);
+
+    expect(createRun).toHaveBeenCalledTimes(1);
+    const runArgs = createRun.mock.calls[0][0] as CreateRunArgs;
+    expect(runArgs.user).toEqual({ id: 'api-user' });
+
+    const streamConfig = processStream.mock.calls[0][1] as ProcessStreamConfig;
+    // No role present → the runtime MCP check fails closed.
+    expect(streamConfig.configurable?.user).toEqual({ id: 'api-user' });
+    expect(streamConfig.configurable?.user).not.toHaveProperty('role');
+  });
+});

packages/api/src/agents/openai/service.ts

@@ -39,6 +39,7 @@ import {
   createChunk,
   writeSSE,
 } from './handlers';
+import { createSafeUser } from '~/utils';
 import type { ToolExecuteOptions } from '../handlers';
 
 /**
@@ -500,7 +501,17 @@ export async function createAgentChatCompletion(
 
     // Create and run the agent
     if (deps.createRun) {
-      const userId = (req as unknown as { user?: { id?: string } }).user?.id ?? 'api-user';
+      const reqUser = (req as unknown as { user?: Parameters<typeof createSafeUser>[0] }).user;
+      const userId = reqUser?.id ?? 'api-user';
+      /**
+       * Propagate the full safe user (id + role), matching the in-repo agent
+       * controllers (responses.js / openai.js). The runtime MCP permission
+       * check reads `configurable.user`; passing only `user_id` would make
+       * every MCP tool call fail closed for an authenticated caller. When the
+       * host app didn't attach a user, this falls back to a bare id, which
+       * correctly leaves MCP gated.
+       */
+      const safeUser: Record<string, unknown> = { ...createSafeUser(reqUser), id: userId };
 
       const run = await deps.createRun({
         agents: [initializedAgent],
@@ -512,7 +523,7 @@ export async function createAgentChatCompletion(
           messageId: requestId,
           conversationId,
         },
-        user: { id: userId },
+        user: safeUser,
       });
 
       if (run) {
@@ -523,6 +534,7 @@ export async function createAgentChatCompletion(
             configurable: {
               thread_id: conversationId,
               user_id: userId,
+              user: safeUser,
             },
             signal: abortController.signal,
             streamMode: 'values',

packages/api/src/mcp/__tests__/parsers.test.ts

@@ -29,6 +29,18 @@ describe('formatToolContent', () => {
       expect(content).toBe('(No response)');
       expect(artifacts).toBeUndefined();
     });
+
+    it('should preserve the image payload in the string for unrecognized providers', () => {
+      const result: t.MCPToolCallResponse = {
+        content: [{ type: 'image', data: 'iVBORw0KGgoAAAA...', mimeType: 'image/png' }],
+      };
+
+      const [content, artifacts] = formatToolContent(result, 'unknown' as t.Provider);
+
+      expect(artifacts).toBeUndefined();
+      expect(content).toContain('iVBORw0KGgoAAAA...');
+      expect(content).toContain('image/png');
+    });
   });
 
   describe('recognized providers', () => {
@@ -91,6 +103,16 @@ describe('formatToolContent', () => {
   });
 
   describe('image handling', () => {
+    const originalMaxImageBytes = process.env.MCP_IMAGE_DATA_MAX_BYTES;
+
+    afterEach(() => {
+      if (originalMaxImageBytes === undefined) {
+        delete process.env.MCP_IMAGE_DATA_MAX_BYTES;
+        return;
+      }
+      process.env.MCP_IMAGE_DATA_MAX_BYTES = originalMaxImageBytes;
+    });
+
     it('should handle images with http URLs', () => {
       const result: t.MCPToolCallResponse = {
         content: [{ type: 'image', data: 'https://example.com/image.png', mimeType: 'image/png' }],
@@ -147,6 +169,83 @@ describe('formatToolContent', () => {
       expect(artifacts).toBeDefined();
       expect(artifacts?.content).toHaveLength(2);
     });
+
+    it('should reject oversized base64 image data before creating artifacts', () => {
+      process.env.MCP_IMAGE_DATA_MAX_BYTES = '3';
+      const result: t.MCPToolCallResponse = {
+        content: [{ type: 'image', data: 'QUJDRA==', mimeType: 'image/png' }],
+      };
+
+      expect(() => formatToolContent(result, 'openai')).toThrow(
+        'MCP image result exceeds maximum size of 3 bytes',
+      );
+    });
+
+    it('should allow base64 image data when decoded size is within the cap', () => {
+      process.env.MCP_IMAGE_DATA_MAX_BYTES = '4';
+      const result: t.MCPToolCallResponse = {
+        content: [{ type: 'image', data: 'QUJDRA==', mimeType: 'image/png' }],
+      };
+
+      const [content, artifacts] = formatToolContent(result, 'openai');
+
+      expect(content).toBe('');
+      expect(artifacts?.content?.[0]).toEqual({
+        type: 'image_url',
+        image_url: { url: 'data:image/png;base64,QUJDRA==' },
+      });
+    });
+
+    it('should reject oversized image data for unrecognized providers before stringifying', () => {
+      process.env.MCP_IMAGE_DATA_MAX_BYTES = '3';
+      const result: t.MCPToolCallResponse = {
+        content: [{ type: 'image', data: 'QUJDRA==', mimeType: 'image/png' }],
+      };
+
+      expect(() => formatToolContent(result, 'unknown' as t.Provider)).toThrow(
+        'MCP image result exceeds maximum size of 3 bytes',
+      );
+    });
+
+    it('should not apply the image data cap to remote image URLs', () => {
+      process.env.MCP_IMAGE_DATA_MAX_BYTES = '3';
+      const result: t.MCPToolCallResponse = {
+        content: [{ type: 'image', data: 'https://example.com/large.png', mimeType: 'image/png' }],
+      };
+
+      const [content, artifacts] = formatToolContent(result, 'openai');
+
+      expect(content).toBe('');
+      expect(artifacts?.content?.[0]).toEqual({
+        type: 'image_url',
+        image_url: { url: 'https://example.com/large.png' },
+      });
+    });
+
+    it('should enforce the image cap on base64 data that merely starts with "http"', () => {
+      process.env.MCP_IMAGE_DATA_MAX_BYTES = '3';
+      const result: t.MCPToolCallResponse = {
+        content: [{ type: 'image', data: 'httpAAAAAAAA', mimeType: 'image/png' }],
+      };
+
+      expect(() => formatToolContent(result, 'openai')).toThrow(
+        'MCP image result exceeds maximum size of 3 bytes',
+      );
+    });
+
+    it('should treat base64 starting with "http" as inline data, not a remote URL', () => {
+      const result: t.MCPToolCallResponse = {
+        content: [{ type: 'image', data: 'httpAAAA', mimeType: 'image/png' }],
+      };
+
+      const [content, artifacts] = formatToolContent(result, 'openai');
+
+      expect(content).toBe('');
+      expect(artifacts?.content?.[0]).toEqual({
+        type: 'image_url',
+        image_url: { url: 'data:image/png;base64,httpAAAA' },
+      });
+    });
   });
 
   describe('resource handling', () => {

packages/api/src/mcp/parsers.ts

@@ -3,10 +3,57 @@ import { Tools } from 'librechat-data-provider';
 import type { UIResource } from 'librechat-data-provider';
 import type * as t from './types';
 
+export const DEFAULT_MCP_IMAGE_DATA_MAX_BYTES = 10 * 1024 * 1024;
+
 function generateResourceId(text: string): string {
   return crypto.createHash('sha256').update(text).digest('hex').substring(0, 10);
 }
 
+function getMCPImageDataMaxBytes(): number {
+  const raw = process.env.MCP_IMAGE_DATA_MAX_BYTES;
+  if (!raw) {
+    return DEFAULT_MCP_IMAGE_DATA_MAX_BYTES;
+  }
+
+  const parsed = Number(raw);
+  return Number.isSafeInteger(parsed) && parsed > 0 ? parsed : DEFAULT_MCP_IMAGE_DATA_MAX_BYTES;
+}
+
+function getBase64Padding(data: string): number {
+  if (data.endsWith('==')) {
+    return 2;
+  }
+  if (data.endsWith('=')) {
+    return 1;
+  }
+  return 0;
+}
+
+function estimateBase64ImageBytes(data: string): number {
+  const padding = getBase64Padding(data);
+  return Math.max(0, Math.floor((data.length * 3) / 4) - padding);
+}
+
+function isRemoteImageUrl(data: string): boolean {
+  return data.startsWith('http://') || data.startsWith('https://');
+}
+
+function assertImageDataWithinLimit(item: t.ImageContent): void {
+  if (isRemoteImageUrl(item.data)) {
+    return;
+  }
+
+  const maxBytes = getMCPImageDataMaxBytes();
+  const estimatedBytes = estimateBase64ImageBytes(item.data);
+  if (estimatedBytes <= maxBytes) {
+    return;
+  }
+
+  throw new Error(
+    `MCP image result exceeds maximum size of ${maxBytes} bytes: ${estimatedBytes} bytes`,
+  );
+}
+
 const RECOGNIZED_PROVIDERS = new Set([
   'google',
   'anthropic',
@@ -38,7 +85,7 @@ const imageFormatters: Record<string, undefined | t.ImageFormatter> = {
   default: (item) => ({
     type: 'image_url',
     image_url: {
-      url: item.data.startsWith('http') ? item.data : `data:${item.mimeType};base64,${item.data}`,
+      url: isRemoteImageUrl(item.data) ? item.data : `data:${item.mimeType};base64,${item.data}`,
     },
   }),
 };
@@ -71,6 +118,9 @@ function parseAsString(result: t.MCPToolCallResponse): string {
         }
         return resourceText.join('\n');
       }
+      if (isImageContent(item)) {
+        assertImageDataWithinLimit(item);
+      }
       return JSON.stringify(item, null, 2);
     })
     .filter(Boolean)
@@ -120,6 +170,7 @@ export function formatToolContent(
       if (!isImageContent(item)) {
         return;
       }
+      assertImageDataWithinLimit(item);
       const formatter = imageFormatters.default as t.ImageFormatter;
       const formattedImage = formatter(item);