fix(auth): remove unverified client-asserted OAuth identity branch + throttle callback (#3866)

* fix(auth): remove unverified client-asserted OAuth identity branch + throttle callback

* fix(auth): throttle the OAuth initiation route for consistency

* 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)

Author: PierreBrisorgueil

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(

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
 };

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(