Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/gem-push.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 5 additions & 0 deletions .gitignore
Original file line number Diff line number Diff line change
@@ -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/
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
6 changes: 4 additions & 2 deletions Gemfile
Original file line number Diff line number Diff line change
@@ -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.
23 changes: 17 additions & 6 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -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
36 changes: 29 additions & 7 deletions lib/browserstack/local.rb
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
require 'browserstack/localbinary'
require 'browserstack/localexception'
require 'json'
require 'fileutils'

module BrowserStack

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down
9 changes: 8 additions & 1 deletion lib/browserstack/localbinary.rb
Original file line number Diff line number Diff line change
Expand Up @@ -133,7 +133,14 @@
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
Expand Down
2 changes: 1 addition & 1 deletion lib/browserstack/version.rb
Original file line number Diff line number Diff line change
@@ -1,3 +1,3 @@
module BrowserStack
VERSION = '1.5.0'.freeze
VERSION = '1.5.1'.freeze
end
145 changes: 145 additions & 0 deletions test/browserstack-local-test.rb
Original file line number Diff line number Diff line change
@@ -1,24 +1,37 @@
require 'rubygems'
require 'minitest'
require 'minitest/autorun'
require 'minitest/mock'
require 'tmpdir'
require 'browserstack/local'

class BrowserStackLocalTest < Minitest::Test
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')
Expand Down Expand Up @@ -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 <marker> ; 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)
Expand Down Expand Up @@ -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)
Expand Down
Loading