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', () => {