From cb26a6f7d220dcb7185d46a39a2d3f14f5149402 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 29 Aug 2026 17:29:39 +0000 Subject: [PATCH] =?UTF-8?q?=E2=8F=B3=20fix:=20Keep=20Elapsed=20Expiries=20?= =?UTF-8?q?Elapsed=20and=20Revoke=20the=20Superseded=20Session?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the five findings from the Codex pass on fcdc15885 — the two that are defects in code this branch introduced rather than design questions about the bridge. `getSkewedTokenExpiresAtMs` floored every result at a second in the future, including an expiry the provider had already declared elapsed. An exchange answering `expires_in: 0` or a past `expires_at` was handed to the MCP connection stamped valid for another second, which only moves the failure downstream. The floor now applies to a lifetime that is still live, which is what it was for; an elapsed one stays elapsed so the caller rejects it. Same for the cache TTL, which falls back to the elapsed-credential floor. Bridge recovery left the stale token's durable Session behind. Only the token it recovered through was passed as `existingRefreshToken`, so that one's session was replaced while the token the browser actually presented kept its record until its original expiry. That record, with the marker cookie still bound to it, is what authorizes local image access for OpenID users — so a copy of the stale cookie outlived the rotation it had lost. Revoked explicitly on successful recovery. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01SxKWxwqxAGckYpRsYTqx3F --- api/server/controllers/AuthController.js | 22 ++++++++++++++++++- api/server/controllers/AuthController.spec.js | 7 +++++- packages/api/src/oauth/expiry.spec.ts | 12 ++++++++-- packages/api/src/oauth/expiry.ts | 9 ++++++++ 4 files changed, 46 insertions(+), 4 deletions(-) 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); }