diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 893429d68..b0d051814 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -636,43 +636,11 @@ 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.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/routes/auth.routes.js b/modules/auth/routes/auth.routes.js index ca5267748..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/callback').get(auth.oauthCallback); - app.route('/api/auth/:strategy/callback').post(auth.oauthCallback); // specific for apple call back + 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 }; diff --git a/modules/auth/tests/auth.integration.tests.js b/modules/auth/tests/auth.integration.tests.js index 37352c9ce..37ea59b51 100644 --- a/modules/auth/tests/auth.integration.tests.js +++ b/modules/auth/tests/auth.integration.tests.js @@ -571,47 +571,40 @@ 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'; - try { + 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'; + 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: '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 — 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).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(); + expect(result.status).not.toBe(200); } finally { - try { - const u = await UserService.getBrut({ email: oauthEmail }); - if (u) await UserService.remove(u); - } catch (_) { /* cleanup */ } + try { if (victim) 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(