LibreChat/api/server/controllers/__tests__/UserController.mcpOAuth.spec.js
Danny Avila 49f4b659f6
🔐 fix: Honor Admin-Panel MCP Allowlist Overrides Without Restart (#13814)
* 🔐 fix: Honor Admin-Panel MCP Allowlist Overrides Without Restart

MCPServersRegistry was built once at boot from getAppConfig({ baseOnly:
true }), freezing allowedDomains/allowedAddresses to YAML. Admin-panel
mcpSettings overrides were ignored by both inspection (addServer/
reinspectServer/updateServer/lazyInitConfigServer) and runtime connection
enforcement (assertResolvedRuntimeConfigAllowed), so a domain allowed only
via the panel failed inspection and never connected.

Make the registry's effective allowlists mutable and refresh them from the
merged admin-panel config: seed at boot, and re-apply on every config
mutation via invalidateConfigCaches -> clearMcpConfigCache. Both inspection
and connection paths read the same getters, so both honor overrides without
a restart. Fail-safe: current allowlists are preserved when the merged read
fails.

* 🛡️ fix: Scope MCP allowlist refresh to global config, fail-safe on DB error

Address Codex P1 review findings on the allowlist-refresh path:

- Tenant-scoped config mutations no longer push one tenant's merged
  mcpSettings into the process-wide registry singleton (read by all MCP
  connection paths), which would leak allowlists across tenants. Only
  global (non-tenant) mutations refresh the registry; tenant mutations
  still evict the config-server cache.
- The refresh read now uses strictOverrides:true so a transient DB error
  throws instead of silently returning YAML base config — preserving the
  last-known allowlists rather than overwriting them with fallback values.
  Adds the strictOverrides option to getAppConfig (default off, no behavior
  change for existing callers).

* ♻️ refactor: Resolve MCP allowlists per-request (tenant-scoped) instead of a global singleton

Supersedes the prior global-mutation approach. MCP allowlists live in
mcpSettings, which is tenant/principal-scoped admin config, so a process-wide
singleton value is the wrong model — it caused cross-tenant bleed and stale
reads.

Instead, inject a resolver (from the app layer, where the merged config lives)
that the registry calls per inspection and per connection. It reads the ALS
tenant context via getAppConfig and accepts the acting user so user/role-scoped
overrides resolve; config-source inspection (no user) resolves at tenant scope.
Falls back to the YAML base allowlists when no resolver is set or the lookup
fails, so a transient error fails to the operator baseline rather than
disabling the allowlist.

Removes the now-unnecessary setAllowlists / boot-seed / invalidateConfigCaches
refresh / getAppConfig.strictOverrides machinery.

* 🔒 fix: Scope config-source cache by allowlist; resolve OAuth allowlists per-request

Address Codex review of the per-request resolver:

- Config-source cache key now folds in the resolved allowlists, not just the
  raw-config hash. Inspection results became allowlist-dependent, so without
  this a tenant whose allowlist rejects a URL could poison the shared key with
  an inspectionFailed stub for a tenant that allows it (and vice versa). The
  tenant-scoped allowlist is resolved once per ensureConfigServers pass and
  threaded through the cache key + inspection.
- The two remaining request-time OAuth allowlist reads now use the merged
  config instead of the YAML base getters: the fallback OAuth-initiate path
  (routes/mcp.js) via resolveAllowlists, and OAuth revocation
  (UserController.maybeUninstallOAuthMCP) via the request's already-merged
  appConfig.mcpSettings. Without this, an OAuth endpoint allowed only by an
  admin-panel override was rejected while inspection/connection allowed it.

*  test: Update MCP OAuth registry/config mocks for per-request allowlists

CI fix for the Finding-12 change. The OAuth-initiate route now calls
registry.resolveAllowlists() and the revocation path reads the merged
appConfig.mcpSettings, so the affected specs' mocks were asserting the old
base-getter values:
- routes/__tests__/mcp.spec.js: add resolveAllowlists to the registry mock.
- UserController.mcpOAuth.spec.js: provide mcpSettings on the getAppConfig
  mock so revokeOAuthToken still receives the expected allowlists.

* 🧪 test: e2e proof that admin-panel MCP allowlist override takes effect

Adds a Playwright mock-harness spec for #13809. A URL-based MCP fixture
(e2e-http, streamable-http SDK server) boots inspectionFailed because its
origin is omitted from the YAML mcpSettings.allowedDomains; the spec adds that
origin via an admin config override (PUT /api/admin/config/user/:id) and
asserts the server reinitializes — exercising the real resolver path through
the backend + DB. Before the fix, reinspection used the frozen YAML allowlist
and the server stayed unreachable.

- e2e/setup/fake-mcp-http-server.js: streamable-HTTP MCP fixture (health GET /).
- e2e/playwright.config.mock.ts: boot the fixture as a second webServer.
- e2e/config/librechat.e2e.yaml: mcpSettings.allowedDomains (excludes 127.0.0.1)
  + the e2e-http server.
- e2e/specs/mock/mcp-allowlist-override.spec.ts: login → baseline reinit fails →
  apply override → reinit succeeds.
2026-06-17 20:14:53 -04:00

457 lines
16 KiB
JavaScript

const mockUpdateUserPlugins = jest.fn();
const mockFindToken = jest.fn();
const mockDeleteUserPluginAuth = jest.fn();
const mockGetAppConfig = jest.fn();
const mockInvalidateCachedTools = jest.fn();
const mockGetLogStores = jest.fn();
const mockGetMCPManager = jest.fn();
const mockGetFlowStateManager = jest.fn();
const mockGetMCPServersRegistry = jest.fn();
jest.mock('@librechat/data-schemas', () => ({
logger: { error: jest.fn(), info: jest.fn(), warn: jest.fn() },
getTenantId: jest.fn(),
webSearchKeys: [],
}));
jest.mock('librechat-data-provider', () => ({
Tools: {},
CacheKeys: { FLOWS: 'flows' },
Constants: { mcp_delimiter: '_mcp_', mcp_prefix: 'mcp_' },
FileSources: {},
}));
jest.mock('@librechat/api', () => ({
MCPOAuthHandler: {
generateFlowId: jest.fn((userId, serverName, tenantId) => {
const flowId = `${userId}:${serverName}`;
return tenantId ? `tenant:${encodeURIComponent(tenantId)}:${flowId}` : flowId;
}),
generateTokenFlowId: jest.fn((userId, serverName, tenantId) => {
const flowId = `${userId}:${serverName}`;
return tenantId ? `tenant:${encodeURIComponent(tenantId)}:${flowId}` : flowId;
}),
revokeOAuthToken: jest.fn(),
},
MCPTokenStorage: {
getClientInfoAndMetadata: jest.fn(),
getTokens: jest.fn(),
deleteUserTokens: jest.fn().mockResolvedValue(undefined),
},
normalizeHttpError: jest.fn((error) => error),
extractWebSearchEnvVars: jest.fn((params) => params.keys),
needsRefresh: jest.fn(),
getNewS3URL: jest.fn(),
}));
jest.mock('~/models', () => ({
updateUserPlugins: (...args) => mockUpdateUserPlugins(...args),
findToken: mockFindToken,
deleteTokens: jest.fn(),
}));
jest.mock('~/server/services/PluginService', () => ({
updateUserPluginAuth: jest.fn(),
deleteUserPluginAuth: (...args) => mockDeleteUserPluginAuth(...args),
}));
jest.mock('~/server/services/twoFactorService', () => ({
verifyOTPOrBackupCode: jest.fn(),
}));
jest.mock('~/server/services/AuthService', () => ({
verifyEmail: jest.fn(),
resendVerificationEmail: jest.fn(),
}));
jest.mock('~/config', () => ({
getMCPManager: (...args) => mockGetMCPManager(...args),
getFlowStateManager: (...args) => mockGetFlowStateManager(...args),
getMCPServersRegistry: (...args) => mockGetMCPServersRegistry(...args),
}));
jest.mock('~/server/services/Config/getCachedTools', () => ({
invalidateCachedTools: (...args) => mockInvalidateCachedTools(...args),
}));
jest.mock('~/server/services/Files/process', () => ({
processDeleteRequest: jest.fn().mockResolvedValue({ deletedFileIds: [], failedFileIds: [] }),
}));
jest.mock('~/server/services/Config', () => ({
getAppConfig: (...args) => mockGetAppConfig(...args),
}));
jest.mock('~/cache', () => ({
getLogStores: (...args) => mockGetLogStores(...args),
}));
const { logger, getTenantId } = require('@librechat/data-schemas');
const { MCPTokenStorage, MCPOAuthHandler } = require('@librechat/api');
const { updateUserPluginsController } = require('~/server/controllers/UserController');
function createResponse() {
const res = {};
res.status = jest.fn().mockReturnValue(res);
res.json = jest.fn().mockReturnValue(res);
res.send = jest.fn().mockReturnValue(res);
return res;
}
function createRequest() {
return {
user: {
id: 'user-1',
_id: 'user-1',
plugins: [],
role: 'USER',
},
body: {
pluginKey: 'mcp_test-server',
action: 'uninstall',
auth: {},
},
};
}
function setupMCPMocks() {
const flowManager = {
deleteFlow: jest.fn().mockResolvedValue(true),
};
const mcpManager = {
disconnectUserConnection: jest.fn().mockResolvedValue(),
};
const registry = {
getServerConfig: jest.fn().mockResolvedValue({
url: 'https://example.com/mcp',
oauth: {},
oauth_headers: {},
}),
getOAuthServers: jest.fn().mockResolvedValue(new Set(['test-server'])),
getAllowedDomains: jest.fn().mockReturnValue([]),
getAllowedAddresses: jest.fn().mockReturnValue(null),
};
// Revocation reads the merged config's mcpSettings allowlists (not the registry getters).
mockGetAppConfig.mockResolvedValue({
mcpSettings: { allowedDomains: [], allowedAddresses: null },
});
mockUpdateUserPlugins.mockResolvedValue();
mockDeleteUserPluginAuth.mockResolvedValue();
mockInvalidateCachedTools.mockResolvedValue();
mockGetLogStores.mockReturnValue({});
mockGetFlowStateManager.mockReturnValue(flowManager);
mockGetMCPManager.mockReturnValue(mcpManager);
mockGetMCPServersRegistry.mockReturnValue(registry);
return { flowManager, mcpManager, registry };
}
beforeEach(() => {
jest.clearAllMocks();
getTenantId.mockReturnValue(undefined);
});
describe('updateUserPluginsController MCP OAuth cleanup', () => {
it('clears stored OAuth token state when client metadata is missing', async () => {
const { flowManager, mcpManager } = setupMCPMocks();
MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue(null);
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(MCPTokenStorage.getClientInfoAndMetadata).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
findToken: mockFindToken,
});
expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
deleteToken: expect.any(Function),
});
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens');
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth');
expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled();
expect(mcpManager.disconnectUserConnection).toHaveBeenCalledWith('user-1', 'test-server');
});
it('still clears OAuth flow state when stored token deletion fails', async () => {
const { flowManager } = setupMCPMocks();
const cleanupError = new Error('DB down');
MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue(null);
MCPTokenStorage.deleteUserTokens.mockRejectedValueOnce(cleanupError);
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens');
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth');
expect(logger.warn).toHaveBeenCalledWith(
'[clearStoredMCPOAuthState] Failed to delete MCP OAuth tokens for test-server:',
cleanupError,
);
});
it('logs all flow cleanup failures without failing MCP OAuth cleanup', async () => {
const { flowManager } = setupMCPMocks();
const getTokensFlowError = new Error('get tokens flow cache down');
const oauthFlowError = new Error('oauth flow cache down');
MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue(null);
flowManager.deleteFlow
.mockRejectedValueOnce(getTokensFlowError)
.mockRejectedValueOnce(oauthFlowError);
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens');
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth');
expect(logger.warn).toHaveBeenCalledWith(
'[clearStoredMCPOAuthState] Failed to clear MCP OAuth flow state for test-server:',
getTokensFlowError,
);
expect(logger.warn).toHaveBeenCalledWith(
'[clearStoredMCPOAuthState] Failed to clear MCP OAuth flow state for test-server:',
oauthFlowError,
);
});
it('clears stored OAuth token state when client metadata cannot be loaded', async () => {
const { flowManager } = setupMCPMocks();
MCPTokenStorage.getClientInfoAndMetadata.mockRejectedValue(new Error('invalid client info'));
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(logger.warn).toHaveBeenCalledWith(
'[maybeUninstallOAuthMCP] Unable to load OAuth client metadata for test-server; clearing local MCP OAuth state only.',
expect.any(Error),
);
expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
deleteToken: expect.any(Function),
});
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens');
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth');
expect(MCPTokenStorage.getTokens).not.toHaveBeenCalled();
expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled();
});
it('clears tenant-scoped and legacy OAuth flow state when tenant context exists', async () => {
const { flowManager } = setupMCPMocks();
getTenantId.mockReturnValue('tenant-a');
MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue(null);
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(flowManager.deleteFlow).toHaveBeenCalledWith(
'tenant:tenant-a:user-1:test-server',
'mcp_get_tokens',
);
expect(flowManager.deleteFlow).toHaveBeenCalledWith(
'tenant:tenant-a:user-1:test-server',
'mcp_oauth',
);
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens');
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth');
});
it('clears stored OAuth token state when server config is missing', async () => {
const { flowManager, registry } = setupMCPMocks();
registry.getServerConfig.mockResolvedValue(undefined);
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
deleteToken: expect.any(Function),
});
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens');
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth');
expect(MCPTokenStorage.getClientInfoAndMetadata).not.toHaveBeenCalled();
expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled();
});
it('clears stored OAuth token state when server no longer requires OAuth', async () => {
const { flowManager, registry } = setupMCPMocks();
registry.getOAuthServers.mockResolvedValue(new Set());
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
deleteToken: expect.any(Function),
});
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens');
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth');
expect(MCPTokenStorage.getClientInfoAndMetadata).not.toHaveBeenCalled();
expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled();
});
it('clears stored OAuth token state when token loading fails before provider revocation', async () => {
const { flowManager } = setupMCPMocks();
MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue({
clientInfo: { client_id: 'client-1' },
clientMetadata: {},
});
MCPTokenStorage.getTokens.mockRejectedValue(new Error('token lookup failed'));
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(MCPTokenStorage.getTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
findToken: mockFindToken,
});
expect(logger.warn).toHaveBeenCalledWith(
'[maybeUninstallOAuthMCP] Unable to load OAuth tokens for test-server; clearing local token state.',
expect.any(Error),
);
expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
deleteToken: expect.any(Function),
});
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_get_tokens');
expect(flowManager.deleteFlow).toHaveBeenCalledWith('user-1:test-server', 'mcp_oauth');
expect(MCPOAuthHandler.revokeOAuthToken).not.toHaveBeenCalled();
});
it('revokes provider tokens before clearing local token state when token data is available', async () => {
setupMCPMocks();
MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue({
clientInfo: { client_id: 'client-1', client_secret: 'secret-1' },
clientMetadata: { revocation_endpoint: 'https://example.com/revoke' },
});
MCPTokenStorage.getTokens.mockResolvedValue({
access_token: 'access-token',
refresh_token: 'refresh-token',
});
MCPOAuthHandler.revokeOAuthToken.mockResolvedValue();
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(MCPTokenStorage.getTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
findToken: mockFindToken,
});
expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledWith(
'test-server',
'access-token',
'access',
{
serverUrl: 'https://example.com/mcp',
clientId: 'client-1',
clientSecret: 'secret-1',
revocationEndpoint: 'https://example.com/revoke',
revocationEndpointAuthMethodsSupported: undefined,
},
{},
[],
null,
);
expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledWith(
'test-server',
'refresh-token',
'refresh',
{
serverUrl: 'https://example.com/mcp',
clientId: 'client-1',
clientSecret: 'secret-1',
revocationEndpoint: 'https://example.com/revoke',
revocationEndpointAuthMethodsSupported: undefined,
},
{},
[],
null,
);
expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
deleteToken: expect.any(Function),
});
});
it('revokes only the access token when refresh token data is absent', async () => {
setupMCPMocks();
MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue({
clientInfo: { client_id: 'client-1', client_secret: 'secret-1' },
clientMetadata: {},
});
MCPTokenStorage.getTokens.mockResolvedValue({
access_token: 'access-token',
});
MCPOAuthHandler.revokeOAuthToken.mockResolvedValue();
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledTimes(1);
expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledWith(
'test-server',
'access-token',
'access',
expect.objectContaining({ clientId: 'client-1' }),
{},
[],
null,
);
expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
deleteToken: expect.any(Function),
});
});
it('revokes only the refresh token when access token data is absent', async () => {
setupMCPMocks();
MCPTokenStorage.getClientInfoAndMetadata.mockResolvedValue({
clientInfo: { client_id: 'client-1', client_secret: 'secret-1' },
clientMetadata: {},
});
MCPTokenStorage.getTokens.mockResolvedValue({
refresh_token: 'refresh-token',
});
MCPOAuthHandler.revokeOAuthToken.mockResolvedValue();
const res = createResponse();
await updateUserPluginsController(createRequest(), res);
expect(res.status).toHaveBeenCalledWith(200);
expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledTimes(1);
expect(MCPOAuthHandler.revokeOAuthToken).toHaveBeenCalledWith(
'test-server',
'refresh-token',
'refresh',
expect.objectContaining({ clientId: 'client-1' }),
{},
[],
null,
);
expect(MCPTokenStorage.deleteUserTokens).toHaveBeenCalledWith({
userId: 'user-1',
serverName: 'test-server',
deleteToken: expect.any(Function),
});
});
});