From d17ac8f06dd4956f29e657a068f4cc5a241adf08 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Mon, 16 Mar 2026 09:23:46 -0400 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=8F=20fix:=20Remove=20Federated=20Toke?= =?UTF-8?q?ns=20from=20OpenID=20Refresh=20Response=20(#12264)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🔒 fix: Remove OpenID federated tokens from refresh endpoint response The refresh controller was attaching federatedTokens (including the refresh_token) to the user object returned in the JSON response, exposing HttpOnly-protected tokens to client-side JavaScript. The tokens are already stored server-side by setOpenIDAuthTokens and re-attached by the JWT strategy on authenticated requests. * 🔒 fix: Strip sensitive fields from OpenID refresh response user object The OpenID refresh path returned the raw findOpenIDUser result without field projection, unlike the non-OpenID path which excludes password, __v, totpSecret, and backupCodes via getUserById projection. Destructure out sensitive fields before serializing. Also strengthens the regression test: uses not.toHaveProperty for true property-absence checks (expect.anything() misses null/undefined), adds positive shape assertion, and DRYs up duplicated mock user setup. --- api/server/controllers/AuthController.js | 10 +---- api/server/controllers/AuthController.spec.js | 44 +++++++++++++------ 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/api/server/controllers/AuthController.js b/api/server/controllers/AuthController.js index 13d024cd03..eb44feffa4 100644 --- a/api/server/controllers/AuthController.js +++ b/api/server/controllers/AuthController.js @@ -119,14 +119,8 @@ const refreshController = async (req, res) => { const token = setOpenIDAuthTokens(tokenset, req, res, user._id.toString(), refreshToken); - user.federatedTokens = { - access_token: tokenset.access_token, - id_token: tokenset.id_token, - refresh_token: refreshToken, - expires_at: claims.exp, - }; - - return res.status(200).send({ token, user }); + const { password: _pw, __v: _v, totpSecret: _ts, backupCodes: _bc, ...safeUser } = user; + return res.status(200).send({ token, user: safeUser }); } catch (error) { logger.error('[refreshController] OpenID token refresh error', error); return res.status(403).send('Invalid OpenID refresh token'); diff --git a/api/server/controllers/AuthController.spec.js b/api/server/controllers/AuthController.spec.js index fef670baa8..964947def9 100644 --- a/api/server/controllers/AuthController.spec.js +++ b/api/server/controllers/AuthController.spec.js @@ -163,6 +163,16 @@ describe('refreshController – OpenID path', () => { exp: 9999999999, }; + const defaultUser = { + _id: 'user-db-id', + email: baseClaims.email, + openidId: baseClaims.sub, + password: '$2b$10$hashedpassword', + __v: 0, + totpSecret: 'encrypted-totp-secret', + backupCodes: ['hashed-code-1', 'hashed-code-2'], + }; + let req, res; beforeEach(() => { @@ -174,6 +184,7 @@ describe('refreshController – OpenID path', () => { mockTokenset.claims.mockReturnValue(baseClaims); getOpenIdEmail.mockReturnValue(baseClaims.email); setOpenIDAuthTokens.mockReturnValue('new-app-token'); + findOpenIDUser.mockResolvedValue({ user: { ...defaultUser }, error: null, migration: false }); updateUser.mockResolvedValue({}); req = { @@ -189,13 +200,6 @@ describe('refreshController – OpenID path', () => { }); it('should call getOpenIdEmail with token claims and use result for findOpenIDUser', async () => { - const user = { - _id: 'user-db-id', - email: baseClaims.email, - openidId: baseClaims.sub, - }; - findOpenIDUser.mockResolvedValue({ user, error: null, migration: false }); - await refreshController(req, res); expect(getOpenIdEmail).toHaveBeenCalledWith(baseClaims); @@ -229,13 +233,6 @@ describe('refreshController – OpenID path', () => { it('should fall back to claims.email when configured claim is absent from token claims', async () => { getOpenIdEmail.mockReturnValue(baseClaims.email); - const user = { - _id: 'user-db-id', - email: baseClaims.email, - openidId: baseClaims.sub, - }; - findOpenIDUser.mockResolvedValue({ user, error: null, migration: false }); - await refreshController(req, res); expect(findOpenIDUser).toHaveBeenCalledWith( @@ -243,6 +240,25 @@ describe('refreshController – OpenID path', () => { ); }); + it('should not expose sensitive fields or federatedTokens in refresh response', async () => { + await refreshController(req, res); + + const sentPayload = res.send.mock.calls[0][0]; + expect(sentPayload).toEqual({ + token: 'new-app-token', + user: expect.objectContaining({ + _id: 'user-db-id', + email: baseClaims.email, + openidId: baseClaims.sub, + }), + }); + expect(sentPayload.user).not.toHaveProperty('federatedTokens'); + expect(sentPayload.user).not.toHaveProperty('password'); + expect(sentPayload.user).not.toHaveProperty('totpSecret'); + expect(sentPayload.user).not.toHaveProperty('backupCodes'); + expect(sentPayload.user).not.toHaveProperty('__v'); + }); + it('should update openidId when migration is triggered on refresh', async () => { const user = { _id: 'user-db-id', email: baseClaims.email, openidId: null }; findOpenIDUser.mockResolvedValue({ user, error: null, migration: true });