From f78fecb7f3b4e267ca5d68de04bc8a8ff34ee614 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 4 Jul 2026 12:52:58 +0200 Subject: [PATCH 1/3] =?UTF-8?q?fix(auth):=20guard=20OAuth=20:strategy=20ro?= =?UTF-8?q?ute=20=E2=80=94=20404=20unknown/disabled=20+=20stateless=20auth?= =?UTF-8?q?enticate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit oauthCall and oauthCallback passed req.params.strategy straight into passport.authenticate() with no validation: an unknown strategy made passport throw synchronously ("Unknown authentication strategy") -> 500, and a registered-but-unguarded provider defaulted to session:true, which fails on this stateless JWT stack (no express-session mounted). Add isEnabledOAuthProvider(), shared by both routes: allowlisted (in ALLOWED_PROVIDERS) AND actually registered with passport (passport._strategy(strategy)) -> otherwise reject with a clean 404 (OAUTH_PROVIDER_NOT_FOUND) before ever reaching passport.authenticate(). oauthCall now also passes { session: false } explicitly. Update the invitations alias-shadow test to assert on the new deliberate 404 (code) instead of "not 404", and add strategy-registration stubs to the existing oauthCallback integration tests so they keep exercising the post-authenticate path. New unit tests cover unknown/disabled/enabled strategies for oauthCall. --- modules/auth/controllers/auth.controller.js | 47 ++++- modules/auth/tests/auth.integration.tests.js | 16 ++ .../auth.oauthCall.controller.unit.tests.js | 197 ++++++++++++++++++ .../tests/invitations.integration.tests.js | 14 +- 4 files changed, 261 insertions(+), 13 deletions(-) create mode 100644 modules/auth/tests/auth.oauthCall.controller.unit.tests.js diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 714477488..0cf4bf0d4 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -439,6 +439,31 @@ const token = async (req, res) => { .json({ user, tokenExpiresIn: Date.now() + config.jwt.expiresIn * 1000, abilities, pendingRequests }); }; +/** + * Known OAuth providers — used to validate the `provider` argument and `key` argument + * before constructing dynamic query paths, preventing prototype-pollution-style injections. + */ +const ALLOWED_PROVIDERS = new Set(['google', 'apple']); +const ALLOWED_PROVIDER_KEYS = new Set(['sub', 'id', 'email']); + +/** + * @desc Check whether `:strategy` (from `req.params.strategy`) is both an allowlisted + * provider AND actually registered with passport at boot time. A provider in + * ALLOWED_PROVIDERS is only registered (modules/auth/strategies/local/{provider}.js) + * when its config carries valid credentials — otherwise passport never sees it. + * Guards the `/api/auth/:strategy` and `/api/auth/:strategy/callback` routes so an + * unknown or disabled strategy name is rejected with a clean 404 before it can reach + * passport.authenticate(), which otherwise throws synchronously ("Unknown + * authentication strategy") for an unregistered name. + * `passport._strategy` is documented `@api private` in passport's own source, but it + * is the single source of truth for "is this strategy registered" — asking config + * directly would mean duplicating each provider's own credential-presence check here + * and risking drift from modules/auth/strategies/local/*.js over time. + * @param {string} strategy - strategy name from req.params.strategy + * @returns {boolean} true when strategy is both allowlisted and registered with passport + */ +const isEnabledOAuthProvider = (strategy) => ALLOWED_PROVIDERS.has(strategy) && Boolean(passport._strategy(strategy)); + /** * @desc Endpoint for oautCall * @param {Object} req - Express request object @@ -447,16 +472,13 @@ const token = async (req, res) => { */ const oauthCall = (req, res, next) => { const strategy = req.params.strategy; - passport.authenticate(strategy)(req, res, next); + if (!isEnabledOAuthProvider(strategy)) { + return next(new AppError('OAuth provider not supported', { status: 404, code: 'OAUTH_PROVIDER_NOT_FOUND' })); + } + // session: false — stateless JWT stack, no express-session middleware is mounted. + return passport.authenticate(strategy, { session: false })(req, res, next); }; -/** - * Known OAuth providers — used to validate the `provider` argument and `key` argument - * before constructing dynamic query paths, preventing prototype-pollution-style injections. - */ -const ALLOWED_PROVIDERS = new Set(['google', 'apple']); -const ALLOWED_PROVIDER_KEYS = new Set(['sub', 'id', 'email']); - /** * @desc Resolve or create a user from an OAuth profile. Lookup order: * 1. Primary identity (provider + providerData[key]) @@ -659,12 +681,19 @@ const oauthErrorRedirect = (res, err, fallbackTitle) => { */ const oauthCallback = async (req, res, next) => { const strategy = req.params.strategy; + if (!isEnabledOAuthProvider(strategy)) { + return next(new AppError('OAuth provider not supported', { status: 404, code: 'OAUTH_PROVIDER_NOT_FOUND' })); + } // 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) => { + // No `{ session: false }` here: passport.authenticate()'s callback form (a + // function as the 2nd/3rd arg) never calls req.logIn() itself — session + // establishment is entirely the caller's responsibility below (JWT + cookie, + // no express-session) — so the `session` option has nothing to act on. + return passport.authenticate(strategy, (err, user) => { if (err) { logger.error( { err: { message: err?.message, code: err?.code, stack: err?.stack }, strategy }, diff --git a/modules/auth/tests/auth.integration.tests.js b/modules/auth/tests/auth.integration.tests.js index d6e876577..e1fe9a472 100644 --- a/modules/auth/tests/auth.integration.tests.js +++ b/modules/auth/tests/auth.integration.tests.js @@ -656,6 +656,11 @@ describe('Auth integration tests:', () => { test('should set tokenCookieOptions and redirect on classic web oAuth success', async () => { const mockUserId = 'mock-oauth-user-id-123'; + // Simulate 'google' being registered with passport (isEnabledOAuthProvider's + // guard check) — this test targets oauthCallback's post-authenticate + // handling, not the provider-registration guard itself (covered separately + // for oauthCall). + const strategySpy = jest.spyOn(passport, '_strategy').mockReturnValue({}); const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce( (strategy, callback) => () => callback(null, { id: mockUserId }), ); @@ -675,9 +680,11 @@ describe('Auth integration tests:', () => { expect(redirectCalls[0]).toMatchObject({ code: 302 }); expect(redirectCalls[0].url).toMatch(/\/token$/); authenticateSpy.mockRestore(); + strategySpy.mockRestore(); }); test('should handle GET callback when req.body is undefined (Express 5)', async () => { + const strategySpy = jest.spyOn(passport, '_strategy').mockReturnValue({}); const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce( (strategy, callback) => () => callback(null, { id: 'mock-get-cb-user' }), ); @@ -694,11 +701,13 @@ describe('Auth integration tests:', () => { expect(cookies.TOKEN).toBeDefined(); expect(redirectCalls[0]).toMatchObject({ code: 302 }); authenticateSpy.mockRestore(); + strategySpy.mockRestore(); }); test('should log and redirect with canonical error envelope when classic web oAuth errors out', async () => { const oauthErr = new Error('token exchange failed'); oauthErr.code = 'OAUTH_TOKEN_EXCHANGE'; + const strategySpy = jest.spyOn(passport, '_strategy').mockReturnValue({}); const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce( (strategy, callback) => () => callback(oauthErr, null), ); @@ -743,6 +752,7 @@ describe('Auth integration tests:', () => { loggerSpy.mockRestore(); authenticateSpy.mockRestore(); + strategySpy.mockRestore(); }); test('should redirect with canonical envelope preserving AppError details.message', async () => { @@ -751,6 +761,7 @@ describe('Auth integration tests:', () => { code: 'VALIDATION_ERROR', details: { message: 'Registration is currently deactivated' }, }); + const strategySpy = jest.spyOn(passport, '_strategy').mockReturnValue({}); const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce( (strategy, callback) => () => callback(oauthErr, null), ); @@ -781,9 +792,11 @@ describe('Auth integration tests:', () => { loggerSpy.mockRestore(); authenticateSpy.mockRestore(); + strategySpy.mockRestore(); }); test('should fall back to OAUTH_ERROR code for plain Error without code', async () => { + const strategySpy = jest.spyOn(passport, '_strategy').mockReturnValue({}); const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce( (strategy, callback) => () => callback(new Error('boom'), null), ); @@ -805,9 +818,11 @@ describe('Auth integration tests:', () => { loggerSpy.mockRestore(); authenticateSpy.mockRestore(); + strategySpy.mockRestore(); }); test('should log and redirect with canonical envelope when no user is returned by passport', async () => { + const strategySpy = jest.spyOn(passport, '_strategy').mockReturnValue({}); const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce( (strategy, callback) => () => callback(null, null), ); @@ -851,6 +866,7 @@ describe('Auth integration tests:', () => { loggerSpy.mockRestore(); authenticateSpy.mockRestore(); + strategySpy.mockRestore(); }); test('should find an existing OAuth user via checkOAuthUserProfile', async () => { diff --git a/modules/auth/tests/auth.oauthCall.controller.unit.tests.js b/modules/auth/tests/auth.oauthCall.controller.unit.tests.js new file mode 100644 index 000000000..d287dc0f3 --- /dev/null +++ b/modules/auth/tests/auth.oauthCall.controller.unit.tests.js @@ -0,0 +1,197 @@ +/** + * Module dependencies. + */ +import { jest, describe, test, expect, beforeEach } from '@jest/globals'; + +/** + * Unit tests for auth.controller oauthCall() (issue #3900). + * + * Before this guard, `oauthCall` passed `req.params.strategy` straight into + * `passport.authenticate()` with no validation and no `{ session: false }`: + * - an unknown strategy name made passport throw synchronously ("Unknown + * authentication strategy") -> 500. + * - an allowlisted-but-unregistered provider hit the same throw. + * - a registered provider defaulted to `session: true`, which fails on this + * stateless JWT stack ("Login sessions require session support"). + * + * These tests verify the new isEnabledOAuthProvider() guard turns the first + * two cases into a clean 404 and never reaches passport.authenticate(), and + * that the third case delegates with `{ session: false }`. + */ +describe('auth.controller oauthCall:', () => { + let mockPassport; + + beforeEach(() => { + jest.resetModules(); + + mockPassport = { + authenticate: jest.fn().mockReturnValue(jest.fn()), + _strategy: jest.fn().mockReturnValue(undefined), + }; + jest.unstable_mockModule('passport', () => ({ + default: mockPassport, + })); + + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, + })); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + sign: { up: true, in: true }, + jwt: { secret: 's', expiresIn: 3600 }, + cookie: { secure: true, sameSite: 'lax' }, + organizations: { enabled: false }, + app: { title: 'Test', contact: 'a@b.com' }, + }, + })); + jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({ + default: { create: jest.fn(), getBrut: jest.fn(), update: jest.fn(), remove: jest.fn(), search: jest.fn(), count: jest.fn().mockResolvedValue(0) }, + })); + // auth.controller imports the generic eligibility registry (not invitation code). + jest.unstable_mockModule('../../../modules/auth/services/auth.eligibility.js', () => ({ + default: { + registerSignupEligibility: jest.fn(), + assertSignupEligible: jest.fn().mockResolvedValue(undefined), + _reset: jest.fn(), + }, + })); + jest.unstable_mockModule('../../../modules/users/repositories/users.repository.js', () => ({ + default: { update: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({ + default: { handleSignupOrganization: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.crud.service.js', () => ({ + default: { autoSetCurrentOrganization: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.membership.service.js', () => ({ + default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) }, + })); + jest.unstable_mockModule('../../../modules/users/models/users.schema.js', () => ({ + default: { User: {} }, + })); + jest.unstable_mockModule('../../../lib/middlewares/model.js', () => ({ + default: { getResultFromZod: jest.fn(), checkError: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ + default: { defineAbilityFor: jest.fn().mockResolvedValue({}) }, + })); + jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { isConfigured: jest.fn().mockReturnValue(false), sendMail: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ + default: { + success: jest.fn().mockReturnValue(jest.fn()), + error: jest.fn().mockReturnValue(jest.fn()), + }, + })); + jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({ + default: { getMessage: jest.fn().mockReturnValue('error') }, + })); + jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({ + default: class AppError extends Error { + constructor(msg, opts) { + super(msg); + this.status = opts?.status; + this.code = opts?.code; + this.details = opts?.details; + } + }, + })); + jest.unstable_mockModule('../../../lib/helpers/abilities.js', () => ({ + default: jest.fn().mockReturnValue([]), + })); + jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), + })); + jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ + default: { identify: jest.fn(), groupIdentify: jest.fn() }, + })); + }); + + test('unknown strategy (not in ALLOWED_PROVIDERS) is rejected with a 404, passport.authenticate is never called', async () => { + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + + const req = { params: { strategy: 'me' } }; + const res = {}; + const next = jest.fn(); + + AuthController.oauthCall(req, res, next); + + expect(mockPassport.authenticate).not.toHaveBeenCalled(); + expect(next).toHaveBeenCalledTimes(1); + const err = next.mock.calls[0][0]; + expect(err).toBeInstanceOf(Error); + expect(err.status).toBe(404); + expect(err.code).toBe('OAUTH_PROVIDER_NOT_FOUND'); + }); + + test('another unknown strategy segment (e.g. "callback") is rejected with a 404', async () => { + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + + const req = { params: { strategy: 'callback' } }; + const res = {}; + const next = jest.fn(); + + AuthController.oauthCall(req, res, next); + + expect(mockPassport.authenticate).not.toHaveBeenCalled(); + const err = next.mock.calls[0][0]; + expect(err.status).toBe(404); + expect(err.code).toBe('OAUTH_PROVIDER_NOT_FOUND'); + }); + + test('allowlisted but unregistered provider (not enabled in config) is rejected with a 404', async () => { + // passport._strategy() returns undefined by default in this suite's mock — + // simulating a provider that is in ALLOWED_PROVIDERS but was never + // passport.use()'d at boot (e.g. missing clientID/clientSecret). + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + + const req = { params: { strategy: 'google' } }; + const res = {}; + const next = jest.fn(); + + AuthController.oauthCall(req, res, next); + + expect(mockPassport._strategy).toHaveBeenCalledWith('google'); + expect(mockPassport.authenticate).not.toHaveBeenCalled(); + const err = next.mock.calls[0][0]; + expect(err.status).toBe(404); + expect(err.code).toBe('OAUTH_PROVIDER_NOT_FOUND'); + }); + + test('enabled + allowlisted provider delegates to passport.authenticate with { session: false }', async () => { + mockPassport._strategy.mockReturnValue({ name: 'google' }); // simulate registered strategy + const authenticateMiddleware = jest.fn(); + mockPassport.authenticate.mockReturnValue(authenticateMiddleware); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + + const req = { params: { strategy: 'google' } }; + const res = {}; + const next = jest.fn(); + + AuthController.oauthCall(req, res, next); + + expect(mockPassport.authenticate).toHaveBeenCalledWith('google', { session: false }); + expect(authenticateMiddleware).toHaveBeenCalledWith(req, res, next); + expect(next).not.toHaveBeenCalled(); + }); + + test('apple (the other allowlisted provider) also delegates when registered', async () => { + mockPassport._strategy.mockReturnValue({ name: 'apple' }); + const authenticateMiddleware = jest.fn(); + mockPassport.authenticate.mockReturnValue(authenticateMiddleware); + + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + + const req = { params: { strategy: 'apple' } }; + const res = {}; + const next = jest.fn(); + + AuthController.oauthCall(req, res, next); + + expect(mockPassport.authenticate).toHaveBeenCalledWith('apple', { session: false }); + expect(authenticateMiddleware).toHaveBeenCalledWith(req, res, next); + }); +}); diff --git a/modules/invitations/tests/invitations.integration.tests.js b/modules/invitations/tests/invitations.integration.tests.js index 846682b73..6f66e59e8 100644 --- a/modules/invitations/tests/invitations.integration.tests.js +++ b/modules/invitations/tests/invitations.integration.tests.js @@ -177,11 +177,17 @@ describe('Signup invitations:', () => { }); test('the alias did NOT shadow the greedy /api/auth/:strategy wildcard (OAuth routing intact)', async () => { - // /api/auth/google must still hit the oauth strategy handler, not 404 as an - // unknown invitations sub-path. Passport returns a 3xx redirect (or a strategy - // 4xx/5xx) — the contract asserted here is simply "not a 404 Not Found". + // /api/auth/google must still hit the OAuth :strategy route handler, not + // fall through as an unmatched invitations sub-path. 'google' is + // allowlisted but unregistered in test config (no clientID) so the + // handler's own guard (#3900) now returns a deliberate 404 with + // `code: OAUTH_PROVIDER_NOT_FOUND` — asserting on that code (rather than + // just "not a 404") is what proves the wildcard route was actually + // matched and its handler ran, as opposed to Express's own generic + // route-miss 404 the alias mount could otherwise cause. const res = await request(app).get('/api/auth/google'); - expect(res.status).not.toBe(404); + expect(res.status).toBe(404); + expect(res.body.code).toBe('OAUTH_PROVIDER_NOT_FOUND'); }); }); From 9e2a11d88401057c545ba1996786d2e114d1bea8 Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 4 Jul 2026 13:21:46 +0200 Subject: [PATCH 2/3] test(auth): cover oauthCallback 404 guard + restore session-mint regression test Adversarial review of the #3900 guard diff found the oauthCallback reject branch had zero coverage (all 6 existing tests stub _strategy truthy) and that the pre-existing "should NOT mint a session" security regression test was silently defeated: since google is unregistered in test config, the new guard now 404s before the request ever reaches the post-authenticate identity-trust logic the test protects. - Add oauthCallback guard-reject unit tests (unknown + allowlisted-but- unregistered strategy), mirroring the existing oauthCall coverage. - Stub passport._strategy + passport.authenticate in the session-mint regression test so it again reaches the post-auth "no user" branch instead of short-circuiting at the guard; assert the redirect carries the no-user error, never OAUTH_PROVIDER_NOT_FOUND. - Reuse the existing 'oAuth, unsupported provider' AppError message for both new guards instead of a fresh string, for consistency. - Add a describe-scoped afterEach(jest.restoreAllMocks()) safety net so a failing assertion mid-test can't leak a stubbed spy into later tests (clearMocks alone doesn't restore spy implementations). --- modules/auth/controllers/auth.controller.js | 4 +- modules/auth/tests/auth.integration.tests.js | 29 ++++ .../auth.oauthCall.controller.unit.tests.js | 136 ++++++++++++++++++ 3 files changed, 167 insertions(+), 2 deletions(-) diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 0cf4bf0d4..4e2e02c0f 100644 --- a/modules/auth/controllers/auth.controller.js +++ b/modules/auth/controllers/auth.controller.js @@ -473,7 +473,7 @@ const isEnabledOAuthProvider = (strategy) => ALLOWED_PROVIDERS.has(strategy) && const oauthCall = (req, res, next) => { const strategy = req.params.strategy; if (!isEnabledOAuthProvider(strategy)) { - return next(new AppError('OAuth provider not supported', { status: 404, code: 'OAUTH_PROVIDER_NOT_FOUND' })); + return next(new AppError('oAuth, unsupported provider', { status: 404, code: 'OAUTH_PROVIDER_NOT_FOUND' })); } // session: false — stateless JWT stack, no express-session middleware is mounted. return passport.authenticate(strategy, { session: false })(req, res, next); @@ -682,7 +682,7 @@ const oauthErrorRedirect = (res, err, fallbackTitle) => { const oauthCallback = async (req, res, next) => { const strategy = req.params.strategy; if (!isEnabledOAuthProvider(strategy)) { - return next(new AppError('OAuth provider not supported', { status: 404, code: 'OAUTH_PROVIDER_NOT_FOUND' })); + return next(new AppError('oAuth, unsupported provider', { status: 404, code: 'OAUTH_PROVIDER_NOT_FOUND' })); } // The identity is always derived server-side from the provider's response via // passport.authenticate(). The part that was previously unsafe was trusting a diff --git a/modules/auth/tests/auth.integration.tests.js b/modules/auth/tests/auth.integration.tests.js index e1fe9a472..2c29fbcd4 100644 --- a/modules/auth/tests/auth.integration.tests.js +++ b/modules/auth/tests/auth.integration.tests.js @@ -558,6 +558,15 @@ describe('Auth integration tests:', () => { } }); + // Safety net alongside each test's own manual mockRestore(): jest.config.js + // only sets `clearMocks` (resets calls, not implementations), so a spy left + // dangling by a failing assertion earlier in a test would otherwise leak its + // stubbed truthy passport._strategy()/mockRejectedValueOnce() into later + // tests in this block. + afterEach(() => { + jest.restoreAllMocks(); + }); + test('should create user with default provider when none is specified', async () => { const result = await UserService.create({ firstName: 'No', @@ -625,6 +634,18 @@ describe('Auth integration tests:', () => { // 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. + // + // Simulate 'google' being registered with passport (bypasses the + // isEnabledOAuthProvider guard, covered separately for oauthCall/oauthCallback) + // and stub passport.authenticate the same way a real OAuth2 strategy would + // behave for this request: there is no valid provider code, so authentication + // fails regardless of what the attacker put in the body. Without this stub the + // request would 404 at the guard before ever reaching the identity-trust logic + // this test protects. + const strategySpy = jest.spyOn(passport, '_strategy').mockReturnValue({}); + const authenticateSpy = jest.spyOn(passport, 'authenticate').mockImplementationOnce( + (strategy, callback) => () => callback(null, false), + ); const victimEmail = 'oauthcb-victim@test.com'; let victim; try { @@ -649,7 +670,15 @@ describe('Auth integration tests:', () => { const tokenCookie = result.headers['set-cookie']?.find((c) => c.startsWith('TOKEN=')); expect(tokenCookie).toBeUndefined(); expect(result.status).not.toBe(200); + // Confirm the request reached oauthCallback's post-authenticate + // identity-trust logic (not the 404 provider-registration guard) — + // the redirect must carry the "no user" error, never the guard's code. + expect(result.status).toBe(302); + expect(result.headers.location).toContain('/token'); + expect(result.headers.location).not.toContain('OAUTH_PROVIDER_NOT_FOUND'); } finally { + authenticateSpy.mockRestore(); + strategySpy.mockRestore(); try { if (victim) await UserService.remove(victim); } catch (_) { /* cleanup */ } } }); diff --git a/modules/auth/tests/auth.oauthCall.controller.unit.tests.js b/modules/auth/tests/auth.oauthCall.controller.unit.tests.js index d287dc0f3..90f7d7d10 100644 --- a/modules/auth/tests/auth.oauthCall.controller.unit.tests.js +++ b/modules/auth/tests/auth.oauthCall.controller.unit.tests.js @@ -195,3 +195,139 @@ describe('auth.controller oauthCall:', () => { expect(authenticateMiddleware).toHaveBeenCalledWith(req, res, next); }); }); + +/** + * Unit tests for auth.controller oauthCallback()'s isEnabledOAuthProvider guard + * (issue #3900). The integration suite (auth.integration.tests.js) covers the + * post-authenticate handling with `_strategy`/`authenticate` both stubbed truthy; + * these tests cover the guard's reject branch specifically — an unknown or + * allowlisted-but-unregistered strategy must 404 before passport.authenticate() + * is ever reached, mirroring the oauthCall reject-path tests above. + */ +describe('auth.controller oauthCallback:', () => { + let mockPassport; + + beforeEach(() => { + jest.resetModules(); + + mockPassport = { + authenticate: jest.fn().mockReturnValue(jest.fn()), + _strategy: jest.fn().mockReturnValue(undefined), + }; + jest.unstable_mockModule('passport', () => ({ + default: mockPassport, + })); + + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, + })); + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + sign: { up: true, in: true }, + jwt: { secret: 's', expiresIn: 3600 }, + cookie: { secure: true, sameSite: 'lax' }, + organizations: { enabled: false }, + app: { title: 'Test', contact: 'a@b.com' }, + }, + })); + jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({ + default: { create: jest.fn(), getBrut: jest.fn(), update: jest.fn(), remove: jest.fn(), search: jest.fn(), count: jest.fn().mockResolvedValue(0) }, + })); + // auth.controller imports the generic eligibility registry (not invitation code). + jest.unstable_mockModule('../../../modules/auth/services/auth.eligibility.js', () => ({ + default: { + registerSignupEligibility: jest.fn(), + assertSignupEligible: jest.fn().mockResolvedValue(undefined), + _reset: jest.fn(), + }, + })); + jest.unstable_mockModule('../../../modules/users/repositories/users.repository.js', () => ({ + default: { update: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({ + default: { handleSignupOrganization: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.crud.service.js', () => ({ + default: { autoSetCurrentOrganization: jest.fn() }, + })); + jest.unstable_mockModule('../../../modules/organizations/services/organizations.membership.service.js', () => ({ + default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) }, + })); + jest.unstable_mockModule('../../../modules/users/models/users.schema.js', () => ({ + default: { User: {} }, + })); + jest.unstable_mockModule('../../../lib/middlewares/model.js', () => ({ + default: { getResultFromZod: jest.fn(), checkError: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ + default: { defineAbilityFor: jest.fn().mockResolvedValue({}) }, + })); + jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { isConfigured: jest.fn().mockReturnValue(false), sendMail: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ + default: { + success: jest.fn().mockReturnValue(jest.fn()), + error: jest.fn().mockReturnValue(jest.fn()), + }, + })); + jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({ + default: { getMessage: jest.fn().mockReturnValue('error') }, + })); + jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({ + default: class AppError extends Error { + constructor(msg, opts) { + super(msg); + this.status = opts?.status; + this.code = opts?.code; + this.details = opts?.details; + } + }, + })); + jest.unstable_mockModule('../../../lib/helpers/abilities.js', () => ({ + default: jest.fn().mockReturnValue([]), + })); + jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), + })); + jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ + default: { identify: jest.fn(), groupIdentify: jest.fn() }, + })); + }); + + test('unknown strategy (not in ALLOWED_PROVIDERS) is rejected with a 404, passport.authenticate is never called', async () => { + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + + const req = { params: { strategy: 'me' }, body: {} }; + const res = {}; + const next = jest.fn(); + + await AuthController.oauthCallback(req, res, next); + + expect(mockPassport.authenticate).not.toHaveBeenCalled(); + expect(next).toHaveBeenCalledTimes(1); + const err = next.mock.calls[0][0]; + expect(err).toBeInstanceOf(Error); + expect(err.status).toBe(404); + expect(err.code).toBe('OAUTH_PROVIDER_NOT_FOUND'); + }); + + test('allowlisted but unregistered provider (not enabled in config) is rejected with a 404', async () => { + // passport._strategy() returns undefined by default in this suite's mock — + // simulating a provider that is in ALLOWED_PROVIDERS but was never + // passport.use()'d at boot (e.g. missing clientID/clientSecret). + const { default: AuthController } = await import('../../../modules/auth/controllers/auth.controller.js'); + + const req = { params: { strategy: 'google' }, body: {} }; + const res = {}; + const next = jest.fn(); + + await AuthController.oauthCallback(req, res, next); + + expect(mockPassport._strategy).toHaveBeenCalledWith('google'); + expect(mockPassport.authenticate).not.toHaveBeenCalled(); + const err = next.mock.calls[0][0]; + expect(err.status).toBe(404); + expect(err.code).toBe('OAUTH_PROVIDER_NOT_FOUND'); + }); +}); From cdba63407c4fcca5956750a4e0405f20ffb8d4bd Mon Sep 17 00:00:00 2001 From: Pierre Brisorgueil Date: Sat, 4 Jul 2026 13:46:52 +0200 Subject: [PATCH 3/3] test(auth): extract shared oauth controller mock setup into a fixture (CodeRabbit #3947) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Deduplicates ~85 lines of identical jest.unstable_mockModule setup that was copy-pasted across the oauthCall and oauthCallback describe blocks' beforeEach hooks into a single setupAuthControllerMocks() helper, called from both. Placed under tests/fixtures/ (matching lib/middlewares/tests/fixtures/) so Jest's testPathIgnorePatterns keeps it out of testMatch — a tests/helpers/ name would have made Jest try to run it as its own (empty) test suite. --- .../auth.oauthCall.controller.unit.tests.js | 173 +----------------- .../fixtures/auth-controller.mock-setup.js | 106 +++++++++++ 2 files changed, 109 insertions(+), 170 deletions(-) create mode 100644 modules/auth/tests/fixtures/auth-controller.mock-setup.js diff --git a/modules/auth/tests/auth.oauthCall.controller.unit.tests.js b/modules/auth/tests/auth.oauthCall.controller.unit.tests.js index 90f7d7d10..a266be19f 100644 --- a/modules/auth/tests/auth.oauthCall.controller.unit.tests.js +++ b/modules/auth/tests/auth.oauthCall.controller.unit.tests.js @@ -2,6 +2,7 @@ * Module dependencies. */ import { jest, describe, test, expect, beforeEach } from '@jest/globals'; +import { setupAuthControllerMocks } from './fixtures/auth-controller.mock-setup.js'; /** * Unit tests for auth.controller oauthCall() (issue #3900). @@ -22,91 +23,7 @@ describe('auth.controller oauthCall:', () => { let mockPassport; beforeEach(() => { - jest.resetModules(); - - mockPassport = { - authenticate: jest.fn().mockReturnValue(jest.fn()), - _strategy: jest.fn().mockReturnValue(undefined), - }; - jest.unstable_mockModule('passport', () => ({ - default: mockPassport, - })); - - jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ - default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, - })); - jest.unstable_mockModule('../../../config/index.js', () => ({ - default: { - sign: { up: true, in: true }, - jwt: { secret: 's', expiresIn: 3600 }, - cookie: { secure: true, sameSite: 'lax' }, - organizations: { enabled: false }, - app: { title: 'Test', contact: 'a@b.com' }, - }, - })); - jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({ - default: { create: jest.fn(), getBrut: jest.fn(), update: jest.fn(), remove: jest.fn(), search: jest.fn(), count: jest.fn().mockResolvedValue(0) }, - })); - // auth.controller imports the generic eligibility registry (not invitation code). - jest.unstable_mockModule('../../../modules/auth/services/auth.eligibility.js', () => ({ - default: { - registerSignupEligibility: jest.fn(), - assertSignupEligible: jest.fn().mockResolvedValue(undefined), - _reset: jest.fn(), - }, - })); - jest.unstable_mockModule('../../../modules/users/repositories/users.repository.js', () => ({ - default: { update: jest.fn() }, - })); - jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({ - default: { handleSignupOrganization: jest.fn() }, - })); - jest.unstable_mockModule('../../../modules/organizations/services/organizations.crud.service.js', () => ({ - default: { autoSetCurrentOrganization: jest.fn() }, - })); - jest.unstable_mockModule('../../../modules/organizations/services/organizations.membership.service.js', () => ({ - default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) }, - })); - jest.unstable_mockModule('../../../modules/users/models/users.schema.js', () => ({ - default: { User: {} }, - })); - jest.unstable_mockModule('../../../lib/middlewares/model.js', () => ({ - default: { getResultFromZod: jest.fn(), checkError: jest.fn() }, - })); - jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ - default: { defineAbilityFor: jest.fn().mockResolvedValue({}) }, - })); - jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ - default: { isConfigured: jest.fn().mockReturnValue(false), sendMail: jest.fn() }, - })); - jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ - default: { - success: jest.fn().mockReturnValue(jest.fn()), - error: jest.fn().mockReturnValue(jest.fn()), - }, - })); - jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({ - default: { getMessage: jest.fn().mockReturnValue('error') }, - })); - jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({ - default: class AppError extends Error { - constructor(msg, opts) { - super(msg); - this.status = opts?.status; - this.code = opts?.code; - this.details = opts?.details; - } - }, - })); - jest.unstable_mockModule('../../../lib/helpers/abilities.js', () => ({ - default: jest.fn().mockReturnValue([]), - })); - jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ - default: jest.fn().mockReturnValue('http://localhost:3000'), - })); - jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ - default: { identify: jest.fn(), groupIdentify: jest.fn() }, - })); + mockPassport = setupAuthControllerMocks(); }); test('unknown strategy (not in ALLOWED_PROVIDERS) is rejected with a 404, passport.authenticate is never called', async () => { @@ -208,91 +125,7 @@ describe('auth.controller oauthCallback:', () => { let mockPassport; beforeEach(() => { - jest.resetModules(); - - mockPassport = { - authenticate: jest.fn().mockReturnValue(jest.fn()), - _strategy: jest.fn().mockReturnValue(undefined), - }; - jest.unstable_mockModule('passport', () => ({ - default: mockPassport, - })); - - jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ - default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, - })); - jest.unstable_mockModule('../../../config/index.js', () => ({ - default: { - sign: { up: true, in: true }, - jwt: { secret: 's', expiresIn: 3600 }, - cookie: { secure: true, sameSite: 'lax' }, - organizations: { enabled: false }, - app: { title: 'Test', contact: 'a@b.com' }, - }, - })); - jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({ - default: { create: jest.fn(), getBrut: jest.fn(), update: jest.fn(), remove: jest.fn(), search: jest.fn(), count: jest.fn().mockResolvedValue(0) }, - })); - // auth.controller imports the generic eligibility registry (not invitation code). - jest.unstable_mockModule('../../../modules/auth/services/auth.eligibility.js', () => ({ - default: { - registerSignupEligibility: jest.fn(), - assertSignupEligible: jest.fn().mockResolvedValue(undefined), - _reset: jest.fn(), - }, - })); - jest.unstable_mockModule('../../../modules/users/repositories/users.repository.js', () => ({ - default: { update: jest.fn() }, - })); - jest.unstable_mockModule('../../../modules/organizations/services/organizations.service.js', () => ({ - default: { handleSignupOrganization: jest.fn() }, - })); - jest.unstable_mockModule('../../../modules/organizations/services/organizations.crud.service.js', () => ({ - default: { autoSetCurrentOrganization: jest.fn() }, - })); - jest.unstable_mockModule('../../../modules/organizations/services/organizations.membership.service.js', () => ({ - default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) }, - })); - jest.unstable_mockModule('../../../modules/users/models/users.schema.js', () => ({ - default: { User: {} }, - })); - jest.unstable_mockModule('../../../lib/middlewares/model.js', () => ({ - default: { getResultFromZod: jest.fn(), checkError: jest.fn() }, - })); - jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ - default: { defineAbilityFor: jest.fn().mockResolvedValue({}) }, - })); - jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ - default: { isConfigured: jest.fn().mockReturnValue(false), sendMail: jest.fn() }, - })); - jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ - default: { - success: jest.fn().mockReturnValue(jest.fn()), - error: jest.fn().mockReturnValue(jest.fn()), - }, - })); - jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({ - default: { getMessage: jest.fn().mockReturnValue('error') }, - })); - jest.unstable_mockModule('../../../lib/helpers/AppError.js', () => ({ - default: class AppError extends Error { - constructor(msg, opts) { - super(msg); - this.status = opts?.status; - this.code = opts?.code; - this.details = opts?.details; - } - }, - })); - jest.unstable_mockModule('../../../lib/helpers/abilities.js', () => ({ - default: jest.fn().mockReturnValue([]), - })); - jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ - default: jest.fn().mockReturnValue('http://localhost:3000'), - })); - jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ - default: { identify: jest.fn(), groupIdentify: jest.fn() }, - })); + mockPassport = setupAuthControllerMocks(); }); test('unknown strategy (not in ALLOWED_PROVIDERS) is rejected with a 404, passport.authenticate is never called', async () => { diff --git a/modules/auth/tests/fixtures/auth-controller.mock-setup.js b/modules/auth/tests/fixtures/auth-controller.mock-setup.js new file mode 100644 index 000000000..7c5938614 --- /dev/null +++ b/modules/auth/tests/fixtures/auth-controller.mock-setup.js @@ -0,0 +1,106 @@ +// Fixture: shared jest.unstable_mockModule() registration for auth.controller +// oauthCall()/oauthCallback() unit tests (issue #3900, CodeRabbit nit on #3947). +// +// Both `oauthCall` and `oauthCallback` describe blocks in +// auth.oauthCall.controller.unit.tests.js need the identical set of mocks +// registered — and registered BEFORE each test's dynamic +// `await import('.../auth.controller.js')` — so this helper must be invoked +// from each block's own `beforeEach`, never from a one-time top-level setup. +import { jest } from '@jest/globals'; + +/** + * Reset the module registry and register every mock auth.controller's + * dependency graph needs, mirroring what each test's dynamic import expects. + * Call this from `beforeEach` (not `beforeAll`) so the mocks are registered + * before that test's `await import('.../auth.controller.js')`. + * @returns {{authenticate: import('@jest/globals').Mock, _strategy: import('@jest/globals').Mock}} mockPassport - the passport mock, so callers can further customize per-test behavior (e.g. `mockPassport._strategy.mockReturnValue(...)`) + */ +export function setupAuthControllerMocks() { + jest.resetModules(); + + const mockPassport = { + authenticate: jest.fn().mockReturnValue(jest.fn()), + _strategy: jest.fn().mockReturnValue(undefined), + }; + jest.unstable_mockModule('passport', () => ({ + default: mockPassport, + })); + + jest.unstable_mockModule('../../../../lib/services/logger.js', () => ({ + default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, + })); + jest.unstable_mockModule('../../../../config/index.js', () => ({ + default: { + sign: { up: true, in: true }, + jwt: { secret: 's', expiresIn: 3600 }, + cookie: { secure: true, sameSite: 'lax' }, + organizations: { enabled: false }, + app: { title: 'Test', contact: 'a@b.com' }, + }, + })); + jest.unstable_mockModule('../../../../modules/users/services/users.service.js', () => ({ + default: { create: jest.fn(), getBrut: jest.fn(), update: jest.fn(), remove: jest.fn(), search: jest.fn(), count: jest.fn().mockResolvedValue(0) }, + })); + // auth.controller imports the generic eligibility registry (not invitation code). + jest.unstable_mockModule('../../../../modules/auth/services/auth.eligibility.js', () => ({ + default: { + registerSignupEligibility: jest.fn(), + assertSignupEligible: jest.fn().mockResolvedValue(undefined), + _reset: jest.fn(), + }, + })); + jest.unstable_mockModule('../../../../modules/users/repositories/users.repository.js', () => ({ + default: { update: jest.fn() }, + })); + jest.unstable_mockModule('../../../../modules/organizations/services/organizations.service.js', () => ({ + default: { handleSignupOrganization: jest.fn() }, + })); + jest.unstable_mockModule('../../../../modules/organizations/services/organizations.crud.service.js', () => ({ + default: { autoSetCurrentOrganization: jest.fn() }, + })); + jest.unstable_mockModule('../../../../modules/organizations/services/organizations.membership.service.js', () => ({ + default: { findByUserAndOrganization: jest.fn(), listPendingByUser: jest.fn().mockResolvedValue([]) }, + })); + jest.unstable_mockModule('../../../../modules/users/models/users.schema.js', () => ({ + default: { User: {} }, + })); + jest.unstable_mockModule('../../../../lib/middlewares/model.js', () => ({ + default: { getResultFromZod: jest.fn(), checkError: jest.fn() }, + })); + jest.unstable_mockModule('../../../../lib/middlewares/policy.js', () => ({ + default: { defineAbilityFor: jest.fn().mockResolvedValue({}) }, + })); + jest.unstable_mockModule('../../../../lib/helpers/mailer/index.js', () => ({ + default: { isConfigured: jest.fn().mockReturnValue(false), sendMail: jest.fn() }, + })); + jest.unstable_mockModule('../../../../lib/helpers/responses.js', () => ({ + default: { + success: jest.fn().mockReturnValue(jest.fn()), + error: jest.fn().mockReturnValue(jest.fn()), + }, + })); + jest.unstable_mockModule('../../../../lib/helpers/errors.js', () => ({ + default: { getMessage: jest.fn().mockReturnValue('error') }, + })); + jest.unstable_mockModule('../../../../lib/helpers/AppError.js', () => ({ + default: class AppError extends Error { + constructor(msg, opts) { + super(msg); + this.status = opts?.status; + this.code = opts?.code; + this.details = opts?.details; + } + }, + })); + jest.unstable_mockModule('../../../../lib/helpers/abilities.js', () => ({ + default: jest.fn().mockReturnValue([]), + })); + jest.unstable_mockModule('../../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), + })); + jest.unstable_mockModule('../../../../lib/services/analytics.js', () => ({ + default: { identify: jest.fn(), groupIdentify: jest.fn() }, + })); + + return mockPassport; +}