diff --git a/lib/services/express.js b/lib/services/express.js index eda3edab6..76e9a612c 100644 --- a/lib/services/express.js +++ b/lib/services/express.js @@ -321,11 +321,28 @@ const initModulesServerRoutes = async (app) => { /** * Configure error handling + * @param {import('express').Express} app - Express application instance + * @returns {void} */ const initErrorRoutes = (app) => { + /** + * Express 4-arity error handler. + * In production: returns a generic 500 to prevent leaking internal error details (#3726). + * In non-production: includes err.message and err.code for debugging. + * @param {Error} err - The error object caught by Express + * @param {import('express').Request} req - Express request object + * @param {import('express').Response} res - Express response object + * @param {import('express').NextFunction} next - Express next function + * @returns {void} + */ app.use((err, req, res, next) => { if (!err) return next(); logger.error('Unhandled express error', { error: err }); + // In production never leak internal error details (message, code) to clients. + // Detailed errors are still logged above for observability. + if (process.env.NODE_ENV === 'production') { + return res.status(err.status || 500).send({ message: 'Internal Server Error' }); + } res.status(err.status || 500).send({ message: err.message, code: err.code, diff --git a/lib/services/tests/express.errorroutes.unit.tests.js b/lib/services/tests/express.errorroutes.unit.tests.js new file mode 100644 index 000000000..584265191 --- /dev/null +++ b/lib/services/tests/express.errorroutes.unit.tests.js @@ -0,0 +1,167 @@ +/** + * Module dependencies. + * + * Unit tests for express.js initErrorRoutes. + * Security regression guard: err.message/code must NOT be sent to clients in production (#3726). + */ +import { jest, describe, test, expect, beforeEach, afterEach } from '@jest/globals'; + +describe('express initErrorRoutes — prod error leak guard (#3726):', () => { + let originalNodeEnv; + + beforeEach(() => { + originalNodeEnv = process.env.NODE_ENV; + jest.resetModules(); + }); + + afterEach(() => { + process.env.NODE_ENV = originalNodeEnv; + }); + + /** + * Helper: extract initErrorRoutes from express.js (with all heavy deps mocked). + * @returns {Promise} The initErrorRoutes function extracted from the module + */ + const getInitErrorRoutes = async () => { + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + domain: 'http://localhost:3000', + app: { title: 'Test', description: '', keywords: '', url: '', logo: '' }, + secure: { ssl: false }, + log: {}, + bodyParser: {}, + csrf: {}, + cors: { origin: [], credentials: false, optionsSuccessStatus: 200 }, + trust: { proxy: false }, + swagger: { enable: false }, + files: { routes: [], configs: [], policies: [], preRoutes: [], swagger: [], guides: [] }, + analytics: { posthog: { autoCapture: false } }, + docs: {}, + }, + })); + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { + warn: jest.fn(), + error: jest.fn(), + info: jest.fn(), + debug: jest.fn(), + getLogFormat: jest.fn().mockReturnValue('combined'), + getMorganOptions: jest.fn().mockReturnValue({}), + }, + })); + jest.unstable_mockModule('../../../lib/helpers/guides.js', () => ({ + default: { loadGuides: jest.fn().mockReturnValue([]), mergeGuidesIntoSpec: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/middlewares/requestId.js', () => ({ + default: jest.fn((req, res, next) => next()), + })); + jest.unstable_mockModule('../../../lib/middlewares/posthog-context.middleware.js', () => ({ + posthogContextMiddleware: jest.fn((req, res, next) => next()), + })); + jest.unstable_mockModule('../../../lib/services/errorTracker.js', () => ({ + default: { setupExpressErrorHandler: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({ + default: { init: jest.fn().mockResolvedValue(undefined), identify: jest.fn(), groupIdentify: jest.fn() }, + })); + jest.unstable_mockModule('../../../lib/middlewares/analytics.js', () => ({ + default: jest.fn((req, res, next) => next()), + })); + jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({ + default: { discoverPolicies: jest.fn().mockResolvedValue(undefined), defineAbilityFor: jest.fn().mockResolvedValue({}) }, + })); + + const mod = await import('../../../lib/services/express.js'); + return mod.default.initErrorRoutes; + }; + + test('in production: responds with generic { message: "Internal Server Error" } — no err.message/code leak', async () => { + process.env.NODE_ENV = 'production'; + const initErrorRoutes = await getInitErrorRoutes(); + + // Capture the error handler registered via app.use + let errorHandler; + const app = { + use: jest.fn((handler) => { errorHandler = handler; }), + }; + initErrorRoutes(app); + expect(typeof errorHandler).toBe('function'); + + const internalErr = new Error('E11000 duplicate key index: email_1'); + internalErr.code = 11000; + internalErr.status = 500; + + const sendPayloads = []; + const res = { + status: jest.fn().mockReturnThis(), + send: jest.fn((body) => { sendPayloads.push(body); return res; }), + }; + + await errorHandler(internalErr, {}, res, jest.fn()); + + expect(res.status).toHaveBeenCalledWith(500); + expect(sendPayloads).toHaveLength(1); + const body = sendPayloads[0]; + // Must NOT include internal error detail + expect(body.message).toBe('Internal Server Error'); + expect(body.code).toBeUndefined(); + // Must NOT contain the raw error message + expect(JSON.stringify(body)).not.toContain('E11000'); + expect(JSON.stringify(body)).not.toContain('email_1'); + }); + + test('in non-production (development): responds with detailed err.message + err.code', async () => { + process.env.NODE_ENV = 'development'; + const initErrorRoutes = await getInitErrorRoutes(); + + let errorHandler; + const app = { + use: jest.fn((handler) => { errorHandler = handler; }), + }; + initErrorRoutes(app); + + const devErr = new Error('Something went wrong internally'); + devErr.code = 'ERR_INTERNAL'; + devErr.status = 500; + + const sendPayloads = []; + const res = { + status: jest.fn().mockReturnThis(), + send: jest.fn((body) => { sendPayloads.push(body); return res; }), + }; + + await errorHandler(devErr, {}, res, jest.fn()); + + const body = sendPayloads[0]; + expect(body.message).toBe('Something went wrong internally'); + expect(body.code).toBe('ERR_INTERNAL'); + }); + + test('in test env: responds with detailed err.message + err.code (non-production path)', async () => { + process.env.NODE_ENV = 'test'; + const initErrorRoutes = await getInitErrorRoutes(); + + let errorHandler; + const app = { + use: jest.fn((handler) => { errorHandler = handler; }), + }; + initErrorRoutes(app); + + const testErr = new Error('test internal error'); + testErr.code = 'ERR_TEST'; + testErr.status = 422; + + const sendPayloads = []; + const res = { + status: jest.fn().mockReturnThis(), + send: jest.fn((body) => { sendPayloads.push(body); return res; }), + }; + + await errorHandler(testErr, {}, res, jest.fn()); + + const body = sendPayloads[0]; + expect(res.status).toHaveBeenCalledWith(422); + expect(body.message).toBe('test internal error'); + expect(body.code).toBe('ERR_TEST'); + }); +}); diff --git a/modules/auth/controllers/auth.password.controller.js b/modules/auth/controllers/auth.password.controller.js index fba2ccd47..2becdc718 100644 --- a/modules/auth/controllers/auth.password.controller.js +++ b/modules/auth/controllers/auth.password.controller.js @@ -25,16 +25,36 @@ const tokenCookieOptions = { * @returns {Promise} Sends a JSON response with reset status. */ const forgot = async (req, res) => { - let user; + /** + * @returns {void} Sends a uniform 200 response regardless of account existence or mail outcome. + */ + const uniformSuccess = () => responses.success(res, 'If that email exists, a reset link has been sent.')({ status: true }); + // check input if (!req.body.email) return responses.error(res, 422, 'Unprocessable Entity', 'Mail field must not be blank')(); const email = String(req.body.email); - // get user generate and add token + + let user; try { user = await UserService.getBrut({ email }); - if (!user) return responses.error(res, 400, 'Bad Request', 'No account with that email has been found')(); - if (user.provider !== 'local') - return responses.error(res, 400, 'Bad Request', `It seems like you signed up using your ${user.provider} account`)(); + } catch (err) { + return responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err); + } + + // Unknown email or OAuth user — perform a dummy bcrypt compare to equalise + // response timing (anti-timing-oracle). The sentinel is a valid precomputed + // bcrypt hash (cost=10, 60 chars) so bcrypt runs the full KDF instead of + // short-circuiting on format validation. + if (!user || user.provider !== 'local') { + await AuthService.comparePassword( + 'dummy-timing-equaliser', + '$2b$10$wotSb2xPb8VF8lpBkulk0.e2xvYvDedhjMyuG/7w3GdPl1vdvogY2', + ); + return uniformSuccess(); + } + + // Known local user — generate token, persist, and send the reset email. + try { const edit = { resetPasswordToken: jwt.sign({ exp: Date.now() + 3600000 }, config.jwt.secret, { algorithm: 'HS256' }), resetPasswordExpires: Date.now() + 3600000, @@ -43,7 +63,7 @@ const forgot = async (req, res) => { } catch (err) { return responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err); } - // send mail + try { const mail = await mails.sendMail({ template: 'reset-password-email', @@ -56,11 +76,13 @@ const forgot = async (req, res) => { appContact: config.app.contact, }, }); - if (!mail || !mail.accepted) return responses.error(res, 400, 'Bad Request', 'Failure sending email')(); - return responses.success(res, 'An email has been sent with further instructions')({ status: true }); - } catch (_mailErr) { - return responses.error(res, 400, 'Bad Request', 'Failure sending email')(); + if (!mail || !mail.accepted) { + logger.error('auth.password.forgot: mail not accepted', { email: user.email }); + } + } catch (mailErr) { + logger.error('auth.password.forgot: sendMail threw', { message: mailErr?.message, stack: mailErr?.stack }); } + return uniformSuccess(); }; /** diff --git a/modules/auth/tests/auth.integration.tests.js b/modules/auth/tests/auth.integration.tests.js index 5b4f63fad..fe3c58f2a 100644 --- a/modules/auth/tests/auth.integration.tests.js +++ b/modules/auth/tests/auth.integration.tests.js @@ -292,17 +292,20 @@ describe('Auth integration tests:', () => { } }); - test('forgot password request for non-existent email should return 400', async () => { + test('forgot password request for non-existent email should return uniform 200 (no enumeration)', async () => { try { const result = await agent .post('/api/auth/forgot') .send({ email: 'falseemail@gmail.com', }) - .expect(400); + .expect(200); - expect(result.body.message).toBe('Bad Request'); - expect(result.body.description).toBe('No account with that email has been found'); + expect(result.body.type).toBe('success'); + expect(result.body.message).toBe('If that email exists, a reset link has been sent.'); + // Must NOT contain any hint of account existence + expect(JSON.stringify(result.body)).not.toContain('No account'); + expect(JSON.stringify(result.body)).not.toContain('found'); } catch (err) { console.log(err); expect(err).toBeFalsy(); @@ -342,7 +345,7 @@ describe('Auth integration tests:', () => { } }); - test('forgot password request for non-local provider should return 400', async () => { + test('forgot password request for non-local provider should return uniform 200 (no provider enumeration)', async () => { _userEdited.provider = 'facebook'; try { @@ -359,9 +362,12 @@ describe('Auth integration tests:', () => { .send({ email: userEdited.email, }) - .expect(400); - expect(result.body.message).toBe('Bad Request'); - expect(result.body.description).toBe(`It seems like you signed up using your ${userEdited.provider} account`); + .expect(200); + expect(result.body.type).toBe('success'); + expect(result.body.message).toBe('If that email exists, a reset link has been sent.'); + // Must NOT reveal the OAuth provider name + expect(JSON.stringify(result.body)).not.toContain('facebook'); + expect(JSON.stringify(result.body)).not.toContain('signed up using'); } catch (err) { console.log(err); expect(err).toBeFalsy(); @@ -375,16 +381,16 @@ describe('Auth integration tests:', () => { } }); - test('should initiate password reset process for valid email', async () => { + test('should initiate password reset process for valid email and return uniform 200', async () => { try { const result = await agent .post('/api/auth/forgot') .send({ email: user.email, }) - .expect(400); - expect(result.body.message).toBe('Bad Request'); - expect(result.body.description).toBe('Failure sending email'); + .expect(200); + expect(result.body.type).toBe('success'); + expect(result.body.message).toBe('If that email exists, a reset link has been sent.'); } catch (err) { console.log(err); expect(err).toBeFalsy(); @@ -410,10 +416,10 @@ describe('Auth integration tests:', () => { .send({ email: user.email, }) - .expect(400); + .expect(200); - expect(result.body.message).toBe('Bad Request'); - expect(result.body.description).toBe('Failure sending email'); + expect(result.body.type).toBe('success'); + expect(result.body.message).toBe('If that email exists, a reset link has been sent.'); } catch (err) { console.log(err); expect(err).toBeFalsy(); @@ -447,10 +453,10 @@ describe('Auth integration tests:', () => { .send({ email: user.email, }) - .expect(400); + .expect(200); - expect(result.body.message).toBe('Bad Request'); - expect(result.body.description).toBe('Failure sending email'); + expect(result.body.type).toBe('success'); + expect(result.body.message).toBe('If that email exists, a reset link has been sent.'); } catch (err) { console.log(err); expect(err).toBeFalsy(); @@ -1246,9 +1252,9 @@ describe('Auth integration tests:', () => { }); test('should reject password reset with a weak password (zxcvbn strength gate)', async () => { - // Trigger forgot to generate a reset token (email send fails in test env, which is expected) + // Trigger forgot to generate a reset token — returns uniform 200 regardless of mail outcome try { - await agent.post('/api/auth/forgot').send({ email: credentials[0].email }).expect(400); + await agent.post('/api/auth/forgot').send({ email: credentials[0].email }).expect(200); } catch (err) { console.log(err); expect(err).toBeFalsy(); @@ -1277,9 +1283,9 @@ describe('Auth integration tests:', () => { }); test('should successfully reset password with a valid token', async () => { - // Trigger forgot to generate a reset token (email send fails in test env, which is expected) + // Trigger forgot to generate a reset token — returns uniform 200 regardless of mail outcome try { - await agent.post('/api/auth/forgot').send({ email: credentials[0].email }).expect(400); + await agent.post('/api/auth/forgot').send({ email: credentials[0].email }).expect(200); } catch (err) { console.log(err); expect(err).toBeFalsy(); diff --git a/modules/auth/tests/auth.password.controller.unit.tests.js b/modules/auth/tests/auth.password.controller.unit.tests.js new file mode 100644 index 000000000..1c7c4617d --- /dev/null +++ b/modules/auth/tests/auth.password.controller.unit.tests.js @@ -0,0 +1,231 @@ +/** + * Module dependencies. + * + * Unit tests for auth.password.controller `forgot` handler. + * Security regression guard: + * - Must return a UNIFORM response for both known and unknown emails (anti-enumeration, #3722). + * - Must run a dummy bcrypt hash on the unknown-user path to equalise timing (anti-timing-oracle, #3722). + * - Mail-send failure must NOT re-introduce an enumeration oracle (#3722 follow-up). + */ +import { jest, describe, test, expect, beforeEach } from '@jest/globals'; + +const UNIFORM_MSG = 'If that email exists, a reset link has been sent.'; + +describe('auth.password.controller.forgot — account enumeration + timing oracle (#3722):', () => { + let mockGetBrut; + let mockUpdate; + let mockSendMail; + let mockBcryptCompare; + let mockErrorFn; + let mockSuccessFn; + + beforeEach(() => { + jest.resetModules(); + + mockGetBrut = jest.fn(); + mockUpdate = jest.fn(); + mockSendMail = jest.fn(); + mockBcryptCompare = jest.fn().mockResolvedValue(false); + mockErrorFn = jest.fn().mockReturnValue(jest.fn()); + mockSuccessFn = jest.fn().mockReturnValue(jest.fn()); + + jest.unstable_mockModule('../../../lib/services/logger.js', () => ({ + default: { warn: jest.fn(), error: jest.fn(), info: jest.fn() }, + })); + + jest.unstable_mockModule('../../../modules/users/services/users.service.js', () => ({ + default: { getBrut: mockGetBrut, update: mockUpdate }, + })); + + jest.unstable_mockModule('../../../modules/auth/services/auth.service.js', () => ({ + default: { + comparePassword: mockBcryptCompare, + hashPassword: jest.fn().mockResolvedValue('$2b$hashed'), + checkPassword: jest.fn((p) => p), + generateRandomPassphrase: jest.fn().mockReturnValue('random-passphrase'), + }, + })); + + jest.unstable_mockModule('../../../lib/helpers/mailer/index.js', () => ({ + default: { + isConfigured: jest.fn().mockReturnValue(true), + sendMail: mockSendMail, + }, + })); + + jest.unstable_mockModule('../../../lib/helpers/getBaseUrl.js', () => ({ + default: jest.fn().mockReturnValue('http://localhost:3000'), + })); + + jest.unstable_mockModule('../../../lib/helpers/errors.js', () => ({ + default: { getMessage: jest.fn().mockReturnValue('error') }, + })); + + jest.unstable_mockModule('../../../lib/helpers/responses.js', () => ({ + default: { + success: mockSuccessFn, + error: mockErrorFn, + }, + })); + + jest.unstable_mockModule('../../../config/index.js', () => ({ + default: { + jwt: { secret: 'test-secret', expiresIn: 3600 }, + cookie: { secure: false, sameSite: 'lax' }, + app: { title: 'Test App', contact: 'contact@test.com' }, + }, + })); + }); + + test('should return a UNIFORM 200 success response for an unknown email (no enumeration)', async () => { + // Unknown user — getBrut returns null + mockGetBrut.mockResolvedValueOnce(null); + + const { default: PasswordController } = await import('../controllers/auth.password.controller.js'); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + + const req = { body: { email: 'unknown@example.com' } }; + const res = {}; + + await PasswordController.forgot(req, res); + + // Must NOT call responses.error with user-not-found detail + const errorCalls = responses.error.mock.calls; + const enumLeakCall = errorCalls.find( + (call) => typeof call[3] === 'string' && (call[3].toLowerCase().includes('account') || call[3].toLowerCase().includes('email') || call[3].toLowerCase().includes('found')), + ); + expect(enumLeakCall).toBeUndefined(); + + // Must call responses.success with the EXACT uniform message + expect(responses.success).toHaveBeenCalledWith(res, UNIFORM_MSG); + }); + + test('should perform a dummy hash on the unknown-user path to equalise timing', async () => { + // Unknown user — getBrut returns null + mockGetBrut.mockResolvedValueOnce(null); + + const { default: PasswordController } = await import('../controllers/auth.password.controller.js'); + const { default: AuthService } = await import('../../../modules/auth/services/auth.service.js'); + + const req = { body: { email: 'ghost@example.com' } }; + const res = {}; + + await PasswordController.forgot(req, res); + + // Dummy hash must have been called (comparePassword or hashPassword) + const dummyCalled = AuthService.comparePassword.mock.calls.length > 0 || AuthService.hashPassword.mock.calls.length > 0; + expect(dummyCalled).toBe(true); + }); + + test('should return a UNIFORM success response for a known local user (same as unknown path)', async () => { + const knownUser = { + id: 'u1', + email: 'known@example.com', + provider: 'local', + firstName: 'Jane', + lastName: 'Doe', + resetPasswordToken: 'tok123', + }; + mockGetBrut.mockResolvedValueOnce(knownUser); + mockUpdate.mockResolvedValueOnce({ ...knownUser, resetPasswordToken: 'tok123', resetPasswordExpires: Date.now() + 3600000 }); + mockSendMail.mockResolvedValueOnce({ accepted: ['known@example.com'] }); + + const { default: PasswordController } = await import('../controllers/auth.password.controller.js'); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + + const req = { body: { email: 'known@example.com' } }; + const res = {}; + + await PasswordController.forgot(req, res); + + // Must call responses.success with the EXACT uniform message — regression guard against distinct responses + expect(responses.success).toHaveBeenCalledWith(res, UNIFORM_MSG); + // Must NOT call responses.error (no 400/422 for known local user) + expect(responses.error).not.toHaveBeenCalled(); + }); + + test('should NOT reveal OAuth provider in error response (no provider enumeration)', async () => { + const oauthUser = { + id: 'u2', + email: 'oauth@example.com', + provider: 'google', + firstName: 'John', + lastName: 'Smith', + }; + mockGetBrut.mockResolvedValueOnce(oauthUser); + + const { default: PasswordController } = await import('../controllers/auth.password.controller.js'); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + + const req = { body: { email: 'oauth@example.com' } }; + const res = {}; + + await PasswordController.forgot(req, res); + + // Must NOT leak the provider name (google, facebook, etc.) + const errorCalls = responses.error.mock.calls; + const providerLeakCall = errorCalls.find( + (call) => typeof call[3] === 'string' && call[3].toLowerCase().includes('google'), + ); + expect(providerLeakCall).toBeUndefined(); + + // Must call responses.success with the EXACT uniform message + expect(responses.success).toHaveBeenCalledWith(res, UNIFORM_MSG); + }); + + test('should return the UNIFORM success response even when sendMail throws (no enumeration oracle on mail failure)', async () => { + const knownUser = { + id: 'u3', + email: 'mailfail@example.com', + provider: 'local', + firstName: 'Mail', + lastName: 'Fail', + resetPasswordToken: 'tok456', + }; + mockGetBrut.mockResolvedValueOnce(knownUser); + mockUpdate.mockResolvedValueOnce({ ...knownUser, resetPasswordToken: 'tok456', resetPasswordExpires: Date.now() + 3600000 }); + // Simulate sendMail throwing (SMTP down, network error, etc.) + mockSendMail.mockRejectedValueOnce(new Error('ECONNREFUSED: cannot connect to SMTP server')); + + const { default: PasswordController } = await import('../controllers/auth.password.controller.js'); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + + const req = { body: { email: 'mailfail@example.com' } }; + const res = {}; + + await PasswordController.forgot(req, res); + + // Must NOT call responses.error — mail failure must not be distinguishable from success + expect(responses.error).not.toHaveBeenCalled(); + // Must still return the exact uniform success message + expect(responses.success).toHaveBeenCalledWith(res, UNIFORM_MSG); + }); + + test('should return the UNIFORM success response when sendMail returns a rejected result (not accepted)', async () => { + const knownUser = { + id: 'u4', + email: 'notaccepted@example.com', + provider: 'local', + firstName: 'Not', + lastName: 'Accepted', + resetPasswordToken: 'tok789', + }; + mockGetBrut.mockResolvedValueOnce(knownUser); + mockUpdate.mockResolvedValueOnce({ ...knownUser, resetPasswordToken: 'tok789', resetPasswordExpires: Date.now() + 3600000 }); + // Simulate sendMail returning a result without .accepted + mockSendMail.mockResolvedValueOnce({ rejected: ['notaccepted@example.com'] }); + + const { default: PasswordController } = await import('../controllers/auth.password.controller.js'); + const { default: responses } = await import('../../../lib/helpers/responses.js'); + + const req = { body: { email: 'notaccepted@example.com' } }; + const res = {}; + + await PasswordController.forgot(req, res); + + // Must NOT call responses.error — mail rejection must not be distinguishable from success + expect(responses.error).not.toHaveBeenCalled(); + // Must still return the exact uniform success message + expect(responses.success).toHaveBeenCalledWith(res, UNIFORM_MSG); + }); +});