From 15c2790bdb0b24447640630da6e6cb90e75f9866 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Sun, 17 May 2026 21:34:44 -0500 Subject: [PATCH] fix(security): never echo err.message verbatim on 4xx (audit #73 P1-B) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Before: error-handler.js returned err.message verbatim for any status < 500. A future middleware / library that threw with detail (Sequelize validation error revealing column names, pg driver hostname leak, third-party library stack frame) would surface that to the client without filtering. After: 4xx responses use generic per-status messages by default (GENERIC_MESSAGES table). Echo the original err.message ONLY when the error is explicitly marked safe via err.expose === true — the http-errors / express body-parser convention. Body-parser's PayloadTooLargeError, BadRequest from malformed JSON, etc. all set expose=true, so legitimate client-side errors keep their descriptive messages. Sequelize, pg, and most third-party libs don't set expose, so their messages now stay in the log instead of the response body. 5xx behavior unchanged — still always "Error!" with a requestId for log correlation. Tests: - Existing "doesn't leak 500 detail" coverage preserved. - New cases: * 4xx without expose → generic, no detail * 4xx with expose=true → message echoed * PayloadTooLargeError-style → message echoed * Sequelize-style 4xx → generic, no "Sequelize" / column name Closes #73 P1-B. Suite: 280 / 280 + 4 integration skipped (was 276 / 276). Co-Authored-By: Claude Opus 4.7 (1M context) --- app/middleware/error-handler.js | 50 +++++++++++++++++++++++++++-- tests/api/error-handler.test.js | 57 ++++++++++++++++++++++++++++++++- 2 files changed, 103 insertions(+), 4 deletions(-) diff --git a/app/middleware/error-handler.js b/app/middleware/error-handler.js index 77231ac..f817d76 100644 --- a/app/middleware/error-handler.js +++ b/app/middleware/error-handler.js @@ -26,6 +26,35 @@ const log = require('../config/logger.js'); * Stack traces are NEVER returned to the client. They land in the * structured log instead, where they're searchable + filtered. */ +// Generic per-status fallback messages used when the underlying error +// isn't marked safe-to-expose. Keeps the response shape predictable +// for clients while denying information disclosure via err.message. +const GENERIC_MESSAGES = { + 400: 'Bad request.', + 401: 'Unauthorized.', + 403: 'Forbidden.', + 404: 'Not found.', + 405: 'Method not allowed.', + 409: 'Conflict.', + 413: 'Payload too large.', + 415: 'Unsupported media type.', + 422: 'Unprocessable entity.', + 429: 'Too many requests.', +}; + +function isSafeMessage(err) { + if (!err) return false; + // http-errors / express body-parser convention: errors that are + // intended to be surfaced to the client set `expose: true`. This + // includes PayloadTooLargeError (413), malformed-JSON SyntaxError + // wrapped by body-parser (400), etc. Sequelize / pg-driver errors + // do NOT set expose=true, so they're correctly filtered. + if (err.expose === true && typeof err.message === 'string' && err.message.length > 0) { + return true; + } + return false; +} + function errorHandler(err, req, res, next) { if (res.headersSent) { // Express docs say: delegate to default handler if headers @@ -52,9 +81,24 @@ function errorHandler(err, req, res, next) { log.warn(logBody, 'request error'); } - const body = { - message: status >= 500 ? 'Error!' : (err.message || 'Bad Request'), - }; + // 5xx: always generic. The full error lands in the log; clients + // get a stable shape + a requestId to correlate. + // + // 4xx: echo err.message only when the error is explicitly marked + // safe (err.expose === true, the http-errors convention). Falls + // back to a per-status generic to avoid leaking internals from + // unexpected error sources (Sequelize, pg-driver, third-party + // middleware that throws with detail). + let message; + if (status >= 500) { + message = 'Error!'; + } else if (isSafeMessage(err)) { + message = err.message; + } else { + message = GENERIC_MESSAGES[status] || 'Request error.'; + } + + const body = { message }; if (logBody.requestId) { body.requestId = logBody.requestId; } diff --git a/tests/api/error-handler.test.js b/tests/api/error-handler.test.js index 844f011..dd8cc58 100644 --- a/tests/api/error-handler.test.js +++ b/tests/api/error-handler.test.js @@ -21,8 +21,34 @@ beforeAll(() => { next(new Error('boom')); }); app.get('/explode/with-status', (req, res, next) => { + // 4xx WITHOUT expose=true: message should NOT pass through + // (it could leak internals from an internal middleware / + // library that throws with detail). + const err = new Error('Sensitive internal detail'); + err.status = 418; + next(err); + }); + app.get('/explode/exposed', (req, res, next) => { + // 4xx WITH expose=true (http-errors convention): message + // is safe to surface to the client. const err = new Error('I am a teapot'); err.status = 418; + err.expose = true; + next(err); + }); + app.get('/explode/payload-too-large', (req, res, next) => { + // Simulates body-parser's PayloadTooLargeError shape. + const err = new Error('request entity too large'); + err.status = 413; + err.expose = true; // body-parser sets this + next(err); + }); + app.get('/explode/sequelize-style-4xx', (req, res, next) => { + // 4xx-status from an internal library (Sequelize, pg driver, + // etc.) — these set a status but DON'T set expose. We must + // NOT echo their .message. + const err = new Error('SequelizeValidationError: column "X" cannot be null'); + err.status = 400; next(err); }); app.get('/explode/leaky', (req, res, next) => { @@ -55,9 +81,38 @@ describe('global error handler', () => { test('honors a numeric err.status in 4xx range', async () => { const res = await request(app).get('/explode/with-status'); expect(res.status).toBe(418); - // For 4xx the message goes through (it's a client error, not a server one) + }); + + test('4xx without err.expose returns a generic message (no detail leak)', async () => { + const res = await request(app).get('/explode/with-status'); + // Status echoed; sensitive .message NOT echoed. + expect(res.body.message).not.toMatch(/sensitive internal detail/i); + // We use a per-status generic, or fall back to "Request error." + // for non-canonical statuses (like 418). + expect(typeof res.body.message).toBe('string'); + expect(res.body.message.length).toBeGreaterThan(0); + }); + + test('4xx WITH err.expose=true echoes the message (http-errors convention)', async () => { + const res = await request(app).get('/explode/exposed'); + expect(res.status).toBe(418); expect(res.body.message).toBe('I am a teapot'); }); + + test('PayloadTooLargeError-style 413 (expose=true) passes the message through', async () => { + const res = await request(app).get('/explode/payload-too-large'); + expect(res.status).toBe(413); + expect(res.body.message).toMatch(/request entity too large/i); + }); + + test('Sequelize-style 4xx (no expose) gets a generic 400 message', async () => { + const res = await request(app).get('/explode/sequelize-style-4xx'); + expect(res.status).toBe(400); + const text = JSON.stringify(res.body); + expect(text).not.toMatch(/Sequelize/); + expect(text).not.toMatch(/column.*X/); + expect(res.body.message).toMatch(/bad request/i); + }); }); describe('404 fallthrough', () => {