Skip to content

Commit 428fb18

Browse files
PierreBrisorgueilPierreBrisorgueil
authored
fix(security): auth enumeration + prod error message leak (#3790)
* fix(security): auth enumeration + prod error leak (#3722 #3726) - forgot endpoint: uniform response for known/unknown/OAuth email paths + dummy bcrypt.compare on unknown-user path to equalise timing - initErrorRoutes: sanitise err.message/code in production (return generic 'Internal Server Error'); detailed body kept in non-prod - Unit tests: 4 for auth.password.controller + 3 for express.errorroutes Closes #3722 Closes #3726 * fix(security): uniform response on mail failure, JSDoc, stronger enumeration tests - auth.password.controller: mail failure now logs + returns uniformSuccess() instead of 400, closing enumeration oracle on SMTP errors (#3722) - auth.integration.tests: update all forgot-flow assertions to uniform 200 (unknown email, OAuth provider, mail failure, password-reset setup calls) - auth.password.controller.unit.tests: strengthen to exact UNIFORM_MSG; add tests for sendMail throws and sendMail !accepted paths - express.js initErrorRoutes: add JSDoc on function and 4-arity handler (#3726) - express.errorroutes.unit.tests: add @returns to getInitErrorRoutes JSDoc
1 parent 853b36b commit 428fb18

5 files changed

Lines changed: 475 additions & 32 deletions

File tree

‎lib/services/express.js‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -321,11 +321,28 @@ const initModulesServerRoutes = async (app) => {
321321

322322
/**
323323
* Configure error handling
324+
* @param {import('express').Express} app - Express application instance
325+
* @returns {void}
324326
*/
325327
const initErrorRoutes = (app) => {
328+
/**
329+
* Express 4-arity error handler.
330+
* In production: returns a generic 500 to prevent leaking internal error details (#3726).
331+
* In non-production: includes err.message and err.code for debugging.
332+
* @param {Error} err - The error object caught by Express
333+
* @param {import('express').Request} req - Express request object
334+
* @param {import('express').Response} res - Express response object
335+
* @param {import('express').NextFunction} next - Express next function
336+
* @returns {void}
337+
*/
326338
app.use((err, req, res, next) => {
327339
if (!err) return next();
328340
logger.error('Unhandled express error', { error: err });
341+
// In production never leak internal error details (message, code) to clients.
342+
// Detailed errors are still logged above for observability.
343+
if (process.env.NODE_ENV === 'production') {
344+
return res.status(err.status || 500).send({ message: 'Internal Server Error' });
345+
}
329346
res.status(err.status || 500).send({
330347
message: err.message,
331348
code: err.code,

‎lib/services/tests/express.errorroutes.unit.tests.js‎

Lines changed: 167 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,167 @@
1+
/**
2+
* Module dependencies.
3+
*
4+
* Unit tests for express.js initErrorRoutes.
5+
* Security regression guard: err.message/code must NOT be sent to clients in production (#3726).
6+
*/
7+
import { jest, describe, test, expect, beforeEach, afterEach } from '@jest/globals';
8+
9+
describe('express initErrorRoutes — prod error leak guard (#3726):', () => {
10+
let originalNodeEnv;
11+
12+
beforeEach(() => {
13+
originalNodeEnv = process.env.NODE_ENV;
14+
jest.resetModules();
15+
});
16+
17+
afterEach(() => {
18+
process.env.NODE_ENV = originalNodeEnv;
19+
});
20+
21+
/**
22+
* Helper: extract initErrorRoutes from express.js (with all heavy deps mocked).
23+
* @returns {Promise<Function>} The initErrorRoutes function extracted from the module
24+
*/
25+
const getInitErrorRoutes = async () => {
26+
jest.unstable_mockModule('../../../config/index.js', () => ({
27+
default: {
28+
domain: 'http://localhost:3000',
29+
app: { title: 'Test', description: '', keywords: '', url: '', logo: '' },
30+
secure: { ssl: false },
31+
log: {},
32+
bodyParser: {},
33+
csrf: {},
34+
cors: { origin: [], credentials: false, optionsSuccessStatus: 200 },
35+
trust: { proxy: false },
36+
swagger: { enable: false },
37+
files: { routes: [], configs: [], policies: [], preRoutes: [], swagger: [], guides: [] },
38+
analytics: { posthog: { autoCapture: false } },
39+
docs: {},
40+
},
41+
}));
42+
jest.unstable_mockModule('../../../lib/services/logger.js', () => ({
43+
default: {
44+
warn: jest.fn(),
45+
error: jest.fn(),
46+
info: jest.fn(),
47+
debug: jest.fn(),
48+
getLogFormat: jest.fn().mockReturnValue('combined'),
49+
getMorganOptions: jest.fn().mockReturnValue({}),
50+
},
51+
}));
52+
jest.unstable_mockModule('../../../lib/helpers/guides.js', () => ({
53+
default: { loadGuides: jest.fn().mockReturnValue([]), mergeGuidesIntoSpec: jest.fn() },
54+
}));
55+
jest.unstable_mockModule('../../../lib/middlewares/requestId.js', () => ({
56+
default: jest.fn((req, res, next) => next()),
57+
}));
58+
jest.unstable_mockModule('../../../lib/middlewares/posthog-context.middleware.js', () => ({
59+
posthogContextMiddleware: jest.fn((req, res, next) => next()),
60+
}));
61+
jest.unstable_mockModule('../../../lib/services/errorTracker.js', () => ({
62+
default: { setupExpressErrorHandler: jest.fn() },
63+
}));
64+
jest.unstable_mockModule('../../../lib/services/analytics.js', () => ({
65+
default: { init: jest.fn().mockResolvedValue(undefined), identify: jest.fn(), groupIdentify: jest.fn() },
66+
}));
67+
jest.unstable_mockModule('../../../lib/middlewares/analytics.js', () => ({
68+
default: jest.fn((req, res, next) => next()),
69+
}));
70+
jest.unstable_mockModule('../../../lib/middlewares/policy.js', () => ({
71+
default: { discoverPolicies: jest.fn().mockResolvedValue(undefined), defineAbilityFor: jest.fn().mockResolvedValue({}) },
72+
}));
73+
74+
const mod = await import('../../../lib/services/express.js');
75+
return mod.default.initErrorRoutes;
76+
};
77+
78+
test('in production: responds with generic { message: "Internal Server Error" } — no err.message/code leak', async () => {
79+
process.env.NODE_ENV = 'production';
80+
const initErrorRoutes = await getInitErrorRoutes();
81+
82+
// Capture the error handler registered via app.use
83+
let errorHandler;
84+
const app = {
85+
use: jest.fn((handler) => { errorHandler = handler; }),
86+
};
87+
initErrorRoutes(app);
88+
expect(typeof errorHandler).toBe('function');
89+
90+
const internalErr = new Error('E11000 duplicate key index: email_1');
91+
internalErr.code = 11000;
92+
internalErr.status = 500;
93+
94+
const sendPayloads = [];
95+
const res = {
96+
status: jest.fn().mockReturnThis(),
97+
send: jest.fn((body) => { sendPayloads.push(body); return res; }),
98+
};
99+
100+
await errorHandler(internalErr, {}, res, jest.fn());
101+
102+
expect(res.status).toHaveBeenCalledWith(500);
103+
expect(sendPayloads).toHaveLength(1);
104+
const body = sendPayloads[0];
105+
// Must NOT include internal error detail
106+
expect(body.message).toBe('Internal Server Error');
107+
expect(body.code).toBeUndefined();
108+
// Must NOT contain the raw error message
109+
expect(JSON.stringify(body)).not.toContain('E11000');
110+
expect(JSON.stringify(body)).not.toContain('email_1');
111+
});
112+
113+
test('in non-production (development): responds with detailed err.message + err.code', async () => {
114+
process.env.NODE_ENV = 'development';
115+
const initErrorRoutes = await getInitErrorRoutes();
116+
117+
let errorHandler;
118+
const app = {
119+
use: jest.fn((handler) => { errorHandler = handler; }),
120+
};
121+
initErrorRoutes(app);
122+
123+
const devErr = new Error('Something went wrong internally');
124+
devErr.code = 'ERR_INTERNAL';
125+
devErr.status = 500;
126+
127+
const sendPayloads = [];
128+
const res = {
129+
status: jest.fn().mockReturnThis(),
130+
send: jest.fn((body) => { sendPayloads.push(body); return res; }),
131+
};
132+
133+
await errorHandler(devErr, {}, res, jest.fn());
134+
135+
const body = sendPayloads[0];
136+
expect(body.message).toBe('Something went wrong internally');
137+
expect(body.code).toBe('ERR_INTERNAL');
138+
});
139+
140+
test('in test env: responds with detailed err.message + err.code (non-production path)', async () => {
141+
process.env.NODE_ENV = 'test';
142+
const initErrorRoutes = await getInitErrorRoutes();
143+
144+
let errorHandler;
145+
const app = {
146+
use: jest.fn((handler) => { errorHandler = handler; }),
147+
};
148+
initErrorRoutes(app);
149+
150+
const testErr = new Error('test internal error');
151+
testErr.code = 'ERR_TEST';
152+
testErr.status = 422;
153+
154+
const sendPayloads = [];
155+
const res = {
156+
status: jest.fn().mockReturnThis(),
157+
send: jest.fn((body) => { sendPayloads.push(body); return res; }),
158+
};
159+
160+
await errorHandler(testErr, {}, res, jest.fn());
161+
162+
const body = sendPayloads[0];
163+
expect(res.status).toHaveBeenCalledWith(422);
164+
expect(body.message).toBe('test internal error');
165+
expect(body.code).toBe('ERR_TEST');
166+
});
167+
});

‎modules/auth/controllers/auth.password.controller.js‎

Lines changed: 32 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -25,16 +25,36 @@ const tokenCookieOptions = {
2525
* @returns {Promise<void>} Sends a JSON response with reset status.
2626
*/
2727
const forgot = async (req, res) => {
28-
let user;
28+
/**
29+
* @returns {void} Sends a uniform 200 response regardless of account existence or mail outcome.
30+
*/
31+
const uniformSuccess = () => responses.success(res, 'If that email exists, a reset link has been sent.')({ status: true });
32+
2933
// check input
3034
if (!req.body.email) return responses.error(res, 422, 'Unprocessable Entity', 'Mail field must not be blank')();
3135
const email = String(req.body.email);
32-
// get user generate and add token
36+
37+
let user;
3338
try {
3439
user = await UserService.getBrut({ email });
35-
if (!user) return responses.error(res, 400, 'Bad Request', 'No account with that email has been found')();
36-
if (user.provider !== 'local')
37-
return responses.error(res, 400, 'Bad Request', `It seems like you signed up using your ${user.provider} account`)();
40+
} catch (err) {
41+
return responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err);
42+
}
43+
44+
// Unknown email or OAuth user — perform a dummy bcrypt compare to equalise
45+
// response timing (anti-timing-oracle). The sentinel is a valid precomputed
46+
// bcrypt hash (cost=10, 60 chars) so bcrypt runs the full KDF instead of
47+
// short-circuiting on format validation.
48+
if (!user || user.provider !== 'local') {
49+
await AuthService.comparePassword(
50+
'dummy-timing-equaliser',
51+
'$2b$10$wotSb2xPb8VF8lpBkulk0.e2xvYvDedhjMyuG/7w3GdPl1vdvogY2',
52+
);
53+
return uniformSuccess();
54+
}
55+
56+
// Known local user — generate token, persist, and send the reset email.
57+
try {
3858
const edit = {
3959
resetPasswordToken: jwt.sign({ exp: Date.now() + 3600000 }, config.jwt.secret, { algorithm: 'HS256' }),
4060
resetPasswordExpires: Date.now() + 3600000,
@@ -43,7 +63,7 @@ const forgot = async (req, res) => {
4363
} catch (err) {
4464
return responses.error(res, 422, 'Unprocessable Entity', errors.getMessage(err))(err);
4565
}
46-
// send mail
66+
4767
try {
4868
const mail = await mails.sendMail({
4969
template: 'reset-password-email',
@@ -56,11 +76,13 @@ const forgot = async (req, res) => {
5676
appContact: config.app.contact,
5777
},
5878
});
59-
if (!mail || !mail.accepted) return responses.error(res, 400, 'Bad Request', 'Failure sending email')();
60-
return responses.success(res, 'An email has been sent with further instructions')({ status: true });
61-
} catch (_mailErr) {
62-
return responses.error(res, 400, 'Bad Request', 'Failure sending email')();
79+
if (!mail || !mail.accepted) {
80+
logger.error('auth.password.forgot: mail not accepted', { email: user.email });
81+
}
82+
} catch (mailErr) {
83+
logger.error('auth.password.forgot: sendMail threw', { message: mailErr?.message, stack: mailErr?.stack });
6384
}
85+
return uniformSuccess();
6486
};
6587

6688
/**

‎modules/auth/tests/auth.integration.tests.js‎

Lines changed: 28 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -292,17 +292,20 @@ describe('Auth integration tests:', () => {
292292
}
293293
});
294294

295-
test('forgot password request for non-existent email should return 400', async () => {
295+
test('forgot password request for non-existent email should return uniform 200 (no enumeration)', async () => {
296296
try {
297297
const result = await agent
298298
.post('/api/auth/forgot')
299299
.send({
300300
email: 'falseemail@gmail.com',
301301
})
302-
.expect(400);
302+
.expect(200);
303303

304-
expect(result.body.message).toBe('Bad Request');
305-
expect(result.body.description).toBe('No account with that email has been found');
304+
expect(result.body.type).toBe('success');
305+
expect(result.body.message).toBe('If that email exists, a reset link has been sent.');
306+
// Must NOT contain any hint of account existence
307+
expect(JSON.stringify(result.body)).not.toContain('No account');
308+
expect(JSON.stringify(result.body)).not.toContain('found');
306309
} catch (err) {
307310
console.log(err);
308311
expect(err).toBeFalsy();
@@ -342,7 +345,7 @@ describe('Auth integration tests:', () => {
342345
}
343346
});
344347

345-
test('forgot password request for non-local provider should return 400', async () => {
348+
test('forgot password request for non-local provider should return uniform 200 (no provider enumeration)', async () => {
346349
_userEdited.provider = 'facebook';
347350

348351
try {
@@ -359,9 +362,12 @@ describe('Auth integration tests:', () => {
359362
.send({
360363
email: userEdited.email,
361364
})
362-
.expect(400);
363-
expect(result.body.message).toBe('Bad Request');
364-
expect(result.body.description).toBe(`It seems like you signed up using your ${userEdited.provider} account`);
365+
.expect(200);
366+
expect(result.body.type).toBe('success');
367+
expect(result.body.message).toBe('If that email exists, a reset link has been sent.');
368+
// Must NOT reveal the OAuth provider name
369+
expect(JSON.stringify(result.body)).not.toContain('facebook');
370+
expect(JSON.stringify(result.body)).not.toContain('signed up using');
365371
} catch (err) {
366372
console.log(err);
367373
expect(err).toBeFalsy();
@@ -375,16 +381,16 @@ describe('Auth integration tests:', () => {
375381
}
376382
});
377383

378-
test('should initiate password reset process for valid email', async () => {
384+
test('should initiate password reset process for valid email and return uniform 200', async () => {
379385
try {
380386
const result = await agent
381387
.post('/api/auth/forgot')
382388
.send({
383389
email: user.email,
384390
})
385-
.expect(400);
386-
expect(result.body.message).toBe('Bad Request');
387-
expect(result.body.description).toBe('Failure sending email');
391+
.expect(200);
392+
expect(result.body.type).toBe('success');
393+
expect(result.body.message).toBe('If that email exists, a reset link has been sent.');
388394
} catch (err) {
389395
console.log(err);
390396
expect(err).toBeFalsy();
@@ -410,10 +416,10 @@ describe('Auth integration tests:', () => {
410416
.send({
411417
email: user.email,
412418
})
413-
.expect(400);
419+
.expect(200);
414420

415-
expect(result.body.message).toBe('Bad Request');
416-
expect(result.body.description).toBe('Failure sending email');
421+
expect(result.body.type).toBe('success');
422+
expect(result.body.message).toBe('If that email exists, a reset link has been sent.');
417423
} catch (err) {
418424
console.log(err);
419425
expect(err).toBeFalsy();
@@ -447,10 +453,10 @@ describe('Auth integration tests:', () => {
447453
.send({
448454
email: user.email,
449455
})
450-
.expect(400);
456+
.expect(200);
451457

452-
expect(result.body.message).toBe('Bad Request');
453-
expect(result.body.description).toBe('Failure sending email');
458+
expect(result.body.type).toBe('success');
459+
expect(result.body.message).toBe('If that email exists, a reset link has been sent.');
454460
} catch (err) {
455461
console.log(err);
456462
expect(err).toBeFalsy();
@@ -1246,9 +1252,9 @@ describe('Auth integration tests:', () => {
12461252
});
12471253

12481254
test('should reject password reset with a weak password (zxcvbn strength gate)', async () => {
1249-
// Trigger forgot to generate a reset token (email send fails in test env, which is expected)
1255+
// Trigger forgot to generate a reset token — returns uniform 200 regardless of mail outcome
12501256
try {
1251-
await agent.post('/api/auth/forgot').send({ email: credentials[0].email }).expect(400);
1257+
await agent.post('/api/auth/forgot').send({ email: credentials[0].email }).expect(200);
12521258
} catch (err) {
12531259
console.log(err);
12541260
expect(err).toBeFalsy();
@@ -1277,9 +1283,9 @@ describe('Auth integration tests:', () => {
12771283
});
12781284

12791285
test('should successfully reset password with a valid token', async () => {
1280-
// Trigger forgot to generate a reset token (email send fails in test env, which is expected)
1286+
// Trigger forgot to generate a reset token — returns uniform 200 regardless of mail outcome
12811287
try {
1282-
await agent.post('/api/auth/forgot').send({ email: credentials[0].email }).expect(400);
1288+
await agent.post('/api/auth/forgot').send({ email: credentials[0].email }).expect(200);
12831289
} catch (err) {
12841290
console.log(err);
12851291
expect(err).toBeFalsy();

0 commit comments

Comments
 (0)