fix(webapp): catch loader/action throws before Remix serializes them#3664
Conversation
Two webapp routes left their loader/action bodies uncaught. When the
underlying call (Prisma, etc.) threw, Remix's default error path
serialized `error.message` into the 500 response body, surfacing
implementation detail to API consumers — and via the SDK, to users.
This complements the earlier sweep over `catch (e) { return json({error:
e.message}, 500) }` shapes; that fix could not reach routes which had no
catch in the first place.
Each handler now wraps its body in try/catch, re-throws `Response`
instances so auth helpers' `throw json(...)` / `throw redirect(...)`
pass through unchanged, logs non-Response errors, and returns a generic
body. The polling changelogs widget returns `{ changelogs: [] }` 200
instead of a 500 — degrading silently across a transient blip is better
UX for a 60s-cadence widget, and the leak risk is identical (neither
shape carries the error message).
Covers:
- apps/webapp/app/routes/api.v1.projects.\$projectRef.envvars.\$slug.\$name.ts (loader + action)
- apps/webapp/app/routes/resources.platform-changelogs.tsx (loader)
…leaks
Earlier passes covered routes with leaking `catch (e) { return json({error:
e.message}, 500) }` shapes, and two specific naked routes. This sweep
covers the rest of the API surface that doesn't go through
`createLoaderApiRoute`/`createActionApiRoute` — 26 files across deploy,
projects, orgs, deployments, auth-jwt, artifacts, and alert-channel routes.
Each handler now wraps its body in try/catch, re-throws `Response`
instances so auth helpers' `throw json(...)` / `throw redirect(...)`
pass through unchanged, logs non-Response errors via `logger.error`, and
returns a generic `{ error: "Internal Server Error" }` 500.
Routes that already had an inner try/catch covering a service call but
with auth/parsing outside the try (alertChannels, batches.results,
deployments.finalize, several others) get an outer try/catch added so the
inner typed-error handling is preserved. The api.v1.authorization-code.ts
catch branch was returning `error.message` verbatim — switched to a generic
body.
Each route verified locally with the established synthetic-throw probe:
inject `throw new Error("SYNTHETIC ...")` at the top of the catch'd try,
curl with a dummy bearer, confirm the response body is the generic shape
and that the synthetic message lands server-side via `logger.error`.
Sampled legitimate 4xx paths (no-auth 401s, auth-helper Response throws,
happy 200 returns) across each pattern variant to confirm the wrap does
not interfere with normal control flow.
…e leak The previous sweep collapsed two existing 400 branches to 500 along with the leak-sanitization. Keep the 400 status — the inner branches were catching errors that callers had been informed of via 400, and clients may depend on that. Just replace the leaky `error.message` body with a generic per-route message. - api.v1.orgs.$orgParam.projects.ts (createProject failure): 500 → 400 with `"Failed to create project"`. - api.v1.authorization-code.ts (instanceof Error branch): 500 → 400 with `"Failed to create authorization code"`. Both branches probed locally: synthetic failure forces the path and the response is the documented 400 + generic body.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📜 Recent review details⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
🧰 Additional context used🧠 Learnings (1)📚 Learning: 2026-05-14T14:54:39.095ZApplied to files:
🔇 Additional comments (1)
WalkthroughThis PR adds centralized error handling to 30+ API route handlers across the webapp. Each affected route now wraps its main execution in a Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
d-cs
force-pushed
the
sanitize-loader-action-leaks
branch
from
6336713 to
7529ea8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
2 participants
Summary
Companion to #3536, which patched routes that already had a leaking
catch (e) { return json({error: e.message}, 500) }. That pattern can't reach routes which have no catch in the first place — when those throw, Remix's default error path serializeserror.messageinto the response body, and the SDK then wraps the leaked string asTriggerApiError.Across 28 raw api.v1 loaders/actions plus one dashboard polling endpoint, each handler now:
try { ... } catch (error) { ... }.Responseinstances so auth helpers'throw json(...)/throw redirect(...)pass through unchanged.logger.errorso server-side visibility is preserved.{"error": "Internal Server Error"}500 for raw API routes, or{ changelogs: [] }200 for the polling widget (degrade silently across transient blips; the consumer hook already coped with empty payloads).For six routes where #3536 left an inner try/catch covering only a service call (
alertChannels,batches.results,deployments.finalize,deployments.background-workers,deployments.promote,projects.background-workers): an outer try/catch is added so auth/parsing failures are also sanitized. Inner typed-error handling (ServiceValidationError→ 422 with message, etc.) is preserved exactly.For two routes whose existing catch returned 400 +
error.message(api.v1.authorization-code,api.v1.orgs.\$orgParam.projectsaction): the body is sanitized to a generic per-route string. Status code stays 400 — clients that key on the 4xx/5xx distinction (and the SDK's no-retry-on-4xx behavior) are unaffected.Test plan