fix(webapp): catch loader/action throws before Remix serializes them#3664
fix(webapp): catch loader/action throws before Remix serializes them#3664d-cs wants to merge 5 commits into
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.
|
WalkthroughThis PR applies a systematic error-handling pattern across approximately 30 API routes in the webapp. Each route's loader or action handler is wrapped in try/catch to prevent unexpected errors from propagating into Remix's default 500 serialization. Response errors are rethrown unchanged to preserve auth helper behavior ( Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 docstrings
🧪 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.
|
|
||
| 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 `{ error: "Internal Server Error" }` 500. For 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), an outer try/catch is 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 was verified locally with a synthetic-throw probe: inject `throw new Error("SYNTHETIC ...")` at the top of the catch'd try, curl the route with a dummy bearer token, confirm the response body is the generic shape and that the synthetic message is captured server-side via `logger.error`. |
There was a problem hiding this comment.
The .server-changes files should be more like a high-level 1 or 2 sentence summary of the change. Imagine that it will eventually become a single item in the list of changes in a changelog. For example, this one should just be something like "Expands better API error messages across additional API endpoints" or something like that.
| type: fix | ||
| --- | ||
|
|
||
| Wrap two loaders/actions that previously let thrown errors propagate to Remix's default 500 serializer, which writes `error.message` into the response body. When the underlying call (Prisma, etc.) fails, the raw error string was reaching API consumers — including the SDK, which surfaces it back to users via `TriggerApiError`. Each handler now catches non-Response errors, logs server-side, and returns a generic 500 body. `throw json(...)` / `throw redirect(...)` from auth helpers is re-thrown unchanged. |
There was a problem hiding this comment.
Definitely no need for two .server-changes files in this pr
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