From d3a6f78ff84b4304fba8826c4bbde802665da132 Mon Sep 17 00:00:00 2001 From: "J.C. Bartle" Date: Sun, 28 Jun 2026 12:15:43 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A0=EF=B8=8F=20fix:=20keep=20OIDC=20re?= =?UTF-8?q?fresh=20bridge=20during=20recovery=20grace?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit After successful bridged refresh recovery, re-store the stale-cookie bridge with a short grace TTL instead of deleting it immediately. This lets parallel /api/auth/refresh requests that already sent the stale browser cookie recover before they can observe the first response's Set-Cookie. Retarget the bridge to the refresh token returned by the bridged retry so B-to-C refresh-token rotation remains recoverable. The grace TTL is parsed with math() and defaults to 60s, which shrinks the replay window from the original REFRESH_TOKEN_EXPIRY bridge lifetime to the short recovery grace period. Remove the now-unused explicit bridge delete path from the service and data-schemas method surface. Add regression coverage for grace re-store, identity symmetry, retry failure behavior, and same-key upsert replacement. --- api/server/controllers/AuthController.js | 21 +++++++--- api/server/controllers/AuthController.spec.js | 41 +++++++++++++------ api/server/services/RefreshTokenBridge.js | 27 +----------- .../services/RefreshTokenBridge.spec.js | 33 --------------- .../src/methods/refreshTokenBridge.spec.ts | 26 +----------- .../src/methods/refreshTokenBridge.ts | 15 ------- 6 files changed, 48 insertions(+), 115 deletions(-) diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js index 973aa70df9..88f3f955e3 100644 --- a/api/server/controllers/AuthController.js +++ b/api/server/controllers/AuthController.js @@ -28,11 +28,13 @@ const { getGraphApiToken } = require('~/server/services/GraphTokenService'); const { getOpenIdConfig, getOpenIdEmail } = require('~/strategies'); const { getRefreshTokenBridge, - deleteRefreshTokenBridge, + storeRefreshTokenBridge, } = require('~/server/services/RefreshTokenBridge'); const AUTH_REFRESH_USER_PROJECTION = '-password -__v -totpSecret -backupCodes -federatedTokens'; const OPENID_REUSE_EXPIRY_BUFFER_SECONDS = 30; +/** Short stale-cookie recovery window after bridged refresh succeeds. */ +const OPENID_REFRESH_BRIDGE_GRACE_MS = math(process.env.OPENID_REFRESH_BRIDGE_GRACE_MS, 60 * 1000); /** * Max age (ms) LibreChat reuses a cached OpenID session token before forcing an IdP refresh. * Env-overridable (accepts an arithmetic expression, e.g. `60 * 60 * 24 * 1000`, like @@ -335,15 +337,24 @@ const refreshController = async (req, res) => { tenantId: retryUser.tenantId, }); try { - await deleteRefreshTokenBridge({ + /** + * Keep the stale-cookie bridge briefly so parallel /refresh requests that + * already sent the old cookie can recover too. Re-storing also shrinks the + * remaining replay window from REFRESH_TOKEN_EXPIRY (potentially days) to + * this short grace TTL while Mongo/expiresAt cleanup removes it. + */ + await storeRefreshTokenBridge({ oldRefreshToken: refreshToken, + newRefreshToken: retryTokenset.refresh_token || bridgedRefreshToken, userId, tenantId: bridgeUser.tenantId, + openidIssuer: bridgeUser.openidIssuer, + ttl: OPENID_REFRESH_BRIDGE_GRACE_MS, }); - } catch (cleanupError) { + } catch (graceError) { logger.warn( - '[refreshController] Bridge cleanup failed after successful recovery', - cleanupError, + '[refreshController] Bridge grace-period storage failed after successful recovery', + graceError, ); } return res diff --git a/api/server/controllers/AuthController.spec.js b/api/server/controllers/AuthController.spec.js index ea294220af..19ac73d07b 100644 --- a/api/server/controllers/AuthController.spec.js +++ b/api/server/controllers/AuthController.spec.js @@ -23,7 +23,7 @@ jest.mock('~/models', () => ({ })); jest.mock('~/server/services/RefreshTokenBridge', () => ({ getRefreshTokenBridge: jest.fn(), - deleteRefreshTokenBridge: jest.fn(), + storeRefreshTokenBridge: jest.fn(), })); jest.mock('@librechat/api', () => ({ math: jest.fn((value, fallback) => fallback), @@ -57,7 +57,7 @@ const { getOpenIdConfig, getOpenIdEmail } = require('~/strategies'); const { getUserById, findSession, updateUser } = require('~/models'); const { getRefreshTokenBridge, - deleteRefreshTokenBridge, + storeRefreshTokenBridge, } = require('~/server/services/RefreshTokenBridge'); const ORIGINAL_OPENID_SCOPE = process.env.OPENID_SCOPE; @@ -246,7 +246,7 @@ describe('refreshController – OpenID path', () => { setCloudFrontAuthCookies.mockReturnValue(true); findOpenIDUser.mockResolvedValue({ user: { ...defaultUser }, error: null, migration: false }); getRefreshTokenBridge.mockResolvedValue(null); - deleteRefreshTokenBridge.mockReturnValue(true); + storeRefreshTokenBridge.mockResolvedValue(undefined); getUserById.mockResolvedValue({ _id: 'user-db-id', email: baseClaims.email, @@ -336,6 +336,7 @@ describe('refreshController – OpenID path', () => { expect(openIdClient.refreshTokenGrant).not.toHaveBeenCalled(); expect(setOpenIDAuthTokens).not.toHaveBeenCalled(); + expect(storeRefreshTokenBridge).not.toHaveBeenCalled(); expect(getUserById).toHaveBeenCalledWith( 'user-db-id', '-password -__v -totpSecret -backupCodes -federatedTokens', @@ -725,7 +726,7 @@ describe('refreshController – OpenID path', () => { expect(res.send).toHaveBeenCalledWith('Invalid OpenID refresh token'); }); - it('recovers stale refresh-token cookies with a bridge after session loss', async () => { + it('recovers stale refresh-token cookies and keeps a short grace bridge', async () => { setOpenIDReuseCookies(); req.session = {}; const bridgeUser = { @@ -770,15 +771,28 @@ describe('refreshController – OpenID path', () => { existingRefreshToken: 'bridged-refresh', tenantId: undefined, }); - expect(deleteRefreshTokenBridge).toHaveBeenCalledWith({ + expect(storeRefreshTokenBridge).toHaveBeenCalledWith({ oldRefreshToken: 'stored-refresh', + newRefreshToken: 'new-refresh', userId: 'user-db-id', tenantId: 'tenant-1', + openidIssuer: 'https://issuer.example.com', + ttl: 60000, }); + const lookupIdentity = getRefreshTokenBridge.mock.calls[0][0]; + const graceIdentity = storeRefreshTokenBridge.mock.calls[0][0]; + expect(graceIdentity).toEqual( + expect.objectContaining({ + oldRefreshToken: lookupIdentity.oldRefreshToken, + userId: lookupIdentity.userId, + tenantId: lookupIdentity.tenantId, + openidIssuer: lookupIdentity.openidIssuer, + }), + ); expect(res.status).toHaveBeenCalledWith(200); }); - it('does not delete the bridge when bridged refresh retry fails', async () => { + it('does not re-store the bridge when bridged refresh retry fails', async () => { setOpenIDReuseCookies(); req.session = {}; getUserById.mockResolvedValue({ @@ -794,11 +808,11 @@ describe('refreshController – OpenID path', () => { await refreshController(req, res); expect(getRefreshTokenBridge).toHaveBeenCalled(); - expect(deleteRefreshTokenBridge).not.toHaveBeenCalled(); + expect(storeRefreshTokenBridge).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(403); }); - it('returns success when bridge cleanup fails after bridged refresh succeeds', async () => { + it('returns success when bridge grace-period storage fails after bridged refresh succeeds', async () => { setOpenIDReuseCookies(); req.session = {}; getUserById.mockResolvedValue({ @@ -809,7 +823,7 @@ describe('refreshController – OpenID path', () => { openidIssuer: 'https://issuer.example.com', }); getRefreshTokenBridge.mockResolvedValue('bridged-refresh'); - deleteRefreshTokenBridge.mockRejectedValueOnce(new Error('delete failed')); + storeRefreshTokenBridge.mockRejectedValueOnce(new Error('grace failed')); openIdClient.refreshTokenGrant .mockRejectedValueOnce(new Error('invalid_grant')) .mockResolvedValueOnce(mockTokenset); @@ -821,13 +835,16 @@ describe('refreshController – OpenID path', () => { existingRefreshToken: 'bridged-refresh', tenantId: undefined, }); - expect(deleteRefreshTokenBridge).toHaveBeenCalledWith({ + expect(storeRefreshTokenBridge).toHaveBeenCalledWith({ oldRefreshToken: 'stored-refresh', + newRefreshToken: 'new-refresh', userId: 'user-db-id', tenantId: 'tenant-1', + openidIssuer: 'https://issuer.example.com', + ttl: 60000, }); expect(logger.warn).toHaveBeenCalledWith( - '[refreshController] Bridge cleanup failed after successful recovery', + '[refreshController] Bridge grace-period storage failed after successful recovery', expect.any(Error), ); expect(res.status).toHaveBeenCalledWith(200); @@ -842,7 +859,7 @@ describe('refreshController – OpenID path', () => { await refreshController(req, res); expect(getRefreshTokenBridge).not.toHaveBeenCalled(); - expect(deleteRefreshTokenBridge).not.toHaveBeenCalled(); + expect(storeRefreshTokenBridge).not.toHaveBeenCalled(); expect(res.status).toHaveBeenCalledWith(403); }); diff --git a/api/server/services/RefreshTokenBridge.js b/api/server/services/RefreshTokenBridge.js index a023d4ec2d..0a448cd00c 100644 --- a/api/server/services/RefreshTokenBridge.js +++ b/api/server/services/RefreshTokenBridge.js @@ -93,8 +93,8 @@ async function storeRefreshTokenBridge({ /** * Looks up and retrieves a stored bridge, verifying it matches the user context * and hasn't expired. Returns the decrypted rotated token on success, null - * otherwise. Does NOT consume the bridge; callers delete it only after a - * successful bridged refresh. + * otherwise. Does NOT consume the bridge; the recovery path relies on TTL expiry + * and may re-store a short grace bridge after a successful bridged refresh. * * @param {object} args * @param {string} args.oldRefreshToken — the token to look up (hashed for key) @@ -138,32 +138,9 @@ async function getRefreshTokenBridge({ oldRefreshToken, userId, tenantId, openid return decryptV2(bridge.encryptedNewRefreshToken); } -/** - * Deletes a bridge after the bridged refresh has succeeded. - * - * @param {object} args - * @param {string} args.oldRefreshToken - * @returns {Promise} - */ -async function deleteRefreshTokenBridge({ oldRefreshToken, userId, tenantId }) { - const identity = resolveBridgeIdentity({ userId, tenantId }); - - if (!oldRefreshToken || !identity) { - return false; - } - const oldRefreshTokenHash = hashRefreshToken(oldRefreshToken); - const result = await db.deleteRefreshTokenBridge({ - oldRefreshTokenHash, - userId: identity.userId, - tenantId: identity.tenantId, - }); - return (result.deletedCount ?? 0) > 0; -} - module.exports = { storeRefreshTokenBridge, getRefreshTokenBridge, - deleteRefreshTokenBridge, __internals: { hashRefreshToken, getBridgeTtlMs, diff --git a/api/server/services/RefreshTokenBridge.spec.js b/api/server/services/RefreshTokenBridge.spec.js index b83cb6c761..87f1b58dbf 100644 --- a/api/server/services/RefreshTokenBridge.spec.js +++ b/api/server/services/RefreshTokenBridge.spec.js @@ -18,7 +18,6 @@ jest.mock('@librechat/api', () => ({ jest.mock('~/models', () => ({ upsertRefreshTokenBridge: jest.fn(), findRefreshTokenBridge: jest.fn(), - deleteRefreshTokenBridge: jest.fn(), })); const { encryptV2, decryptV2 } = require('@librechat/data-schemas'); @@ -27,7 +26,6 @@ const db = require('~/models'); const { storeRefreshTokenBridge, getRefreshTokenBridge, - deleteRefreshTokenBridge, __internals, } = require('./RefreshTokenBridge'); @@ -36,7 +34,6 @@ describe('RefreshTokenBridge', () => { jest.clearAllMocks(); db.upsertRefreshTokenBridge.mockResolvedValue({}); db.findRefreshTokenBridge.mockResolvedValue(null); - db.deleteRefreshTokenBridge.mockResolvedValue({ deletedCount: 0 }); }); describe('storeRefreshTokenBridge', () => { @@ -169,34 +166,4 @@ describe('RefreshTokenBridge', () => { expect(decryptV2).not.toHaveBeenCalled(); }); }); - - describe('deleteRefreshTokenBridge', () => { - it('deletes an existing bridge explicitly', async () => { - db.deleteRefreshTokenBridge.mockResolvedValue({ deletedCount: 1 }); - - const result = await deleteRefreshTokenBridge({ - oldRefreshToken: 'rt-old', - userId: 'user-123', - tenantId: 'tenant-1', - }); - - expect(result).toBe(true); - expect(db.deleteRefreshTokenBridge).toHaveBeenCalledWith({ - oldRefreshTokenHash: __internals.hashRefreshToken('rt-old'), - userId: 'user-123', - tenantId: 'tenant-1', - }); - }); - - it('returns false when the bridge does not exist', async () => { - await expect( - deleteRefreshTokenBridge({ oldRefreshToken: 'missing', userId: 'user-123' }), - ).resolves.toBe(false); - }); - - it('returns false when userId is omitted', async () => { - await expect(deleteRefreshTokenBridge({ oldRefreshToken: 'missing' })).resolves.toBe(false); - expect(db.deleteRefreshTokenBridge).not.toHaveBeenCalled(); - }); - }); }); diff --git a/packages/data-schemas/src/methods/refreshTokenBridge.spec.ts b/packages/data-schemas/src/methods/refreshTokenBridge.spec.ts index 653f117119..0b162916c3 100644 --- a/packages/data-schemas/src/methods/refreshTokenBridge.spec.ts +++ b/packages/data-schemas/src/methods/refreshTokenBridge.spec.ts @@ -75,6 +75,7 @@ describe('RefreshTokenBridge Methods', () => { expect(found?.encryptedNewRefreshToken).toBe('encrypted-new'); expect(found?.expiresAt.getTime()).toBe(nextExpiresAt.getTime()); + expect(await mongoose.models.RefreshTokenBridge.countDocuments()).toBe(1); }); it('does not return expired bridges before Mongo TTL cleanup runs', async () => { @@ -92,29 +93,4 @@ describe('RefreshTokenBridge Methods', () => { }), ).resolves.toBeNull(); }); - - it('deletes a bridge by old token hash, user, and tenant', async () => { - await methods.upsertRefreshTokenBridge({ - oldRefreshTokenHash: 'old-hash', - encryptedNewRefreshToken: 'encrypted-new', - userId: 'user-1', - tenantId: 'tenant-1', - expiresAt: new Date(Date.now() + 60000), - }); - - const result = await methods.deleteRefreshTokenBridge({ - oldRefreshTokenHash: 'old-hash', - userId: 'user-1', - tenantId: 'tenant-1', - }); - - expect(result.deletedCount).toBe(1); - await expect( - methods.findRefreshTokenBridge({ - oldRefreshTokenHash: 'old-hash', - userId: 'user-1', - tenantId: 'tenant-1', - }), - ).resolves.toBeNull(); - }); }); diff --git a/packages/data-schemas/src/methods/refreshTokenBridge.ts b/packages/data-schemas/src/methods/refreshTokenBridge.ts index c5084ab56c..04e1251f6b 100644 --- a/packages/data-schemas/src/methods/refreshTokenBridge.ts +++ b/packages/data-schemas/src/methods/refreshTokenBridge.ts @@ -23,7 +23,6 @@ export function createRefreshTokenBridgeMethods(mongoose: typeof import('mongoos bridgeData: RefreshTokenBridgeCreateData, ) => Promise; findRefreshTokenBridge: (query: RefreshTokenBridgeQuery) => Promise; - deleteRefreshTokenBridge: (query: RefreshTokenBridgeQuery) => Promise<{ deletedCount?: number }>; } { async function upsertRefreshTokenBridge( bridgeData: RefreshTokenBridgeCreateData, @@ -77,23 +76,9 @@ export function createRefreshTokenBridgeMethods(mongoose: typeof import('mongoos } } - async function deleteRefreshTokenBridge( - query: RefreshTokenBridgeQuery, - ): Promise<{ deletedCount?: number }> { - try { - const RefreshTokenBridge = mongoose.models.RefreshTokenBridge as Model; - const result = await RefreshTokenBridge.deleteOne(bridgeFilter(query)); - return { deletedCount: result.deletedCount }; - } catch (error) { - logger.debug('[deleteRefreshTokenBridge] Error deleting bridge:', error); - throw error; - } - } - return { upsertRefreshTokenBridge, findRefreshTokenBridge, - deleteRefreshTokenBridge, }; }