diff --git a/lib/demo_scripts.rb b/lib/demo_scripts.rb index debbe94..55a59eb 100644 --- a/lib/demo_scripts.rb +++ b/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' diff --git a/lib/demo_scripts/demo_creator.rb b/lib/demo_scripts/demo_creator.rb index 0af3106..2c3667e 100644 --- a/lib/demo_scripts/demo_creator.rb +++ b/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) diff --git a/lib/demo_scripts/github_spec_parser.rb b/lib/demo_scripts/github_spec_parser.rb new file mode 100644 index 0000000..da7ba9b --- /dev/null +++ b/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 diff --git a/lib/demo_scripts/pre_flight_checks.rb b/lib/demo_scripts/pre_flight_checks.rb index 75498e0..9e65de1 100644 --- a/lib/demo_scripts/pre_flight_checks.rb +++ b/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 diff --git a/spec/demo_scripts/demo_creator_spec.rb b/spec/demo_scripts/demo_creator_spec.rb index 1dfb6ef..be0384d 100644 --- a/spec/demo_scripts/demo_creator_spec.rb +++ b/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') diff --git a/spec/demo_scripts/github_spec_parser_spec.rb b/spec/demo_scripts/github_spec_parser_spec.rb new file mode 100644 index 0000000..b849b71 --- /dev/null +++ b/spec/demo_scripts/github_spec_parser_spec.rb @@ -0,0 +1,244 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe DemoScripts::GitHubSpecParser do + # Create a test class that includes the module + let(:test_class) do + Class.new do + include DemoScripts::GitHubSpecParser + end + end + + let(:parser) { test_class.new } + + describe '#parse_github_spec' do + context 'with valid input' do + it 'parses repo without branch' do + repo, branch = parser.parse_github_spec('shakacode/shakapacker') + expect(repo).to eq('shakacode/shakapacker') + expect(branch).to be_nil + end + + it 'parses repo with branch' do + repo, branch = parser.parse_github_spec('shakacode/shakapacker@main') + expect(repo).to eq('shakacode/shakapacker') + expect(branch).to eq('main') + end + + it 'handles multiple @ symbols (uses first as delimiter)' do + repo, branch = parser.parse_github_spec('shakacode/shakapacker@branch@with@symbols') + expect(repo).to eq('shakacode/shakapacker') + expect(branch).to eq('branch@with@symbols') + end + + it 'parses repo with branch containing slashes' do + repo, branch = parser.parse_github_spec('shakacode/shakapacker@release/v1.0') + expect(repo).to eq('shakacode/shakapacker') + expect(branch).to eq('release/v1.0') + end + end + + context 'with invalid input' do + it 'raises error for empty repository' do + expect do + parser.parse_github_spec('@main') + end.to raise_error(DemoScripts::Error, /empty repository/) + end + + it 'raises error for empty branch' do + expect do + parser.parse_github_spec('shakacode/shakapacker@') + end.to raise_error(DemoScripts::Error, /empty branch/) + end + end + end + + describe '#validate_github_repo' do + context 'with valid repos' do + it 'accepts valid org/repo format' do + expect { parser.validate_github_repo('shakacode/shakapacker') }.not_to raise_error + end + + it 'accepts repos with hyphens' do + expect { parser.validate_github_repo('shaka-code/shaka-packer') }.not_to raise_error + end + + it 'accepts repos with underscores' do + expect { parser.validate_github_repo('shaka_code/shaka_packer') }.not_to raise_error + end + + it 'accepts repos with periods' do + expect { parser.validate_github_repo('shaka.code/shaka.packer') }.not_to raise_error + end + + it 'accepts repos with mixed valid characters' do + expect { parser.validate_github_repo('shaka-code_2.0/shaka.packer-v2_0') }.not_to raise_error + end + end + + context 'with invalid repos' do + it 'rejects nil repo' do + expect do + parser.validate_github_repo(nil) + end.to raise_error(DemoScripts::Error, /cannot be empty/) + end + + it 'rejects empty repo' do + expect do + parser.validate_github_repo('') + end.to raise_error(DemoScripts::Error, /cannot be empty/) + end + + it 'rejects repo without slash' do + expect do + parser.validate_github_repo('shakapacker') + end.to raise_error(DemoScripts::Error, %r{expected 'org/repo'}) + end + + it 'rejects repo with too many slashes' do + expect do + parser.validate_github_repo('shakacode/shakapacker/extra') + end.to raise_error(DemoScripts::Error, %r{expected 'org/repo'}) + end + + it 'rejects repo with empty organization' do + expect do + parser.validate_github_repo('/shakapacker') + end.to raise_error(DemoScripts::Error, /empty organization/) + end + + it 'rejects repo with empty repository name' do + expect do + parser.validate_github_repo('shakacode/') + end.to raise_error(DemoScripts::Error, /empty repository name/) + end + + it 'rejects repo with invalid characters (spaces)' do + expect do + parser.validate_github_repo('shaka code/shakapacker') + end.to raise_error(DemoScripts::Error, /invalid characters/) + end + + it 'rejects repo with invalid characters (special chars)' do + expect do + parser.validate_github_repo('shaka$code/shakapacker') + end.to raise_error(DemoScripts::Error, /invalid characters/) + end + end + end + + describe '#validate_github_branch' do + context 'with valid branches' do + it 'accepts simple branch name' do + expect { parser.validate_github_branch('main') }.not_to raise_error + end + + it 'accepts branch with hyphens' do + expect { parser.validate_github_branch('fix-security-bug') }.not_to raise_error + end + + it 'accepts branch with underscores' do + expect { parser.validate_github_branch('feature_new_api') }.not_to raise_error + end + + it 'accepts branch with slashes' do + expect { parser.validate_github_branch('release/v1.0') }.not_to raise_error + end + + it 'accepts branch with dots' do + expect { parser.validate_github_branch('v1.0.0') }.not_to raise_error + end + + it 'accepts branch with numbers' do + expect { parser.validate_github_branch('feature-123') }.not_to raise_error + end + end + + context 'with invalid branches' do + it 'rejects nil branch' do + expect do + parser.validate_github_branch(nil) + end.to raise_error(DemoScripts::Error, /cannot be empty/) + end + + it 'rejects empty branch' do + expect do + parser.validate_github_branch('') + end.to raise_error(DemoScripts::Error, /cannot be empty/) + end + + it 'rejects branch with ..' do + expect do + parser.validate_github_branch('feature..bug') + end.to raise_error(DemoScripts::Error, /invalid character/) + end + + it 'rejects branch with ~' do + expect do + parser.validate_github_branch('branch~1') + end.to raise_error(DemoScripts::Error, /invalid character/) + end + + it 'rejects branch with ^' do + expect do + parser.validate_github_branch('branch^1') + end.to raise_error(DemoScripts::Error, /invalid character/) + end + + it 'rejects branch with :' do + expect do + parser.validate_github_branch('branch:name') + end.to raise_error(DemoScripts::Error, /invalid character/) + end + + it 'rejects branch with ?' do + expect do + parser.validate_github_branch('branch?') + end.to raise_error(DemoScripts::Error, /invalid character/) + end + + it 'rejects branch with *' do + expect do + parser.validate_github_branch('branch*') + end.to raise_error(DemoScripts::Error, /invalid character/) + end + + it 'rejects branch with [' do + expect do + parser.validate_github_branch('branch[1]') + end.to raise_error(DemoScripts::Error, /invalid character/) + end + + it 'rejects branch with \\' do + expect do + parser.validate_github_branch('branch\\name') + end.to raise_error(DemoScripts::Error, /invalid character/) + end + + it 'rejects branch with spaces' do + expect do + parser.validate_github_branch('branch name') + end.to raise_error(DemoScripts::Error, /invalid character/) + end + + it 'rejects branch ending with .lock' do + expect do + parser.validate_github_branch('feature.lock') + end.to raise_error(DemoScripts::Error, /cannot end with \.lock/) + end + + it 'rejects branch containing @{' do + expect do + parser.validate_github_branch('branch@{0}') + end.to raise_error(DemoScripts::Error, /cannot contain @\{/) + end + + it 'rejects branch that is just @' do + expect do + parser.validate_github_branch('@') + end.to raise_error(DemoScripts::Error, /cannot be just @/) + end + end + end +end diff --git a/spec/demo_scripts/pre_flight_checks_spec.rb b/spec/demo_scripts/pre_flight_checks_spec.rb index 8eceb4b..ad90244 100644 --- a/spec/demo_scripts/pre_flight_checks_spec.rb +++ b/spec/demo_scripts/pre_flight_checks_spec.rb @@ -82,7 +82,11 @@ before do allow(checks).to receive(:system).with(/git rev-parse/).and_return(true) allow(checks).to receive(:system).with(/git diff-index/).and_return(true) - allow(checks).to receive(:`).with(/git ls-remote/).and_return('') + allow(Open3).to receive(:capture2).with( + 'git', 'ls-remote', '--heads', + 'https://github.com/shakacode/shakapacker.git', + 'refs/heads/nonexistent-branch' + ).and_return(['', instance_double(Process::Status, success?: false)]) end it 'raises PreFlightCheckError' do @@ -105,7 +109,11 @@ before do allow(checks).to receive(:system).with(/git rev-parse/).and_return(true) allow(checks).to receive(:system).with(/git diff-index/).and_return(true) - allow(checks).to receive(:`).with(/git ls-remote/).and_return('abc123 refs/heads/main') + allow(Open3).to receive(:capture2).with( + 'git', 'ls-remote', '--heads', + 'https://github.com/shakacode/shakapacker.git', + 'refs/heads/main' + ).and_return(['abc123 refs/heads/main', instance_double(Process::Status, success?: true)]) end it 'does not raise an error' do @@ -128,7 +136,7 @@ end it 'does not check branch existence (uses default)' do - expect(checks).not_to receive(:`).with(/git ls-remote/) + expect(Open3).not_to receive(:capture2) expect { checks.run! }.not_to raise_error end end