Redact RSC rendering error details from client DOM in production#4631
Redact RSC rendering error details from client DOM in production#4631AbanoubGhadban wants to merge 8 commits into
Conversation
Greptile SummaryThis PR redacts RSC render error details from the client DOM in production. The main changes are:
Confidence Score: 4/5The standard streaming path looks correct, but the exported injection helper can still expose production error details when the new option is omitted.
packages/react-on-rails-pro/src/injectRSCPayload.ts
|
| Filename | Overview |
|---|---|
| packages/react-on-rails-pro/src/injectRSCPayload.ts | Adds production redaction at the diagnostic script boundary, but leaves omitted railsEnv on the verbose path. |
| packages/react-on-rails-pro/src/streamServerRenderedReactComponent.ts | Passes railsContext.railsEnv into RSC payload injection for the standard streaming render path. |
| packages/react-on-rails-pro/tests/injectRSCPayload.test.ts | Adds coverage for production redaction, development diagnostics, and legacy missing-env behavior. |
Reviews (1): Last reviewed commit: "Redact RSC rendering error details from ..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
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:
WalkthroughRSC stream errors now propagate through a buffered ChangesRSC error propagation and reporting
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant streamServerRenderedComponent
participant StreamingUtils
participant runInVM
participant ErrorReporter
participant injectRSCPayload
streamServerRenderedComponent->>StreamingUtils: notifyRenderingError(error)
StreamingUtils->>runInVM: emit renderingError
runInVM->>ErrorReporter: report formatted error once
streamServerRenderedComponent->>injectRSCPayload: pass railsContext.railsEnv
injectRSCPayload->>injectRSCPayload: redact renderingError outside development and test
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: baf9c60644
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Code ReviewOverviewFixes #4629 by redacting the RSC diagnostic script's Strengths
Main concern: denylist env check (see inline comment)The redaction gate is Minor
Other
|
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-on-rails-pro/tests/injectRSCPayload.test.ts (1)
471-493: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the edge case where
hasErrorsis nottruebutrenderingErrorhas a signal.The production test only covers
hasErrors: true. The early return increateRSCDiagnosticScript(line 108) also proceeds whenhasRenderingErrorSignal(renderingError)is true even ifhasErrorsis nottrue. A test for that path would verify the client still receiveshasErrors: truein production and thatrenderingErrordetails are redacted.🧪 Suggested additional test
it('redacts renderingError and signals hasErrors:true in production even when hasErrors is not true', async () => { const mockRSC = createMockRSCStreamWithMetadata('{"test": "data"}', { hasErrors: false, renderingError: { message: 'Internal server failure', stack: 'Error: Internal server failure\n at Server (/app/lib/server.ts:42:7)', }, }); const mockHTML = createMockHTMLStream(['<html><body><div>Hello, world!</div></body></html>']); const { rscRequestTracker, domNodeId } = setupTest(mockRSC); const result = injectRSCPayload(mockHTML, rscRequestTracker, domNodeId, undefined, { railsEnv: 'production', }); const resultStr = await collectStreamData(result); expect(resultStr).toContain('"hasErrors":true'); expect(resultStr).not.toContain('Internal server failure'); expect(resultStr).not.toContain('server.ts'); expect(resultStr).not.toContain('renderingError'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/react-on-rails-pro/tests/injectRSCPayload.test.ts` around lines 471 - 493, Add a production test alongside the existing injectRSCPayload diagnostic tests using hasErrors: false with a populated renderingError. Verify the collected result still reports hasErrors as true while redacting the rendering error message, stack details, and renderingError field.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-on-rails-pro/src/injectRSCPayload.ts`:
- Line 111: Update the production branch of the clientPayload construction in
injectRSCPayload so it always emits hasErrors: true whenever the early-return
path is reached, including renderingError signal cases; preserve the
non-production payload’s existing hasErrors and renderingError values.
---
Nitpick comments:
In `@packages/react-on-rails-pro/tests/injectRSCPayload.test.ts`:
- Around line 471-493: Add a production test alongside the existing
injectRSCPayload diagnostic tests using hasErrors: false with a populated
renderingError. Verify the collected result still reports hasErrors as true
while redacting the rendering error message, stack details, and renderingError
field.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 05958d85-254a-4e62-b870-8444eafd6601
📒 Files selected for processing (3)
packages/react-on-rails-pro/src/injectRSCPayload.tspackages/react-on-rails-pro/src/streamServerRenderedReactComponent.tspackages/react-on-rails-pro/tests/injectRSCPayload.test.ts
|
All review feedback addressed in af56d88:
|
|
Review: Redact RSC rendering error details from client DOM in production Overview Code quality Issues found (posted inline) Nits (not blocking) Security Overall: solid, well-motivated fix. Main ask is closing the test-coverage gap on the error-reporting side before merge. |
|
All issues from this review are addressed in later commits:
|
ReviewReviewed the diff (redaction gate in Design is sound overall:
Two things worth a look (posted as inline comments):
No other correctness, performance, or security issues found in the diff. |
Review: #4631 — Redact RSC rendering error details from client DOM in productionOverviewThis PR closes two real gaps around RSC render errors:
The Main concern: the fix doesn't cover the other place the same data leaks
Concretely, in production:
So today, a production render error still lands the full server error message + stack trace directly in the page's HTML source (not just JS-readable via a script tag) — arguably a more severe version of the exact issue this PR sets out to fix (#4629), just via a different code path that predates this PR and wasn't touched by it. Given the PR title/description explicitly frames this as closing the "leak full server message and stack into client DOM" issue, I'd flag this as a gap worth addressing (either in this PR or as an immediate fast-follow) — e.g. apply the same Other notes (also left inline)
Nice-to-have but not blocking: the CHANGELOG/PR description says "delete 1 redundant backwards-compat test" — confirmed in the diff, no concerns there. |
Review: Redact RSC rendering error details from client DOM in production (#4631)OverviewTwo fixes bundled together:
What's solid
Issues found (see inline comments)
Other notes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 534a919ef7
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Review: Redact RSC rendering error details from client DOM in production Overview
The redaction logic ( Likely test-suite regression (please verify before merge) it("shouldn't notify error reporter when throwJsErrors is false and shell error happens", ...)
expect(errorReporter.message).not.toHaveBeenCalled();
it('should not notify error reporter when throwJsErrors is false and async error happens', ...)
expect(errorReporter.message).not.toHaveBeenCalled();Both use the The PR's test-plan checklist references '141 streaming tests / 5 suites' and '23 RSC tests / 7 suites' in Minor
Security |
In production, the `REACT_ON_RAILS_RSC_ERRORS` inline script now emits
only `{ hasErrors: true }` instead of the full `renderingError` object
(message + stack trace with source-mapped file paths). Full diagnostics
remain available server-side via the `onDiagnosticError` callback
(Sentry/Honeybadger), and the wire protocol to Ruby is unchanged so
`raise_prerender_error` still receives the complete error.
The gate is at `createRSCDiagnosticScript` — the single DOM injection
boundary — keyed on `railsEnv === 'production'`. All other environments
(development, test, staging, undefined) continue to receive full error
details for debugging. The client-side `buildRSCStreamDiagnosticError`
already handles the absence of `renderingError` gracefully, producing a
generic fallback message.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
With default config (throwJsErrors: false), RSC rendering errors were silently swallowed — never reaching error reporters like Sentry. Add a custom 'renderingError' stream event that the node renderer listens for, avoiding the standard 'error' event which would break stream consumers like text(). Uses WeakSet dedup so errors are reported exactly once even when throwJsErrors is true. Also removes redundant backwards-compat test and fixes misleading onDiagnosticError comment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Flip from denylist (=== 'production') to allowlist: only development/test show full diagnostics; all other explicit envs (staging, uat, etc.) redact. Undefined railsEnv preserves full diagnostics for backwards compatibility. - Force hasErrors:true in redacted payload (was passing through the destructured value, which could be false when renderingError signal exists) - Add tests for staging env redaction and hasErrors:false edge case - Add CHANGELOG entry for the fix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Test that the custom 'renderingError' stream event is emitted when throwJsErrors is false (both shell and async error paths), and that the standard 'error' event is used instead when throwJsErrors is true. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
For a security gate, unknown/missing railsEnv should default to redacted (safe) rather than full diagnostics (leak risk). Only explicitly development/test envs now show full error details. Updated pre-existing tests that relied on undefined railsEnv to pass railsEnv: 'test' explicitly. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
534a919 to
daff2b5
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daff2b5789
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Review summarySolid, well-scoped fix for a real security issue (production RSC render errors leaking server message/stack into client DOM) plus a genuine observability gap (silent errors when What's good
Suggestions (posted inline)
Risk assessmentNo correctness bugs or security regressions found. The behavior change (redact-by-default for any non-development/test |
…observability Addresses the observability half of #4629 (PR #4631) that CI can't cover (node-renderer jobs are skipped on this PR). - vm.ts: report stream errors with a realm-agnostic native-Error check (util.types.isNativeError + duck-type) instead of `instanceof Error`. A stream error thrown inside the sandboxed VM realm fails the worker-realm instanceof check, so it was wrapped in `new Error(String(error))`, discarding the original message and stack before reaching the error reporter. Genuine Errors (including VM-realm ones) now pass through with their message+stack intact; only true non-Error throwables are wrapped. (codex P2) - tests/vm.test.ts: add reception-side coverage for the 'renderingError' listener + WeakSet dedup — the reporter fires on a 'renderingError' stream event, a genuine VM-realm Error keeps its message/stack (not wrapped), the same Error instance is deduped across 'renderingError' and 'error' (reported once), and two distinct Errors are both reported. - tests/htmlStreaming.test.js: the two throwJsErrors:false cases previously asserted the reporter was NOT called. Built the dummy server bundle from current source and confirmed empirically that the reporter is now called with the genuine render error (the intended fix). Updated both to assert the reporter IS called with the real error, with a comment explaining the behavior change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…sErrors differently Review nit: the redacted (production) branch forces `hasErrors: true` while the development/test branch passes the raw metadata through. Add a comment explaining the intent — production emits a positive generic signal even when only `hasRenderingErrorSignal` tripped; dev/test preserves the exact historical payload shape for debugging — since this function is the security gate. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
+ci-force-full Requesting the full hosted suite: the standard optimized run skips the node-renderer jobs ( |
Force-Full Hosted CI RequestedTriggered 9 workflow(s) for View progress in the Actions tab. |
github-actions
Bot
added
force-full-hosted-ci
Review: Redact RSC rendering error details from client DOM in productionOverviewThis PR fixes two real issues around RSC render errors:
Both fixes are well-targeted and the changes are consistent across the touched files. Correctness
Security
Test Coverage
Minor / Nitpick
OverallSolid, well-scoped security + observability fix with good test coverage. No blocking issues found. |
Review: Redact RSC rendering error details from client DOM in productionOverall this is a well-structured, well-tested security fix. The redaction gate lives exactly at the DOM-injection boundary (
Nice touches
Minor observations (non-blocking)
Test coverageCoverage looks solid — production/staging/undefined redaction, development/test full diagnostics, the No correctness bugs or security gaps found in the diff as submitted. |
2 participants
Summary
Fixes #4629 — Two issues with RSC render errors in production:
messageand source-mappedstackinto client DOM viaREACT_ON_RAILS_RSC_ERRORSinline<script>tagsthrowJsErrors: false), rendering errors never reached error reporters (Sentry/Honeybadger)Production error redaction
createRSCDiagnosticScript): outside development/test, emit only{ hasErrors: true }— nomessage, nostack, no file pathsdevelopmentortestenvs show full diagnostics; all other envs (production, staging, uat, undefined) redact by default — safe for a security gatebuildRenderMetadatastill sends the fullrenderingErrorto Ruby soraise_prerender_errorand server-side error handling work as beforeError reporting via custom stream event
emitError()emits standard Node.js'error'events whichtext()fromstream/consumersrejects on — can't just remove thethrowJsErrorsgate'renderingError'stream event that standard consumers (text(),pipe(), async iterators) ignore, but the node renderer listens forWeakSet<Error>in vm.ts ensures same error is reported exactly once, even whenthrowJsErrors: truecauses both events to firenotifyRenderingErrormirrorsemitError's abort guard — no reporting after client disconnectChanges
streamingUtils.tsnotifyRenderingErrortobufferStream()andtransformRenderStreamChunksToResultObject()— buffers custom events before reading starts, wraps with consumer-abort guardstreamServerRenderedReactComponent.tsrailsContext.railsEnvintoinjectRSCPayloadoptions; callnotifyRenderingErrorinreportErrorwhen!throwJsErrorsproRSC.tsnotifyRenderingErrorinreportErrorwhen!throwJsErrors; remove misleadingconsole.error(Sentry doesn't capture console)vm.ts(node renderer)'renderingError'on raw stream; deduplicate withWeakSet<Error>againsthandleStreamError's'error'handlerinjectRSCPayload.tsrailsEnvparam; fail-closed allowlist (development/test show full diagnostics, everything else redacts)injectRSCPayload.test.tshasErrors:falseedge casestreamServerRenderedReactComponent.test.jsxrenderingErroremission on shell/async errors withthrowJsErrors:false, no emission whenthrowJsErrors:trueCHANGELOG.md[Unreleased] > FixedTest plan
Release / cherry-pick
This is a security fix (production RSC render errors leaking the server error
message and source-mapped stack — including file paths and potentially user data —
into the client DOM). Candidate for cherry-pick into the
17.0.1patch release.The behavior change is intentional security hardening: setups that never set
railsEnvnow redact by default (fail-closed allowlist — full diagnostics only forexplicit
development/test).🤖 Generated with Claude Code
Summary by CodeRabbit