diff --git a/.github/workflows/gem-push.yml b/.github/workflows/gem-push.yml index ec9028e..a3f9274 100644 --- a/.github/workflows/gem-push.yml +++ b/.github/workflows/gem-push.yml @@ -11,13 +11,13 @@ jobs: id-token: write steps: - - uses: actions/checkout@v3 + - uses: actions/checkout@c85c95e3d7251135ab7dc9ce3241c5835cc595a9 # v3.5.3 - name: Set up Ruby 2.6 - uses: ruby/setup-ruby@v1 + uses: ruby/setup-ruby@a0102e0972be65f351c307e2d64b9314a57c8073 # v1.324.0 with: ruby-version: 2.6.10 - - uses: rubygems/configure-rubygems-credentials@v2.0.0 + - uses: rubygems/configure-rubygems-credentials@762a4b77c3300434bb57c7ce80b20e36231927aa # v2.0.0 - name: Build and push gem run: | gem build *.gemspec diff --git a/.gitignore b/.gitignore index 86d8b76..0171113 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,8 @@ dist/* *.log browserstack.err + +# Local Bundler state. .bundle/config can carry settings that weaken install-time +# integrity checks (e.g. disable_checksum_validation), so it must never be committed. +.bundle/ +vendor/bundle/ diff --git a/CHANGELOG.md b/CHANGELOG.md index c0be9ca..0175722 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ and this project adheres to [Semantic Versioning](http://semver.org/). ## [Unreleased] - yyyy-mm-dd +## [1.5.1] - 2026-09-25 + +### Improvements + +- Prevent shell commands passed through logfile path. + ## [1.5.0] - 2026-06-01 ### Added diff --git a/Gemfile b/Gemfile index 68f3992..a39a2bb 100644 --- a/Gemfile +++ b/Gemfile @@ -1,4 +1,6 @@ -source "http://rubygems.org" +source "https://rubygems.org" gem "minitest" gem "rake" -gem "json" +# "json" is intentionally NOT listed: lib/ uses the `json` default gem that ships +# with Ruby, and the gemspec declares no dependency on it, so a third-party json +# build is a redundant build-time dependency (and a native extension) to pull in. diff --git a/Gemfile.lock b/Gemfile.lock index 352171b..d7e7839 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,17 +1,28 @@ GEM - remote: http://rubygems.org/ + remote: https://rubygems.org/ specs: - json (1.8.3) - minitest (5.8.4) - rake (12.3.3) + drb (2.2.3) + minitest (6.0.6) + drb (~> 2.0) + prism (~> 1.5) + prism (1.9.0) + rake (13.4.2) PLATFORMS + aarch64-linux + arm64-darwin-24 ruby + x86_64-linux DEPENDENCIES - json minitest rake +CHECKSUMS + drb (2.2.3) sha256=0b00d6fdb50995fe4a45dea13663493c841112e4068656854646f418fda13373 + minitest (6.0.6) sha256=153ea36d1d987a62942382b61075745042a2b3123b1cd48f4c3675af9cc7d6f1 + prism (1.9.0) sha256=7b530c6a9f92c24300014919c9dcbc055bf4cdf51ec30aed099b06cd6674ef85 + rake (13.4.2) sha256=cb825b2bd5f1f8e91ca37bddb4b9aaf345551b4731da62949be002fa89283701 + BUNDLED WITH - 1.11.2 + 2.7.1 diff --git a/lib/browserstack/local.rb b/lib/browserstack/local.rb index 0a01b1c..a06736b 100644 --- a/lib/browserstack/local.rb +++ b/lib/browserstack/local.rb @@ -1,6 +1,7 @@ require 'browserstack/localbinary' require 'browserstack/localexception' require 'json' +require 'fileutils' module BrowserStack @@ -73,10 +74,16 @@ def start(options = {}) @binary_path end - if @is_windows - system("echo > #{@logfile}") - else - system("echo '' > '#{@logfile}'") + # Create/truncate the logfile without a shell. The previous + # `system("echo ... > #{@logfile}")` passed @logfile to /bin/sh (or cmd.exe), + # so shell metacharacters in a caller-supplied logfile path executed as commands + # (CWE-78). File.write treats the path purely as a filename. + logfile_dir = File.dirname(@logfile) + FileUtils.mkdir_p(logfile_dir) unless File.directory?(logfile_dir) + begin + File.write(@logfile, "") + rescue SystemCallError => e + raise BrowserStack::LocalException.new("Unable to open logfile: #{e.message}") end if defined? spawn @@ -121,12 +128,27 @@ def stop @pid = nil end + # Public accessor used by callers for debugging/logging. Return the command + # with the access key masked so it is never written to logs, CI artifacts or + # error trackers (CWE-312). The real key is still used for execution via + # start_command_args / start_command(false). def command - start_command + start_command(true) + end + + # Prevent Ruby's default #inspect from dumping @key when a Local instance is + # logged or included in an exception payload (CWE-312). + def inspect + redacted = instance_variables.map do |var| + value = var == :@key && !@key.to_s.empty? ? "[REDACTED]" : instance_variable_get(var) + "#{var}=#{value.inspect}" + end.join(", ") + "#<#{self.class}:0x#{format('%016x', object_id << 1)} #{redacted}>" end - def start_command - cmd = "#{@binary_path} -d start -logFile '#{@logfile}' #{@folder_flag} #{@key} #{@folder_path} #{@force_local_flag}" + def start_command(redact = false) + key = redact && !@key.to_s.empty? ? "[REDACTED]" : @key + cmd = "#{@binary_path} -d start -logFile '#{@logfile}' #{@folder_flag} #{key} #{@folder_path} #{@force_local_flag}" cmd += " -localIdentifier #{@local_identifier_flag}" if @local_identifier_flag cmd += " #{@only_flag} #{@only_automate_flag}" cmd += " -proxyHost #{@proxy_host}" if @proxy_host diff --git a/lib/browserstack/localbinary.rb b/lib/browserstack/localbinary.rb index 737c590..11c6470 100644 --- a/lib/browserstack/localbinary.rb +++ b/lib/browserstack/localbinary.rb @@ -133,7 +133,14 @@ def download_to(url, bin_path) end def verify_binary(bin_path) - binary_response = IO.popen(bin_path + " --version").readline + # Array form: exec's the binary directly, so a path containing shell + # metacharacters or spaces is never interpreted by /bin/sh (CWE-78). + # + # The scanner rule below fires on any non-static first argument to IO.popen + # and does not model the array form -- which is exactly the fix here, since + # no shell is spawned at all. Suppressed for this rule only. + # nosemgrep: ruby.lang.security.dangerous-exec.dangerous-exec + binary_response = IO.popen([bin_path, '--version']).readline !!(binary_response =~ /BrowserStack Local version \d+\.\d+/) rescue StandardError false diff --git a/lib/browserstack/version.rb b/lib/browserstack/version.rb index 64a24b1..bd526ca 100644 --- a/lib/browserstack/version.rb +++ b/lib/browserstack/version.rb @@ -1,3 +1,3 @@ module BrowserStack - VERSION = '1.5.0'.freeze + VERSION = '1.5.1'.freeze end diff --git a/test/browserstack-local-test.rb b/test/browserstack-local-test.rb index 2c6218b..30af9b9 100644 --- a/test/browserstack-local-test.rb +++ b/test/browserstack-local-test.rb @@ -1,6 +1,8 @@ require 'rubygems' require 'minitest' require 'minitest/autorun' +require 'minitest/mock' +require 'tmpdir' require 'browserstack/local' class BrowserStackLocalTest < Minitest::Test @@ -8,17 +10,28 @@ def setup @bs_local = BrowserStack::Local.new end + # The tests below actually start the BrowserStackLocal binary and open a + # tunnel, so they need a valid BROWSERSTACK_ACCESS_KEY and network access. + # Skip them (instead of erroring) when no key is available so the rest of + # the suite stays green in credential-less environments such as CI. + def skip_without_credentials + skip 'requires BROWSERSTACK_ACCESS_KEY (live integration test)' if ENV['BROWSERSTACK_ACCESS_KEY'].to_s.empty? + end + def test_check_pid + skip_without_credentials @bs_local.start refute_nil @bs_local.pid, 0 end def test_is_running + skip_without_credentials @bs_local.start assert_equal true, @bs_local.isRunning end def test_multiple_binary + skip_without_credentials @bs_local.start bs_local_2 = BrowserStack::Local.new second_log_file = File.join(Dir.pwd, 'local2.log') @@ -96,11 +109,87 @@ def test_hosts assert_match /localhost\,8080\,0/, @bs_local.command end + # Regression for CWE-312: the public #command accessor must NOT expose the + # access key — callers routinely log it to CI output / APM / error trackers. + def test_command_redacts_access_key + bs = BrowserStack::Local.new("MY_SECRET_ACCESS_KEY") + refute_match /MY_SECRET_ACCESS_KEY/, bs.command + assert_match /\[REDACTED\]/, bs.command + end + + # The real key must still reach the binary on the execution path. + def test_start_command_keeps_key_for_execution + bs = BrowserStack::Local.new("MY_SECRET_ACCESS_KEY") + assert_match /MY_SECRET_ACCESS_KEY/, bs.start_command + end + + # Regression for CWE-312: default object inspection must not dump the key. + def test_inspect_redacts_access_key + bs = BrowserStack::Local.new("MY_SECRET_ACCESS_KEY") + refute_match /MY_SECRET_ACCESS_KEY/, bs.inspect + assert_match /\[REDACTED\]/, bs.inspect + end + def teardown @bs_local.stop end end +# Regression tests for the logfile-creation step in Local#start (CWE-78). +# The logfile used to be created with `system("echo ... > #{@logfile}")`, which +# passed the caller-supplied path through a shell. These tests drive the public +# `start` entry point but abort just after the logfile step (a fake binarypath +# skips the download; stubbing start_command_args prevents launching the binary), +# so they need no credentials, network, or tunnel. +class BrowserStackLocalLogfileTest < Minitest::Test + class AbortAfterLogfile < StandardError; end + + # Runs `start` with the given logfile value, aborting right after the logfile + # is created (before the real binary is spawned). + def start_up_to_logfile(logfile_value) + bs = BrowserStack::Local.new('dummy_key') + bs.stub(:start_command_args, ->(*) { raise AbortAfterLogfile }) do + begin + # An existing, harmless executable as binarypath skips the binary download. + bs.start('binarypath' => existing_executable, 'logfile' => logfile_value) + rescue AbortAfterLogfile + # expected: we intentionally stop before launching the binary + end + end + end + + def existing_executable + ['/bin/true', '/usr/bin/true'].find { |p| File.executable?(p) } || RbConfig.ruby + end + + def test_shell_metacharacters_in_logfile_path_are_not_executed + Dir.mktmpdir do |dir| + Dir.chdir(dir) do + marker = File.join(dir, 'pwned') + # Unix payload: close the single quote around @logfile, run touch, reopen. + # Pre-fix this expands to: echo '' > 'log' ; touch ; echo 'x' + payload = "log' ; touch #{marker} ; echo 'x" + + start_up_to_logfile(payload) + + refute File.exist?(marker), + 'shell metacharacters in the logfile path were executed (command injection)' + end + end + end + + def test_logfile_path_is_treated_as_a_literal_filename + Dir.mktmpdir do |dir| + logfile = File.join(dir, 'sub', 'my log.txt') # spaces + missing subdir + start_up_to_logfile(logfile) + + assert File.file?(logfile), + 'the logfile should be created as a literal path, even with spaces / a missing dir' + assert_equal '', File.read(logfile), 'the logfile should be truncated to empty' + end + end +end + class BrowserStackLocalBinaryTest < Minitest::Test def test_default_user_agent_contains_gem_name_and_version ua = BrowserStack::LocalBinary.new(auth_token: 'fake').instance_variable_get(:@user_agent) @@ -159,6 +248,62 @@ def test_local_binary_accepts_proxy_conf assert_equal 8080, bin.instance_variable_get(:@proxy_port) end + # Regression: verify_binary must exec the binary directly, never via a shell, + # so shell metacharacters in the cached-binary path cannot run commands (CWE-78). + def test_verify_binary_does_not_interpret_shell_metacharacters_in_path + marker = File.join(Dir.tmpdir, "bs_local_verify_injection_#{Process.pid}") + File.delete(marker) if File.exist?(marker) + injected = "/nonexistent;touch #{marker};echo BrowserStack Local version 9.9;#" + + assert_equal false, BrowserStack::LocalBinary.new(auth_token: 'fake').send(:verify_binary, injected) + refute File.exist?(marker), 'shell metacharacters in the binary path were executed' + ensure + File.delete(marker) if marker && File.exist?(marker) + end + + # Stronger form of the above: a REAL binary living under a hostile-looking + # directory name. Pins the array-form behaviour itself rather than just an + # ENOENT, so a future "fix" that swapped the array form for a character + # allowlist would fail here — the injected command must not run AND the + # legitimate binary at that path must still verify. + def test_verify_binary_runs_a_real_binary_at_a_path_containing_shell_metacharacters + skip 'needs a POSIX shell to stand in for the binary' if Gem.win_platform? + + marker = File.join(Dir.tmpdir, "bs_local_verify_dir_injection_#{Process.pid}") + File.delete(marker) if File.exist?(marker) + + base = Dir.mktmpdir('bs_local') + dir = File.join(base, "h;touch #{marker};echo BrowserStack Local version 9.9;#") + FileUtils.mkdir_p(dir) + bin = File.join(dir, 'BrowserStackLocal') + File.write(bin, "#!/bin/sh\necho 'BrowserStack Local version 9.9'\n") + FileUtils.chmod(0755, bin) + + assert_equal true, BrowserStack::LocalBinary.new(auth_token: 'fake').send(:verify_binary, bin) + refute File.exist?(marker), 'shell metacharacters in the binary path were executed' + ensure + File.delete(marker) if marker && File.exist?(marker) + FileUtils.remove_entry(base) if base && File.directory?(base) + end + + # Same fix, benign side: a legitimate path containing spaces must still verify + # (the shell used to split it and the check failed for every such user). + def test_verify_binary_accepts_a_path_containing_spaces + skip 'needs a POSIX shell to stand in for the binary' if Gem.win_platform? + + base = Dir.mktmpdir('bs_local') + dir = File.join(base, 'my binary dir') + FileUtils.mkdir_p(dir) + bin = File.join(dir, 'BrowserStackLocal') + File.write(bin, "#!/bin/sh\necho 'BrowserStack Local version 9.9'\n") + FileUtils.chmod(0755, bin) + + assert_includes bin, ' ' + assert_equal true, BrowserStack::LocalBinary.new(auth_token: 'fake').send(:verify_binary, bin) + ensure + FileUtils.remove_entry(base) if base && File.directory?(base) + end + private def with_host_config(host_os, host_cpu)