Skip to content

Commit 2fbb5e8

Browse files
LiaSpectralOne
andcommitted
fix: Drop unread credentials from the MCP early domain gate
The early domain gate resolves the whole server config through `processMCPEnv`, but decides from the URL alone. A credential placeholder in any other field therefore raised `OpenIDReauthRequiredError` from a stale request-time OpenID snapshot, before the connection path could refresh that bearer, and the tool was dropped from the agent's toolset. `buildMCPDomainValidationConfig` narrows the config to what the decision reads, so the gate needs no live credential. A URL placeholder still fails closed, and the argument is never mutated, so direct-bearer recovery keeps the placeholder it knows how to refresh. Replaces the per-tool `upstreamTokenProvider` call the gate would otherwise make (1 + N per request per server) and covers every credential-bearing field, not only `Authorization`. Co-authored-by: Artyom Bogachenko <SpectralOne@users.noreply.github.com>
1 parent 3e64c75 commit 2fbb5e8

5 files changed

Lines changed: 207 additions & 55 deletions

File tree

api/server/services/MCP.js

Lines changed: 14 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ const {
4242
isOAuthServer,
4343
isAbortError,
4444
isDirectOpenIDBearerRecoveryEnabled,
45-
resolveDirectOpenIDBearerConfig,
45+
buildMCPDomainValidationConfig,
4646
OpenIDReauthRequiredError,
4747
MCPAuthenticationRefreshError,
4848
MCPAuthenticationRejectedError,
@@ -507,35 +507,15 @@ async function resolveAllMcpConfigs(userId, user) {
507507
return await registry.getAllServerConfigs(userId, configServers);
508508
}
509509

510-
/**
511-
* Resolves the live session bearer for the early domain gate. The gate renders
512-
* placeholders with the request-time user snapshot, which a stale OpenID access
513-
* token poisons (`OpenIDReauthRequiredError`) before the connection path's
514-
* recovery hook ever runs; resolving first refreshes the session token instead.
515-
* Only the validation copy is resolved, `serverConfig` keeps its placeholder,
516-
* so downstream direct-bearer recovery stays enabled.
517-
* @param {Object} params
518-
* @param {import('@librechat/api').ParsedServerConfig} params.serverConfig
519-
* @param {import('@librechat/api').UpstreamTokenProvider} [params.upstreamTokenProvider]
520-
* @param {AbortSignal} [params.signal]
521-
* @returns {Promise<import('@librechat/api').ParsedServerConfig>}
522-
*/
523-
async function resolveEarlyValidationConfig({ serverConfig, upstreamTokenProvider, signal }) {
524-
if (upstreamTokenProvider == null || !isDirectOpenIDBearerRecoveryEnabled(serverConfig)) {
525-
return serverConfig;
526-
}
527-
return await resolveDirectOpenIDBearerConfig({
528-
signal,
529-
config: serverConfig,
530-
upstreamTokenProvider,
531-
});
532-
}
533-
534510
/**
535511
* Best-effort early gate; the authoritative check is
536-
* `assertResolvedRuntimeConfigAllowed` in `@librechat/api`, whose resolution
537-
* this must mirror. Graph placeholders resolve later (async), so a URL still
538-
* carrying one defers to the authoritative check instead of rejecting here.
512+
* `assertResolvedRuntimeConfigAllowed` in `@librechat/api`, whose *URL*
513+
* resolution this must mirror. It mirrors only that much: credential-bearing
514+
* fields are dropped here (`buildMCPDomainValidationConfig`) because this gate
515+
* runs before the connection path resolves them, so requiring them would reject
516+
* a server the authoritative check goes on to allow. Graph placeholders resolve
517+
* later (async), so a URL still carrying one defers to the authoritative check
518+
* instead of rejecting here.
539519
*/
540520
async function isEarlyDomainAllowed({
541521
serverConfig,
@@ -550,7 +530,10 @@ async function isEarlyDomainAllowed({
550530
user,
551531
body: requestBody,
552532
dbSourced: isUserSourced(serverConfig),
553-
options: serverConfig,
533+
/** The decision reads the URL alone, and resolving a credential-bearing field it
534+
* never reads would fail the gate on a stale request-time OpenID snapshot —
535+
* before the connection path can refresh that bearer. */
536+
options: buildMCPDomainValidationConfig(serverConfig),
554537
customUserVars: getServerCustomUserVars(userMCPAuthMap, serverName),
555538
});
556539
if (
@@ -924,11 +907,7 @@ async function createMCPTools({
924907
const allowedDomains = appConfig?.mcpSettings?.allowedDomains;
925908
const allowedAddresses = appConfig?.mcpSettings?.allowedAddresses;
926909
const isDomainAllowed = await isEarlyDomainAllowed({
927-
serverConfig: await resolveEarlyValidationConfig({
928-
serverConfig,
929-
upstreamTokenProvider,
930-
signal,
931-
}),
910+
serverConfig,
932911
user,
933912
requestBody,
934913
userMCPAuthMap,
@@ -1101,11 +1080,7 @@ async function createMCPTool({
11011080
const allowedDomains = appConfig?.mcpSettings?.allowedDomains;
11021081
const allowedAddresses = appConfig?.mcpSettings?.allowedAddresses;
11031082
const isDomainAllowed = await isEarlyDomainAllowed({
1104-
serverConfig: await resolveEarlyValidationConfig({
1105-
serverConfig,
1106-
upstreamTokenProvider,
1107-
signal,
1108-
}),
1083+
serverConfig,
11091084
user,
11101085
requestBody,
11111086
userMCPAuthMap,

api/server/services/MCP.spec.js

Lines changed: 22 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2971,7 +2971,7 @@ describe('User parameter passing tests', () => {
29712971
);
29722972
});
29732973

2974-
it('resolves the live session bearer for the early domain gate when the snapshot token is expired', async () => {
2974+
it('loads the tool when the early domain gate meets an expired OpenID snapshot token', async () => {
29752975
const mockUser = {
29762976
id: 'expired-token-user',
29772977
role: 'user',
@@ -2983,13 +2983,14 @@ describe('User parameter passing tests', () => {
29832983
};
29842984
const mockRes = { write: jest.fn(), flush: jest.fn() };
29852985

2986-
mockRegistryInstance.getServerConfig.mockResolvedValue({
2986+
const serverConfig = {
29872987
type: 'streamable-http',
29882988
url: 'https://mcp.example.com/mcp',
29892989
source: 'yaml',
29902990
requiresOAuth: false,
29912991
headers: { Authorization: 'Bearer {{LIBRECHAT_OPENID_TOKEN}}' },
2992-
});
2992+
};
2993+
mockRegistryInstance.getServerConfig.mockResolvedValue(serverConfig);
29932994
mockGetAppConfig.mockResolvedValue({
29942995
mcpSettings: { allowedDomains: ['mcp.example.com'] },
29952996
});
@@ -3015,18 +3016,19 @@ describe('User parameter passing tests', () => {
30153016
});
30163017

30173018
expect(result).toBeDefined();
3018-
expect(upstreamTokenProvider).toHaveBeenCalledWith({ forceRefresh: false });
3019-
expect(mockIsMCPDomainAllowed).toHaveBeenCalledWith(
3020-
expect.objectContaining({
3021-
url: 'https://mcp.example.com/mcp',
3022-
headers: { Authorization: 'Bearer fresh-token' },
3023-
}),
3024-
['mcp.example.com'],
3025-
undefined,
3026-
);
3019+
/** The gate decides from the URL, so it must not spend an IdP round trip per tool. */
3020+
expect(upstreamTokenProvider).not.toHaveBeenCalled();
3021+
const [validationConfig, domains, addresses] = mockIsMCPDomainAllowed.mock.calls[0];
3022+
expect(validationConfig.url).toBe('https://mcp.example.com/mcp');
3023+
expect(validationConfig.headers).toBeUndefined();
3024+
expect([domains, addresses]).toEqual([['mcp.example.com'], undefined]);
3025+
/** The connection path still receives the placeholder it can refresh. */
3026+
expect(serverConfig.headers).toEqual({
3027+
Authorization: 'Bearer {{LIBRECHAT_OPENID_TOKEN}}',
3028+
});
30273029
});
30283030

3029-
it('still fails closed when no live session bearer is available for the early domain gate', async () => {
3031+
it('still fails closed when the URL itself carries an unresolvable OpenID credential', async () => {
30303032
const { OpenIDReauthRequiredError } = require('@librechat/api');
30313033
const mockUser = {
30323034
id: 'no-session-user',
@@ -3041,7 +3043,7 @@ describe('User parameter passing tests', () => {
30413043

30423044
mockRegistryInstance.getServerConfig.mockResolvedValue({
30433045
type: 'streamable-http',
3044-
url: 'https://mcp.example.com/mcp',
3046+
url: 'https://mcp.example.com/mcp/{{LIBRECHAT_OPENID_ACCESS_TOKEN}}',
30453047
source: 'yaml',
30463048
requiresOAuth: false,
30473049
headers: { Authorization: 'Bearer {{LIBRECHAT_OPENID_TOKEN}}' },
@@ -3072,7 +3074,7 @@ describe('User parameter passing tests', () => {
30723074
).rejects.toThrow(OpenIDReauthRequiredError);
30733075
});
30743076

3075-
it('resolves the live session bearer before the createMCPTools domain gate', async () => {
3077+
it('loads the batch path when the domain gate meets an expired OpenID snapshot token', async () => {
30763078
const mockUser = {
30773079
id: 'expired-token-batch-user',
30783080
role: 'user',
@@ -3118,7 +3120,11 @@ describe('User parameter passing tests', () => {
31183120
});
31193121

31203122
expect(tools).toHaveLength(1);
3121-
expect(upstreamTokenProvider).toHaveBeenCalled();
3123+
expect(upstreamTokenProvider).not.toHaveBeenCalled();
3124+
const [validationConfig] = mockIsMCPDomainAllowed.mock.calls[0];
3125+
expect(validationConfig.url).toBe('https://mcp.example.com/mcp');
3126+
expect(validationConfig.headers).toBeUndefined();
3127+
/** Recovery still owns the placeholder on the connection path. */
31223128
expect(mockReinitMCPServer).toHaveBeenCalledWith(
31233129
expect.objectContaining({ upstreamTokenProvider }),
31243130
);

packages/api/src/index.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@ export * from './mcp/reinitialize';
3939
export * from './mcp/icons';
4040
/* Utilities */
4141
export * from './mcp/utils';
42+
export * from './mcp/domainValidation';
4243
export * from './mcp/context';
4344
export * from './utils';
4445
export { default as Tokenizer, countTokens } from './utils/tokenizer';
Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
import type { MCPOptions } from 'librechat-data-provider';
2+
import type { IUser } from '@librechat/data-schemas';
3+
import type { ParsedServerConfig } from './types';
4+
import { buildMCPDomainValidationConfig } from './domainValidation';
5+
import { OpenIDReauthRequiredError } from '~/utils/oidc';
6+
import { isMCPDomainAllowed } from '~/auth/domain';
7+
import { processMCPEnv } from '~/utils/env';
8+
9+
/** What `openIdJwtStrategy` attaches from `req.session.openidTokens` without refreshing it. */
10+
const staleOpenIDUser = (): Partial<IUser> =>
11+
({
12+
id: 'user-1',
13+
provider: 'openid',
14+
openidId: 'oidc-sub-1',
15+
federatedTokens: {
16+
access_token: 'stale-access-token',
17+
expires_at: Math.floor(Date.now() / 1000) - 3600,
18+
},
19+
}) as Partial<IUser>;
20+
21+
/** `MCPOptions` is a union by transport, so a test reads these two through the union. */
22+
const urlOf = (options: MCPOptions | ParsedServerConfig): string | undefined =>
23+
(options as { url?: string }).url;
24+
const headersOf = (options: ParsedServerConfig): Record<string, string> | undefined =>
25+
(options as { headers?: Record<string, string> }).headers;
26+
27+
const yamlServer = (overrides: Record<string, unknown> = {}): ParsedServerConfig =>
28+
({
29+
type: 'streamable-http',
30+
url: 'https://mcp.example.com/mcp',
31+
source: 'yaml',
32+
requiresOAuth: false,
33+
...overrides,
34+
}) as ParsedServerConfig;
35+
36+
describe('buildMCPDomainValidationConfig', () => {
37+
it('lets a stale OpenID snapshot resolve a URL that an Authorization placeholder blocked', async () => {
38+
const config = yamlServer({ headers: { Authorization: 'Bearer {{LIBRECHAT_OPENID_TOKEN}}' } });
39+
const user = staleOpenIDUser();
40+
41+
/** The trap: a header no domain decision reads fails the whole resolution. */
42+
expect(() => processMCPEnv({ user, options: config })).toThrow(OpenIDReauthRequiredError);
43+
44+
const resolved = processMCPEnv({ user, options: buildMCPDomainValidationConfig(config) });
45+
expect(urlOf(resolved)).toBe('https://mcp.example.com/mcp');
46+
expect('headers' in resolved).toBe(false);
47+
await expect(isMCPDomainAllowed(resolved, ['mcp.example.com'])).resolves.toBe(true);
48+
});
49+
50+
it.each([
51+
['headers', { headers: { 'X-Auth-Token': '{{LIBRECHAT_OPENID_ACCESS_TOKEN}}' } }],
52+
['oauth_headers', { oauth_headers: { Authorization: 'Bearer {{LIBRECHAT_OPENID_TOKEN}}' } }],
53+
['env', { env: { UPSTREAM_TOKEN: '{{LIBRECHAT_OPENID_ACCESS_TOKEN}}' } }],
54+
['args', { args: ['--token={{LIBRECHAT_OPENID_ACCESS_TOKEN}}'] }],
55+
['oauth', { oauth: { client_secret: '{{LIBRECHAT_OPENID_ACCESS_TOKEN}}' } }],
56+
[
57+
'apiKey',
58+
{
59+
apiKey: {
60+
source: 'admin',
61+
key: '{{LIBRECHAT_OPENID_ACCESS_TOKEN}}',
62+
authorization_type: 'bearer',
63+
},
64+
},
65+
],
66+
])(
67+
'drops a credential placeholder carried by %s, not only by Authorization',
68+
(_field, overrides) => {
69+
const config = yamlServer(overrides);
70+
const user = staleOpenIDUser();
71+
72+
expect(() => processMCPEnv({ user, options: config })).toThrow(OpenIDReauthRequiredError);
73+
expect(urlOf(processMCPEnv({ user, options: buildMCPDomainValidationConfig(config) }))).toBe(
74+
'https://mcp.example.com/mcp',
75+
);
76+
},
77+
);
78+
79+
it('keeps failing closed when the URL itself carries a credential placeholder', () => {
80+
const config = yamlServer({
81+
url: 'https://mcp.example.com/mcp/{{LIBRECHAT_OPENID_ACCESS_TOKEN}}',
82+
headers: { Authorization: 'Bearer {{LIBRECHAT_OPENID_TOKEN}}' },
83+
});
84+
85+
expect(() =>
86+
processMCPEnv({
87+
user: staleOpenIDUser(),
88+
options: buildMCPDomainValidationConfig(config),
89+
}),
90+
).toThrow(OpenIDReauthRequiredError);
91+
});
92+
93+
it('preserves every field resolution and the domain decision depend on, and mutates nothing', () => {
94+
const headers = { Authorization: 'Bearer {{LIBRECHAT_OPENID_TOKEN}}' };
95+
const config = yamlServer({ headers, dbId: 'server-1', source: 'user', consumeOnly: true });
96+
97+
expect(buildMCPDomainValidationConfig(config)).toEqual({
98+
type: 'streamable-http',
99+
url: 'https://mcp.example.com/mcp',
100+
source: 'user',
101+
requiresOAuth: false,
102+
dbId: 'server-1',
103+
consumeOnly: true,
104+
});
105+
/** The connection path still needs the placeholder it knows how to refresh. */
106+
expect(headersOf(config)).toBe(headers);
107+
});
108+
109+
it('still resolves the user placeholders a URL depends on', async () => {
110+
const config = yamlServer({
111+
url: 'https://{{LIBRECHAT_USER_ID}}.mcp.example.com/mcp',
112+
headers: { Authorization: 'Bearer {{LIBRECHAT_OPENID_TOKEN}}' },
113+
});
114+
115+
const resolved = processMCPEnv({
116+
user: staleOpenIDUser(),
117+
options: buildMCPDomainValidationConfig(config),
118+
});
119+
120+
expect(urlOf(resolved)).toBe('https://user-1.mcp.example.com/mcp');
121+
await expect(isMCPDomainAllowed(resolved, ['*.mcp.example.com'])).resolves.toBe(true);
122+
await expect(isMCPDomainAllowed(resolved, ['other.example.com'])).resolves.toBe(false);
123+
});
124+
});
Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,46 @@
1+
import type { ParsedServerConfig } from '~/mcp/types';
2+
3+
/**
4+
* Fields an MCP domain decision never reads, dropped before placeholder
5+
* resolution so the decision cannot depend on a credential it does not inspect.
6+
*
7+
* `isMCPDomainAllowed` decides from `url` alone, but `processMCPEnv` resolves
8+
* every field it is handed, and `{{LIBRECHAT_OPENID_ACCESS_TOKEN}}` (or its
9+
* `{{LIBRECHAT_OPENID_TOKEN}}` alias) in any of them raises
10+
* `OpenIDReauthRequiredError` when the request-time user snapshot carries a stale
11+
* access token — the snapshot `openIdJwtStrategy` populates from
12+
* `req.session.openidTokens` without refreshing. A header the decision never
13+
* looks at could therefore fail a server whose bearer the connection path
14+
* refreshes a moment later through `resolveDirectOpenIDBearerConfig`, and the
15+
* tool was dropped from the agent's toolset instead of being loaded.
16+
*
17+
* `url` is deliberately absent from this list: it decides which host is
18+
* contacted, so an unresolvable credential placeholder there must keep failing
19+
* closed.
20+
*/
21+
const UNREAD_BY_DOMAIN_VALIDATION = [
22+
'apiKey',
23+
'args',
24+
'env',
25+
'headers',
26+
'oauth',
27+
'oauth_headers',
28+
] as const;
29+
30+
/**
31+
* Narrows a server config to what a domain check reads, so resolving it needs no
32+
* live credential. Every other field is preserved, including the `source` and
33+
* `dbId` that decide which placeholders `processMCPEnv` resolves at all, and the
34+
* presence or absence of `url` that `isMCPDomainAllowed` fails closed on.
35+
*
36+
* Callers pass the result to `processMCPEnv` and then to `isMCPDomainAllowed`.
37+
* The argument is never mutated, so a direct-bearer server keeps its placeholder
38+
* for the connection path that knows how to refresh it.
39+
*/
40+
export function buildMCPDomainValidationConfig(config: ParsedServerConfig): ParsedServerConfig {
41+
const validationConfig: Record<string, unknown> = { ...(config as Record<string, unknown>) };
42+
for (const field of UNREAD_BY_DOMAIN_VALIDATION) {
43+
delete validationConfig[field];
44+
}
45+
return validationConfig as unknown as ParsedServerConfig;
46+
}

0 commit comments

Comments
 (0)