fix(controllers): stop echoing raw error details to clients in 5xx bodies#140
Merged
Merged
Conversation
…dies
Every controller's try/catch was returning
`res.status(500).json({ message: "Error!", error: String(error) })`.
The `error` field surfaced raw Sequelize / pg-driver text to clients:
"value too long for type character varying(255)", "connection to
server at \"db\" (10.0.0.5) failed", "duplicate key value violates
unique constraint \"…\"", etc. That's exactly the kind of detail the
global error-handler in app/middleware/error-handler.js was already
written to NOT surface — but the per-controller catches intercepted
the error before it could reach the global path, so the policy was
silently bypassed across all 17 controllers (137 occurrences total).
Strip `, error: String(error)` from every 5xx return. Controllers
still log the full error via `log.error({ err: error }, …)` so the
operator-side signal is unchanged; only the client-facing body
loses the leak. One static-string variant in customercontroller.js
(`error: "Sequelize Op not available"`) is also removed since it
violated the same policy.
New `tests/unit/controller-error-shape.test.js` does a source-level
regression pin via test.each() over every controller, asserting that
no file contains `error: String(error)`, `error: err.message`, or
the static-string variant in a 5xx body. Future copy-paste of the
old pattern fails CI immediately.
No public test changed expectations — the existing tests' 5xx
assertions match only on status code + `message`, never on the
removed `error` field, so the suite passes unchanged at 495 then
+20 new structural assertions = 515.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CryptoJones
added a commit
that referenced
this pull request
May 19, 2026
…lback (+ extend regression test to middleware/) (#154) 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: Aaron K. Clark <akclark@thenetwerk.net> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
CryptoJones
added a commit
that referenced
this pull request
May 19, 2026
`tests/unit/controller-error-shape.test.js` scanned for variants of `error: String(err) / err.message / error.message` to catch the original 137-occurrence leak that #140 cleaned up. But the runtime-shipped response body uses `message:` as its key — and a future contributor who writes `res.status(500).json({ message: err.message })` would route around the global error-handler's policy without the test catching the regression. Extend the regex set to cover the parallel `message:` patterns: - `message: String(error)` / `message: String(err)` - `message: err.message` - `message: error.message` A grep of `app/` confirms zero existing occurrences — this is a pure-tightening change. The global error-handler in `app/middleware/error-handler.js` is the ONE place allowed to echo err.message (under err.expose === true); the test specifically scans `app/controllers` and `app/middleware` so the handler's own file is covered too — its `message = err.message` assignment is NOT a `message: err.message` object-literal shape, so the regex doesn't false-positive there. Co-authored-by: Aaron K. Clark <akclark@thenetwerk.net> Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #139.
Summary
Every controller's try/catch returned
res.status(500).json({ message: "Error!", error: String(error) }). Theerrorfield surfaced raw Sequelize / pg-driver text to clients — internal column names + lengths from "value too long for type character varying(255)", internal hostnames + IPs from connection errors, table/constraint names from unique-violations, etc. The global error-handler inapp/middleware/error-handler.jswas already written to suppress this detail, but the per-controller catches intercepted the error before it could reach that path, silently bypassing the policy across all 17 controllers (137 occurrences).Strip
, error: String(error)from every 5xx return. Controllers still log the full error vialog.error({ err: error }, …)so operator-side signal is unchanged; only the client-facing body loses the leak. One static-string variant incustomercontroller.js(error: "Sequelize Op not available") removed for the same reason.New
tests/unit/controller-error-shape.test.jsdoes a source-level regression pin viatest.each()over every controller file. Future copy-paste of the old shape fails CI immediately.Test plan
npm run lint— cleannpm test— 515 passed (was 495 + 20 new structural assertions: 1 sanity-floor + 19 per-controller), 15 skippedmessage, never the removederrorfieldProudly Made in Nebraska. Go Big Red! 🌽 https://xkcd.com/2347/