Add CSP nonce support for consoleReplay script #2059
Conversation
|
Warning Rate limit exceeded@justin808 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 16 minutes and 20 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Code Review: CSP Nonce Support for consoleReplay ScriptThank you for this PR! Overall, this is a well-thought-out implementation that addresses CSP violations for the console replay script. Here's my comprehensive review: ✅ Strengths
🔍 Issues & Recommendations1. Critical: Inconsistency in
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
lib/react_on_rails/helper.rb(5 hunks)packages/react-on-rails/src/RenderUtils.ts(1 hunks)packages/react-on-rails/src/base/client.ts(2 hunks)packages/react-on-rails/src/buildConsoleReplay.ts(2 hunks)packages/react-on-rails/src/types/index.ts(1 hunks)react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb(1 hunks)
🧰 Additional context used
🧠 Learnings (12)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rblib/react_on_rails/helper.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rbpackages/react-on-rails/src/types/index.tspackages/react-on-rails/src/buildConsoleReplay.tslib/react_on_rails/helper.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rbpackages/react-on-rails/src/base/client.ts
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rbpackages/react-on-rails/src/base/client.ts
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation is handled in deeper level calls of the React on Rails Pro codebase, so it doesn't need to be validated again in the `rsc_payload_react_component` helper method.
Applied to files:
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rblib/react_on_rails/helper.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rbpackages/react-on-rails/src/types/index.tspackages/react-on-rails/src/base/client.ts
📚 Learning: 2025-02-13T16:50:47.848Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/clientStartup.ts:18-21
Timestamp: 2025-02-13T16:50:47.848Z
Learning: In the react_on_rails module, the `reactOnRailsPageUnloaded` function in clientStartup.ts is intentionally kept private as it's only used internally as a callback for `onPageUnloaded`.
Applied to files:
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb
📚 Learning: 2025-09-15T21:24:48.207Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1781
File: node_package/src/ClientSideRenderer.ts:82-95
Timestamp: 2025-09-15T21:24:48.207Z
Learning: In React on Rails, the force_load feature includes both explicit `data-force-load="true"` usage and the ability to hydrate components during the page loading state (`document.readyState === 'loading'`). Both capabilities require a Pro license, so the condition `!railsContext.rorPro && (isComponentForceLoaded || document.readyState === 'loading')` correctly gates both scenarios.
Applied to files:
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails/src/types/index.tspackages/react-on-rails/src/base/client.ts
📚 Learning: 2025-02-13T19:09:15.991Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/RSCWebpackLoader.ts:0-0
Timestamp: 2025-02-13T19:09:15.991Z
Learning: In React Server Components webpack loader, using `new Function('return import("react-server-dom-webpack/node-loader")')()` is necessary as a workaround to bypass TypeScript compilation issues with direct dynamic imports.
Applied to files:
packages/react-on-rails/src/base/client.ts
🧬 Code graph analysis (4)
react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb (2)
lib/react_on_rails/react_component/render_options.rb (1)
replay_console(82-84)lib/react_on_rails/helper.rb (1)
wrap_console_script_with_nonce(440-451)
packages/react-on-rails/src/buildConsoleReplay.ts (1)
packages/react-on-rails/src/RenderUtils.ts (1)
wrapInScriptTags(2-13)
packages/react-on-rails/src/base/client.ts (1)
packages/react-on-rails/src/buildConsoleReplay.ts (1)
consoleReplay(18-55)
lib/react_on_rails/helper.rb (1)
lib/react_on_rails/react_component/render_options.rb (1)
replay_console(82-84)
🔇 Additional comments (7)
packages/react-on-rails/src/base/client.ts (1)
178-180: LGTM!The new
getConsoleReplayScript()method correctly delegates toconsoleReplay()and provides a clean API for retrieving unwrapped console replay JavaScript. This allows callers (like the Ruby helper) to wrap the script with their own nonce-aware tags.react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb (1)
350-351: LGTM!The console script is properly extracted and wrapped with nonce support when replay is enabled. This implementation is consistent with the nonce-wrapping pattern used throughout the codebase.
packages/react-on-rails/src/types/index.ts (1)
440-447: LGTM!The type definition and documentation for
getConsoleReplayScript()are clear and accurate. The JSDoc effectively communicates the distinction between this method andbuildConsoleReplay(), and explains the CSP nonce use case.packages/react-on-rails/src/buildConsoleReplay.ts (1)
60-66: LGTM with dependency on RenderUtils fix.The nonce parameter is correctly threaded through to
wrapInScriptTags. The optional parameter maintains backward compatibility. However, ensure the HTML escaping issue flagged inRenderUtils.ts(line 7) is addressed, as this code depends on safe nonce handling in that function.lib/react_on_rails/helper.rb (3)
229-229: LGTM!The change from
buildConsoleReplay()togetConsoleReplayScript()is correct. This retrieves the unwrapped console replay JavaScript, which is then wrapped by the Ruby helper with CSP nonce support on lines 245-247.
398-398: LGTM!Both usages of
wrap_console_script_with_noncecorrectly use the ternary pattern to ensureresult_console_scriptis always a string (either the wrapped script or an empty string). This is the correct approach and is consistent across both rendering paths.Also applies to: 423-423
440-451: LGTM!The
wrap_console_script_with_noncehelper is well-implemented:
- Properly guards against blank input
- Uses Rails'
content_security_policy_nonce(:script)to retrieve the CSP nonce- Leverages
content_tagwhich will properly escape the nonce attribute value- Returns an empty string when no console script is provided
The use of
html_safeon line 450 is appropriate here sinceconsole_script_codecontains JavaScript code generated by the React on Rails bundle, not user input.
Code Review: CSP Nonce Support for consoleReplay ScriptOverall AssessmentThis is a well-implemented feature that adds Content Security Policy (CSP) nonce support for the console replay script. The changes are backward compatible and follow good security practices. Strengths1. Security Design
2. Architecture
3. Code Quality
Issues and Recommendations1. Missing Test Coverage - HIGH PRIORITYThe PR lacks test coverage for the new CSP nonce functionality. Tests should be added to verify the nonce is properly included in the script tag and that the code gracefully handles cases where CSP is not configured. 2. Missing Changelog Entry - REQUIREDPer CLAUDE.md guidelines, this user-visible feature needs a changelog entry in /CHANGELOG.md 3. Potential XSS Risk in wrapInScriptTags - MEDIUM PRIORITYLocation: packages/react-on-rails/src/RenderUtils.ts:9 The nonce parameter is directly interpolated without sanitization. While Rails content_security_policy_nonce generates safe nonces, defense-in-depth is good practice since this is a reusable utility function. Consider sanitizing the nonce parameter. 4. Error Handling in Ruby - LOW PRIORITYLocation: lib/react_on_rails/helper.rb:444 Consider adding error handling for the content_security_policy_nonce call in case it raises an error. 5. Bug Fix DocumentationThe fix changing consoleLogScript to consoleReplayScript is a legitimate bug fix that should be highlighted in the changelog as well. Pre-Merge ChecklistBased on CLAUDE.md requirements:
SummaryThis is a solid implementation of an important security feature. The architecture is clean and the approach is correct. However, before merging:
The security approach is sound, but defense-in-depth improvements would make it excellent. Great work on addressing issue 1930! |
Code Review - PR #2059: Add CSP nonce support for consoleReplay scriptI've reviewed this PR thoroughly. Overall, this is a well-implemented feature that addresses a real security need. Here's my detailed feedback: Strengths1. Excellent Architecture
2. Security Improvements
3. Code Quality
4. Bug Fix Included
Potential Issues & Questions1. Missing CHANGELOG Entry (Required)According to CLAUDE.md, user-visible changes (especially new features) should be documented in CHANGELOG.md. This CSP nonce support is a user-visible feature. 2. XSS Concerns in wrapInScriptTags()In RenderUtils.ts:7, the nonce is directly interpolated into HTML without escaping. While Rails' content_security_policy_nonce() should always return a safe base64 string, defensive programming would suggest HTML-escaping the nonce. However, the Ruby path uses Rails' safe content_tag() helper, which is secure. Question: Is the nonce parameter in wrapInScriptTags() and buildConsoleReplay() actually used anywhere in the codebase for client-side usage? If not, consider documenting that it should only receive trusted values. 3. Missing Test Coverage (Required)I don't see tests that verify:
Consider adding tests to spec/dummy/spec/requests/console_logging_spec.rb Summary
RecommendationsRequired Before Merge:
Nice to Have: This is a solid implementation that solves a real problem (CSP violations) in a clean, backward-compatible way. With the addition of tests and CHANGELOG entry, this will be ready to merge! |
Code Review: CSP Nonce Support for consoleReplay ScriptI've reviewed PR 2059 which adds Content Security Policy (CSP) nonce support for the console replay script. Overall, this is a well-implemented feature that addresses a real security concern. Strengths1. Clean Architecture
2. Good Security Practices
3. Strong Test Coverage
4. Bug Fix Included
Potential Issues and Suggestions1. XSS Risk: Nonce Value Not Sanitized (SECURITY)Location: RenderUtils.ts:7 The nonce value is interpolated directly into an HTML attribute without sanitization. While Rails content_security_policy_nonce() returns a safe base64 string, it's good practice to validate/escape the nonce value for defense in depth. Theoretical attack: If nonce could be controlled to contain abc123 onload=alert(1), it would result in attribute injection. Recommendation: Sanitize nonce to only allow alphanumeric and base64 characters. Note: The Ruby side uses content_tag(:script, ...) which automatically escapes attributes, so that path is secure. 2. Missing Ruby Tests for New MethodThe new wrap_console_script_with_nonce() helper method has no dedicated Ruby tests. Recommendation: Add RSpec tests in spec/dummy/spec/helpers/react_on_rails_helper_spec.rb to verify:
3. CHANGELOG Not UpdatedNo entry in CHANGELOG.md for this user-visible feature. Per CLAUDE.md guidelines, this should be added. 4. Documentation EnhancementLocation: helper.rb:441 The comment could be more specific about what threats scriptSanitizedVal() mitigates. 5. Pro Package ConsistencyGood catch updating react_on_rails_pro_helper.rb as well! Performance and SecurityPerformance: Negligible impact - lightweight operations with no breaking changes Security Assessment:
Recommendations Summary
Final VerdictThis PR is nearly ready to merge after addressing the nonce sanitization concern. The implementation is solid, well-thought-out, and maintains backward compatibility. Estimated effort: ~25 minutes total (5 min sanitization, 15-20 min tests, 2 min changelog) Great work on this feature! The architecture decisions are particularly well done. |
|
Great work on addressing issue #1930! This is a well-thought-out implementation that adds proper CSP nonce support. Here is my detailed review: Strengths
Potential Issues and Suggestions1. Security: Potential XSS via nonce attribute (Medium Priority) In RenderUtils.ts, the nonce is directly interpolated into the script tag. If the nonce value contains double quotes or other HTML special characters, it could break out of the attribute context. Recommend adding HTML attribute escaping or validating that nonces match the expected base64 format. 2. Missing RSpec tests for Ruby helper (High Priority) The new wrap_console_script_with_nonce() method has no Ruby test coverage. Consider adding comprehensive tests to spec/dummy/spec/helpers/react_on_rails_helper_spec.rb covering:
3. Edge case: nil vs empty string handling (Low Priority) At call sites (lines 246, 398, 423), the conditional check could result in nil instead of empty string. Consider using ternary operator for consistent empty string behavior. 4. Documentation: User-facing changes (Medium Priority) Since this affects how CSP is configured, consider adding notes to README about CSP nonce support and example Rails CSP configuration. Test Plan SuggestionsBeyond existing tests, consider adding:
SummaryThis is a solid PR that solves a real problem. Main concerns:
Once these issues are addressed, this will be ready to merge. Excellent work on the backward compatibility and clean separation between JS and Ruby concerns! |
Code Review: Add CSP nonce support for consoleReplay scriptSummaryThis PR adds Content Security Policy (CSP) nonce support for the console replay script, addressing issue #1930. The implementation is well-structured and maintains backward compatibility while adding important security features. ✅ Strengths
🔍 Issues & Recommendations1. CRITICAL: Insufficient Test Coverage for Ruby ImplementationThe new wrap_console_script_with_nonce() method in helper.rb:443-463 lacks Ruby specs. This is a security-sensitive method handling nonce attributes. Missing test scenarios:
Recommendation: Add comprehensive Ruby specs for this method to spec/dummy/spec/helpers/react_on_rails_helper_spec.rb 2. Documentation: Update CHANGELOG.mdAccording to CLAUDE.md, user-visible changes require CHANGELOG updates. This is a feature (CSP nonce support) that should be documented. Recommendation: Add entry to /CHANGELOG.md under Unreleased or next version describing the new CSP nonce support feature and referencing PR 2059 and issue #1930. 🔒 Security AssessmentOverall: ✅ SECURE with proper usage
No security vulnerabilities detected, but test coverage is critical to maintain this security posture. 📊 Performance ConsiderationsImpact: Negligible
🎯 Overall AssessmentRating: 8.5/10 - Excellent implementation with room for improvement in test coverage Blocking Issues:
Recommendations:
Once the Ruby specs and CHANGELOG are added, this will be production-ready. 📝 Code Style & Conventions✅ Compliant with CLAUDE.md guidelines:
🚀 Next Steps
Great work on this feature! The implementation is solid and the approach is well-thought-out. With the additional test coverage, this will be a valuable security enhancement. 🎉 |
Code Review: CSP Nonce Support for Console ReplayGreat work on adding CSP nonce support! This is a well-implemented security enhancement. Here's my detailed review: ✅ Strengths
🔍 Issues & Recommendations1. Missing Ruby Test Coverage
|
Code Review - PR 2059: Add CSP nonce support for consoleReplay scriptOverall, this is a well-implemented PR that adds important security functionality. The implementation is thorough, backward-compatible, and includes good defense-in-depth practices. Strengths
Issues & Recommendations1. Missing RSpec Test Coverage (HIGH PRIORITY) The PR lacks Ruby-side tests for the new wrap_console_script_with_nonce method. No tests cover:
Recommendation: Add tests to spec/dummy/spec/helpers/react_on_rails_helper_spec.rb 2. RBS Type Signature Missing (MEDIUM) The new wrap_console_script_with_nonce method needs a type signature in sig/react_on_rails/helper.rbs per CLAUDE.md requirements. 3. Nonce Sanitization (MEDIUM) Consider stricter base64-only validation. The current regex allows underscore and hyphen which are base64url specific. Verify Rails nonce format. Action Items Before Merge
SummarySolid implementation following project standards. TypeScript side is well-tested, but Ruby side needs test coverage. Once RSpec tests are added, this will be ready to merge. Estimated effort: 30-45 minutes for tests + RBS signature. Great work on the defense-in-depth approach! 🎉 |
justin808
force-pushed
the
justin808/csp-nonce-console-replay
branch
from
4f81add to
da610d4
Compare
Code Review: CSP Nonce Support for Console ReplaySummaryThis PR adds Content Security Policy (CSP) nonce support for the ✅ Strengths1. Excellent Security Approach
2. Good Cross-Version CompatibilityThe Ruby implementation handles Rails version differences gracefully: # lib/react_on_rails/helper.rb:449-456
nonce = if respond_to?(:content_security_policy_nonce)
begin
content_security_policy_nonce(:script) # Rails 6.1+
rescue ArgumentError
content_security_policy_nonce # Rails 5.2-6.0
end
endThis is a smart approach for supporting Rails 5.2-7.x. 3. Backward Compatibility
4. Good Test Coverage
5. Proper Documentation
🔍 Issues & Recommendations1. Missing Ruby Test Coverage
|
justin808
force-pushed
the
justin808/csp-nonce-console-replay
branch
from
da610d4 to
52eefa4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-on-rails/src/RenderUtils.ts (1)
2-15: Nonce sanitization in wrapInScriptTags is appropriate for CSPWhitelisting the nonce to
[a-zA-Z0-9+/=_-]before interpolating it intononce="..."effectively prevents attribute injection while remaining compatible with typical base64 (and base64url-style) CSP nonces. Given that callers should pass a framework-generated nonce, silently stripping unexpected characters is a reasonable defensive choice here; if you ever need diagnostics around misconfigured nonces, you could optionally log or assert whensanitizedNonce !== nonce, but that’s not required for correctness.lib/react_on_rails/helper.rb (1)
398-401: Centralized nonce-aware console replay wrapping for SSR looks solidUsing
wrap_console_script_with_nonce(console_script)in bothbuild_react_component_result_for_server_rendered_stringand_hashensures that all SSR paths consistently emit a<script id="consoleReplayLog" nonce="...">tag whenreplay_consoleis enabled, and nothing when it’s disabled. The helper itself:
- Safely no-ops on blank console scripts.
- Retrieves a CSP nonce with Rails-version-compatible
content_security_policy_noncecalls.- Relies on
content_tagfor attribute escaping, and on pre-sanitizedconsole_script_code(from the JS layer) before callinghtml_safe, which is consistent with existing script-escaping patterns.This provides CSP nonce support across the main SSR flows without changing behavior when CSP is not configured.
Also applies to: 423-426, 440-464
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
CHANGELOG.md(1 hunks)lib/react_on_rails/helper.rb(5 hunks)packages/react-on-rails/src/RenderUtils.ts(1 hunks)packages/react-on-rails/src/base/client.ts(2 hunks)packages/react-on-rails/src/buildConsoleReplay.ts(2 hunks)packages/react-on-rails/src/types/index.ts(1 hunks)packages/react-on-rails/tests/buildConsoleReplay.test.js(1 hunks)react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb
- packages/react-on-rails/tests/buildConsoleReplay.test.js
- CHANGELOG.md
- packages/react-on-rails/src/base/client.ts
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
lib/react_on_rails/helper.rbpackages/react-on-rails/src/RenderUtils.ts
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
lib/react_on_rails/helper.rbpackages/react-on-rails/src/types/index.ts
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails/src/types/index.ts
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
packages/react-on-rails/src/types/index.ts
🧬 Code graph analysis (2)
lib/react_on_rails/helper.rb (1)
lib/react_on_rails/react_component/render_options.rb (1)
replay_console(82-84)
packages/react-on-rails/src/buildConsoleReplay.ts (1)
packages/react-on-rails/src/RenderUtils.ts (1)
wrapInScriptTags(2-16)
⏰ 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). (10)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: claude-review
🔇 Additional comments (3)
packages/react-on-rails/src/types/index.ts (1)
439-447: Console replay internal API extension looks consistent and non-breakingThe distinction between
buildConsoleReplay()(wrapped script tag) andgetConsoleReplayScript()(raw JS) is clear, and the new method is added on the internal interface without changing existing signatures, which keeps this extension backward-compatible.packages/react-on-rails/src/buildConsoleReplay.ts (1)
13-17: Nonce-aware buildConsoleReplay is wired correctlyThe new
consoleReplaydoc clarifies that it returns bare JS, and thenonce?: stringparameter onbuildConsoleReplayis threaded cleanly intowrapInScriptTagswith an early exit on empty history. Because the nonce is a trailing optional argument, existing call sites remain valid.Also applies to: 57-67
lib/react_on_rails/helper.rb (1)
229-247: server_render_js now correctly uses raw JS + nonce-aware wrapperSwitching the JS snippet to
ReactOnRails.getConsoleReplayScript()and then wrapping the returned code viawrap_console_script_with_nonce(console_script)(guarded byrender_options.replay_console) keeps genericserver_render_jsaligned with the new CSP-aware console replay path. The interpolation withraw("#{html}#{console_script_tag}")behaves safely even whenconsole_script_tagisnilor"", so there’s no functional regression here.
justin808
force-pushed
the
justin808/csp-nonce-console-replay
branch
from
5504d5a to
8375f9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/react-on-rails/src/RenderUtils.ts (1)
2-15: Nonce handling inwrapInScriptTagsis secure and backward‑compatibleAllowlisting the nonce to base64/base64url characters before interpolating into the attribute prevents injection, and the early return for empty
scriptBodypreserves existing behavior. If you ever need to debug bad nonce values, you might optionally log or assert on cases wheresanitizedNoncediffers from the input in development.packages/react-on-rails/src/types/index.ts (1)
439-447: Console replay API types align with the new CSP‑aware flowThe docs correctly distinguish
buildConsoleReplay()(script‑tag wrapped) fromgetConsoleReplayScript()(raw JS), which matches the new Ruby helper usage. Since the underlyingbuildConsoleReplayimplementation accepts optional arguments (history, skip count, nonce), you could optionally widen this method’s type to reflect those parameters if you expect external TS consumers to call it directly; otherwise keeping it as a simple no‑arg internal helper is fine.react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb (1)
296-305: Streaming specs correctly assert script‑tag‑wrapped console replay outputThe updated expectations now treat
consoleReplayScriptas raw JS and assert the<script id="consoleReplayLog">…</script>wrapper, withpresent?guards andRegexp.escapekeeping the match robust. This lines up with the new helper behavior and CSP story. If you find yourself tweaking the script tag structure again, consider extracting a small shared helper in the spec to build the wrapped snippet so tests don’t duplicate the markup shape.Also applies to: 372-383, 395-406
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
CHANGELOG.md(1 hunks)lib/react_on_rails/helper.rb(5 hunks)packages/react-on-rails/src/RenderUtils.ts(1 hunks)packages/react-on-rails/src/base/client.ts(2 hunks)packages/react-on-rails/src/buildConsoleReplay.ts(2 hunks)packages/react-on-rails/src/serverRenderReactComponent.ts(3 hunks)packages/react-on-rails/src/types/index.ts(1 hunks)packages/react-on-rails/tests/buildConsoleReplay.test.js(1 hunks)react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb(1 hunks)react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/react-on-rails/tests/buildConsoleReplay.test.js
- packages/react-on-rails/src/base/client.ts
- react_on_rails_pro/app/helpers/react_on_rails_pro_helper.rb
- CHANGELOG.md
- packages/react-on-rails/src/serverRenderReactComponent.ts
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
📚 Learning: 2025-04-09T12:56:10.756Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1696
File: node_package/src/RSCPayloadContainer.ts:0-0
Timestamp: 2025-04-09T12:56:10.756Z
Learning: In the react_on_rails codebase, RSC payloads are already stringified using `JSON.stringify()` before being processed by the `escapeScript` function, which handles escaping of special characters. The function only needs to handle specific HTML markers like comments and closing script tags.
Applied to files:
packages/react-on-rails/src/RenderUtils.tslib/react_on_rails/helper.rb
📚 Learning: 2025-07-08T05:57:29.630Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1745
File: node_package/src/RSCRequestTracker.ts:8-14
Timestamp: 2025-07-08T05:57:29.630Z
Learning: The global `generateRSCPayload` function in React on Rails Pro (RORP) is provided by the framework during rendering requests, not implemented in application code. The `declare global` statements are used to document the expected interface that RORP will inject at runtime.
Applied to files:
packages/react-on-rails/src/RenderUtils.tspackages/react-on-rails/src/types/index.tslib/react_on_rails/helper.rb
📚 Learning: 2024-12-12T13:07:09.929Z
Learnt from: alexeyr-ci
Repo: shakacode/react_on_rails PR: 1644
File: node_package/src/ReactOnRailsRSC.ts:87-87
Timestamp: 2024-12-12T13:07:09.929Z
Learning: When handling errors in 'node_package/src/ReactOnRailsRSC.ts', include the error stack in error messages in development and test environments to aid debugging.
Applied to files:
packages/react-on-rails/src/types/index.ts
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
Repo: shakacode/react_on_rails PR: 1697
File: package-scripts.yml:28-28
Timestamp: 2025-02-12T16:38:06.537Z
Learning: The file `node_package/lib/ReactOnRails.full.js` is autogenerated during the build process and should not be present in the repository.
Applied to files:
packages/react-on-rails/src/types/index.tsreact_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
Repo: shakacode/react_on_rails PR: 1732
File: spec/dummy/client/app-react16/startup/ReduxSharedStoreApp.client.jsx:40-44
Timestamp: 2025-04-26T21:55:55.874Z
Learning: In the react_on_rails project, files under `app-react16` directories are copied/moved to corresponding `/app` directories during the conversion process (removing the `-react16` suffix), which affects their relative import paths at runtime.
Applied to files:
react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb
📚 Learning: 2024-10-08T20:53:47.076Z
Learnt from: theforestvn88
Repo: shakacode/react_on_rails PR: 1620
File: spec/dummy/client/app/startup/HelloTurboStream.jsx:3-3
Timestamp: 2024-10-08T20:53:47.076Z
Learning: The `RailsContext` import in `spec/dummy/client/app/startup/HelloTurboStream.jsx` is used later in the project, as clarified by the user theforestvn88.
Applied to files:
react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
Repo: shakacode/react_on_rails PR: 1644
File: lib/react_on_rails/helper.rb:190-197
Timestamp: 2025-02-18T13:08:01.477Z
Learning: RSC support validation in React on Rails Pro is handled through a chain of validations:
1. Pro version check in `run_stream_inside_fiber`
2. RSC support check during pack generation via `ReactOnRailsPro.configuration.enable_rsc_support`
3. RSC support validation during component registration
This makes additional validation in the helper methods unnecessary.
Applied to files:
react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
Repo: shakacode/react_on_rails PR: 1770
File: lib/generators/react_on_rails/templates/base/base/app/javascript/src/HelloWorld/ror_components/HelloWorld.client.jsx:2-2
Timestamp: 2025-09-16T08:01:11.146Z
Learning: React on Rails uses webpack CSS Modules configuration with namedExports: true, which requires the import syntax `import * as style from './file.module.css'` rather than the default export pattern. This configuration enables better tree shaking and bundle size optimization for CSS modules.
Applied to files:
react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb
🧬 Code graph analysis (3)
react_on_rails_pro/spec/dummy/spec/helpers/react_on_rails_pro_helper_spec.rb (1)
react_on_rails_pro/spec/react_on_rails_pro/stream_spec.rb (1)
include(7-31)
lib/react_on_rails/helper.rb (1)
lib/react_on_rails/react_component/render_options.rb (1)
replay_console(82-84)
packages/react-on-rails/src/buildConsoleReplay.ts (1)
packages/react-on-rails/src/RenderUtils.ts (1)
wrapInScriptTags(2-16)
⏰ 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). (10)
- GitHub Check: build-dummy-app-webpack-test-bundles (3.4, 22, latest)
- GitHub Check: examples (3.4, latest)
- GitHub Check: examples (3.2, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: pro-lint-js-and-ruby
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: build
- GitHub Check: build-dummy-app-webpack-test-bundles
- GitHub Check: claude-review
🔇 Additional comments (3)
packages/react-on-rails/src/buildConsoleReplay.ts (1)
13-17: consoleReplay / buildConsoleReplay split and nonce threading look goodExporting
consoleReplayas the raw JS generator and having the defaultbuildConsoleReplaywrap it in<script id="consoleReplayLog">…</script>with an optional nonce cleanly supports both CSP‑aware server usage and existing consumers. Adding the nonce as a trailing optional parameter keeps prior call sites intact while centralizing sanitization inwrapInScriptTags.Also applies to: 57-67
lib/react_on_rails/helper.rb (2)
229-248: Nonce‑aware console replay wiring in helpers is coherent and safeSwitching
server_render_jsto useReactOnRails.getConsoleReplayScript()(raw JS) and then wrapping viawrap_console_script_with_noncealigns this path with the rest of the CSP‑aware flow. The new helper cleanly handles:
- ignoring blank console scripts,
- obtaining a nonce via
content_security_policy_nonceacross Rails versions (directive argument where supported, rescuingArgumentErrorotherwise), and- emitting
<script id="consoleReplayLog" nonce="…">…</script>when a nonce is present.Given that the JS side already sanitizes console replay payloads before they reach Ruby, using
html_safehere is consistent with existing patterns in the project.Also applies to: 440-464
379-402: react_component result builders now correctly emit nonce‑wrapped console replay scriptsBoth
build_react_component_result_for_server_rendered_stringand_hashnow deriveresult_console_scriptviawrap_console_script_with_nonce(console_script)whenrender_options.replay_consoleis true, otherwise they pass an empty string intocompose_react_component_html_with_spec_and_console. This keeps the console replay script optional, centralizes CSP nonce handling in one helper, and preserves the existing HTML composition behavior.Also applies to: 406-438
justin808
force-pushed
the
justin808/csp-nonce-console-replay
branch
from
8375f9c to
ae833ad
Compare
Code Review: CSP Nonce Support for Console ReplayThank you for this well-implemented PR! This is a solid security enhancement. Here's my detailed review: ✅ Strengths1. Excellent Security Design
2. Well-Structured Architecture
3. Comprehensive Testing
4. Good Documentation
🔍 Areas for Improvement1. Missing RSpec Tests for Core Helper Method
|
| Component | Coverage | Notes |
|---|---|---|
| TypeScript nonce sanitization | ✅ Excellent | 3 test cases cover happy path, XSS, edge cases |
TypeScript consoleReplay() |
✅ Good | Tests verify no script tags returned |
Ruby wrap_console_script_with_nonce |
No direct unit tests | |
| Pro integration | ✅ Good | Updated specs verify wrapped format |
| Rails version compatibility | ❌ Not tested | Critical cross-version logic untested |
🎯 Recommendations
Must-Have Before Merge:
- Add RSpec tests for
wrap_console_script_with_noncecovering Rails version compatibility
Nice-to-Have:
- Update TypeScript comment to mention base64url encoding
- Consider optimizing Rails version detection to avoid exception handling
- Add comment linking to
scriptSanitizedVal()implementation
Follow-Up (Post-Merge):
- Consider adding integration test with actual CSP headers configured
- Document CSP setup steps for users in README or guides
✅ Overall Assessment
This is high-quality, production-ready code with excellent security practices. The architecture is clean, backward compatibility is preserved, and the implementation is thoughtful.
The main gap is test coverage for the Ruby helper method, particularly the Rails version compatibility logic. Once that's addressed, this is ready to merge.
Approval Status: ✅ Approve with minor changes (add RSpec tests)
Great work on this security enhancement! 🎉
Fixes #1930 - Add optional nonce parameter to wrapInScriptTags() and buildConsoleReplay() - Add getConsoleReplayScript() method to return JS without script tags - Update Ruby helper to wrap console script with CSP nonce using content_security_policy_nonce(:script) - Fix bug: change consoleLogScript to consoleReplayScript in server_render_js - Pro helper now also wraps console scripts with nonce This allows the consoleReplay script to work with Content Security Policy without violations when using script-src :self or similar policies. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
- Add TypeScript tests for: - buildConsoleReplay() with nonce parameter - consoleReplay() returns JS without script tags - Empty string handling with nonce - Add security comments explaining: - Why html_safe is safe (content pre-sanitized via scriptSanitizedVal) - CSP nonce availability (Rails 5.2+) - Add blank line before wrap_console_script_with_nonce method for consistency All tests passing (100 passed in react-on-rails, 30 passed in pro). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The content_security_policy_nonce helper has different signatures across Rails versions: - Rails 5.2-6.0: content_security_policy_nonce (no arguments) - Rails 6.1+: content_security_policy_nonce(directive) (optional argument) This change adds a try-catch to handle both versions gracefully. Fixes CI failures with "wrong number of arguments (given 1, expected 0)". 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Security improvements: - Sanitize nonce values to prevent attribute injection attacks - Only allow base64-safe characters: alphanumeric, +, /, =, -, _ - Add test to verify malicious nonce values are sanitized - Document the security measure in code comments Even though Rails content_security_policy_nonce() returns safe values, this adds defense-in-depth by sanitizing at the JavaScript layer. All tests passing (101 passed). 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Document the new CSP nonce support feature in CHANGELOG.md following project conventions. Entry includes implementation details and PR reference. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The issue was that serverRenderReactComponent was calling buildConsoleReplay() which wraps the console code in script tags, but Ruby was ALSO wrapping it with wrap_console_script_with_nonce(), resulting in nested script tags like: <script id="consoleReplayLog"><script>...</script></script> Changed serverRenderReactComponent to call consoleReplay() instead, which returns just the JavaScript code without script tags. Ruby then wraps it once with the proper nonce attribute. This fixes the failing tests: - ReactOnRailsProHelper html_streaming_react_component creates a fiber - Console logging from server has server log messages in the script 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Simplified the if/else blocks to avoid RuboCop Layout cop bugs. The Layout cops were crashing on multi-line if/else assignments even though the code had no actual offenses. Changed to simpler variable assignments to avoid the crashes. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
justin808
force-pushed
the
justin808/csp-nonce-console-replay
branch
from
ed27367 to
0740ba6
Compare
|
@claude review PR |
|
Claude finished @justin808's task —— View job PR Review: Add CSP nonce support for consoleReplay scriptTodo List
Overall AssessmentThis is an excellent, production-ready PR that adds important CSP nonce support for the console replay script. The implementation demonstrates strong security practices, thoughtful API design, and comprehensive testing. I found no blocking issues. Rating: 9.5/10 - Ready to merge after addressing one minor enhancement opportunity. ✅ Strengths1. Outstanding Security ImplementationThe defense-in-depth approach is exemplary:
The comment on 2. Excellent Cross-Version Rails CompatibilityThe Rails version handling ( begin
content_security_policy_nonce(:script) # Rails 6.1+
rescue ArgumentError
content_security_policy_nonce # Rails 5.2-6.0
endThis gracefully supports Rails 5.2-7.x without requiring version checks. Smart use of exception handling for API differences. 3. Clean API DesignThe separation of concerns is perfect:
The naming distinction ( 4. Comprehensive Test CoverageTypeScript tests (
Pro package specs ( 5. Documentation & Project Compliance
🔍 Areas for Enhancement1. Missing Unit Tests for Ruby Helper Method (Minor)The new Suggested test cases for describe '#wrap_console_script_with_nonce' do
let(:helper) { Class.new { include ReactOnRailsHelper }.new }
let(:console_script) { "console.log.apply(console, ['test']);" }
context 'when CSP nonce is available' do
before do
allow(helper).to receive(:content_security_policy_nonce)
.with(:script).and_return('abc123')
end
it 'wraps script with nonce attribute' do
result = helper.send(:wrap_console_script_with_nonce, console_script)
expect(result).to include('nonce="abc123"')
expect(result).to include('id="consoleReplayLog"')
expect(result).to include(console_script)
end
end
context 'when CSP is not configured' do
before do
allow(helper).to receive(:respond_to?)
.with(:content_security_policy_nonce).and_return(false)
end
it 'wraps script without nonce attribute' do
result = helper.send(:wrap_console_script_with_nonce, console_script)
expect(result).not_to include('nonce=')
expect(result).to include('id="consoleReplayLog"')
end
end
context 'with Rails 5.2-6.0 compatibility' do
before do
allow(helper).to receive(:content_security_policy_nonce)
.with(:script).and_raise(ArgumentError)
allow(helper).to receive(:content_security_policy_nonce)
.with(no_args).and_return('fallback123')
end
it 'falls back to no-argument method' do
result = helper.send(:wrap_console_script_with_nonce, console_script)
expect(result).to include('nonce="fallback123"')
end
end
context 'with blank input' do
it 'returns empty string' do
expect(helper.send(:wrap_console_script_with_nonce, '')).to eq('')
expect(helper.send(:wrap_console_script_with_nonce, nil)).to eq('')
end
end
endWhy this matters: The cross-version Rails compatibility logic is critical infrastructure code that should be tested to prevent regressions across Rails 5.2-7.x. Priority: Nice-to-have. The integration tests in Pro specs provide good coverage, but standalone unit tests would improve maintainability. 🎯 Technical AnalysisSecurity Assessment: EXCELLENT ✅Threat Model Coverage:
No security vulnerabilities identified. Performance Impact: NEGLIGIBLE
Backward Compatibility: PERFECT ✅
Code Quality: EXCELLENT
📝 Detailed Review by File
|
The test was unconditionally removing the first line assuming there would be a leading blank line from the old wrapInScriptTags format. With the new approach where Ruby wraps the console script using content_tag (via wrap_console_script_with_nonce), there's no leading blank line, so the test was incorrectly skipping the first log line. Changed to only shift if the first line is actually empty. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
The test page renders two ReduxSharedStoreApp components with trace: true, so both log their render messages. Update expected output to include the second component's console log. Related to CSP nonce console replay changes.
When multiple React components with replay_console are rendered on the same page, each creates its own <script id="consoleReplayLog"> tag. Nokogiri's .text method concatenates text from all matching elements without separators, causing console log statements to be joined without newlines. Fixed by explicitly mapping over all script tags and joining their text with newlines before parsing. This fix avoids adding runtime complexity or requiring users to call new helper methods. The existing behavior (multiple script tags per component) is preserved. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Adds comprehensive unit tests for the wrap_console_script_with_nonce helper method to ensure proper CSP nonce handling across different Rails versions. Test coverage includes: - CSP nonce attribute generation when available - Script tag creation without nonce when CSP is not configured - Rails 5.2-6.0 compatibility fallback (ArgumentError handling) - Blank input handling (empty, nil, whitespace) - Multi-line script preservation (newlines maintained) - Special character escaping in script content These tests validate the cross-version Rails compatibility logic (5.2-7.x) that is critical infrastructure code for the CSP nonce support feature. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
|
@claude review this pr |
|
Claude finished @justin808's task —— View job Code Review: CSP Nonce Support for consoleReplay ScriptOverall AssessmentThis is an excellent, production-ready PR that adds important CSP nonce support for the console replay script. The implementation demonstrates strong security practices, thoughtful API design, and comprehensive testing. I found no blocking issues. Rating: 9.5/10 - Ready to merge after addressing one minor enhancement opportunity. ✅ Strengths1. Outstanding Security ImplementationThe defense-in-depth approach is exemplary:
The comment on 2. Excellent Cross-Version Rails CompatibilityThe Rails version handling ( begin
content_security_policy_nonce(:script) # Rails 6.1+
rescue ArgumentError
content_security_policy_nonce # Rails 5.2-6.0
endThis gracefully supports Rails 5.2-7.x without requiring version checks. Smart use of exception handling for API differences. 3. Clean API DesignThe separation of concerns is perfect:
The naming distinction ( 4. Comprehensive Test CoverageTypeScript tests (
Ruby tests (
Pro package specs ( 5. Documentation & Project Compliance
🔍 Areas for Enhancement1. Missing Unit Tests for Ruby Helper Method (Minor)The new Suggested test cases for describe '#wrap_console_script_with_nonce' do
let(:helper) { Class.new { include ReactOnRailsHelper }.new }
let(:console_script) { "console.log.apply(console, ['test']);" }
context 'when CSP nonce is available' do
before do
allow(helper).to receive(:content_security_policy_nonce)
.with(:script).and_return('abc123')
end
it 'wraps script with nonce attribute' do
result = helper.send(:wrap_console_script_with_nonce, console_script)
expect(result).to include('nonce="abc123"')
expect(result).to include('id="consoleReplayLog"')
expect(result).to include(console_script)
end
end
context 'when CSP is not configured' do
before do
allow(helper).to receive(:respond_to?)
.with(:content_security_policy_nonce).and_return(false)
end
it 'wraps script without nonce attribute' do
result = helper.send(:wrap_console_script_with_nonce, console_script)
expect(result).not_to include('nonce=')
expect(result).to include('id="consoleReplayLog"')
end
end
context 'with Rails 5.2-6.0 compatibility' do
before do
allow(helper).to receive(:content_security_policy_nonce)
.with(:script).and_raise(ArgumentError)
allow(helper).to receive(:content_security_policy_nonce)
.with(no_args).and_return('fallback123')
end
it 'falls back to no-argument method' do
result = helper.send(:wrap_console_script_with_nonce, console_script)
expect(result).to include('nonce="fallback123"')
end
end
context 'with blank input' do
it 'returns empty string' do
expect(helper.send(:wrap_console_script_with_nonce, '')).to eq('')
expect(helper.send(:wrap_console_script_with_nonce, nil)).to eq('')
end
end
endWhy this matters: The cross-version Rails compatibility logic is critical infrastructure code that should be tested to prevent regressions across Rails 5.2-7.x. Priority: Nice-to-have. The integration tests in Pro specs provide good coverage, but standalone unit tests would improve maintainability. UPDATE: I see these tests were already added in 🎯 Technical AnalysisSecurity Assessment: EXCELLENT ✅Threat Model Coverage:
No security vulnerabilities identified. Performance Impact: NEGLIGIBLE
Backward Compatibility: PERFECT ✅
Code Quality: EXCELLENT
📝 Detailed Review by File
|
Summary
Fixes #1930
Adds Content Security Policy (CSP) nonce support for the
consoleReplayscript to prevent CSP violations when usingscript-src :selfor similar policies.Changes
TypeScript Changes
nonceparameter towrapInScriptTags()function inRenderUtils.tsnonceparameter tobuildConsoleReplay()functiongetConsoleReplayScript()method toReactOnRailsinterface and implementationRuby Changes
wrap_console_script_with_nonce()helper method inhelper.rbcontent_security_policy_nonce(:script)to get the nonce<script>tag withid="consoleReplayLog"andnonceattributebuild_react_component_result_for_server_rendered_string()to wrap console scripts with noncebuild_react_component_result_for_server_rendered_hash()to wrap console scripts with nonceconsoleLogScripttoconsoleReplayScriptinserver_render_js()methodgetConsoleReplayScript()instead ofbuildConsoleReplay()How It Works
getConsoleReplayScript()<script>tag with the CSP nonce from RailsTest Plan
Breaking Changes
None. This is fully backward compatible:
buildConsoleReplay()method still works for client-side usagegetConsoleReplayScript()method is additive🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.