diff --git a/modules/auth/controllers/auth.controller.js b/modules/auth/controllers/auth.controller.js index 714477488..4e2e02c0f 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, 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); }; -/** - * 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, 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 // 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..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,13 +670,26 @@ 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 */ } } }); 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 +709,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 +730,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 +781,7 @@ describe('Auth integration tests:', () => { loggerSpy.mockRestore(); authenticateSpy.mockRestore(); + strategySpy.mockRestore(); }); test('should redirect with canonical envelope preserving AppError details.message', async () => { @@ -751,6 +790,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 +821,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 +847,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 +895,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..a266be19f --- /dev/null +++ b/modules/auth/tests/auth.oauthCall.controller.unit.tests.js @@ -0,0 +1,166 @@ +/** + * 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). + * + * 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(() => { + mockPassport = setupAuthControllerMocks(); + }); + + 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); + }); +}); + +/** + * 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(() => { + mockPassport = setupAuthControllerMocks(); + }); + + 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'); + }); +}); 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; +} 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'); }); });