Fixes for security issues

[rounak610]

No description provided.

[07souravkunda]

This inline verification (subprocess.check_output + regex on "BrowserStack Local version") duplicates LocalBinary.__verify_binary at browserstack/local_binary.py:159-165. Two copies of the same banner format are likely to drift the next time the binary's banner changes — one site will get updated and the other forgotten, and the wrong one is likely to be the security gate.

LOC-6718's recommended fix called the existing verifier (verifier._LocalBinary__verify_binary()). The PR chose to inline instead, presumably to avoid mangled-name access.

Suggested fix: lift the check into LocalBinary as a @staticmethod (e.g. LocalBinary.verify_binary(path)) and call it from both LocalBinary.get_binary() and the new binarypath branch here. Prefer the new PR's regex (raw string) and narrow exception list (subprocess.SubprocessError, OSError) — drop the bare except: currently in __verify_binary.

Question: was the duplication intentional (e.g., to avoid invoking private name mangling), or just expedience? If intentional, can we still factor out a shared helper?

[rounak610]

fixed, moved the regex to a constant

[07souravkunda]

subprocess.check_output has no timeout. The attacker model from LOC-6718 (attacker influences kwargs to control binarypath) extends naturally to a malicious binary that blocks on stdin or sleeps forever instead of printing a banner — Local.start() will hang indefinitely. The fix closes RCE + key theft but leaves a survivable DoS in the same threat model.

Suggested fix:

version_output = subprocess.check_output(
    [candidate, '--version'], timeout=10
).decode('utf-8')

Python 3.3+ supports the kwarg natively. subprocess.TimeoutExpired inherits from SubprocessError, so the existing except tuple already covers it; the failure message is clearer if handled explicitly.

[rounak610]

fixed

[07souravkunda]

This block truncates self.local_logfile_path after subprocess.Popen + communicate() (lines 102-103). By that point the binary has already written its startup output (the JSON ack, plus anything before backgrounding) to the same file — the truncation wipes it. Users debugging "why didn't my tunnel start" lose the binary's startup messages.

The truncation appears intended to clear stale output from a prior run — which it can't do here because by the time it runs, the current daemon has already written.

This ordering is pre-existing (master had the same shape with os.system), but since the PR is already editing these lines, this is the natural moment to fix it.

Suggested fix: move the truncation up — right after local_logfile_path is finalized (around line 94), before Popen. That way the file starts empty and the daemon's writes are preserved.

Question: is the post-Popen position load-bearing for some reason? If not, recommend moving it up.

[rounak610]

fixed

[07souravkunda]

Thanks for the PR — all three tickets (LOC-6717/6718/6720) are addressed in the right shape, and the realpath ordering + narrow exception handling are actually cleaner than the existing patterns in LocalBinary.

Posted 5 inline comments grouped roughly as:

• One on the duplicate of LocalBinary.__verify_binary (DRY/format-drift risk; LOC-6718 recommended reusing the existing method)
• Two on subprocess.check_output having no timeout; logfile truncation still happening after subprocess.Popen (pre-existing, but in the edited block)
• One on defense-in-depth: logfile path is not scoped, arbitrary file truncation + tilde-expansion regression
• One on the Semgrep digest pin being latest's digest rather than a version-tagged digest; Dependabot for Docker still pending

A small low-severity nit not posted inline (no diff anchor): no tests cover the new validation error paths in tests/test_local.py — four new BrowserStackLocalErrors and one dep cap can silently regress in future refactors. Consider adding ~3 smoke tests (nonexistent path, non-BrowserStack binary, unwritable logfile).

Also worth confirming separately: sibling SDKs (browserstack-local-ruby, browserstack-local-nodejs) appear to have the same binarypath / logfile shapes — Ruby still does system("echo > '#{@logfile}'") and stores binarypath verbatim. If LOC-6717/6718 don't yet have sibling tickets there, worth filing while the threat model is fresh.