Skip to content

Avoid caching async props prerender streams - #4376

Merged
justin808 merged 5 commits into
mainfrom
jg-codex/batch-a-pro-cache-4359
Jul 2, 2026
Merged

Avoid caching async props prerender streams#4376
justin808 merged 5 commits into
mainfrom
jg-codex/batch-a-pro-cache-4359

Conversation

@justin808

@justin808 justin808 commented Jul 2, 2026

Copy link
Copy Markdown
Member

Summary

  • bypass Pro prerender stream caching whenever a render uses an async props block
  • add a regression spec proving per-request async stream output is not replayed from another request
  • document the release/runtime fix in the changelog

Rationale

Issue #4359 is a release-blocking security/cache-isolation bug: async props can emit user-specific data after the initial render path, so reusing a prerender stream cache entry can replay another request's async content. This PR keeps static prerender stream caching intact but excludes async-props renders from that cache path.

Fixes #4359.

Tests

  • BUNDLE_GEMFILE=react_on_rails_pro/Gemfile bundle exec rspec react_on_rails_pro/spec/react_on_rails_pro/server_rendering_pool/pro_rendering_spec.rb
  • cd react_on_rails_pro && BUNDLE_GEMFILE=../Gemfile bundle exec rubocop --ignore-parent-exclusion
  • script/check-pro-license-headers
  • pnpm exec prettier --check CHANGELOG.md
  • git diff --check origin/main...HEAD
  • codex review --base origin/main (no findings)

Codex Decision Log

  • Non-blocking: Whether to try to cache async-props prerender streams with a richer cache key.
    • Decision: Disable prerender stream caching when async_props_block is present.
    • Why: The async block can encode request/user state that is not part of the current stream cache key, and the release-blocking risk is cross-user replay.
    • Review later: A future design could add explicit async-props-safe cache keys.

Batch A

  • Workflow config: UNKNOWN repo-local .agents/agent-workflow.yml and .agents/workflows/pr-processing.md absent; used installed PR-processing workflow.
  • QA lane: required, owner qa/batch-a, pending final batch QA.
  • Changelog: included in CHANGELOG.md.

@coderabbitai

coderabbitai Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

@justin808, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 20 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 4a1ec23c-eb2a-480b-8261-3fe859e68e78

📥 Commits

Reviewing files that changed from the base of the PR and between a6c5867 and 939eed6.

📒 Files selected for processing (3)
  • CHANGELOG.md
  • react_on_rails_pro/lib/react_on_rails_pro/server_rendering_pool/pro_rendering.rb
  • react_on_rails_pro/spec/react_on_rails_pro/server_rendering_pool/pro_rendering_spec.rb
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch jg-codex/batch-a-pro-cache-4359

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review Summary

This is a small, well-scoped security fix. Verified the change end-to-end:

Correctness of the fix

  • cache_enabled_for? now also bypasses caching when render_options.internal_option(:async_props_block) is set (pro_rendering.rb:71).
  • Confirmed async_props_block is only ever set via stream_react_component_with_async_props / rsc_payload_react_component_with_async_props, both of which force streaming? — so the fix correctly targets render_streaming_with_cache, the path that persists the entire chunk stream (including any per-request data emitted by the async props block) via StreamCache::CachingComponent#each_chunk. That's the actual leak: a second request hitting the same cache key (derived only from the static shell JS digest) would previously replay the first request's async/user-specific chunks.
  • The new spec ("does not replay a cached stream when async props can emit per-request data") directly reproduces the pre-fix vulnerability (two requests, two different async payloads) and asserts each gets its own render. Good regression coverage.
  • No regressions to the existing cache paths (skip_prerender_cache, non-streaming cache, RORP_CACHE_KEY/HIT metadata).

Left an inline note on a related, currently-unreachable edge case: the explicit fragment-style cache (cached_stream_react_componentRails.cache.write in react_on_rails_pro_helper.rb) is a separate caching mechanism not touched by this fix. It can't currently be combined with async_props_block through public APIs, so it's out of scope today, but there's no explicit guard preventing that combination if a future helper wires them together. Worth a follow-up/tracking note.

Minor / non-blocking

Overall: correct, minimal, appropriately conservative (disables caching rather than trying to build a safe-but-complex cache key), and backed by a test that actually proves the bug is fixed.

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review

This PR bypasses the Pro prerender stream cache (ProRendering.cache_enabled_for? / ReactOnRailsPro::StreamCache) whenever render_options.internal_option(:async_props_block) is present, plus adds a regression spec proving that path no longer replays a cached stream across two renders. The mechanism itself is sound: async-emitted content isn't part of the request-digest cache key, so caching it was a real cross-request data leak.

The fix is incomplete — the same class of bug is still reachable through the higher-level fragment-cache helpers

react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb has a second, independent caching layer that this PR doesn't touch:

  • cached_stream_react_componentfetch_stream_react_component (react_on_rails_pro_helper.rb:419-440) always sets skip_prerender_cache: true on the options it passes down (see render_stream_component_with_props, ~line 496), so it was never protected by ProRendering.cache_enabled_for? in the first place — this PR's new async_props_block check is a no-op for this path. Instead, handle_stream_cache_miss (react_on_rails_pro_helper.rb:466-490) collects every streamed chunk into all_chunks via consumer_stream_async's on_complete callback and writes it straight to Rails.cache.write(view_cache_key, chunks, cache_options) (line 475), keyed only by the developer-supplied cache_key: (ReactOnRailsPro::Cache.react_component_cache_key) — with no relation to per-request async content.
  • check_caching_options! (react_on_rails_pro_helper.rb:503) only validates that :props isn't passed and that :cache_key is present. Nothing rejects async_props_block: being passed through raw_options, unlike cached_buffered_stream_react_component, which explicitly raises if on_complete is supplied (lines 318-322) but has no equivalent guard for async_props_block either, and shares the same fetch_react_component/Rails.cache.fetch write-through pattern.

Concretely: cached_stream_react_component("Dashboard", cache_key: "dashboard", async_props_block: ->(emit) { emit.call(:currentUser, current_user.email) }) { props } will still cache the first request's async-emitted currentUser chunk under the "dashboard" cache key and replay it verbatim to every subsequent visitor until the entry expires — the exact cross-request replay scenario described in #4359, just one layer up from the code this PR changes. There's no test in pro_rendering_spec.rb or react_on_rails_pro_helper_spec.rb exercising async_props_block together with either cached_stream_react_component or cached_buffered_stream_react_component.

Given the PR is described as release-blocking for a cache-isolation security bug, I'd suggest either extending check_caching_options! to raise when raw_options[:async_props_block] is present (mirroring the existing on_complete guard), or explicitly documenting/scoping this PR as only closing the internal ProRendering/StreamCache path and filing a fast-follow for the helper-level fragment cache.

Minor: dead test additions

react_on_rails_pro/spec/react_on_rails_pro/server_rendering_pool/pro_rendering_spec.rb:173-179 adds read/write methods to the fake cache_store, but the new async-props spec (and every other spec in the file) never exercises them — cache_enabled_for? returns false for the async-props case, so execution never reaches ReactOnRailsPro::StreamCache/Rails.cache.read/Rails.cache.write at all. Fine to drop unless there's a follow-up test planned that needs them.


🤖 Automated review

@justin808

Copy link
Copy Markdown
Member Author

Address-review summary

Scan scope: full current-head unresolved review threads as of 2026-07-02T08:00Z.

Mattered

  • None.

Optional

  • Fixed inline: removed unused fake cache read/write helpers in 8d160219d.
  • Auto-deferred: future defensive guard for an explicit fragment stream-cache + async-props combination that is not reachable through documented public helpers today.

Skipped

  • None.

Next default scan starts after this comment. Say check all reviews to rescan the full PR.

…cache-4359

* origin/main:
  Fetch remote state during release-finish dry runs (#4382)
  Fix stale incremental stream timeouts (#4354)
  Fix Pro RSC preload replay after eviction (#4353)
@justin808

Copy link
Copy Markdown
Member Author

+ci-run-hosted

@justin808 justin808 added the ready-for-hosted-ci Run optimized hosted GitHub CI for this PR label Jul 2, 2026
@github-actions

github-actions Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Hosted CI Requested

Triggered 9 workflow(s) for 7ca5d1cc9100.
Mode: optimized hosted CI (path-selected by script/ci-changes-detector).
Added ready-for-hosted-ci, so future commits will keep running optimized hosted CI until +ci-stop-hosted is used.

View progress in the Actions tab.

…cache-4359

* origin/main:
  Guard preload links for older Shakapacker (#4377)
  Document runtime RC blocker fixes (#4373)
  Warn on undetermined RSC Rspack boot validation (#4355)
  Fail fast when local benchmark server exits (#4380)
  Preserve agent-coord-bounded output on bounded exits (#4381)

# Conflicts:
#	CHANGELOG.md
@justin808
justin808 marked this pull request as ready for review July 2, 2026 10:07
@greptile-apps

greptile-apps Bot commented Jul 2, 2026

Copy link
Copy Markdown

Greptile Summary

This PR prevents async-props stream output from being reused across cached prerenders. The main changes are:

  • Pro prerender caching now skips renders with an async_props_block.
  • The streaming regression spec checks that two async-props renders produce separate stream output.
  • The changelog documents the cache-isolation fix.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The cache guard uses the same internal async-props option that the rendering path reads.
  • The added spec covers the cross-request stream replay case described by the fix.

Important Files Changed

Filename Overview
react_on_rails_pro/lib/react_on_rails_pro/server_rendering_pool/pro_rendering.rb Adds an async-props check to prerender cache eligibility so async stream renders bypass cache reuse.
react_on_rails_pro/spec/react_on_rails_pro/server_rendering_pool/pro_rendering_spec.rb Adds a regression spec proving async-props streaming renders are executed separately instead of replayed from cache.
CHANGELOG.md Documents the Pro async-props prerender stream cache-isolation fix.

Reviews (1): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Review: Avoid caching async props prerender streams (#4376)

Summary of change: Adds a check in ProRendering.cache_enabled_for? so that when render_options.internal_option(:async_props_block) is present, the low-level Pro prerender stream cache (ReactOnRailsPro::StreamCache) is bypassed entirely — closing the cross-user replay bug from #4359. A regression spec and changelog entry are included. I traced async_props_block through node_rendering_pool.rb, request.rb, and the helper entry points (stream_react_component_with_async_props, rsc_payload_react_component_with_async_props) and confirmed the logic is correct for that specific cache layer: internal_option is a plain hash lookup, .nil? correctly gates on presence, and async props are only ever wired up for streaming renders, so the added check is a no-op (harmless) for non-streaming renders.

Findings

react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb:419-464 — The same cache-replay vulnerability class is unpatched in the app-level fragment-cache path.

This PR only patches ProRendering.cache_enabled_for?, which guards ReactOnRailsPro::StreamCache (used by render_streaming_with_cache). It does not touch the separate, sibling fragment-cache layer built on Rails.cache directly: cached_stream_react_componentfetch_stream_react_componenthandle_stream_cache_hit, plus fetch_react_component and fetch_async_react_component.

None of these consult async_props_block (or skip_prerender_cache) before serving a cache hit:

  • check_caching_options! (helper.rb:503-509) only validates :props and :cache_key.
  • ReactOnRailsPro::Cache.use_cache? (cache.rb:169-177) only checks :if/:unless.
  • react_component_cache_key (cache.rb:206-221) never folds async_props_block into the key.
  • On a cache HIT, fetch_stream_react_component (helper.rb:419-440) calls handle_stream_cache_hit, which replays the previously cached chunks without ever invoking the block — the block (and any async_props_block inside raw_options) is only invoked on a cache miss, via render_stream_component_with_props's props = yield (helper.rb:493).

Concretely: cached_stream_react_component(name, options.merge(async_props_block: proc, cache_key: "k"), &props_block) on a second request with the same cache_key returns the first request's cached async-emitted chunks verbatim — the identical bug class as #4359, just at a different layer. This combination isn't the documented API surface (stream_react_component_with_async_props doesn't go through the fragment-cache helpers today), and there's no spec exercising it, but nothing prevents a caller from passing async_props_block alongside cache_key, and the PR title/changelog ("Async-props prerender stream cache isolation") reads as a general guarantee rather than one scoped to a single cache layer. Given this PR is closing a release-blocking security issue, worth deciding explicitly whether to (a) add the same guard to the fragment-cache helpers, or (b) document that async_props_block must never be combined with the cached_* fragment-caching helpers, and ideally add a regression spec/guard clause that raises if both are supplied together.

Minor (test coverage)

react_on_rails_pro/spec/react_on_rails_pro/server_rendering_pool/pro_rendering_spec.rb:318-337 — the new "does not replay a cached stream when async props can emit per-request data" spec never stubs ReactOnRailsPro::StreamCache.fetch_stream/wrap_and_cache, so it doesn't directly assert that StreamCache is bypassed; it only asserts pool.exec_server_render_js is called twice. That's still a valid regression test (it would fail pre-fix, since StreamCache.fetch_stream would hit Rails.cache.read, which the test's cache_store double doesn't implement), but a more direct assertion (e.g. expect(ReactOnRailsPro::StreamCache).not_to have_received(:fetch_stream)) would make the intent explicit and decouple the test from that incidental double gap.

Everything else (changelog formatting/placement, the cache_enabled_for? logic itself, and the streaming/non-streaming code paths touched) looks correct and consistent with the surrounding code.

@justin808
justin808 added this pull request to the merge queue Jul 2, 2026
Merged via the queue into main with commit cdcba54 Jul 2, 2026
71 of 81 checks passed
@justin808
justin808 deleted the jg-codex/batch-a-pro-cache-4359 branch July 2, 2026 15:53
justin808 added a commit that referenced this pull request Jul 2, 2026
…nsport

* origin/main:
  Avoid caching async props prerender streams (#4376)
justin808 added a commit that referenced this pull request Jul 2, 2026
…derer-shutdown-restart

* origin/main:
  Add mechanical parity guards for Ruby↔TS protocol constants (#4412) (#4427)
  Move Node tsconfigs from @tsconfig/node14 to @tsconfig/node18 (#4410) (#4429)
  [Pro] Remove unused addressable and rainbow runtime deps from gemspec (#4416) (#4422)
  Delete finished #3313 Prism Gemfile-rewriter spike (#4421)
  Extract generator scan/tracking helpers (#4405) (#4430)
  Extract install_dependency_group helper in JsDependencyManager (#4403) (#4424)
  Remove obsolete Ruby<2.6 YAML-aliases capability shim (#4417) (#4428)
  Remove inert config.server_render_method option (#4415) (#4423)
  Prune stale knip ignores and enforce binaries in CI (#4408) (#4425)
  Extract shared redux_store kwargs validator (#4402) (#4420)
  Avoid caching async props prerender streams (#4376)
  Release incremental render context on setup failure (#4383)
  Optimize response type emitter snapshots (#4397)
  Skip generated stylesheet metadata for OSS renders (#4395)
  Avoid mutating render option inputs (#4396)
  Changelog: document PR 4282 registry cleanup (#4399)

# Conflicts:
#	CHANGELOG.md
justin808 added a commit that referenced this pull request Jul 2, 2026
* origin/main:
  Add mechanical parity guards for Ruby↔TS protocol constants (#4412) (#4427)
  Move Node tsconfigs from @tsconfig/node14 to @tsconfig/node18 (#4410) (#4429)
  [Pro] Remove unused addressable and rainbow runtime deps from gemspec (#4416) (#4422)
  Delete finished #3313 Prism Gemfile-rewriter spike (#4421)
  Extract generator scan/tracking helpers (#4405) (#4430)
  Extract install_dependency_group helper in JsDependencyManager (#4403) (#4424)
  Remove obsolete Ruby<2.6 YAML-aliases capability shim (#4417) (#4428)
  Remove inert config.server_render_method option (#4415) (#4423)
  Prune stale knip ignores and enforce binaries in CI (#4408) (#4425)
  Extract shared redux_store kwargs validator (#4402) (#4420)
  Avoid caching async props prerender streams (#4376)
  Release incremental render context on setup failure (#4383)
  Optimize response type emitter snapshots (#4397)
  Skip generated stylesheet metadata for OSS renders (#4395)
  Avoid mutating render option inputs (#4396)
  Changelog: document PR 4282 registry cleanup (#4399)

# Conflicts:
#	react_on_rails_pro/Gemfile.lock
#	react_on_rails_pro/react_on_rails_pro.gemspec
#	react_on_rails_pro/spec/dummy/Gemfile.lock
justin808 added a commit that referenced this pull request Jul 2, 2026
…cache-4317

* origin/main:
  Drop deprecation-tombstone config options for 17.0.0 (#4419) (#4432)
  Add mechanical parity guards for Ruby↔TS protocol constants (#4412) (#4427)
  Move Node tsconfigs from @tsconfig/node14 to @tsconfig/node18 (#4410) (#4429)
  [Pro] Remove unused addressable and rainbow runtime deps from gemspec (#4416) (#4422)
  Delete finished #3313 Prism Gemfile-rewriter spike (#4421)
  Extract generator scan/tracking helpers (#4405) (#4430)
  Extract install_dependency_group helper in JsDependencyManager (#4403) (#4424)
  Remove obsolete Ruby<2.6 YAML-aliases capability shim (#4417) (#4428)
  Remove inert config.server_render_method option (#4415) (#4423)
  Prune stale knip ignores and enforce binaries in CI (#4408) (#4425)
  Extract shared redux_store kwargs validator (#4402) (#4420)
  Avoid caching async props prerender streams (#4376)
  Release incremental render context on setup failure (#4383)
  Optimize response type emitter snapshots (#4397)
  Skip generated stylesheet metadata for OSS renders (#4395)
  Avoid mutating render option inputs (#4396)
  Changelog: document PR 4282 registry cleanup (#4399)
justin808 added a commit that referenced this pull request Jul 3, 2026
…-4364

* origin/main: (24 commits)
  Preserve streaming LoadError during dependency failures (#4388)
  Wire eslint-rules RuleTester suite into a runner, CI, and knip (#4409) (#4433)
  Handle fire-and-forget RSCRoute retry failures (#4378)
  Fix visible hydration cleanup for detached roots (#4374)
  Avoid full locale default obsolete scans (#4398)
  Document RSC public-page validation and sidecar patterns (#4387)
  Remove dead methods; prune always-false Rails<5.0 spec branches (#4418) (#4431)
  Drop deprecation-tombstone config options for 17.0.0 (#4419) (#4432)
  Add mechanical parity guards for Ruby↔TS protocol constants (#4412) (#4427)
  Move Node tsconfigs from @tsconfig/node14 to @tsconfig/node18 (#4410) (#4429)
  [Pro] Remove unused addressable and rainbow runtime deps from gemspec (#4416) (#4422)
  Delete finished #3313 Prism Gemfile-rewriter spike (#4421)
  Extract generator scan/tracking helpers (#4405) (#4430)
  Extract install_dependency_group helper in JsDependencyManager (#4403) (#4424)
  Remove obsolete Ruby<2.6 YAML-aliases capability shim (#4417) (#4428)
  Remove inert config.server_render_method option (#4415) (#4423)
  Prune stale knip ignores and enforce binaries in CI (#4408) (#4425)
  Extract shared redux_store kwargs validator (#4402) (#4420)
  Avoid caching async props prerender streams (#4376)
  Release incremental render context on setup failure (#4383)
  ...

# Conflicts:
#	CHANGELOG.md
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-hosted-ci Run optimized hosted GitHub CI for this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Pro] Prerender stream caching replays async-props content across requests and users (cross-user data leak)

1 participant