diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js index a54c5ab661..c019a78a9f 100644 --- a/api/server/controllers/AuthController.js +++ b/api/server/controllers/AuthController.js @@ -2,7 +2,7 @@ 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 { logger, runAsSystem } = require('@librechat/data-schemas'); const { math, isEnabled, @@ -24,6 +24,7 @@ const { } = require('~/server/services/AuthService'); const { deleteAllUserSessions, + deleteSession, getUserById, findSession, updateUser, @@ -183,6 +184,7 @@ const sendOpenIDAuthResponse = async ({ tokenset, user, existingRefreshToken, + revokedRefreshToken, openidSubject, openidIssuer, req, @@ -201,6 +203,23 @@ const sendOpenIDAuthResponse = async ({ user.tenantId, existingRefreshToken, ); + /** + * Bridge recovery replaces a token the IdP already rejected, and only the token it recovered + * through is passed above. The stale one the browser presented keeps its own durable Session + * until its original expiry otherwise, and that record — with the marker cookie still bound to + * it — is what authorizes local image access, so a copy of that cookie would outlive the + * rotation it lost. Revoked explicitly here. + */ + if (revokedRefreshToken && revokedRefreshToken !== existingRefreshToken) { + try { + await runAsSystem(() => deleteSession({ refreshToken: revokedRefreshToken })); + } catch (error) { + logger.warn( + '[refreshController] Failed to revoke the superseded refresh-token session', + error, + ); + } + } const token = setOpenIDAuthTokens(tokenset, req, res, { userId, existingRefreshToken, @@ -450,6 +469,7 @@ const refreshController = async (req, res) => { tokenset: retryTokenset, user: retryUser, existingRefreshToken: bridgedRefreshToken, + revokedRefreshToken: refreshToken, openidSubject: retryClaims?.sub, openidIssuer: retryOpenidIssuer, req, diff --git a/api/server/controllers/AuthController.spec.js b/api/server/controllers/AuthController.spec.js index 3f4712ee90..8f8740666a 100644 --- a/api/server/controllers/AuthController.spec.js +++ b/api/server/controllers/AuthController.spec.js @@ -1,5 +1,6 @@ jest.mock('@librechat/data-schemas', () => ({ logger: { error: jest.fn(), debug: jest.fn(), warn: jest.fn(), info: jest.fn() }, + runAsSystem: (callback) => callback(), })); jest.mock('~/server/services/GraphTokenService', () => ({ getGraphApiToken: jest.fn(), @@ -17,6 +18,7 @@ jest.mock('~/strategies', () => ({ getOpenIdConfig: jest.fn(), getOpenIdEmail: j jest.mock('openid-client', () => ({ refreshTokenGrant: jest.fn() })); jest.mock('~/models', () => ({ deleteAllUserSessions: jest.fn(), + deleteSession: jest.fn(), getUserById: jest.fn(), findSession: jest.fn(), updateUser: jest.fn(), @@ -84,7 +86,7 @@ const { setAuthTokens, } = require('~/server/services/AuthService'); const { getOpenIdConfig, getOpenIdEmail } = require('~/strategies'); -const { getUserById, findSession, updateUser } = require('~/models'); +const { getUserById, findSession, updateUser, deleteSession } = require('~/models'); const { getRefreshTokenBridge, storeRefreshTokenBridge, @@ -1040,6 +1042,9 @@ describe('refreshController – OpenID path', () => { 'tenant-1', 'bridged-refresh', ); + /** The stale token the browser presented is dead upstream, but its durable Session — and the + * marker bound to it — would otherwise keep authorizing local image access until expiry. */ + expect(deleteSession).toHaveBeenCalledWith({ refreshToken: 'stored-refresh' }); const lookupIdentity = getRefreshTokenBridge.mock.calls[0][0]; const graceIdentity = storeRefreshTokenBridge.mock.calls[0][0]; expect(graceIdentity).toEqual( diff --git a/packages/api/src/oauth/expiry.spec.ts b/packages/api/src/oauth/expiry.spec.ts index 8257c5ed42..0e982da296 100644 --- a/packages/api/src/oauth/expiry.spec.ts +++ b/packages/api/src/oauth/expiry.spec.ts @@ -211,12 +211,20 @@ describe('skew helpers', () => { expect(getSkewedTokenCacheTtlMs(expiresAt, now)).toBe(120_000 - bufferMs); }); - it('floors a lifetime shorter than the buffer to a usable minimum, never 0', () => { + it('floors a live lifetime shorter than the buffer to a usable minimum, never 0', () => { const expiresAt = now + 10_000; expect(getSkewedTokenExpiresAtMs(expiresAt, now)).toBe(now + 1000); expect(getSkewedTokenCacheTtlMs(expiresAt, now)).toBe(1000); - expect(getSkewedTokenCacheTtlMs(now - 60_000, now)).toBe(1000); + }); + + /** The floor exists to keep a short-but-real credential usable, not to revive a dead one: a + * provider that declares an elapsed expiry must not have it stamped into the future. */ + it('leaves an already-elapsed expiry elapsed', () => { + expect(getSkewedTokenExpiresAtMs(now - 60_000, now)).toBe(now - 60_000); + expect(getSkewedTokenExpiresAtMs(now, now)).toBe(now); + expect(getSkewedTokenCacheTtlMs(now - 60_000, now)).toBe(1); + expect(getSkewedTokenCacheTtlMs(now, now)).toBe(1); }); }); diff --git a/packages/api/src/oauth/expiry.ts b/packages/api/src/oauth/expiry.ts index 6441cee800..84981d01f7 100644 --- a/packages/api/src/oauth/expiry.ts +++ b/packages/api/src/oauth/expiry.ts @@ -129,11 +129,20 @@ export function getTokenExpiresAtMs({ * buffer, so a credential handed on with this stamp cannot be accepted into its final seconds. */ export function getSkewedTokenExpiresAtMs(expiresAt: number, now: number): number { + /** An expiry already in the past is the provider saying the credential is dead. Flooring it to a + * moment in the future would hand a consumer a token that cannot work, so it stays elapsed and + * the caller rejects the exchange instead of failing downstream. */ + if (expiresAt <= now) { + return expiresAt; + } return Math.max(now + MIN_LIVE_TOKEN_TTL_MS, expiresAt - OPENID_EXPIRY_BUFFER_SECONDS * 1000); } /** Cache TTL for a token whose absolute expiry is already known, buffered as above. */ export function getSkewedTokenCacheTtlMs(expiresAt: number, now: number): number { + if (expiresAt <= now) { + return EXPIRED_CACHE_TTL_MS; + } return Math.max(MIN_LIVE_TOKEN_TTL_MS, expiresAt - now - OPENID_EXPIRY_BUFFER_SECONDS * 1000); }