fix(security): never echo err.message verbatim on 4xx

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;
     }

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