Improve server bundle security test coverage and fix misleading comments (#1815)

This commit addresses critical gaps in test coverage and terminology issues
found in the server bundle security implementation:

## Test Coverage Improvements

- **Added comprehensive tests for enforce_private_server_bundles=true**:
  - Tests for bundle_js_file_path() with enforcement enabled
  - Tests for server_bundle_js_file_path() with enforcement enabled
  - Tests for rsc_bundle_js_file_path() with enforcement enabled
  - Tests verify that public directories are never checked when enforcement is enabled

- **Enhanced existing test scenarios**:
  - Added tests for all file existence combinations when enforcement is disabled
  - Added proper fallback behavior tests (private -> public -> configured path)
  - Improved test organization with clearer context descriptions

## Bug Fixes and Code Quality

- **Fixed misleading terminology**:
  - Changed "auto-registration" to "auto-bundling" in comments and method names
  - Updated method name: update_gitignore_for_auto_registration -> update_gitignore_for_generated_bundles

- **Improved test reliability**:
  - Added proper mocking for all configuration dependencies
  - Used parametrized tests for comprehensive file state coverage
  - Added clear expectations that File.exist? should not be called on public paths when enforcement is enabled

## Security Testing

The new tests ensure that the critical security feature `enforce_private_server_bundles=true`
works as intended:
- Server bundles are never loaded from public directories when enforcement is enabled
- RSC bundles follow the same security model as server bundles
- Fallback behavior is properly disabled when security enforcement is active

All tests pass with 0 failures. RuboCop linting passes with 0 violations.

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-authored-by: Claude <[email protected]>

Author: AbanoubGhadban

lib/generators/react_on_rails/base_generator.rb

@@ -95,7 +95,7 @@ def add_base_gems_to_gemfile
         run "bundle"
       end
 
-      def update_gitignore_for_auto_registration
+      def update_gitignore_for_generated_bundles
         gitignore_path = File.join(destination_root, ".gitignore")
         return unless File.exist?(gitignore_path)
 

lib/generators/react_on_rails/react_no_redux_generator.rb

@@ -37,7 +37,7 @@ def create_appropriate_templates
           component_name: "HelloWorld"
         }
 
-        # Only create the view template - no manual bundle needed for auto registration
+        # Only create the view template - no manual bundle needed for auto-bundling
         template("#{base_path}/app/views/hello_world/index.html.erb.tt",
                  "app/views/hello_world/index.html.erb", config)
       end

lib/generators/react_on_rails/react_with_redux_generator.rb

@@ -68,7 +68,7 @@ def create_appropriate_templates
           component_name: "HelloWorldApp"
         }
 
-        # Only create the view template - no manual bundle needed for auto registration
+        # Only create the view template - no manual bundle needed for auto-bundling
         template("#{base_path}/app/views/hello_world/index.html.erb.tt",
                  "app/views/hello_world/index.html.erb", config)
       end

spec/react_on_rails/support/shared_examples/react_no_redux_generator_examples.rb

@@ -2,7 +2,7 @@
 
 shared_examples "no_redux_generator" do
   it "creates appropriate templates" do
-    # No manual bundle for non-Redux (auto-registration only)
+    # No manual bundle for non-Redux (auto-bundling only)
     assert_no_file("app/javascript/packs/hello-world-bundle.js")
 
     assert_file("app/views/hello_world/index.html.erb") do |contents|

spec/react_on_rails/utils_spec.rb

@@ -146,8 +146,9 @@ def mock_dev_server_running
             context "with server bundle (SSR/RSC) file not in manifest" do
               let(:server_bundle_name) { "server-bundle.js" }
               let(:ssr_generated_path) { File.expand_path(File.join("ssr-generated", server_bundle_name)) }
+              let(:public_path) { File.expand_path(File.join(packer_public_output_path, server_bundle_name)) }
 
-              context "with server_bundle_output_path configured" do
+              context "with server_bundle_output_path configured and enforce_private_server_bundles=false" do
                 before do
                   mock_missing_manifest_entry(server_bundle_name)
                   allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_js_file")
@@ -158,10 +159,23 @@ def mock_dev_server_running
                     .and_return(false)
                 end
 
-                it "returns configured path directly without checking existence" do
-                  # When enforce_private_server_bundles is false, it will check File.exist?
-                  # for both private and public paths, but should still return the configured path
-                  public_path = File.expand_path(File.join(packer_public_output_path, server_bundle_name))
+                it "returns private path when it exists even if public path also exists" do
+                  allow(File).to receive(:exist?).with(ssr_generated_path).and_return(true)
+                  allow(File).to receive(:exist?).with(public_path).and_return(true)
+
+                  result = described_class.bundle_js_file_path(server_bundle_name)
+                  expect(result).to eq(ssr_generated_path)
+                end
+
+                it "returns public path when private path does not exist and public path exists" do
+                  allow(File).to receive(:exist?).with(ssr_generated_path).and_return(false)
+                  allow(File).to receive(:exist?).with(public_path).and_return(true)
+
+                  result = described_class.bundle_js_file_path(server_bundle_name)
+                  expect(result).to eq(public_path)
+                end
+
+                it "returns configured path if both private and public paths do not exist" do
                   allow(File).to receive(:exist?).with(ssr_generated_path).and_return(false)
                   allow(File).to receive(:exist?).with(public_path).and_return(false)
 
@@ -170,6 +184,41 @@ def mock_dev_server_running
                 end
               end
 
+              context "with server_bundle_output_path configured and enforce_private_server_bundles=true" do
+                before do
+                  mock_missing_manifest_entry(server_bundle_name)
+                  allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_js_file")
+                    .and_return(server_bundle_name)
+                  allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_output_path")
+                    .and_return("ssr-generated")
+                  allow(ReactOnRails).to receive_message_chain("configuration.enforce_private_server_bundles")
+                    .and_return(true)
+                end
+
+                # It should always return the ssr_generated_path, regardless of which files exist
+                file_states_combinations = [
+                  { ssr_exists: true, public_exists: true },
+                  { ssr_exists: true, public_exists: false },
+                  { ssr_exists: false, public_exists: true },
+                  { ssr_exists: false, public_exists: false }
+                ]
+                file_states_combinations.each do |file_states|
+                  it "returns private path when enforce_private_server_bundles=true " \
+                     "(ssr_exists=#{file_states[:ssr_exists]}, " \
+                     "public_exists=#{file_states[:public_exists]})" do
+                    allow(File).to receive(:exist?)
+                      .with(ssr_generated_path)
+                      .and_return(file_states[:ssr_exists])
+                    allow(File).to receive(:exist?)
+                      .with(public_path)
+                      .and_return(file_states[:public_exists])
+
+                    result = described_class.bundle_js_file_path(server_bundle_name)
+                    expect(result).to eq(ssr_generated_path)
+                  end
+                end
+              end
+
               context "without server_bundle_output_path configured" do
                 before do
                   mock_missing_manifest_entry(server_bundle_name)
@@ -190,27 +239,65 @@ def mock_dev_server_running
 
             context "with RSC bundle file not in manifest" do
               let(:rsc_bundle_name) { "rsc-bundle.js" }
+              let(:public_path) { File.expand_path(File.join(packer_public_output_path, rsc_bundle_name)) }
               let(:ssr_generated_path) { File.expand_path(File.join("ssr-generated", rsc_bundle_name)) }
 
-              before do
-                mock_missing_manifest_entry(rsc_bundle_name)
-                allow(ReactOnRails).to receive_message_chain("configuration.rsc_bundle_js_file")
-                  .and_return(rsc_bundle_name)
-                allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_output_path")
-                  .and_return("ssr-generated")
-                allow(ReactOnRails).to receive_message_chain("configuration.enforce_private_server_bundles")
-                  .and_return(false)
+              context "with enforce_private_server_bundles=false" do
+                before do
+                  mock_missing_manifest_entry(rsc_bundle_name)
+                  allow(ReactOnRails).to receive_message_chain("configuration.rsc_bundle_js_file")
+                    .and_return(rsc_bundle_name)
+                  allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_output_path")
+                    .and_return("ssr-generated")
+                  allow(ReactOnRails).to receive_message_chain("configuration.enforce_private_server_bundles")
+                    .and_return(false)
+                end
+
+                it "returns private path when it exists even if public path also exists" do
+                  allow(File).to receive(:exist?).with(ssr_generated_path).and_return(true)
+                  expect(File).not_to receive(:exist?).with(public_path)
+
+                  result = described_class.bundle_js_file_path(rsc_bundle_name)
+                  expect(result).to eq(ssr_generated_path)
+                end
+
+                it "fallbacks to public path when private path does not exist and public path exists" do
+                  allow(File).to receive(:exist?).with(ssr_generated_path).and_return(false)
+                  allow(File).to receive(:exist?).with(public_path).and_return(true)
+
+                  result = described_class.bundle_js_file_path(rsc_bundle_name)
+                  expect(result).to eq(public_path)
+                end
+
+                it "returns configured path if both private and public paths do not exist" do
+                  allow(File).to receive(:exist?).with(ssr_generated_path).and_return(false)
+                  allow(File).to receive(:exist?).with(public_path).and_return(false)
+
+                  result = described_class.bundle_js_file_path(rsc_bundle_name)
+                  expect(result).to eq(ssr_generated_path)
+                end
               end
 
-              it "treats RSC bundles as server bundles and returns configured path directly" do
-                # When enforce_private_server_bundles is false, it will check File.exist?
-                # for both private and public paths, but should still return the configured path
-                public_path = File.expand_path(File.join(packer_public_output_path, rsc_bundle_name))
-                allow(File).to receive(:exist?).with(ssr_generated_path).and_return(false)
-                allow(File).to receive(:exist?).with(public_path).and_return(false)
+              context "with enforce_private_server_bundles=true" do
+                before do
+                  mock_missing_manifest_entry(rsc_bundle_name)
+                  allow(ReactOnRails).to receive_message_chain("configuration.rsc_bundle_js_file")
+                    .and_return(rsc_bundle_name)
+                  allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_output_path")
+                    .and_return("ssr-generated")
+                  allow(ReactOnRails).to receive_message_chain("configuration.enforce_private_server_bundles")
+                    .and_return(true)
+                end
+
+                it "enforces private RSC bundles and never checks public directory" do
+                  public_path = File.expand_path(File.join(packer_public_output_path, rsc_bundle_name))
+
+                  # Should not check public path when enforcement is enabled
+                  expect(File).not_to receive(:exist?).with(public_path)
 
-                result = described_class.bundle_js_file_path(rsc_bundle_name)
-                expect(result).to eq(ssr_generated_path)
+                  result = described_class.bundle_js_file_path(rsc_bundle_name)
+                  expect(result).to eq(ssr_generated_path)
+                end
               end
             end
           end
@@ -263,7 +350,7 @@ def mock_dev_server_running
               expect(path).to end_with("ssr-generated/#{server_bundle_name}")
             end
 
-            context "with server_bundle_output_path configured" do
+            context "with server_bundle_output_path configured and enforce_private_server_bundles=false" do
               it "returns the configured path directly without checking file existence" do
                 server_bundle_name = "server-bundle.js"
                 mock_bundle_configs(server_bundle_name: server_bundle_name)
@@ -283,6 +370,26 @@ def mock_dev_server_running
                 expect(path).to end_with("ssr-generated/#{server_bundle_name}")
               end
             end
+
+            context "with server_bundle_output_path configured and enforce_private_server_bundles=true" do
+              it "returns the private path without checking public directories" do
+                server_bundle_name = "server-bundle.js"
+                mock_missing_manifest_entry(server_bundle_name)
+                allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_js_file")
+                  .and_return(server_bundle_name)
+                allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_output_path")
+                  .and_return("ssr-generated")
+                allow(ReactOnRails).to receive_message_chain("configuration.enforce_private_server_bundles")
+                  .and_return(true)
+
+                # Should not check public paths when enforcement is enabled
+                public_path = File.expand_path(File.join(packer_public_output_path, server_bundle_name))
+                expect(File).not_to receive(:exist?).with(public_path)
+
+                path = described_class.server_bundle_js_file_path
+                expect(path).to end_with("ssr-generated/#{server_bundle_name}")
+              end
+            end
           end
 
           context "with server file in the manifest, used for client", packer_type.to_sym do
@@ -346,14 +453,38 @@ def mock_dev_server_running
           include_context "with #{packer_type} enabled"
 
           context "with server file not in manifest", packer_type.to_sym do
-            it "returns the private ssr-generated path for RSC bundles" do
-              server_bundle_name = "rsc-bundle.js"
-              mock_bundle_configs(rsc_bundle_name: server_bundle_name)
-              mock_missing_manifest_entry(server_bundle_name)
+            context "with enforce_private_server_bundles=false" do
+              it "returns the private ssr-generated path for RSC bundles" do
+                server_bundle_name = "rsc-bundle.js"
+                mock_bundle_configs(rsc_bundle_name: server_bundle_name)
+                mock_missing_manifest_entry(server_bundle_name)
 
-              path = described_class.rsc_bundle_js_file_path
+                path = described_class.rsc_bundle_js_file_path
 
-              expect(path).to end_with("ssr-generated/#{server_bundle_name}")
+                expect(path).to end_with("ssr-generated/#{server_bundle_name}")
+              end
+            end
+
+            context "with enforce_private_server_bundles=true" do
+              it "returns the private ssr-generated path for RSC bundles without checking public paths" do
+                server_bundle_name = "rsc-bundle.js"
+                mock_missing_manifest_entry(server_bundle_name)
+                allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_js_file")
+                  .and_return("server-bundle.js") # Different from RSC bundle name
+                allow(ReactOnRails).to receive_message_chain("configuration.rsc_bundle_js_file")
+                  .and_return(server_bundle_name)
+                allow(ReactOnRails).to receive_message_chain("configuration.server_bundle_output_path")
+                  .and_return("ssr-generated")
+                allow(ReactOnRails).to receive_message_chain("configuration.enforce_private_server_bundles")
+                  .and_return(true)
+
+                # Should not check public paths when enforcement is enabled
+                public_path = File.expand_path(File.join(packer_public_output_path, server_bundle_name))
+                expect(File).not_to receive(:exist?).with(public_path)
+
+                path = described_class.rsc_bundle_js_file_path
+                expect(path).to end_with("ssr-generated/#{server_bundle_name}")
+              end
             end
           end