From d23aea159153b52b93a98df38fa95c1a29dc7ff2 Mon Sep 17 00:00:00 2001 From: "J.C. Bartle" Date: Sun, 28 Jun 2026 19:57:50 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A0=EF=B8=8F=20fix:=20Harden=20OBO=20r?= =?UTF-8?q?efresh-token=20bridge=20lookup=20and=20indexing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reuse getValidOpenIDReuseUserId for the bridge-recovery user lookup in refreshController instead of re-verifying openid_user_id inline. The shared helper enforces the JWT_REFRESH_SECRET presence check and a strict typeof payload.id === 'string' guard, rejecting tokens whose id claim is present but not a string (e.g. a numeric id) that the inline check accepted. Fail closed on issuer mismatch in getRefreshTokenBridge. Both the stored and the expected issuer are now normalized and compared for equality, so a bridge is recovered only when both sides agree (both absent, or both present and equal after normalization). Previously the check was skipped whenever the stored issuer was absent, allowing recovery across mismatched issuer context. Drop the unused {oldRefreshTokenHash, userId, tenantId, openidIssuer} index and the openidIssuer field on RefreshTokenBridgeQuery. The data-layer filter only queries the 3-field {oldRefreshTokenHash, userId, tenantId} index; the issuer is verified in application code, not the query. Hoist the repeated model accessor into getRefreshTokenBridgeModel. Note: issuer is now load-bearing for recovery. A bridge stored with an issuer recovers only when the lookup supplies a matching issuer; the recovery lookup reads user.openidIssuer via AUTH_REFRESH_USER_PROJECTION (an exclusion projection that retains the field). If a user's persisted openidIssuer is empty while the stored bridge has one, recovery fails closed (falls through to normal re-authentication) until the bridge TTLs out — no security regression. Tests cover invalid signed-cookie payloads bypassing the bridge, both asymmetric issuer-presence cases, issuer normalization before comparison, and an index-alignment assertion guarding against re-adding the dropped index. --- api/server/controllers/AuthController.js | 161 +++++++++--------- api/server/controllers/AuthController.spec.js | 11 ++ api/server/services/RefreshTokenBridge.js | 10 +- .../services/RefreshTokenBridge.spec.js | 52 ++++++ .../src/methods/refreshTokenBridge.spec.ts | 12 ++ .../src/methods/refreshTokenBridge.ts | 7 +- .../src/schema/refreshTokenBridge.ts | 6 - .../src/types/refreshTokenBridge.ts | 1 - 8 files changed, 166 insertions(+), 94 deletions(-) diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js index bb2ad39beb..ed1f9cb100 100644 --- a/api/server/controllers/AuthController.js +++ b/api/server/controllers/AuthController.js @@ -354,98 +354,93 @@ const refreshController = async (req, res) => { */ if (isInvalidGrantError(error) && refreshToken) { // Bridge lookup uses the signed user-id cookie because /refresh is unauthenticated. - const openidUserId = parsedCookies.openid_user_id; - if (openidUserId) { + const userId = getValidOpenIDReuseUserId(parsedCookies); + if (userId) { try { - const payload = jwt.verify(openidUserId, process.env.JWT_REFRESH_SECRET); - const userId = typeof payload === 'object' && payload?.id ? payload.id : null; + const bridgeUser = await getUserById(userId, AUTH_REFRESH_USER_PROJECTION); + if (!bridgeUser) { + return res.status(403).send('Invalid OpenID refresh token'); + } - if (userId) { - const bridgeUser = await getUserById(userId, AUTH_REFRESH_USER_PROJECTION); - if (!bridgeUser) { - return res.status(403).send('Invalid OpenID refresh token'); - } + const bridgedRefreshToken = await getRefreshTokenBridge({ + oldRefreshToken: refreshToken, + userId, + tenantId: bridgeUser.tenantId, + openidIssuer: bridgeUser.openidIssuer, + }); - const bridgedRefreshToken = await getRefreshTokenBridge({ - oldRefreshToken: refreshToken, - userId, - tenantId: bridgeUser.tenantId, - openidIssuer: bridgeUser.openidIssuer, - }); + if (bridgedRefreshToken) { + logger.info( + '[refreshController] Recovered via refresh-token bridge after invalid_grant', + { + userId, + }, + ); - if (bridgedRefreshToken) { - logger.info( - '[refreshController] Recovered via refresh-token bridge after invalid_grant', - { - userId, - }, - ); + // Retry with the recovered (rotated) refresh token + try { + const { + tokenset: retryTokenset, + claims: retryClaims, + openidIssuer: retryOpenidIssuer, + user: retryUser, + error: retryError, + } = await refreshOpenIDUser({ + refreshToken: bridgedRefreshToken, + strategyName: 'refreshController (bridge recovery)', + }); - // Retry with the recovered (rotated) refresh token - try { - const { - tokenset: retryTokenset, - claims: retryClaims, - openidIssuer: retryOpenidIssuer, - user: retryUser, - error: retryError, - } = await refreshOpenIDUser({ - refreshToken: bridgedRefreshToken, - strategyName: 'refreshController (bridge recovery)', - }); - - if (retryUser && !retryError) { - if (retryUser._id.toString() !== userId) { - logger.warn( - '[refreshController] Bridge recovery resolved a different user; refusing token issuance', - { - cookieUserId: userId, - resolvedUserId: retryUser._id.toString(), - }, - ); - return res.status(403).send('Invalid OpenID refresh token'); - } - - try { - /** - * 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 (graceError) { - logger.warn( - '[refreshController] Bridge grace-period storage failed after successful recovery', - graceError, - ); - } - return sendOpenIDAuthResponse({ - tokenset: retryTokenset, - user: retryUser, - existingRefreshToken: bridgedRefreshToken, - openidSubject: retryClaims?.sub, - openidIssuer: retryOpenidIssuer, - req, - res, - }); + if (retryUser && !retryError) { + if (retryUser._id.toString() !== userId) { + logger.warn( + '[refreshController] Bridge recovery resolved a different user; refusing token issuance', + { + cookieUserId: userId, + resolvedUserId: retryUser._id.toString(), + }, + ); + return res.status(403).send('Invalid OpenID refresh token'); } - } catch (retryError) { - logger.error('[refreshController] Bridge recovery retry failed', retryError); - // Fall through to generic error response + + try { + /** + * 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 (graceError) { + logger.warn( + '[refreshController] Bridge grace-period storage failed after successful recovery', + graceError, + ); + } + return sendOpenIDAuthResponse({ + tokenset: retryTokenset, + user: retryUser, + existingRefreshToken: bridgedRefreshToken, + openidSubject: retryClaims?.sub, + openidIssuer: retryOpenidIssuer, + req, + res, + }); } + } catch (retryError) { + logger.error('[refreshController] Bridge recovery retry failed', retryError); + // Fall through to generic error response } } - } catch (verifyError) { - logger.debug('[refreshController] Could not verify openid_user_id for bridge lookup', { - error: verifyError.message, + } catch (bridgeError) { + logger.debug('[refreshController] Refresh-token bridge lookup failed', { + error: bridgeError.message, }); } } diff --git a/api/server/controllers/AuthController.spec.js b/api/server/controllers/AuthController.spec.js index 9bf013730b..cd99f8e1c3 100644 --- a/api/server/controllers/AuthController.spec.js +++ b/api/server/controllers/AuthController.spec.js @@ -914,6 +914,17 @@ describe('refreshController – OpenID path', () => { expect(res.send).toHaveBeenCalledWith('Invalid OpenID refresh token'); }); + it('does not use the bridge when signed user-id cookie payload is invalid', async () => { + setOpenIDReuseCookies(jwt.sign({ id: 123 }, process.env.JWT_REFRESH_SECRET)); + 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/RefreshTokenBridge.js b/api/server/services/RefreshTokenBridge.js index 9f474a91b2..bd6cb005a2 100644 --- a/api/server/services/RefreshTokenBridge.js +++ b/api/server/services/RefreshTokenBridge.js @@ -103,7 +103,7 @@ async function storeRefreshTokenBridge({ * @param {string} args.oldRefreshToken — the token to look up (hashed for key) * @param {string} args.userId — current user._id (must match the bridged context) * @param {string} [args.tenantId] — current user.tenantId (optional but verified if present) - * @param {string} [args.openidIssuer] — current user.openidIssuer (optional but verified if present) + * @param {string} [args.openidIssuer] — current user.openidIssuer (must match stored issuer after normalization) * @returns {Promise} the rotated refresh token if found and valid, null otherwise */ async function getRefreshTokenBridge({ oldRefreshToken, userId, tenantId, openidIssuer }) { @@ -124,7 +124,13 @@ async function getRefreshTokenBridge({ oldRefreshToken, userId, tenantId, openid return null; } - if (bridge.openidIssuer && bridge.openidIssuer !== identity.openidIssuer) { + const bridgeIdentity = resolveBridgeIdentity({ + userId: bridge.userId, + tenantId: bridge.tenantId, + openidIssuer: bridge.openidIssuer, + }); + + if (!bridgeIdentity || bridgeIdentity.openidIssuer !== identity.openidIssuer) { logger.warn('[RefreshTokenBridge] Bridge lookup failed: issuer mismatch', { tokenHash: oldRefreshTokenHash, }); diff --git a/api/server/services/RefreshTokenBridge.spec.js b/api/server/services/RefreshTokenBridge.spec.js index 87f1b58dbf..631920aa45 100644 --- a/api/server/services/RefreshTokenBridge.spec.js +++ b/api/server/services/RefreshTokenBridge.spec.js @@ -165,5 +165,57 @@ describe('RefreshTokenBridge', () => { expect(result).toBeNull(); expect(decryptV2).not.toHaveBeenCalled(); }); + + it('returns null when only the expected issuer is present', async () => { + db.findRefreshTokenBridge.mockResolvedValue({ + encryptedNewRefreshToken: 'encrypted:rt-new', + userId: 'user-123', + createdAt: new Date(), + }); + + const result = await getRefreshTokenBridge({ + oldRefreshToken: 'rt-old', + userId: 'user-123', + openidIssuer: 'https://issuer.example.com', + }); + + expect(result).toBeNull(); + expect(decryptV2).not.toHaveBeenCalled(); + }); + + it('returns null when only the stored issuer is present', async () => { + db.findRefreshTokenBridge.mockResolvedValue({ + encryptedNewRefreshToken: 'encrypted:rt-new', + userId: 'user-123', + openidIssuer: 'https://issuer.example.com', + createdAt: new Date(), + }); + + const result = await getRefreshTokenBridge({ + oldRefreshToken: 'rt-old', + userId: 'user-123', + }); + + expect(result).toBeNull(); + expect(decryptV2).not.toHaveBeenCalled(); + }); + + it('normalizes the stored issuer before validation', async () => { + db.findRefreshTokenBridge.mockResolvedValue({ + encryptedNewRefreshToken: 'encrypted:rt-new', + userId: 'user-123', + openidIssuer: 'https://issuer.example.com/.well-known/openid-configuration', + createdAt: new Date(), + }); + + const result = await getRefreshTokenBridge({ + oldRefreshToken: 'rt-old', + userId: 'user-123', + openidIssuer: 'https://issuer.example.com/', + }); + + expect(decryptV2).toHaveBeenCalledWith('encrypted:rt-new'); + expect(result).toBe('rt-new'); + }); }); }); diff --git a/packages/data-schemas/src/methods/refreshTokenBridge.spec.ts b/packages/data-schemas/src/methods/refreshTokenBridge.spec.ts index 0b162916c3..9e211c7777 100644 --- a/packages/data-schemas/src/methods/refreshTokenBridge.spec.ts +++ b/packages/data-schemas/src/methods/refreshTokenBridge.spec.ts @@ -32,6 +32,18 @@ beforeEach(async () => { }); describe('RefreshTokenBridge Methods', () => { + it('keeps the lookup indexes aligned with the data-layer query shape', () => { + const indexKeys = mongoose.models.RefreshTokenBridge.schema.indexes().map(([key]) => key); + + expect(indexKeys).toContainEqual({ oldRefreshTokenHash: 1, userId: 1, tenantId: 1 }); + expect(indexKeys).not.toContainEqual({ + oldRefreshTokenHash: 1, + userId: 1, + tenantId: 1, + openidIssuer: 1, + }); + }); + it('upserts and finds a bridge by old token hash, user, and tenant', async () => { await methods.upsertRefreshTokenBridge({ oldRefreshTokenHash: 'old-hash', diff --git a/packages/data-schemas/src/methods/refreshTokenBridge.ts b/packages/data-schemas/src/methods/refreshTokenBridge.ts index b9fbab992a..038563c4d3 100644 --- a/packages/data-schemas/src/methods/refreshTokenBridge.ts +++ b/packages/data-schemas/src/methods/refreshTokenBridge.ts @@ -24,11 +24,14 @@ export function createRefreshTokenBridgeMethods(mongoose: typeof import('mongoos ) => Promise; findRefreshTokenBridge: (query: RefreshTokenBridgeQuery) => Promise; } { + const getRefreshTokenBridgeModel = () => + mongoose.models.RefreshTokenBridge as Model; + async function upsertRefreshTokenBridge( bridgeData: RefreshTokenBridgeCreateData, ): Promise { try { - const RefreshTokenBridge = mongoose.models.RefreshTokenBridge as Model; + const RefreshTokenBridge = getRefreshTokenBridgeModel(); const filter = bridgeFilter(bridgeData); const update: UpdateQuery = { $set: { @@ -58,7 +61,7 @@ export function createRefreshTokenBridgeMethods(mongoose: typeof import('mongoos query: RefreshTokenBridgeQuery, ): Promise { try { - const RefreshTokenBridge = mongoose.models.RefreshTokenBridge as Model; + const RefreshTokenBridge = getRefreshTokenBridgeModel(); return await RefreshTokenBridge.findOne({ ...bridgeFilter(query), expiresAt: { $gt: new Date() }, diff --git a/packages/data-schemas/src/schema/refreshTokenBridge.ts b/packages/data-schemas/src/schema/refreshTokenBridge.ts index 31e095b8f5..a2634c9666 100644 --- a/packages/data-schemas/src/schema/refreshTokenBridge.ts +++ b/packages/data-schemas/src/schema/refreshTokenBridge.ts @@ -38,11 +38,5 @@ refreshTokenBridgeSchema.index( { oldRefreshTokenHash: 1, userId: 1, tenantId: 1 }, { unique: true }, ); -refreshTokenBridgeSchema.index({ - oldRefreshTokenHash: 1, - userId: 1, - tenantId: 1, - openidIssuer: 1, -}); export default refreshTokenBridgeSchema; diff --git a/packages/data-schemas/src/types/refreshTokenBridge.ts b/packages/data-schemas/src/types/refreshTokenBridge.ts index 10b23ca56d..9acd1f5975 100644 --- a/packages/data-schemas/src/types/refreshTokenBridge.ts +++ b/packages/data-schemas/src/types/refreshTokenBridge.ts @@ -23,5 +23,4 @@ export interface RefreshTokenBridgeQuery { oldRefreshTokenHash: string; userId: string; tenantId?: string; - openidIssuer?: string; }