Skip to content

UID2-7292: prevent optout crash on malformed payload#189

Merged
mcollins-ttd merged 1 commit into
mainfrom
mkc-UID2-7292-optout-crash-fix
Jun 13, 2026
Merged

UID2-7292: prevent optout crash on malformed payload#189
mcollins-ttd merged 1 commit into
mainfrom
mkc-UID2-7292-optout-crash-fix

Conversation

@mcollins-ttd

Copy link
Copy Markdown
Contributor

Why

A malformed payload to POST / (step=optout_submit) was crashing the Node process. Two distinct paths could escape as unhandled rejections / uncaught exceptions:

  1. decrypt() in src/routes/encryption.ts. crypto.scrypt's native callback contained synchronous code (createDecipheriv, decipher.write, decipher.end) sitting outside the Promise's try scope. A throw from any of those — most notably createDecipheriv rejecting an IV of the wrong length — escaped the Promise as an uncaught exception. The decipher stream's 'error' event (bad padding from a wrong key or junk ciphertext) was also unobserved.
  2. handleOptoutSubmit in src/routes/index.ts. OptoutSubmitRequest.parse(req.body) threw a ZodError directly inside the async route handler. With Express 4 having no native async-error forwarding, that throw became an unhandled rejection.

What changed

src/routes/encryption.ts

  • Reject a bad IV length (!== 16) synchronously before entering the new Promise(...) body, so the error surfaces as a normal Promise rejection the caller can try/catch.
  • Wrap the createDecipheriv + stream setup inside the scrypt callback in try/catch, routing any throw through reject.
  • Subscribe to the decipher's 'error' event so async failures (bad padding) also reject the Promise instead of going unobserved.

src/routes/index.ts

  • Wrap OptoutSubmitRequest.parse(req.body) in try/catch; on failure, log via errorLogger and respond with 400 via next(createError(400)).

src/bin/www.ts

  • Add a diagnostic-only unhandledRejection listener that logs via errorLogger (so any future regression at least surfaces in monitoring).
  • No uncaughtException handler. Node's docs are explicit that resuming after one leaves the process in undefined state; the pod supervisor (k8s) is the right restart mechanism.

Follow-up (separate PR)

The file-level try/catch + next(createError(400)) pattern is now duplicated three times in src/routes/index.ts (email_prompt, optout_submit, defaultRouteHandler). The right idiomatic fix is a centralized Express error middleware + a small validateBody(schema) middleware (and ideally an Express 4 → 5 upgrade so async handler rejections forward natively). Out of scope for a crash-fix PR; will follow up.

Test plan

  • CI build (yarn build) and lint pass
  • yarn test passes locally
  • Manual: POST malformed JSON to / → 400, no crash
  • Manual: POST { step: 'optout_submit', encrypted: 'garbage' } (wrong format) → friendly error page, no crash
  • Manual: POST with IV slot that decodes to ≠16 bytes → friendly error page, no crash
  • Manual: POST with valid IV but corrupted ciphertext (bad padding) → friendly error page, no crash, decipher 'error' logged
  • Manual: happy path optout still succeeds

A throw inside the crypto.scrypt native callback in decrypt() escaped
the Promise, and a ZodError from the async handler's parse() became an
unhandled rejection -- either could take the process down. Fix both at
source: synchronous IV-length check + try/catch around the decipher
setup + 'error' listener on the decipher stream; parse() wrapped in
try/catch returning 400 via next(createError(400)).

Also add a diagnostic-only unhandledRejection listener logging via
errorLogger so any future regression surfaces in monitoring. No
uncaughtException handler -- Node's docs warn that resuming after one
leaves the process in undefined state; the k8s pod supervisor restarts
cleanly instead.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@mcollins-ttd
mcollins-ttd merged commit 9a8b56e into main Jun 13, 2026
1 of 4 checks passed
@mcollins-ttd
mcollins-ttd deleted the mkc-UID2-7292-optout-crash-fix branch June 13, 2026 02:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants