diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js index 29c6511474..a54c5ab661 100644 --- a/api/server/controllers/AuthController.js +++ b/api/server/controllers/AuthController.js @@ -1,5 +1,6 @@ const cookies = require('cookie'); const jwt = require('jsonwebtoken'); +const crypto = require('node:crypto'); const openIdClient = require('openid-client'); const { logger } = require('@librechat/data-schemas'); const { @@ -74,7 +75,7 @@ const sanitizeUserForAuthResponse = (user) => { return safeUser; }; -const getValidOpenIDReuseUserId = (parsedCookies) => { +const getValidOpenIDReuseUserId = (parsedCookies, refreshToken) => { const openidUserId = parsedCookies.openid_user_id; if (!openidUserId || !process.env.JWT_REFRESH_SECRET) { return null; @@ -82,9 +83,17 @@ const getValidOpenIDReuseUserId = (parsedCookies) => { try { const payload = jwt.verify(openidUserId, process.env.JWT_REFRESH_SECRET); - return typeof payload === 'object' && payload != null && typeof payload.id === 'string' - ? payload.id - : null; + if (typeof payload !== 'object' || payload == null || typeof payload.id !== 'string') { + return null; + } + if (refreshToken == null) { + return payload.id; + } + if (typeof payload.refreshTokenHash !== 'string') { + return null; + } + const refreshTokenHash = crypto.createHash('sha256').update(refreshToken).digest('base64url'); + return payload.refreshTokenHash === refreshTokenHash ? payload.id : null; } catch { return null; } @@ -368,7 +377,7 @@ const refreshController = async (req, res) => { */ if (isInvalidGrantError(error) && refreshToken) { // Bridge lookup uses the signed user-id cookie because /refresh is unauthenticated. - const userId = getValidOpenIDReuseUserId(parsedCookies); + const userId = getValidOpenIDReuseUserId(parsedCookies, refreshToken); if (userId) { try { const bridgeUser = await getUserById(userId, AUTH_REFRESH_USER_PROJECTION); diff --git a/api/server/controllers/AuthController.spec.js b/api/server/controllers/AuthController.spec.js index 9cd30ca3a8..3f4712ee90 100644 --- a/api/server/controllers/AuthController.spec.js +++ b/api/server/controllers/AuthController.spec.js @@ -70,6 +70,7 @@ jest.mock('@librechat/api', () => ({ }), })); +const { createHash } = require('node:crypto'); const openIdClient = require('openid-client'); const jwt = require('jsonwebtoken'); const { logger } = require('@librechat/data-schemas'); @@ -251,8 +252,19 @@ describe('refreshController – OpenID path', () => { idpSigningSecret, ); - const makeSignedUserId = (id = 'user-db-id', options = { expiresIn: '1h' }) => - jwt.sign({ id }, process.env.JWT_REFRESH_SECRET, options); + const makeSignedUserId = ( + id = 'user-db-id', + options = { expiresIn: '1h' }, + refreshToken = 'stored-refresh', + ) => + jwt.sign( + { + id, + refreshTokenHash: createHash('sha256').update(refreshToken).digest('base64url'), + }, + process.env.JWT_REFRESH_SECRET, + options, + ); const setOpenIDReuseCookies = (signedUserId = makeSignedUserId()) => { req.headers.cookie = [ @@ -935,6 +947,30 @@ describe('refreshController – OpenID path', () => { expect(res.status).toHaveBeenCalledWith(403); }); + it('does not use the bridge when the signed marker belongs to another refresh token', async () => { + setOpenIDReuseCookies(makeSignedUserId('user-db-id', { expiresIn: '1h' }, 'different-refresh')); + openIdClient.refreshTokenGrant.mockRejectedValue(new Error('invalid_grant')); + + await refreshController(req, res); + + expect(getUserById).not.toHaveBeenCalled(); + expect(getRefreshTokenBridge).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + + it('does not use the bridge when the signed marker lacks a refresh-token binding', async () => { + setOpenIDReuseCookies( + jwt.sign({ id: 'user-db-id' }, process.env.JWT_REFRESH_SECRET, { expiresIn: '1h' }), + ); + openIdClient.refreshTokenGrant.mockRejectedValue(new Error('invalid_grant')); + + await refreshController(req, res); + + expect(getUserById).not.toHaveBeenCalled(); + expect(getRefreshTokenBridge).not.toHaveBeenCalled(); + expect(res.status).toHaveBeenCalledWith(403); + }); + it('recovers stale refresh-token cookies and keeps a short grace bridge', async () => { setOpenIDReuseCookies(); req.session = {}; diff --git a/api/server/services/AuthService.js b/api/server/services/AuthService.js index 72626434c5..be27208651 100644 --- a/api/server/services/AuthService.js +++ b/api/server/services/AuthService.js @@ -21,6 +21,7 @@ const { shouldUseSecureCookie, setRefreshTokenCookie, setOpenIDMarkerCookies, + normalizeExpiresIn, createOpenIDSessionIdentity, resolveAppConfigForUser, } = require('@librechat/api'); @@ -858,9 +859,10 @@ const setOpenIDAuthTokens = ( * always trigger a redundant inline refresh whenever the IdP issues * opaque access tokens (e.g. Microsoft Graph audiences). */ - if (typeof tokenset.expires_in === 'number') { + const accessTokenExpiresIn = normalizeExpiresIn(tokenset.expires_in); + if (accessTokenExpiresIn != null) { sessionOpenidTokens.accessTokenExpiresAt = - Math.floor(Date.now() / 1000) + tokenset.expires_in; + Math.floor(Date.now() / 1000) + accessTokenExpiresIn; } req.session.openidTokens = sessionOpenidTokens; } else { diff --git a/api/server/services/AuthService.spec.js b/api/server/services/AuthService.spec.js index ab24985285..0532611d24 100644 --- a/api/server/services/AuthService.spec.js +++ b/api/server/services/AuthService.spec.js @@ -53,6 +53,12 @@ jest.mock( }); } }), + normalizeExpiresIn: (value) => { + const normalized = typeof value === 'string' && value.trim() ? Number(value) : value; + return typeof normalized === 'number' && Number.isFinite(normalized) + ? normalized + : undefined; + }, resolveAppConfigForUser: jest.fn(async (_getAppConfig, _user) => ({})), createOpenIDSessionIdentity: jest.fn( ({ user, userId, openidSubject, tenantId, openidIssuer }) => { @@ -381,6 +387,24 @@ describe('setOpenIDAuthTokens', () => { expect(persisted).toBeLessThanOrEqual(beforeSec + 3610); }); + it('should persist accessTokenExpiresAt when tokenset.expires_in is a numeric string', () => { + const tokenset = { + id_token: 'the-id-token', + access_token: 'the-access-token', + refresh_token: 'the-refresh-token', + expires_in: '3600', + }; + const req = mockRequest(); + const res = mockResponse(); + const beforeSec = Math.floor(Date.now() / 1000); + + setOpenIDAuthTokens(tokenset, req, res, 'user-123'); + + expect(req.session.openidTokens.accessTokenExpiresAt).toBeGreaterThanOrEqual( + beforeSec + 3590, + ); + }); + it('should NOT persist accessTokenExpiresAt when tokenset.expires_in is missing', () => { const tokenset = { id_token: 'the-id-token', diff --git a/api/server/services/OpenIDSessionRefresh.js b/api/server/services/OpenIDSessionRefresh.js index 78b93ff8f4..cb5aa30d69 100644 --- a/api/server/services/OpenIDSessionRefresh.js +++ b/api/server/services/OpenIDSessionRefresh.js @@ -14,6 +14,7 @@ const { setRefreshTokenCookie, setOpenIDMarkerCookies, storeOpenIdSession, + normalizeExpiresIn, } = require('@librechat/api'); const { upsertSession, deleteSession } = require('~/models'); const { getOpenIdConfig } = require('~/strategies/openidStrategy'); @@ -511,8 +512,9 @@ async function performIdpRefreshGrant(req, res, user, tokenPreference, identityC * freshness check will correctly fall through to refresh. */ let nextAccessTokenExp = null; - if (typeof tokenset.expires_in === 'number') { - nextAccessTokenExp = Math.floor(Date.now() / 1000) + tokenset.expires_in; + const accessTokenExpiresIn = normalizeExpiresIn(tokenset.expires_in); + if (accessTokenExpiresIn != null) { + nextAccessTokenExp = Math.floor(Date.now() / 1000) + accessTokenExpiresIn; } else { nextAccessTokenExp = decodeJwtExp(tokenset.access_token); } diff --git a/api/server/services/OpenIDSessionRefresh.spec.js b/api/server/services/OpenIDSessionRefresh.spec.js index 4eb2d1e011..14195e8f6f 100644 --- a/api/server/services/OpenIDSessionRefresh.spec.js +++ b/api/server/services/OpenIDSessionRefresh.spec.js @@ -108,6 +108,10 @@ jest.mock('@librechat/api', () => ({ res.cookie('openid_user_id', `signed:${userId}`, { expires }); } }), + normalizeExpiresIn: (value) => { + const normalized = typeof value === 'string' && value.trim() ? Number(value) : value; + return typeof normalized === 'number' && Number.isFinite(normalized) ? normalized : undefined; + }, storeOpenIdSession: jest.fn(), })); jest.mock('~/models', () => ({ @@ -1422,6 +1426,27 @@ describe('OpenIDSessionRefresh', () => { expect(persistedExp).toBeLessThanOrEqual(beforeSec + 3610); }); + it('persists accessTokenExpiresAt when the refreshed expires_in is a numeric string', async () => { + const expiredExp = Math.floor(Date.now() / 1000) - 60; + const sessionTokens = { + accessToken: 'opaque-stale', + idToken: makeJwt(expiredExp), + refreshToken: 'rt-string-expiry', + }; + openIdClient.refreshTokenGrant.mockResolvedValueOnce({ + access_token: 'opaque-fresh', + expires_in: '3600', + }); + const req = buildReq(sessionTokens); + const beforeSec = Math.floor(Date.now() / 1000); + + await refreshOpenIDSession(req, undefined, makeOpenIdUser(), 'access_token'); + + expect(req.session.openidTokens.accessTokenExpiresAt).toBeGreaterThanOrEqual( + beforeSec + 3590, + ); + }); + it('drops a stale accessTokenExpiresAt when the new tokenset has neither expires_in nor a JWT access_token', async () => { const expiredExp = Math.floor(Date.now() / 1000) - 60; const sessionTokens = { diff --git a/api/server/services/ToolService.js b/api/server/services/ToolService.js index 9eb995a810..200f81fbd0 100644 --- a/api/server/services/ToolService.js +++ b/api/server/services/ToolService.js @@ -1154,6 +1154,8 @@ async function loadToolDefinitionsWrapper({ userMCPAuthMap, requestBody: runtimeRequestBody, requestScopedConnections, + upstreamTokenProvider, + oboIdentityContext, }); rememberMCPAvailableTools(serverName, result?.availableTools); diff --git a/api/server/services/__tests__/ToolService.spec.js b/api/server/services/__tests__/ToolService.spec.js index 4281e485ea..c92edd971b 100644 --- a/api/server/services/__tests__/ToolService.spec.js +++ b/api/server/services/__tests__/ToolService.spec.js @@ -69,6 +69,12 @@ jest.mock('@librechat/api', () => ({ ['AGENT_EXPECTED_MCP_TOOLS_UNAVAILABLE', 'resource_recovery_required'].includes(error?.code), loadToolDefinitions: (...args) => mockLoadToolDefinitions(...args), getUserMCPAuthMap: (...args) => mockGetUserMCPAuthMap(...args), + createAuthIdentityContext: ({ user, tenantId }) => ({ + appUserId: user?._id?.toString?.() ?? user?.id, + openidSubject: user?.openidId, + tenantId: tenantId ?? user?.tenantId, + openidIssuer: user?.openidIssuer, + }), sendEvent: (...args) => mockSendEvent(...args), GenerationJobManager: { emitChunk: (...args) => mockEmitChunk(...args), @@ -2069,6 +2075,56 @@ describe('ToolService - Action Capability Gating', () => { ); }); + it('forwards OBO context through forced MCP catalog refreshes', async () => { + const serverName = 'OBO-Refresh'; + const mcpTool = `search${Constants.mcp_delimiter}${serverName}`; + const capabilities = [AgentCapabilities.tools]; + const req = createMockReq(capabilities); + req.user = { + id: 'user_123', + provider: 'openid', + openidId: 'oidc-sub-123', + tenantId: 'tenant-1', + openidIssuer: 'https://issuer.example.com', + }; + + mockGetEndpointsConfig.mockResolvedValue(createEndpointsConfig(capabilities)); + mockGetServerConfig.mockResolvedValue({ + type: 'streamable-http', + url: 'https://mcp.example.com/obo', + obo: { scopes: 'api://obo/Mcp.Tools.ReadWrite' }, + }); + mockLoadToolDefinitions.mockImplementation(async (params, dependencies) => { + await dependencies.refreshMCPServerTools(params.userId, serverName); + return { + toolDefinitions: [], + toolRegistry: new Map(), + hasDeferredTools: false, + }; + }); + reinitMCPServer.mockResolvedValue({ availableTools: {} }); + + await loadAgentTools({ + req, + agent: { id: 'agent_123', tools: [mcpTool] }, + definitionsOnly: true, + }); + + expect(reinitMCPServer).toHaveBeenCalledWith( + expect.objectContaining({ + serverName, + forceNew: true, + upstreamTokenProvider: expect.any(Function), + oboIdentityContext: expect.objectContaining({ + appUserId: 'user_123', + openidSubject: 'oidc-sub-123', + tenantId: 'tenant-1', + openidIssuer: 'https://issuer.example.com', + }), + }), + ); + }); + it('returns run-scoped MCP tool definitions for request-scoped servers', async () => { const serverName = 'ClickHouse'; const mcpTool = `list_tables${Constants.mcp_delimiter}${serverName}`; diff --git a/packages/api/src/mcp/oauth/obo.spec.ts b/packages/api/src/mcp/oauth/obo.spec.ts index 9b01e10e66..d8fb7fad70 100644 --- a/packages/api/src/mcp/oauth/obo.spec.ts +++ b/packages/api/src/mcp/oauth/obo.spec.ts @@ -125,6 +125,21 @@ describe('resolveOboToken', () => { expect(mockResolver).not.toHaveBeenCalled(); }); + it('marks transient session refresh failures as retryable', async () => { + const failingProvider: UpstreamTokenProvider = jest + .fn() + .mockRejectedValue(Object.assign(new Error('service unavailable'), { status: 503 })); + + await expect( + resolveOboToken(mockUser as IUser, oboConfig, mockResolver, failingProvider), + ).rejects.toMatchObject({ + reason: 'session_refresh_failed', + retryable: true, + userMessage: 'Temporary sign-in session refresh failure.', + }); + expect(mockResolver).not.toHaveBeenCalled(); + }); + it('throws missing_upstream_token when isOpenIDTokenValid returns false (live token expired)', async () => { mockIsOpenIDTokenValid.mockReturnValue(false); diff --git a/packages/api/src/mcp/oauth/obo.ts b/packages/api/src/mcp/oauth/obo.ts index fd70447560..73511dda50 100644 --- a/packages/api/src/mcp/oauth/obo.ts +++ b/packages/api/src/mcp/oauth/obo.ts @@ -202,10 +202,13 @@ export async function resolveOboToken( liveTokens = await upstreamTokenProvider(); } catch (error) { logger.error('[OBO] Upstream session refresh failed:', error); + const retryable = isRetryableOboExchangeError(error); throw new OboTokenResolutionError( 'session_refresh_failed', - 'Your sign-in session expired and could not be refreshed. Please sign in again.', - false, + retryable + ? 'Temporary sign-in session refresh failure.' + : 'Your sign-in session expired and could not be refreshed. Please sign in again.', + retryable, error, ); } diff --git a/packages/api/src/mcp/registry/MCPServersInitializer.ts b/packages/api/src/mcp/registry/MCPServersInitializer.ts index ed62af36b4..143d14c699 100644 --- a/packages/api/src/mcp/registry/MCPServersInitializer.ts +++ b/packages/api/src/mcp/registry/MCPServersInitializer.ts @@ -4,7 +4,7 @@ import type * as t from '~/mcp/types'; import { registryStatusCache as statusCache } from './cache/RegistryStatusCache'; import { resolveServerInstructions, sanitizeUrlForLogging } from '~/mcp/utils'; import { MCPServersRegistry } from './MCPServersRegistry'; -import { isEnabled, withTimeout } from '~/utils'; +import { withTimeout } from '~/utils'; import { isLeader } from '~/cluster'; const DEFAULT_MCP_INIT_TIMEOUT_MS = 30_000; @@ -186,22 +186,6 @@ export class MCPServersInitializer { ); logger.info(`${prefix} Initialized in: ${config.initDuration ?? 'N/A'}ms`); logger.info(`${prefix} -------------------------------------------------┘`); - - if (config.obo != null && !isEnabled(process.env.OPENID_REUSE_TOKENS)) { - /** - * OBO requires `req.session.openidTokens` and `user.federatedTokens`, - * both of which are only populated when `OPENID_REUSE_TOKENS=true` (see - * `socialLogins.js` and `openIdJwtStrategy.js`). Without reuse, every - * OBO tool call will fail at runtime with "No valid OpenID access token - * is available for OBO exchange." Surface this misconfiguration at - * startup so operators don't have to diagnose it from per-call errors. - */ - logger.warn( - `${prefix} OBO is configured on this server but OPENID_REUSE_TOKENS is not enabled. ` + - 'OBO token exchange will fail at runtime because user.federatedTokens is never populated. ' + - "Set OPENID_REUSE_TOKENS=true or remove the `obo` block from this server's config.", - ); - } } private static formatInstructionsForLogging(instructions?: string): string { diff --git a/packages/api/src/mcp/registry/__tests__/MCPServersInitializer.test.ts b/packages/api/src/mcp/registry/__tests__/MCPServersInitializer.test.ts index 1f0b31a546..19379e39b4 100644 --- a/packages/api/src/mcp/registry/__tests__/MCPServersInitializer.test.ts +++ b/packages/api/src/mcp/registry/__tests__/MCPServersInitializer.test.ts @@ -534,7 +534,7 @@ describe('MCPServersInitializer', () => { }); }); - describe('OBO + OPENID_REUSE_TOKENS startup warning', () => { + describe('OBO initialization without browser token reuse', () => { const oboConfigs: t.MCPServers = { obo_server: { type: 'streamable-http', @@ -553,7 +553,7 @@ describe('MCPServersInitializer', () => { } }); - it('warns when OBO is configured and OPENID_REUSE_TOKENS is unset', async () => { + it('does not declare OBO unusable because bearer-auth flows remain valid', async () => { delete process.env.OPENID_REUSE_TOKENS; mockInspect.mockImplementationOnce( async (_n, raw) => @@ -565,34 +565,9 @@ describe('MCPServersInitializer', () => { await MCPServersInitializer.initialize(oboConfigs); - const warnCalls = mockLogger.warn.mock.calls.flat().join(' | '); - expect(warnCalls).toMatch(/OBO is configured/); - expect(warnCalls).toMatch(/OPENID_REUSE_TOKENS/); - }); - - it('does not warn when OPENID_REUSE_TOKENS is enabled', async () => { - process.env.OPENID_REUSE_TOKENS = 'true'; - mockInspect.mockImplementationOnce( - async (_n, raw) => - ({ - ...raw, - requiresOAuth: false, - }) as unknown as t.ParsedServerConfig, - ); - - await MCPServersInitializer.initialize(oboConfigs); - - const warnCalls = mockLogger.warn.mock.calls.flat().join(' | '); - expect(warnCalls).not.toMatch(/OBO is configured/); - }); - - it('does not warn when no OBO config is present', async () => { - delete process.env.OPENID_REUSE_TOKENS; - - await MCPServersInitializer.initialize(testConfigs); - const warnCalls = mockLogger.warn.mock.calls.flat().join(' | '); expect(warnCalls).not.toMatch(/OBO is configured/); + expect(await registry.getServerConfig('obo_server')).toBeDefined(); }); }); }); diff --git a/packages/data-schemas/src/methods/openidRefreshFlight.spec.ts b/packages/data-schemas/src/methods/openidRefreshFlight.spec.ts index 412354c7d9..a9842796dd 100644 --- a/packages/data-schemas/src/methods/openidRefreshFlight.spec.ts +++ b/packages/data-schemas/src/methods/openidRefreshFlight.spec.ts @@ -19,7 +19,6 @@ beforeAll(async () => { if (!mongoose.models.OpenIDRefreshFlight) { mongoose.model('OpenIDRefreshFlight', openidRefreshFlightSchema); } - methods = createOpenIDRefreshFlightMethods(mongoose); }); afterAll(async () => { @@ -29,16 +28,27 @@ afterAll(async () => { beforeEach(async () => { await mongoose.connection.dropDatabase(); - /** - * Mutual exclusion between workers rests entirely on the unique `key` index: without it a second - * `create` succeeds instead of raising a duplicate-key error, and every caller believes it won - * the flight. `dropDatabase` takes the indexes with it, and Mongoose only builds them once when - * the model is compiled, so they are rebuilt here rather than left to that race. - */ - await mongoose.models.OpenIDRefreshFlight.createIndexes(); + methods = createOpenIDRefreshFlightMethods(mongoose); }); describe('OpenIDRefreshFlight Methods', () => { + it('creates coordination indexes before the first acquisition', async () => { + await methods.acquireOpenIDRefreshFlight({ + key: 'flight-key', + ownerId: 'owner-1', + lockExpiresAt: new Date(Date.now() + 30000), + expiresAt: new Date(Date.now() + 60000), + }); + + const indexes = await mongoose.models.OpenIDRefreshFlight.listIndexes(); + expect(indexes).toEqual( + expect.arrayContaining([ + expect.objectContaining({ key: { key: 1 }, unique: true }), + expect.objectContaining({ key: { expiresAt: 1 }, expireAfterSeconds: 0 }), + ]), + ); + }); + it('acquires a new pending flight and returns existing flight to joiners', async () => { const first = await methods.acquireOpenIDRefreshFlight({ key: 'flight-key', diff --git a/packages/data-schemas/src/methods/openidRefreshFlight.ts b/packages/data-schemas/src/methods/openidRefreshFlight.ts index ed77027426..7ddfff6ae9 100644 --- a/packages/data-schemas/src/methods/openidRefreshFlight.ts +++ b/packages/data-schemas/src/methods/openidRefreshFlight.ts @@ -7,6 +7,7 @@ import type { OpenIDRefreshFlightQuery, OpenIDRefreshFlightAcquireResult, } from '~/types'; +import { createIndexesWithRetry } from '~/utils/retry'; import logger from '~/config/winston'; function hasErrorCode(error: unknown): error is { code: number } { @@ -36,12 +37,28 @@ export function createOpenIDRefreshFlightMethods(mongoose: typeof import('mongoo query: OpenIDRefreshFlightQuery, ) => Promise; } { + let indexesPromise: Promise | null = null; + + function ensureIndexes(): Promise { + if (!indexesPromise) { + const OpenIDRefreshFlight = mongoose.models + .OpenIDRefreshFlight as Model; + indexesPromise = createIndexesWithRetry(OpenIDRefreshFlight).catch((error) => { + indexesPromise = null; + throw error; + }); + } + return indexesPromise; + } + async function acquireOpenIDRefreshFlight( data: OpenIDRefreshFlightCreateData, ): Promise { const OpenIDRefreshFlight = mongoose.models.OpenIDRefreshFlight as Model; const now = new Date(); + await ensureIndexes(); + try { const flight = await OpenIDRefreshFlight.create({ ...data,