Fix command injection vulnerability and improve code quality

lib/demo_scripts.rb

@@ -5,6 +5,7 @@
 require_relative 'demo_scripts/command_executor'
 require_relative 'demo_scripts/package_json_cache'
 require_relative 'demo_scripts/demo_manager'
+require_relative 'demo_scripts/github_spec_parser'
 require_relative 'demo_scripts/pre_flight_checks'
 require_relative 'demo_scripts/command_runner'
 require_relative 'demo_scripts/demo_creator'

lib/demo_scripts/demo_creator.rb

@@ -7,6 +7,8 @@
 module DemoScripts
   # Creates a new React on Rails demo
   class DemoCreator
+    include GitHubSpecParser
+
     def initialize(
       demo_name:,
       shakapacker_version: nil,
@@ -145,43 +147,6 @@ def add_gem_from_github(gem_name, version_spec)
       @runner.run!(cmd, dir: @demo_dir)
     end
 
-    def parse_github_spec(github_spec)
-      if github_spec.include?('@')
-        parts = github_spec.split('@', 2)
-        raise Error, 'Invalid GitHub spec: empty repository' if parts[0].empty?
-        raise Error, 'Invalid GitHub spec: empty branch' if parts[1].empty?
-
-        parts
-      else
-        [github_spec, nil]
-      end
-    end
-
-    def validate_github_repo(repo)
-      raise Error, 'Invalid GitHub repo: cannot be empty' if repo.nil? || repo.empty?
-
-      parts = repo.split('/')
-      raise Error, "Invalid GitHub repo format: expected 'org/repo', got '#{repo}'" unless parts.length == 2
-      raise Error, 'Invalid GitHub repo: empty organization' if parts[0].empty?
-      raise Error, 'Invalid GitHub repo: empty repository name' if parts[1].empty?
-
-      # Validate characters (GitHub allows alphanumeric, hyphens, underscores, periods)
-      valid_pattern = %r{\A[\w.-]+/[\w.-]+\z}
-      return if repo.match?(valid_pattern)
-
-      raise Error, "Invalid GitHub repo: '#{repo}' contains invalid characters"
-    end
-
-    def validate_github_branch(branch)
-      raise Error, 'Invalid GitHub branch: cannot be empty' if branch.nil? || branch.empty?
-
-      # Git branch names cannot contain certain characters
-      invalid_chars = ['..', '~', '^', ':', '?', '*', '[', '\\', ' ']
-      invalid_chars.each do |char|
-        raise Error, "Invalid GitHub branch: '#{branch}' contains invalid character '#{char}'" if branch.include?(char)
-      end
-    end
-
     def build_github_bundle_command(gem_name, repo, branch)
       cmd = ['bundle', 'add', gem_name, '--github', repo]
       cmd.push('--branch', branch) if branch
@@ -264,6 +229,8 @@ def convert_to_npm_github_url(version_spec)
     def build_github_npm_package(gem_name, version_spec)
       raise Error, 'Invalid gem name: cannot be empty' if gem_name.nil? || gem_name.strip.empty?
 
+      return if @dry_run
+
       github_spec = version_spec.sub('github:', '').strip
       repo, branch = parse_github_spec(github_spec)
 
@@ -272,6 +239,23 @@ def build_github_npm_package(gem_name, version_spec)
       Dir.mktmpdir("#{gem_name}-") do |temp_dir|
         clone_and_build_package(temp_dir, repo, branch, gem_name)
       end
+    rescue CommandError, IOError, SystemCallError => e
+      error_message = <<~ERROR
+        Failed to build npm package for #{gem_name}
+
+        Error: #{e.message}
+
+        This can happen if:
+        - The repository doesn't have a valid npm package structure
+        - Build dependencies are missing
+        - Network connectivity issues occurred during clone
+
+        You may need to manually build the package or use a published version.
+      ERROR
+
+      new_error = Error.new(error_message)
+      new_error.set_backtrace(e.backtrace)
+      raise new_error
     end
 
     def clone_and_build_package(temp_dir, repo, branch, gem_name)

lib/demo_scripts/github_spec_parser.rb

@@ -0,0 +1,52 @@
+# frozen_string_literal: true
+
+module DemoScripts
+  # Parses and validates GitHub repository specifications
+  module GitHubSpecParser
+    # Parses github:org/repo@branch format
+    # Returns [repo, branch] where branch can be nil
+    def parse_github_spec(github_spec)
+      if github_spec.include?('@')
+        parts = github_spec.split('@', 2)
+        raise Error, 'Invalid GitHub spec: empty repository' if parts[0].empty?
+        raise Error, 'Invalid GitHub spec: empty branch' if parts[1].empty?
+
+        parts
+      else
+        [github_spec, nil]
+      end
+    end
+
+    # Validates GitHub repository format (org/repo)
+    def validate_github_repo(repo)
+      raise Error, 'Invalid GitHub repo: cannot be empty' if repo.nil? || repo.empty?
+
+      parts = repo.split('/', -1) # Use -1 to keep empty strings
+      raise Error, "Invalid GitHub repo format: expected 'org/repo', got '#{repo}'" unless parts.length == 2
+      raise Error, 'Invalid GitHub repo: empty organization' if parts[0].empty?
+      raise Error, 'Invalid GitHub repo: empty repository name' if parts[1].empty?
+
+      # Validate characters (GitHub allows alphanumeric, hyphens, underscores, periods)
+      valid_pattern = %r{\A[\w.-]+/[\w.-]+\z}
+      return if repo.match?(valid_pattern)
+
+      raise Error, "Invalid GitHub repo: '#{repo}' contains invalid characters"
+    end
+
+    # Validates GitHub branch name according to Git ref naming rules
+    def validate_github_branch(branch)
+      raise Error, 'Invalid GitHub branch: cannot be empty' if branch.nil? || branch.empty?
+      raise Error, 'Invalid GitHub branch: cannot be just @' if branch == '@'
+
+      # Git branch names cannot contain certain characters
+      invalid_chars = ['..', '~', '^', ':', '?', '*', '[', '\\', ' ']
+      invalid_chars.each do |char|
+        raise Error, "Invalid GitHub branch: '#{branch}' contains invalid character '#{char}'" if branch.include?(char)
+      end
+
+      # Additional Git ref naming rules
+      raise Error, 'Invalid GitHub branch: cannot end with .lock' if branch.end_with?('.lock')
+      raise Error, 'Invalid GitHub branch: cannot contain @{' if branch.include?('@{')
+    end
+  end
+end

lib/demo_scripts/pre_flight_checks.rb

@@ -1,8 +1,12 @@
 # frozen_string_literal: true
 
+require 'open3'
+
 module DemoScripts
   # Pre-flight checks before creating/updating demos
   class PreFlightChecks
+    include GitHubSpecParser
+
     def initialize(demo_dir:, shakapacker_version: nil, react_on_rails_version: nil, verbose: true)
       @demo_dir = demo_dir
       @shakapacker_version = shakapacker_version
@@ -24,10 +28,9 @@ def run!
     private
 
     def check_target_directory!
-      return unless Dir.exist?(@demo_dir)
+      raise PreFlightCheckError, "Demo directory already exists: #{@demo_dir}" if Dir.exist?(@demo_dir)
 
       puts '✓ Target directory does not exist' if @verbose
-      raise PreFlightCheckError, "Demo directory already exists: #{@demo_dir}"
     end
 
     def check_git_repository!
@@ -60,21 +63,29 @@ def check_github_branches!
       check_github_branch_exists(@react_on_rails_version) if @react_on_rails_version
     end
 
+    # rubocop:disable Metrics/MethodLength
     def check_github_branch_exists(version_spec)
       return unless version_spec.start_with?('github:')
 
       github_spec = version_spec.sub('github:', '').strip
       repo, branch = parse_github_spec(github_spec)
 
+      # Validate repo and branch to prevent command injection
+      validate_github_repo(repo)
+      validate_github_branch(branch) if branch
+
       return unless branch # If no branch specified, will use default branch
 
       puts "  Checking if branch '#{branch}' exists in #{repo}..." if @verbose
 
-      # Use git ls-remote to check if branch exists
-      cmd = "git ls-remote --heads https://github.com/#{repo}.git refs/heads/#{branch} 2>&1"
-      output = `#{cmd}`
+      # Use Open3.capture2 for safe command execution
+      stdout, status = Open3.capture2(
+        'git', 'ls-remote', '--heads',
+        "https://github.com/#{repo}.git",
+        "refs/heads/#{branch}"
+      )
 
-      if output.strip.empty?
+      if stdout.strip.empty? || !status.success?
         error_message = <<~ERROR
           GitHub branch not found: #{repo}@#{branch}
 
@@ -91,14 +102,6 @@ def check_github_branch_exists(version_spec)
 
       puts "  ✓ Branch '#{branch}' exists in #{repo}" if @verbose
     end
-
-    def parse_github_spec(github_spec)
-      if github_spec.include?('@')
-        parts = github_spec.split('@', 2)
-        [parts[0], parts[1]]
-      else
-        [github_spec, nil]
-      end
-    end
+    # rubocop:enable Metrics/MethodLength
   end
 end

spec/demo_scripts/demo_creator_spec.rb

@@ -396,6 +396,17 @@
     end
 
     describe '#build_github_npm_package' do
+      # Override creator for these tests to use dry_run: false
+      subject(:creator) do
+        described_class.new(
+          demo_name: demo_name,
+          shakapacker_version: 'github:shakacode/shakapacker@fix-node-env-default',
+          react_on_rails_version: '~> 16.0',
+          dry_run: false,
+          skip_pre_flight: true
+        )
+      end
+
       it 'clones repository with branch' do
         allow(File).to receive(:directory?).and_return(false)
         allow(Dir).to receive(:mktmpdir).and_yield('/tmp/shakapacker-test')