Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion app/middleware/validate.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
75 changes: 45 additions & 30 deletions tests/unit/controller-error-shape.test.js
Original file line number Diff line number Diff line change
@@ -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:
//
Expand All @@ -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*"/);
});
}
});