Security fixes and generator improvements

- Fix command injection vulnerabilities in dev tools
  - Replace string interpolation with array-form system() calls
  - Use Open3.capture2 instead of backticks for pgrep
  - Add procfile path validation
  - Harden PID validation with Integer() and exception handling

- Improve Shakapacker installation in generators
  - Use bundle add + bundle exec for proper gem management
  - Return boolean from ensure_shakapacker_installed for error handling
  - Enhance gem detection with broader rescue clause

- Modernize package manager usage
  - Replace yarn references with npm in generators and templates
  - Update Redux generator to use npm install instead of yarn add

- Fix spec file issues
  - Add missing require statements for dev classes
  - Replace RSpec anti-patterns (before(:all) with around hooks)
  - Fix Kernel mocking (allow_any_instance_of -> allow(Kernel))
  - Use proper SystemExit expectations instead of stubbing exit

- Remove obsolete registration.js.tt template (v15 uses auto-registration)
- Fix environment variable concatenation in rake task

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

Co-Authored-By: Claude <[email protected]>

Author: justin808

lib/generators/react_on_rails/install_generator.rb

@@ -27,7 +27,8 @@ class InstallGenerator < Rails::Generators::Base
 
       def run_generators
         if installation_prerequisites_met? || options.ignore_warnings?
-          ensure_shakapacker_installed
+          return unless ensure_shakapacker_installed
+
           invoke_generators
           add_bin_scripts
           add_post_install_message
@@ -91,24 +92,36 @@ def add_post_install_message
       end
 
       def ensure_shakapacker_installed
-        return if shakapacker_installed?
+        return true if shakapacker_installed?
 
         GeneratorMessages.add_info("Shakapacker not detected. Installing Shakapacker...")
 
-        result = system("rails shakapacker:install")
-        unless result
-          GeneratorMessages.add_error("Failed to install Shakapacker automatically. " \
-                                      "Please run 'rails shakapacker:install' manually.")
-          return
+        added = system("bundle", "add", "shakapacker")
+        unless added
+          GeneratorMessages.add_error(<<~MSG.strip)
+            Failed to add Shakapacker to your Gemfile.
+            Please run 'bundle add shakapacker' manually and re-run the generator.
+          MSG
+          return false
+        end
+
+        installed = system("bundle", "exec", "rails", "shakapacker:install")
+        unless installed
+          GeneratorMessages.add_error(<<~MSG.strip)
+            Failed to install Shakapacker automatically.
+            Please run 'bundle exec rails shakapacker:install' manually.
+          MSG
+          return false
         end
 
         GeneratorMessages.add_info("Shakapacker installed successfully!")
+        true
       end
 
       def shakapacker_installed?
         Gem::Specification.find_by_name("shakapacker")
         true
-      rescue Gem::MissingSpecError
+      rescue Gem::MissingSpecError, Gem::LoadError
         false
       end
 

lib/generators/react_on_rails/react_with_redux_generator.rb

@@ -59,8 +59,8 @@ def create_appropriate_templates
                  "app/views/hello_world/index.html.erb", config)
       end
 
-      def add_redux_yarn_dependencies
-        run "yarn add redux react-redux"
+      def add_redux_npm_dependencies
+        run "npm install redux react-redux"
       end
 
       def add_redux_specific_messages

lib/generators/react_on_rails/templates/base/base/app/javascript/packs/registration.js.tt

@@ -20,7 +20,7 @@ ReactOnRails.configure do |config|
   #
   # ReactOnRails::TestHelper.configure_rspec_to_compile_assets(config)
   #
-  # with rspec then this controls what yarn command is run
+  # with rspec then this controls what npm command is run
   # to automatically refresh your webpack assets on every test run.
   #
   # Alternately, you can remove the `ReactOnRails::TestHelper.configure_rspec_to_compile_assets`

lib/generators/react_on_rails/templates/base/base/config/initializers/react_on_rails.rb.tt

@@ -30,7 +30,19 @@ def cleanup_rails_pid_file
           server_pid_file = "tmp/pids/server.pid"
           return false unless File.exist?(server_pid_file)
 
-          pid = File.read(server_pid_file).to_i
+          pid_content = File.read(server_pid_file).strip
+          begin
+            pid = Integer(pid_content)
+            # PIDs must be > 1 (0 is kernel, 1 is init)
+            if pid <= 1
+              remove_file_if_exists(server_pid_file, "stale Rails pid file")
+              return true
+            end
+          rescue ArgumentError, TypeError
+            remove_file_if_exists(server_pid_file, "stale Rails pid file")
+            return true
+          end
+
           return false if process_running?(pid)
 
           remove_file_if_exists(server_pid_file, "stale Rails pid file")
@@ -45,6 +57,12 @@ def process_running?(pid)
           true
         rescue Errno::ESRCH
           false
+        rescue Errno::EPERM
+          # Process exists but we don't have permission to signal it
+          true
+        rescue ArgumentError, RangeError
+          # Invalid PID (negative, too large, wrong type)
+          false
         end
 
         def remove_file_if_exists(file_path, description)

lib/react_on_rails/dev/file_manager.rb

@@ -5,7 +5,8 @@ module Dev
     class ProcessManager
       class << self
         def installed?(process)
-          IO.popen "#{process} -v"
+          IO.popen([process, "-v"]) { |io| io.close }
+          true
         rescue Errno::ENOENT
           false
         end
@@ -21,13 +22,19 @@ def ensure_procfile(procfile)
         end
 
         def run_with_process_manager(procfile)
+          # Validate procfile path for security
+          unless valid_procfile_path?(procfile)
+            warn "ERROR: Invalid procfile path: #{procfile}"
+            exit 1
+          end
+
           # Clean up stale files before starting
           FileManager.cleanup_stale_files
 
           if installed?("overmind")
-            system "overmind start -f #{procfile}"
+            system("overmind", "start", "-f", procfile)
           elsif installed?("foreman")
-            system "foreman start -f #{procfile}"
+            system("foreman", "start", "-f", procfile)
           else
             warn <<~MSG
               NOTICE:
@@ -36,6 +43,18 @@ def run_with_process_manager(procfile)
             exit 1
           end
         end
+
+        private
+
+        def valid_procfile_path?(procfile)
+          # Reject paths with shell metacharacters
+          return false if procfile.match?(/[;&|`$(){}[\]<>]/)
+
+          # Ensure it's a readable file
+          File.readable?(procfile)
+        rescue
+          false
+        end
       end
     end
   end

lib/react_on_rails/dev/process_manager.rb

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require "English"
+require "open3"
 
 module ReactOnRails
   module Dev
@@ -58,7 +59,11 @@ def kill_running_processes
         end
 
         def find_process_pids(pattern)
-          `pgrep -f "#{pattern}" 2>/dev/null`.split("\n").map(&:to_i).reject { |pid| pid == Process.pid }
+          stdout, _status = Open3.capture2("pgrep", "-f", pattern, err: File::NULL)
+          stdout.split("\n").map(&:to_i).reject { |pid| pid == Process.pid }
+        rescue Errno::ENOENT
+          # pgrep command not found
+          []
         end
 
         def terminate_processes(pids)

lib/react_on_rails/dev/server_manager.rb

@@ -119,7 +119,13 @@ def run_tests_in(dir, options = {})
 
   command_name = options.fetch(:command_name, path.basename)
   rspec_args = options.fetch(:rspec_args, "")
-  env_vars = "#{options.fetch(:env_vars, '')} TEST_ENV_COMMAND_NAME=\"#{command_name}\""
-  env_vars << "COVERAGE=true" if ENV["USE_COVERALLS"]
+
+  # Build environment variables as an array for proper spacing
+  env_tokens = []
+  env_tokens << options.fetch(:env_vars, '').strip unless options.fetch(:env_vars, '').strip.empty?
+  env_tokens << "TEST_ENV_COMMAND_NAME=\"#{command_name}\""
+  env_tokens << "COVERAGE=true" if ENV["USE_COVERALLS"]
+
+  env_vars = env_tokens.join(' ')
   sh_in_dir(path.realpath, "#{env_vars} bundle exec rspec #{rspec_args}")
 end

rakelib/run_rspec.rake

@@ -1,6 +1,7 @@
 # frozen_string_literal: true
 
 require_relative "../spec_helper"
+require "react_on_rails/dev/file_manager"
 
 RSpec.describe ReactOnRails::Dev::FileManager do
   # Suppress stdout/stderr during tests
@@ -18,12 +19,12 @@
 
   describe ".cleanup_stale_files" do
     before do
-      allow_any_instance_of(Kernel).to receive(:`).and_return("")
+      allow(Kernel).to receive(:`).and_return("")
     end
 
     context "when overmind is not running" do
       before do
-        allow_any_instance_of(Kernel).to receive(:`).with("pgrep -f \"overmind\" 2>/dev/null").and_return("")
+        allow(Kernel).to receive(:`).with("pgrep -f \"overmind\" 2>/dev/null").and_return("")
       end
 
       it "removes stale overmind socket files" do
@@ -47,7 +48,7 @@
 
     context "when overmind is running" do
       before do
-        allow_any_instance_of(Kernel).to receive(:`).with("pgrep -f \"overmind\" 2>/dev/null").and_return("1234")
+        allow(Kernel).to receive(:`).with("pgrep -f \"overmind\" 2>/dev/null").and_return("1234")
         allow(File).to receive(:exist?).and_return(false)
       end
 
@@ -61,7 +62,7 @@
 
     context "when Rails server pid file exists" do
       before do
-        allow_any_instance_of(Kernel).to receive(:`).with("pgrep -f \"overmind\" 2>/dev/null").and_return("")
+        allow(Kernel).to receive(:`).with("pgrep -f \"overmind\" 2>/dev/null").and_return("")
         allow(File).to receive(:exist?).with(".overmind.sock").and_return(false)
         allow(File).to receive(:exist?).with("tmp/sockets/overmind.sock").and_return(false)
         allow(File).to receive(:exist?).with("tmp/pids/server.pid").and_return(true)

spec/react_on_rails/dev/file_manager_spec.rb

@@ -1,42 +1,43 @@
 # frozen_string_literal: true
 
 require_relative "../spec_helper"
+require "react_on_rails/dev/pack_generator"
 
 RSpec.describe ReactOnRails::Dev::PackGenerator do
-  # Suppress stdout/stderr during tests
-  before(:all) do
-    @original_stderr = $stderr
-    @original_stdout = $stdout
-    $stderr = File.open(File::NULL, "w")
-    $stdout = File.open(File::NULL, "w")
-  end
-
-  after(:all) do
-    $stderr = @original_stderr
-    $stdout = @original_stdout
+  # Suppress stdout/stderr during tests using around hook
+  around do |example|
+    original_stderr = $stderr
+    original_stdout = $stdout
+    begin
+      $stderr = File.open(File::NULL, "w")
+      $stdout = File.open(File::NULL, "w")
+      example.run
+    ensure
+      $stderr = original_stderr
+      $stdout = original_stdout
+    end
   end
 
   describe ".generate" do
     it "runs pack generation successfully in verbose mode" do
       command = "bundle exec rake react_on_rails:generate_packs"
-      allow_any_instance_of(Kernel).to receive(:system).with(command).and_return(true)
+      allow(Kernel).to receive(:system).with(command).and_return(true)
 
       expect { described_class.generate(verbose: true) }.not_to raise_error
     end
 
     it "runs pack generation successfully in quiet mode" do
       command = "bundle exec rake react_on_rails:generate_packs > /dev/null 2>&1"
-      allow_any_instance_of(Kernel).to receive(:system).with(command).and_return(true)
+      allow(Kernel).to receive(:system).with(command).and_return(true)
 
       expect { described_class.generate(verbose: false) }.not_to raise_error
     end
 
     it "exits with error when pack generation fails" do
       command = "bundle exec rake react_on_rails:generate_packs > /dev/null 2>&1"
-      allow_any_instance_of(Kernel).to receive(:system).with(command).and_return(false)
-      expect_any_instance_of(Kernel).to receive(:exit).with(1)
+      allow(Kernel).to receive(:system).with(command).and_return(false)
 
-      described_class.generate(verbose: false)
+      expect { described_class.generate(verbose: false) }.to raise_error(SystemExit)
     end
   end
 end