From 39c2a03a8b51bc90fc8c84ae6882fc522728de73 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 14 Jun 2026 20:58:40 +0200 Subject: [PATCH 1/3] fix(auth): remove unverified client-asserted OAuth identity branch + throttle callback --- modules/auth/controllers/auth.controller.js | 41 ++------------- modules/auth/routes/auth.routes.js | 4 +- modules/auth/tests/auth.integration.tests.js | 54 ++++++++------------ 3 files changed, 27 insertions(+), 72 deletions(-) diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 893429d68..d69dfd410 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -636,43 +636,10 @@ const oauthErrorRedirect = (res, err, fallbackTitle) => { */ const oauthCallback = async (req, res, next) => { const strategy = req.params.strategy; - // app Auth with Strategy managed on client side - if (req.body?.strategy === false && req.body?.key) { - const allowedKeys = ['id', 'sub', 'email']; - if (!allowedKeys.includes(req.body.key)) { - return responses.error(res, 422, 'Unprocessable Entity', 'Invalid provider key')(); - } - try { - let user = { - firstName: req.body.firstName, - lastName: req.body.lastName, - email: req.body.email, - providerData: {}, - }; - user.providerData[req.body.key] = req.body.value; - user = await checkOAuthUserProfile(user, req.body.key, strategy); - const token = jwt.sign({ userId: user.id }, config.jwt.secret, { - expiresIn: config.jwt.expiresIn, - }); - return res - .status(200) - .cookie('TOKEN', token, tokenCookieOptions) - .json({ - user, - tokenExpiresIn: Date.now() + config.jwt.expiresIn * 1000, - type: 'success', - message: 'oAuth Ok', - }); - } catch (err) { - return responses.error( - res, - 422, - err instanceof AppError && err.code === 'VALIDATION_ERROR' ? errors.getMessage(err) : 'Unprocessable Entity', - errors.getMessage(err.details || err), - )(err); - } - } - // classic web oAuth + // The identity is always derived server-side from the provider's response via + // passport. A client-asserted identity (request body) is never trusted, since + // nothing on this path verifies a provider token — accepting one would allow a + // caller who knows a victim's identifier to mint that victim's session. passport.authenticate(strategy, (err, user) => { if (err) { logger.error( diff --git a/modules/auth/routes/auth.routes.js b/modules/auth/routes/auth.routes.js index ca5267748..b90b722b9 100644 --- a/modules/auth/routes/auth.routes.js +++ b/modules/auth/routes/auth.routes.js @@ -93,6 +93,6 @@ export default (app) => { // Setting the oauth routes app.route('/api/auth/:strategy').get(auth.oauthCall); - app.route('/api/auth/:strategy/callback').get(auth.oauthCallback); - app.route('/api/auth/:strategy/callback').post(auth.oauthCallback); // specific for apple call back + app.route('/api/auth/:strategy/callback').get(authLimiter, auth.oauthCallback); + app.route('/api/auth/:strategy/callback').post(authLimiter, auth.oauthCallback); // specific for apple call back }; diff --git a/modules/auth/tests/auth.integration.tests.js b/modules/auth/tests/auth.integration.tests.js index 37352c9ce..bdb72eb3c 100644 --- a/modules/auth/tests/auth.integration.tests.js +++ b/modules/auth/tests/auth.integration.tests.js @@ -571,47 +571,35 @@ describe('Auth integration tests:', () => { createSpy.mockRestore(); }); - test('should authenticate via client-side OAuth and set tokenCookieOptions on response', async () => { - const oauthEmail = 'oauthcb-appauth@test.com'; + test('should NOT mint a session from a client-asserted identity on the OAuth callback', async () => { + // Security regression: the callback must never trust a client-supplied + // identity payload. A request that claims to be a known account by email + // (no server-side provider-token verification) must be rejected, not + // turned into a victim session. The callback always delegates to passport. + const victimEmail = 'oauthcb-victim@test.com'; + const victim = await UserService.create({ + firstName: 'Victim', + lastName: 'Owner', + email: victimEmail, + provider: 'google', + providerData: { email: victimEmail }, + roles: ['user'], + }); try { const result = await agent .post('/api/auth/google/callback') - .send({ strategy: false, key: 'id', value: 'cb-app-auth-id-999', firstName: 'OAuth', lastName: 'Callback', email: oauthEmail }) - .expect(200); + .send({ strategy: false, key: 'email', value: victimEmail, email: victimEmail }); + + // No session may be issued to the attacker. const tokenCookie = result.headers['set-cookie']?.find((c) => c.startsWith('TOKEN=')); - expect(tokenCookie).toBeDefined(); - expect(tokenCookie).toMatch(/HttpOnly/i); - expect(tokenCookie).toMatch(/SameSite=Strict/i); - expect(result.body.message).toBe('oAuth Ok'); - } catch (err) { - console.log(err); - expect(err).toBeFalsy(); + expect(tokenCookie).toBeUndefined(); + // The unverified client-asserted path must not succeed. + expect(result.status).toBeGreaterThanOrEqual(400); } finally { - try { - const u = await UserService.getBrut({ email: oauthEmail }); - if (u) await UserService.remove(u); - } catch (_) { /* cleanup */ } + try { await UserService.remove(victim); } catch (_) { /* cleanup */ } } }); - test('should return 422 when client-side OAuth callback receives an invalid profile', async () => { - const result = await agent - .post('/api/auth/google/callback') - .send({ - strategy: false, - key: 'id', - value: 'cb-app-auth-id-invalid-999', - firstName: 'Invalid1', - lastName: 'Callback', - email: 'oauthcb-invalid@test.com', - }) - .expect(422); - - expect(result.body.type).toBe('error'); - expect(result.body.message).toMatch(/^Schema validation error/); - expect(result.body.description).toEqual(expect.any(String)); - }); - test('should set tokenCookieOptions and redirect on classic web oAuth success', async () => { const mockUserId = 'mock-oauth-user-id-123'; const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce( From e4ee5d96cc22832bd79df302b77952f48451af8a Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 14 Jun 2026 21:55:38 +0200 Subject: [PATCH 2/3] fix(auth): throttle the OAuth initiation route for consistency --- modules/auth/routes/auth.routes.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/modules/auth/routes/auth.routes.js b/modules/auth/routes/auth.routes.js index b90b722b9..37ea63c03 100644 --- a/modules/auth/routes/auth.routes.js +++ b/modules/auth/routes/auth.routes.js @@ -92,7 +92,7 @@ export default (app) => { app.route('/api/auth/token').get(passport.authenticate('jwt', { session: false }), auth.token); // Setting the oauth routes - app.route('/api/auth/:strategy').get(auth.oauthCall); + app.route('/api/auth/:strategy').get(authLimiter, auth.oauthCall); app.route('/api/auth/:strategy/callback').get(authLimiter, auth.oauthCallback); app.route('/api/auth/:strategy/callback').post(authLimiter, auth.oauthCallback); // specific for apple call back }; From d1ce8ecd42f1586c487cc58fc2c5070a2a5ea063 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sun, 14 Jun 2026 22:13:55 +0200 Subject: [PATCH 3/3] test(auth): robust oauth-callback no-session assertion + pre-cleanup; clarify comment - Move victim UserService.create() inside try/finally to avoid flakiness on reruns (pre-delete any leftover oauthcb-victim@test.com first) - Replace fragile `status >= 400` with `not.toBe(200)` + no-TOKEN-cookie; the failure path may 302-redirect rather than return 4xx, so the real security invariant (no session minted) is now the hard assertion - Reword oauthCallback comment: passport.authenticate() does verify the provider response; the unsafe part was trusting a client-asserted body identity with no provider-token check (that branch is now removed) --- modules/auth/controllers/auth.controller.js | 7 +++-- modules/auth/tests/auth.integration.tests.js | 29 ++++++++++++-------- 2 files changed, 21 insertions(+), 15 deletions(-) diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index d69dfd410..b0d051814 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -637,9 +637,10 @@ const oauthErrorRedirect = (res, err, fallbackTitle) => { const oauthCallback = async (req, res, next) => { const strategy = req.params.strategy; // The identity is always derived server-side from the provider's response via - // passport. A client-asserted identity (request body) is never trusted, since - // nothing on this path verifies a provider token — accepting one would allow a - // caller who knows a victim's identifier to mint that victim's session. + // passport.authenticate(). The part that was previously unsafe was trusting a + // client-asserted identity from the request body (email, key, value) with no + // provider-token verification — a caller who knows a victim's identifier could + // have minted that victim's session. That branch is now removed entirely. passport.authenticate(strategy, (err, user) => { if (err) { logger.error( diff --git a/modules/auth/tests/auth.integration.tests.js b/modules/auth/tests/auth.integration.tests.js index bdb72eb3c..37ea59b51 100644 --- a/modules/auth/tests/auth.integration.tests.js +++ b/modules/auth/tests/auth.integration.tests.js @@ -577,26 +577,31 @@ describe('Auth integration tests:', () => { // (no server-side provider-token verification) must be rejected, not // turned into a victim session. The callback always delegates to passport. const victimEmail = 'oauthcb-victim@test.com'; - const victim = await UserService.create({ - firstName: 'Victim', - lastName: 'Owner', - email: victimEmail, - provider: 'google', - providerData: { email: victimEmail }, - roles: ['user'], - }); + let victim; try { + const existing = await UserService.getBrut({ email: victimEmail }); + if (existing) await UserService.remove(existing); + victim = await UserService.create({ + firstName: 'Victim', + lastName: 'Owner', + email: victimEmail, + provider: 'google', + providerData: { email: victimEmail }, + roles: ['user'], + }); const result = await agent .post('/api/auth/google/callback') .send({ strategy: false, key: 'email', value: victimEmail, email: victimEmail }); - // No session may be issued to the attacker. + // No session may be issued to the attacker — the real security invariant. + // The failure path may 302-redirect to an error route rather than returning + // a 4xx, so we assert the outcome (no TOKEN cookie, no successful session) + // rather than the transport status code. const tokenCookie = result.headers['set-cookie']?.find((c) => c.startsWith('TOKEN=')); expect(tokenCookie).toBeUndefined(); - // The unverified client-asserted path must not succeed. - expect(result.status).toBeGreaterThanOrEqual(400); + expect(result.status).not.toBe(200); } finally { - try { await UserService.remove(victim); } catch (_) { /* cleanup */ } + try { if (victim) await UserService.remove(victim); } catch (_) { /* cleanup */ } } });