mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-09-21 15:45:22 +00:00
🛠️ fix: keep OIDC refresh bridge during recovery grace
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.
This commit is contained in:
J.C. Bartle
parent
27c6eb1e35
commit
d3a6f78ff8
6 changed files with 48 additions and 115 deletions
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue