fix(rivetkit): keep internal error exposure behavior consistent#4661
fix(rivetkit): keep internal error exposure behavior consistent#4661NathanFlurry wants to merge 1 commit intobreak-up/stabilize-actor-db-testsfrom
Conversation
Code Review: fix(rivetkit): keep internal error exposure behavior consistentOverviewThis PR removes the NODE_ENV=development shortcut from getRequestExposeInternalError in two files, making RIVET_EXPOSE_ERRORS=1 the sole mechanism for exposing internal errors to clients. It also fixes a bug where the isolate sandbox unconditionally forwarded RIVET_EXPOSE_ERRORS=1 to child processes, now only forwarding the env var when explicitly set in the parent. What the PR Does Well
Issues1. Incomplete fix: old.ts still checks NODE_ENV=developmentFile: rivetkit-typescript/packages/rivetkit/src/actor/protocol/old.ts:303-304 This file was not updated, so the behavior is still inconsistent. In the legacy protocol path, internal errors are still exposed whenever NODE_ENV=development, even after this PR. If the goal is consistent behavior, this call site needs the same treatment. 2. Conditional spread copies the raw env value, not normalised "1"getRequestExposeInternalError only returns true when the value is exactly "1". If an operator sets RIVET_EXPOSE_ERRORS=true (or any other truthy string), the value is forwarded into the sandbox but the check inside the sandbox evaluates to false. This could cause the parent and child processes to disagree on whether errors are exposed. Consider normalising to "1" when forwarding: 3. Breaking change for local development workflowsRemoving the NODE_ENV=development shortcut means developers who relied on automatic error detail exposure in local dev environments now need to explicitly set RIVET_EXPOSE_ERRORS=1. This is a reasonable tradeoff for security consistency, but the PR description does not document this as a behaviour change and no docs update is present. Summary
The core security fix (removing the unconditional RIVET_EXPOSE_ERRORS=1 injection into the sandbox) is correct and well-structured. The main outstanding item before merging is updating old.ts to match, otherwise the stated goal of consistent behavior is not fully achieved. |
309ad25 to
b090c6c
Compare
dcb82e8 to
9c1861c
Compare
b090c6c to
ae2a18f
Compare
9c1861c to
8a0be32
Compare
ae2a18f to
feb2f40
Compare
8a0be32 to
f676d13
Compare

Description
Please include a summary of the changes and the related issue. Please also include relevant motivation and context.
Type of change
How Has This Been Tested?
Please describe the tests that you ran to verify your changes.
Checklist: