From 1621b6d77914880f1fbc83d9892a62c8c6242685 Mon Sep 17 00:00:00 2001 From: "Aaron K. Clark" Date: Tue, 19 May 2026 02:16:30 -0500 Subject: [PATCH] fix(validate): stop leaking raw error details in the non-ZodError fallback (+ extend regression test to middleware/) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The `fmt(err)` helper in `app/middleware/validate.js` had the same `error: String(err)` pattern I scrubbed from all 17 controllers in #140 — but the regression test from that PR only walked `app/controllers/`, so the middleware copy of the leak survived. The fallback branch fires when `schema.safeParse` somehow surfaces a non-ZodError (defensive — zod normally always wraps). When it does, the raw stringified error landed in the 400 response body. A future zod release or a quirky schema mutation could trip it, leaking internal text the global error-handler policy explicitly suppresses. Two changes: 1. validate.js: drop `error: String(err)` from the fallback shape. The client gets a generic `{ message: 'Validation failed.' }`; the original error is whatever caller logging propagates. 2. tests/unit/controller-error-shape.test.js: scan `app/middleware/` too, not just `app/controllers/`. Future drift in either directory now fails CI. Also broadens the pattern set to catch `String(err)` (singular variable) and `error.message` / `err.message`, plus widens the status-code match from 5xx to 4xx-or-5xx so this validation 400 case is in scope. Net: 604 tests pass (was 595 + 9 new = 8 per-middleware-file checks + 1 sanity floor for the middleware directory). Co-Authored-By: Claude Opus 4.7 (1M context) --- app/middleware/validate.js | 7 ++- tests/unit/controller-error-shape.test.js | 75 ++++++++++++++--------- 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/app/middleware/validate.js b/app/middleware/validate.js index 218a7a4..7dcf01b 100644 --- a/app/middleware/validate.js +++ b/app/middleware/validate.js @@ -35,7 +35,12 @@ function fmt(err) { })), }; } - return { message: 'Validation failed.', error: String(err) }; + // Fallback for any non-ZodError that `safeParse` somehow surfaces + // (defensive — zod normally always wraps). Match the controller- + // wide 5xx policy from #140: never echo raw error text to the + // client. The full error lands in the structured log via the + // global error-handler if it ever propagates that far. + return { message: 'Validation failed.' }; } function validateOn(key) { diff --git a/tests/unit/controller-error-shape.test.js b/tests/unit/controller-error-shape.test.js index ca2293e..6a75bc0 100644 --- a/tests/unit/controller-error-shape.test.js +++ b/tests/unit/controller-error-shape.test.js @@ -1,11 +1,12 @@ // SPDX-License-Identifier: Apache-2.0 // Copyright 2026 Aaron K. Clark // -// Source-level regression pin: controllers must NOT echo the raw caught -// error back to clients in 5xx responses. The global error handler in -// app/middleware/error-handler.js is the only path allowed to shape -// 5xx bodies — and it deliberately returns `{ message: "Error!", -// requestId? }` with the original error logged but NOT surfaced. +// Source-level regression pin: server-handling code must NOT echo the +// raw caught error back to clients in 4xx/5xx responses. The global +// error handler in app/middleware/error-handler.js is the only path +// allowed to shape error bodies — and it deliberately returns +// `{ message: "...", requestId? }` with the original error logged but +// NOT surfaced. // // Why this lives as a structural test instead of behavioral: // @@ -14,42 +15,56 @@ // fixtures for every code path that catches an error. A grep-based test // on the source pins the policy with a single assertion and catches // any future regression where someone copy-pastes the old pattern back -// into a new controller. +// into a new controller / middleware. // -// If a controller needs to surface error detail in the response body +// If a code path needs to surface error detail in the response body // (e.g., a validation error from zod that already lives behind an // `expose: true` flag), it should `throw` the error and let the global // error-handler decide whether to expose it — never echo `String(error)` // from the catch directly. +// +// Scope: app/controllers/ AND app/middleware/. The middleware sweep was +// added after #140 fixed controllers but missed validate.js's same-shape +// fallback (`error: String(err)` in the non-ZodError branch of fmt()). +// Cover both directories so the regression net doesn't have holes. import { describe, test, expect } from 'vitest'; import { readdirSync, readFileSync } from 'node:fs'; import { resolve, join } from 'node:path'; -const CONTROLLERS_DIR = resolve(__dirname, '../../app/controllers'); +const SCAN_DIRS = [ + { abs: resolve(__dirname, '../../app/controllers'), label: 'controllers' }, + { abs: resolve(__dirname, '../../app/middleware'), label: 'middleware' }, +]; -describe('controllers do not leak raw error details in 5xx bodies', () => { - const files = readdirSync(CONTROLLERS_DIR).filter((f) => f.endsWith('.js')); +describe('server code does not leak raw error details in error responses', () => { + for (const { abs, label } of SCAN_DIRS) { + const files = readdirSync(abs).filter((f) => f.endsWith('.js')); - test('controllers/ has the expected number of files', () => { - // Sanity check: if the directory mysteriously empties, the per-file - // assertions below would vacuously pass. Pin a non-zero floor. - expect(files.length).toBeGreaterThan(10); - }); + test(`${label}/ has the expected number of files`, () => { + // Sanity check: if the directory mysteriously empties, the per- + // file assertions below would vacuously pass. Pin a non-zero + // floor for each scanned dir. + expect(files.length).toBeGreaterThan(0); + }); - test.each(files)('%s does not echo `error: String(error)` to clients', (file) => { - const content = readFileSync(join(CONTROLLERS_DIR, file), 'utf8'); - // The grep that found the original 137-occurrence leak. Catches the - // common copy-paste variants (whitespace, quoted strings around the - // arg). If a real need to surface error detail emerges, prefer - // `next(err)` with `err.expose = true` and let the global - // error-handler do it under controlled conditions. - expect(content).not.toMatch(/error:\s*String\(\s*error\s*\)/); - expect(content).not.toMatch(/error:\s*err\.message/); - // And the static-string variant the customer controller used to have: - // `error: "Sequelize Op not available"`. Same principle — anything - // hardcoded into a 5xx body bypasses the global error-handler's - // policy. - expect(content).not.toMatch(/res\.status\(5\d\d\)\.json\([^)]*error:\s*"/); - }); + test.each(files)(`${label}/%s does not echo raw error details to clients`, (file) => { + const content = readFileSync(join(abs, file), 'utf8'); + // The grep that found the original 137-occurrence leak in + // controllers. Catches the common copy-paste variants + // (whitespace, alternative variable names). If a real need to + // surface error detail emerges, prefer `next(err)` with + // `err.expose = true` and let the global error-handler do it + // under controlled conditions. + expect(content).not.toMatch(/error:\s*String\(\s*error\s*\)/); + expect(content).not.toMatch(/error:\s*String\(\s*err\s*\)/); + expect(content).not.toMatch(/error:\s*err\.message/); + expect(content).not.toMatch(/error:\s*error\.message/); + // And the static-string variant the customer controller used to + // have: `error: "Sequelize Op not available"`. Same principle — + // anything hardcoded into an error body bypasses the global + // error-handler's policy. + expect(content).not.toMatch(/res\.status\([45]\d\d\)\.json\([^)]*error:\s*"/); + }); + } });