fix: create the logfile without a shell (CWE-78 command injection)

[07souravkunda]

What

Local#start created the logfile by shelling out:

if @is_windows
  system("echo > #{@logfile}")          # Windows
else
  system("echo '' > '#{@logfile}'")     # Unix
end

Both pass the caller-supplied logfile path through /bin/sh (or cmd.exe), so shell
metacharacters in the path are interpreted as commands (CWE-78, OS command injection).
A logfile such as log' ; touch /tmp/pwned ; echo 'x (Unix) or NUL & calc.exe (Windows)
runs arbitrary OS commands as the user running the gem. The logfile option is public API
(start(options)) and the block runs on every start.

Fix

Replace the block with shell-free create/truncate:

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

File.write treats the path purely as a filename — no shell is involved, so metacharacters
can no longer execute. As a bonus this fixes logfile paths containing spaces or a
missing subdirectory, which the old shell form silently failed on. Mirrors the fix
already shipped in the Python binding.

Tests

Added BrowserStackLocalLogfileTest (2 regression tests, no credentials/network — they drive
start with a fake binarypath to skip the download and stub start_command_args to abort
before launching the binary):

Test	Pre-fix	Post-fix
metacharacter payload in logfile	FAIL — injected touch executed	pass
space + missing-subdir path	FAIL — sh: … No such file or directory	pass

Both verified to fail on the pre-fix code (revert local.rb to master, keep the test).
Offline suite: 22 runs, 43 assertions, 0 failures.

Live: a real tunnel was opened through the patched binding (BrowserStack::Local#start),
a selenium-webdriver BrowserStack session (Windows 11 / Chrome) ran over it, and it stopped
cleanly via .stop(). The new logfile code was exercised on a space-containing path in a
not-yet-existing directory (created literally, then written to by the binary).

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Central YAML (base), Organization UI (inherited), Workspace UI (inherited)

Review profile: ASSERTIVE

Plan: Enterprise

Run ID: a210fb26-39c8-43d6-a937-0d0ec927d1f8

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[07souravkunda]

Automated security-fix review (locsec pipeline, round 0) — no blocking findings.

The fix replaces the system("echo … > #{@logfile}") block with shell-free FileUtils.mkdir_p + File.write, which fully removes the CWE-78 logfile command-injection sink. Security gates:

• S0 (no internal id in public repo): pass — no LOC-/SC- in title, body, commits, or diff.
• S2 (no drive-by): pass — diff is scoped to the sink replacement + require 'fileutils' + the two regression tests.
• S4 (diff matches assessment): pass — the pre-fix sink is verbatim on origin/master; the one-block replacement removes both the Windows (:77) and Unix (:79) branches, so it also covers sibling LOC-6935; working tree clean.
• S5 (test coverage): pass — reproduced independently: the two BrowserStackLocalLogfileTest cases fail on pre-fix code (injected touch fired; sh: No such file or directory on the space/missing-dir path) and pass post-fix; offline suite 22 runs, 43 assertions, 0 failures (the only errors are the documented pre-existing live-daemon tests). Both directions proven (vuln closed + feature works), plus a live BrowserStack tunnel session id.

One non-blocking nit inline about a pre-existing, effectively-dead legacy shell path (start_command). Keeping this a Draft; a human owns approval and merge.

[07souravkunda]

[nit] This fix correctly closes the logfile-creation shell sink. For completeness, note that the same @logfile value is still interpolated into a shell string elsewhere — start_command (further down this file):

cmd = "#{@binary_path} -d start -logFile '#{@logfile}' ... "

That string is only executed via IO.popen(start_command) in the else of if defined? spawn — i.e. on Ruby < 1.9, where spawn is not defined. On every supported Ruby (1.9+), start_command_args (the array form, no shell) is used instead, so this path is effectively dead and this PR does not regress it.

This is pre-existing and out of scope for this ticket (the finding here is the echo-into-system logfile sink, which is fully removed). Flagging only so a human can decide whether a follow-up ticket to harden/remove the legacy start_command string form (and its unquoted @key/@folder_path/proxy interpolations) is worthwhile. Not blocking.

[07souravkunda]

Automated security review (locsec pipeline, round 0) — covers both siblings on this one PR: F-003 (Unix line, LOC-6935 / WI-ba0962f8) and F-004 (Windows line, LOC-6936 / WI-09e205d7).

Verdict: no blocking findings. The one if @is_windows … system(…) … else … system(…) block on master (0a01b1c) carried both breakers; replacing it with FileUtils.mkdir_p + File.write(@logfile, "") removes the shell from both branches, so a single PR correctly closes both tickets.

Security gates:

• S0 (public-repo id leak): pass — no LOC-/SC- id in title, body, commits, or diff; the fix is described by CWE only.
• S2 (no drive-by): pass — scoped to lib/browserstack/local.rb (fix + require 'fileutils') and the new regression test.
• S4 (diff ↔ assessment): pass — sink verbatim on master; @logfile set in add_args("logfile", …) and consumed unconditionally on every start; the diff is exactly the described block replacement, no behavioural surprises. File.write treats the path as a pure filename → metacharacters can no longer execute.
• S4c (chain): pass — both breakers fixed in one block, one PR, both sibling tickets carry locsec-fix-done completion comments pointing here (neither left in New Item), and the completion honestly states one fix closes both.
• S5 (test coverage): pass — regression test BrowserStackLocalLogfileTest (correctly aborts right after the logfile step via a stubbed start_command_args, no creds/network), fails on pre-fix code and passes post-fix, both directions (injection blocked + space/missing-dir path created literally); binding offline suite green; live selenium-webdriver sessions run through the patched binding (ids b2ec1088… and 341d660b…). CI (CodeQL, Semgrep, CodeRabbit) all green.

One non-blocking nit inline. Keeping this a Draft; a human owns approval + merge, and closing the sibling once merged.

🤖 Generated with Claude Code

[07souravkunda]

[nit] File.write(@logfile, "") truncates to 0 bytes, whereas the old Unix echo '' > wrote a single newline and the Windows echo > wrote a line ending. Immaterial (the binary opens/writes the logfile itself) — just noting the behavioural delta for the record. The shell-free rewrite is correct and rescue SystemCallError correctly covers the Errno::* write failures. No change needed.