Add server bundle security features for private bundle locations #1805
Conversation
This commit adds server bundle security features to prevent server bundles from being served from public directories when configured appropriately. Key changes: - Add server_bundle_output_path configuration for private bundle locations - Add enforce_private_server_bundles security enforcement option - Improve bundle_js_file_path method to check for server bundles first - Add public_bundles_full_path method with deprecation for old name - Add comprehensive validation for security configuration - Add helper methods for server bundle detection and security checks The implementation maintains backwards compatibility while providing enhanced security for server-side rendering bundles. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
WalkthroughAdds two configuration options (server_bundle_output_path, enforce_private_server_bundles) with validation and initialization wiring. Updates Utils path resolution to handle server bundles, explicit server bundle paths, and stricter handling of missing manifest entries. Introduces new helpers and aliases for public bundles path. Adds specs covering configuration defaults and validations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Utils as ReactOnRails::Utils
participant Config as Configuration
participant Manifest as Asset Manifest
App->>Utils: bundle_js_file_path(bundle_name)
Utils->>Utils: server_bundle?(bundle_name)
alt Server bundle
Utils->>Config: read server_bundle_output_path
alt server_bundle_output_path set
Utils-->>App: return explicit server path
else not set
Utils->>Utils: enforce_private_server_bundles?
alt Enforcement ON
Utils-->>App: raise MissingEntryError
else Enforcement OFF
Utils-->>App: fallback to public_bundles_full_path
end
end
else Not a server bundle
Utils->>Manifest: lookup bundle path
alt Found
Manifest-->>Utils: asset path
Utils-->>App: return asset path
else Missing
Utils->>Utils: handle_missing_manifest_entry(bundle_name)
alt Server bundle + Enforcement ON
Utils-->>App: raise MissingEntryError
else Otherwise
Utils-->>App: fallback to public_bundles_full_path
end
end
end
sequenceDiagram
autonumber
participant Config as Configuration
participant Rails as Rails.env/paths
Config->>Config: setup_config_values(...)
Config->>Config: validate_enforce_private_server_bundles
alt enforce_private_server_bundles = true
Config->>Config: ensure server_bundle_output_path present
Config->>Rails: check path not under public/
alt Violates rules
Config-->>Config: raise ArgumentError
else OK
Config-->>Config: continue
end
else Enforcement OFF
Config-->>Config: continue
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 for PR #1805: Server Bundle Security FeaturesOverall AssessmentThis PR adds important security features for protecting server-side rendering bundles from public exposure. The implementation is well-thought-out with good backwards compatibility. However, there are several areas that need attention. ✅ Strengths
🔍 Issues & Suggestions1. Critical: Path Traversal Security RiskIn lib/react_on_rails/utils.rb:80, the code directly uses the configured path without sanitization. If bundle_name contains path traversal sequences (e.g., ../../../etc/passwd), it could potentially access files outside the intended directory. Recommendation: Sanitize the bundle name and validate the resolved path to ensure it stays within the configured directory boundaries. 2. Performance: Redundant Bundle Type CheckingThe server_bundle? method is called multiple times in the execution path. Consider caching this check or restructuring the logic to minimize repeated checks. 3. Error Handling: Insufficient Context in Error MessagesIn lib/react_on_rails/utils.rb:302-306, the error message could be more helpful by including the actual configured path to help with debugging. 4. Configuration Validation: Missing Directory Existence CheckThe validation checks if the path is inside the public directory but doesn't verify if the configured directory actually exists. Consider adding a warning or validation for non-existent directories. 5. Documentation: Missing Migration GuideWhile the PR description includes a configuration example, there's no documentation on how existing applications should migrate to this new security model. 6. Test Coverage: Missing Edge CasesThe tests miss several important scenarios:
🚨 Security Considerations
📝 Style & Convention CompliancePer CLAUDE.md requirements:
💡 Suggestions for Improvement
📊 Test Coverage AssessmentThe added tests are good but could be more comprehensive:
ConclusionThis is a valuable security enhancement that addresses an important concern. However, the path traversal vulnerability must be fixed before merging. Once that critical issue and the other security considerations are addressed, this will be a solid addition to the framework. Verdict: Request changes - primarily due to the path traversal security issue. |
![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 (3)
spec/react_on_rails/configuration_spec.rb (1)
504-512: Add boundary test for “public” vs “publication” to avoid false positives.Strengthen validation by adding a spec ensuring a path like app/publication is not rejected as “inside public”.
Apply this diff to add the test:
@@ describe ".enforce_private_server_bundles" do + it "does not treat directories starting with 'public' as inside public" do + expect do + ReactOnRails.configure do |config| + config.server_bundle_output_path = "app/publication/server" + config.enforce_private_server_bundles = true + end + end.not_to raise_error + endlib/react_on_rails/utils.rb (2)
291-314: Helpers are fine; consider extraction to reduce Utils size.These methods are cohesive but contribute to ModuleLength. Either extract to a small Utils::BundleSecurity module or temporarily disable the cop.
Apply this minimal diff if you prefer to mute the cop now (unblock CI), and plan an extraction later:
@@ -module ReactOnRails - module Utils +module ReactOnRails + # rubocop:disable Metrics/ModuleLength + module Utils @@ end end +# rubocop:enable Metrics/ModuleLength
132-141: Use server_bundle_output_path for server-side manifest when configured.The server-client manifest is produced by the server bundle; with enforcement enabled, prefer the private output path over public.
Apply this diff:
- @react_server_manifest_path = File.join(generated_assets_full_path, asset_name) + base = + if ReactOnRails.configuration.server_bundle_output_path.present? + File.expand_path(File.join(Rails.root || ".", ReactOnRails.configuration.server_bundle_output_path)) + else + public_bundles_full_path + end + @react_server_manifest_path = File.join(base, asset_name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
lib/react_on_rails/configuration.rb(6 hunks)lib/react_on_rails/utils.rb(4 hunks)spec/react_on_rails/configuration_spec.rb(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}
📄 CodeRabbit inference engine (CLAUDE.md)
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru}}: Before every commit/push, runbundle exec rubocopand fix all violations in Ruby code
Let RuboCop handle all Ruby code formatting; never manually format Ruby files
Files:
lib/react_on_rails/configuration.rbspec/react_on_rails/configuration_spec.rblib/react_on_rails/utils.rb
{Gemfile,Rakefile,**/*.{rb,rake,gemspec,ru,js,jsx,ts,tsx,json,yml,yaml,md,css,scss}}
📄 CodeRabbit inference engine (CLAUDE.md)
Ensure all committed files end with a trailing newline character
Files:
lib/react_on_rails/configuration.rbspec/react_on_rails/configuration_spec.rblib/react_on_rails/utils.rb
🧠 Learnings (4)
📚 Learning: 2025-02-18T13:08:01.477Z
Learnt from: AbanoubGhadban
PR: shakacode/react_on_rails#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:
lib/react_on_rails/configuration.rb
📚 Learning: 2025-04-26T21:55:55.874Z
Learnt from: alexeyr-ci2
PR: shakacode/react_on_rails#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:
lib/react_on_rails/utils.rb
📚 Learning: 2025-02-12T16:38:06.537Z
Learnt from: Romex91
PR: shakacode/react_on_rails#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:
lib/react_on_rails/utils.rb
📚 Learning: 2025-09-16T08:01:11.146Z
Learnt from: justin808
PR: shakacode/react_on_rails#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:
lib/react_on_rails/utils.rb
🧬 Code graph analysis (2)
spec/react_on_rails/configuration_spec.rb (1)
lib/react_on_rails/configuration.rb (2)
configure(6-9)configuration(16-60)
lib/react_on_rails/utils.rb (2)
lib/react_on_rails/configuration.rb (1)
configuration(16-60)lib/react_on_rails/packer_utils.rb (2)
bundle_js_uri_from_packer(51-67)packer_public_output_path(100-102)
🪛 GitHub Actions: Lint JS and Ruby
lib/react_on_rails/utils.rb
[error] 10-10: Metrics/ModuleLength: Module has too many lines. [206/180]
⏰ 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). (7)
- GitHub Check: dummy-app-integration-tests (3.4, 22)
- GitHub Check: dummy-app-integration-tests (3.2, 20)
- GitHub Check: rspec-package-tests (3.4, latest)
- GitHub Check: rspec-package-tests (3.2, latest)
- GitHub Check: rspec-package-tests (3.4, minimum)
- GitHub Check: rspec-package-tests (3.2, minimum)
- GitHub Check: claude-review
🔇 Additional comments (10)
spec/react_on_rails/configuration_spec.rb (1)
463-521: Good coverage of defaults and enforcement paths.Tests clearly exercise defaults, happy paths, and error conditions for the two new settings.
lib/react_on_rails/utils.rb (4)
73-84: Server bundle path short‑circuit looks correct.Early return to server_bundle_output_path for server bundles prevents public fallback and manifest lookup when configured.
98-104: Graceful handling of missing manifest entries.Delegating to handle_missing_manifest_entry centralizes behavior and respects enforcement.
176-184: Clarity win on public path naming.public_bundles_full_path and the alias help differentiate public vs private locations.
9-15: CI failing: Metrics/ModuleLength — confirm rubocop output
- .rubocop.yml enables Metrics/ModuleLength (line 94); verification here failed because
bundleis not available ("bundle: command not found").- Action: run locally/CI and paste the output:
bundle exec rubocop lib/react_on_rails/utils.rb -f clangto confirm the exact cop violation.- Unblocks: either extract the new security helpers from lib/react_on_rails/utils.rb into a separate module, or add a temporary RuboCop disable around the module:
# rubocop:disable Metrics/ModuleLength- ...module code...
# rubocop:enable Metrics/ModuleLengthlib/react_on_rails/configuration.rb (5)
55-58: Defaults for new options look good.Nil path and false enforcement preserve backward compatibility.
74-76: Accessors added appropriately.Public API surface matches new config keys.
92-93: Initializer signature extended correctly.Parameters wired through to fields.
137-139: Assignment of new fields is correct.No ordering side-effects detected.
155-156: Validation hook placement is appropriate.Runs with other config checks during setup.
| def validate_enforce_private_server_bundles | ||
| return unless enforce_private_server_bundles | ||
|
|
||
| # Check if server_bundle_output_path is nil | ||
| if server_bundle_output_path.nil? | ||
| raise ReactOnRails::Error, "enforce_private_server_bundles is set to true, but " \ | ||
| "server_bundle_output_path is nil. Please set server_bundle_output_path " \ | ||
| "to a directory outside of the public directory." | ||
| end | ||
|
|
||
| # Check if server_bundle_output_path is inside public directory | ||
| # Skip validation if Rails.root is not available (e.g., in tests) | ||
| return unless defined?(Rails) && Rails.root | ||
|
|
||
| public_path = Rails.root.join("public").to_s | ||
| server_output_path = File.expand_path(server_bundle_output_path, Rails.root.to_s) | ||
|
|
||
| return unless server_output_path.start_with?(public_path) | ||
|
|
||
| raise ReactOnRails::Error, "enforce_private_server_bundles is set to true, but " \ | ||
| "server_bundle_output_path (#{server_bundle_output_path}) is inside " \ | ||
| "the public directory. Please set it to a directory outside of public." | ||
| end |
There was a problem hiding this comment.
Fix public directory containment check (boundary bug).
Using String#start_with? on “/app/public” flags “/app/publication/…” as inside public. Check path boundary.
Apply this diff:
- public_path = Rails.root.join("public").to_s
- server_output_path = File.expand_path(server_bundle_output_path, Rails.root.to_s)
-
- return unless server_output_path.start_with?(public_path)
+ public_path = Rails.root.join("public").expand_path.to_s
+ server_output_path = File.expand_path(server_bundle_output_path, Rails.root.to_s)
+
+ inside_public =
+ server_output_path == public_path ||
+ server_output_path.start_with?("#{public_path}#{File::SEPARATOR}")
+ return unless inside_publicOptional hardening (future): consider using Pathname and existing? checks or realpath with rescue to handle symlinks without requiring path existence.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def validate_enforce_private_server_bundles | |
| return unless enforce_private_server_bundles | |
| # Check if server_bundle_output_path is nil | |
| if server_bundle_output_path.nil? | |
| raise ReactOnRails::Error, "enforce_private_server_bundles is set to true, but " \ | |
| "server_bundle_output_path is nil. Please set server_bundle_output_path " \ | |
| "to a directory outside of the public directory." | |
| end | |
| # Check if server_bundle_output_path is inside public directory | |
| # Skip validation if Rails.root is not available (e.g., in tests) | |
| return unless defined?(Rails) && Rails.root | |
| public_path = Rails.root.join("public").to_s | |
| server_output_path = File.expand_path(server_bundle_output_path, Rails.root.to_s) | |
| return unless server_output_path.start_with?(public_path) | |
| raise ReactOnRails::Error, "enforce_private_server_bundles is set to true, but " \ | |
| "server_bundle_output_path (#{server_bundle_output_path}) is inside " \ | |
| "the public directory. Please set it to a directory outside of public." | |
| end | |
| def validate_enforce_private_server_bundles | |
| return unless enforce_private_server_bundles | |
| # Check if server_bundle_output_path is nil | |
| if server_bundle_output_path.nil? | |
| raise ReactOnRails::Error, "enforce_private_server_bundles is set to true, but " \ | |
| "server_bundle_output_path is nil. Please set server_bundle_output_path " \ | |
| "to a directory outside of the public directory." | |
| end | |
| # Check if server_bundle_output_path is inside public directory | |
| # Skip validation if Rails.root is not available (e.g., in tests) | |
| return unless defined?(Rails) && Rails.root | |
| public_path = Rails.root.join("public").expand_path.to_s | |
| server_output_path = File.expand_path(server_bundle_output_path, Rails.root.to_s) | |
| inside_public = | |
| server_output_path == public_path || | |
| server_output_path.start_with?("#{public_path}#{File::SEPARATOR}") | |
| return unless inside_public | |
| raise ReactOnRails::Error, "enforce_private_server_bundles is set to true, but " \ | |
| "server_bundle_output_path (#{server_bundle_output_path}) is inside " \ | |
| "the public directory. Please set it to a directory outside of public." | |
| end |
🤖 Prompt for AI Agents
In lib/react_on_rails/configuration.rb around lines 204 to 226, the containment
check uses String#start_with? which treats "/app/publication/..." as inside
"/app/public"; change the check to compare canonical expanded paths so you only
treat paths equal to public or actually nested: expand both
Rails.root.join("public") and server_bundle_output_path, normalize them, then
consider server_output_path inside public only if it equals public_path or
begins with public_path + File::SEPARATOR (i.e., a proper path boundary). Keep
the existing Rails.root guard and nil check; this fixes the boundary bug without
relying on path existence or symlink resolution.
|
Closing in favor of updated original PR #1798 which now has the same clean implementation after squashing. |
Code Review: Server Bundle Security Features 🔍I have reviewed this PR thoroughly and here is my feedback: ✅ Strengths1. Security Enhancement
2. Backward Compatibility
3. Code Quality
🎯 Areas for Improvement1. Bundle Path Resolution Logic if server_bundle?(bundle_name) && config.server_bundle_output_path.present?
return File.expand_path(File.join(root_path, config.server_bundle_output_path, bundle_name))
endConcern: This bypasses webpack manifest entirely, which could cause issues when:
Suggestion: Consider checking the manifest first, then falling back to the configured path if not found. 2. Error Messages Could Be More Helpful raise Shakapacker::Manifest::MissingEntryError,
"Server bundle '#{bundle_name}' not found in manifest and " \
"enforce_private_server_bundles is enabled. " \
"Ensure server bundle is built and server_bundle_output_path is configured correctly."Suggestion: Add the actual configured path to help debugging: "Server bundle '#{bundle_name}' not found in manifest. " \
"Expected location: #{config.server_bundle_output_path}. " \
"Ensure the bundle is built to this location."3. Method Naming Consistency
📋 Test CoverageThe test coverage is comprehensive and well-structured:
Minor suggestion: Consider adding an integration test that actually builds a server bundle to a private location and verifies it is loaded correctly. 🔒 Security ConsiderationsThe security implementation is solid:
One additional consideration: Document that users should also configure their web server (nginx/apache) to explicitly block access to server bundle patterns, as defense in depth. 🚀 PerformanceNo performance concerns identified. The path resolution is efficient and properly memoized where needed. 📝 DocumentationConsider adding:
✨ Overall AssessmentThis is a well-implemented security feature that addresses a real concern about SSR code exposure. The implementation is clean, maintains backward compatibility, and includes good test coverage. With the minor improvements suggested above (particularly around manifest handling), this will be a valuable addition to React on Rails. Recommendation: Approve with minor suggestions ✅ The code is ready to merge after considering the bundle path resolution suggestion. The other items are nice-to-haves that could be addressed in follow-up PRs. Great work on this security enhancement! 🎉 🤖 Generated with Claude Code |
Summary
This PR adds server bundle security features to prevent server bundles from being served from public directories, addressing security concerns around server-side rendering code exposure.
• Add
server_bundle_output_pathconfiguration - Allows specifying private locations for server bundles (e.g.,app/assets/builds)• Add
enforce_private_server_bundlessecurity option - When enabled, prevents fallback to public paths for server bundles• Improve bundle path resolution - Enhanced logic to check for server bundles in configured private locations first
• Add comprehensive validation - Ensures server bundles are outside public directory when enforcement is enabled
• Maintain backwards compatibility - All defaults preserve existing behavior
Security Benefits
Configuration Example
Test Plan
🤖 Generated with Claude Code
This change is
Summary by CodeRabbit
New Features
Refactor
Tests