From de3063cba6eba319de46b23dc08e5ef67627a171 Mon Sep 17 00:00:00 2001
From: Pierre Brisorgueil <pierrebrisorgueil@me.com>
Date: Fri, 24 Apr 2026 08:53:57 +0200
Subject: [PATCH] fix(auth): log OAuth callback errors and drop useless
 error={} redirect

JSON.stringify on a native Error serialises to `{}` because message/stack are
non-enumerable, so the OAuth error redirect carried a useless `error={}` query
param and the failure was only visible via morgan's 302 access log.

Emit a structured logger.error() on both error and no-user branches so Sentry /
PostHog actually see OAuth failures, and put a URL-safe message (or code, or a
sensible fallback) in the `error=` query so the Vue error screen shows
something meaningful. Redirect path + status unchanged.

Closes #3495
---
 modules/auth/controllers/auth.controller.js  | 12 +++-
 modules/auth/tests/auth.integration.tests.js | 73 ++++++++++++++++++++
 2 files changed, 83 insertions(+), 2 deletions(-)

diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js
index 2361f4989..cd22e8c40 100644
--- a/modules/auth/controllers/auth.controller.js
+++ b/modules/auth/controllers/auth.controller.js
@@ -441,11 +441,19 @@ const oauthCallback = async (req, res, next) => {
   passport.authenticate(strategy, (err, user) => {
     const url = getBaseUrl();
     if (err) {
-      const _err = JSON.stringify(err);
+      logger.error(
+        { err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy },
+        'OAuth callback failed',
+      );
+      const _err = encodeURIComponent(err?.message || err?.code || 'oauth_error');
       const path = 'token?message=Unprocessable%20Entity';
       res.redirect(302, `${url}/${path}&error=${_err}`);
     } else if (!user) {
-      const _err = JSON.stringify(err);
+      logger.error(
+        { err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy },
+        'OAuth callback failed',
+      );
+      const _err = encodeURIComponent(err?.message || err?.code || 'oauth_no_user');
       const path = 'token?message=Could%20not%20define%20user%20in%20oAuth';
       res.redirect(302, `${url}/${path}&error=${_err}`);
     } else {
diff --git a/modules/auth/tests/auth.integration.tests.js b/modules/auth/tests/auth.integration.tests.js
index 8344941e8..cb27c490f 100644
--- a/modules/auth/tests/auth.integration.tests.js
+++ b/modules/auth/tests/auth.integration.tests.js
@@ -10,6 +10,7 @@ import passport from 'passport';
 import { bootstrap } from '../../../lib/app.js';
 import mongooseService from '../../../lib/services/mongoose.js';
 import config from '../../../config/index.js';
+import logger from '../../../lib/services/logger.js';
 
 /**
  * Unit tests
@@ -645,6 +646,78 @@ describe('Auth integration tests:', () => {
       authenticateSpy.mockRestore();
     });
 
+    test('should log and redirect with message when classic web oAuth errors out', async () => {
+      const oauthErr = new Error('token exchange failed');
+      oauthErr.code = 'OAUTH_TOKEN_EXCHANGE';
+      const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce(
+        (strategy, callback) => () => callback(oauthErr, null),
+      );
+      const loggerSpy = jest.spyOn(logger, 'error').mockImplementation(() => {});
+      const redirectCalls = [];
+      const mockReq = { params: { strategy: 'google' }, body: {} };
+      const mockRes = {
+        cookie() { return this; },
+        redirect(code, url) { redirectCalls.push({ code, url }); },
+      };
+
+      await AuthController.oauthCallback(mockReq, mockRes, () => {});
+
+      expect(loggerSpy).toHaveBeenCalledWith(
+        expect.objectContaining({
+          err: expect.objectContaining({
+            message: 'token exchange failed',
+            code: 'OAUTH_TOKEN_EXCHANGE',
+            stack: expect.any(String),
+          }),
+          strategy: 'google',
+        }),
+        'OAuth callback failed',
+      );
+      expect(redirectCalls[0].code).toBe(302);
+      // Redirect must carry the actual error message, not an empty object
+      expect(redirectCalls[0].url).toContain('error=');
+      expect(redirectCalls[0].url).not.toContain('error={}');
+      expect(redirectCalls[0].url).toContain(encodeURIComponent('token exchange failed'));
+
+      loggerSpy.mockRestore();
+      authenticateSpy.mockRestore();
+    });
+
+    test('should log and redirect with sensible message when no user is returned by passport', async () => {
+      const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce(
+        (strategy, callback) => () => callback(null, null),
+      );
+      const loggerSpy = jest.spyOn(logger, 'error').mockImplementation(() => {});
+      const redirectCalls = [];
+      const mockReq = { params: { strategy: 'google' }, body: {} };
+      const mockRes = {
+        cookie() { return this; },
+        redirect(code, url) { redirectCalls.push({ code, url }); },
+      };
+
+      await AuthController.oauthCallback(mockReq, mockRes, () => {});
+
+      expect(loggerSpy).toHaveBeenCalledWith(
+        expect.objectContaining({
+          err: expect.objectContaining({
+            message: undefined,
+            code: undefined,
+            stack: undefined,
+          }),
+          strategy: 'google',
+        }),
+        'OAuth callback failed',
+      );
+      expect(redirectCalls[0].code).toBe(302);
+      expect(redirectCalls[0].url).toContain('error=');
+      expect(redirectCalls[0].url).not.toContain('error={}');
+      expect(redirectCalls[0].url).toContain('oauth_no_user');
+      expect(redirectCalls[0].url).toContain('Could%20not%20define%20user%20in%20oAuth');
+
+      loggerSpy.mockRestore();
+      authenticateSpy.mockRestore();
+    });
+
     test('should find an existing OAuth user via checkOAuthUserProfile', async () => {
       // Create an OAuth user directly first
       const createdUser = await UserService.create({