Scope binary-download fallback state per Local instance

[07souravkunda]

Summary

The binary-download retry/fallback state is signalled through three process.env
variables — BINARY_DOWNLOAD_FALLBACK_ENABLED, BINARY_DOWNLOAD_ERROR_MESSAGE,
BINARY_DOWNLOAD_SOURCE_URL. process.env is a process-global mutable store, and using it
for inter-instance signalling on the download path causes two issues:

• Cross-instance state bleed (CWE-362). A download/exec failure on one Local instance
  sets these globals for the whole process, so every other concurrent Local instance
  (common in parallel test runners / CI that fan out) inherits the failed instance's fallback
  flag, error text, and cached source URL. Instances silently download from another instance's
  request-context URL and report another instance's error as their own telemetry.
• Unvalidated download source (CWE-494). getSourceUrl/getSourceUrlSync return
  process.env.BINARY_DOWNLOAD_SOURCE_URL verbatim — no scheme/host check — before contacting
  the endpoint API. A value planted in the environment before the process boots therefore
  steers the binary download to an arbitrary host, which is then chmod 0755'd and executed.

Change

Replace the three globals with a per-Local-instance state object
({ fallbackEnabled, errorMessage, sourceURL }), shared by reference with the LocalBinary
objects a single instance recreates during its retry loop. This ends the cross-instance bleed
and removes the environment shortcut, while preserving the same per-instance retry/fallback
behaviour — the resolved fallback URL is still cached to avoid re-requesting the endpoint
API within one instance. A standalone new LocalBinary() keeps its own default state.

Files: lib/Local.js, lib/LocalBinary.js (+ regression tests in test/local.js).
These env vars have no external consumer (internal implementation detail only).

Test plan

• eslint lib/* index.js (repo pretest) passes.
• New regression tests in test/local.js — pass on this change, fail when reverted:
  • a BINARY_DOWNLOAD_SOURCE_URL planted in process.env is no longer honored;
  • download-fallback state stays per Local instance (no cross-instance bleed, no process.env writes).
• Existing arg-building + real-tunnel-start unit tests pass; a real tunnel starts through
  the binding and stops cleanly on the patched download path.
• Manual PoCs: env-shortcut, cross-instance bleed, and a full on-box
  download→chmod 0755→execFile all reproduce before the change and are blocked after it.

🤖 Generated with Claude Code

[07souravkunda]

Automated security-fix review (round 0). 1 blocking, 1 nit, 1 for-human. Keeping this a Draft — a human owns approval.

The fix itself is correct and well-scoped: 3 files, no dependency or lockfile change, no drive-by edits. I re-derived the parity between the old process.env signalling and the new per-instance state and it holds exactly — sourceURL != null matches the old !== undefined, the boolean fallbackEnabled matches the old == 'true' (Local.js assigned a boolean that env coerced to 'true'), and downloadState.sourceURL is written unconditionally at :70/:101 while fallbackEnabled is set only on a binary-exec failure — same as before, so the retry/fallback path is behaviourally unchanged within one instance. lib/Local.js:272-276 is the only new LocalBinary() in lib/, so the shared-by-reference wiring has no gaps, and startSync/start both re-enter getBinaryPath after delete(that.binaryPath), so the state survives the retry loop as intended.

Claims I checked rather than took on trust:

• Every process.env.BINARY_DOWNLOAD_* site on origin/master is replaced — git grep on origin/master returns exactly the 14 sites in lib/Local.js and lib/LocalBinary.js, and the branch leaves only a code comment. No other consumer anywhere in the workspace.
• No environment variable can still steer the download after this change (only BROWSERSTACK_ACCESS_KEY, BROWSERSTACK_LOCAL_DEBUG_GZIP, USER_AGENT remain).
• Both regression tests fail on pre-fix lib/ and pass on this branch — reproduced independently by checking out origin/master's two lib files under the new test file (expected 'https://attacker.example.com/evil' to not equal … and TypeError: Cannot set properties of undefined).
• The repo's real pretest gate (eslint lib/* index.js) exits clean.
• The excluded should stop local test fails identically on pristine origin/master at test/local.js:218 inside a tree-kill callback — the pre-existing-flake write-off is accurate, not a regression being papered over.
• sinon@^1.17.6 is the pinned version, so the 3-arg sandBox.stub(obj, 'm', fn) form the new tests use matches the existing convention in this file.
• PR #176 (the *.browserstack.com allowlist) touches lib/util.js + the two fetchDownloadSourceUrl*.js files — zero overlap with this PR, and genuinely complementary: #176 validates the endpoint-API response, this PR removes the shortcut that bypassed that path entirely.

The one blocking item is not about the fix logic: test/local.js:467 and :472 publish LOC-6804 and C-007 into a public repo, and the Jira locsec-fix-done comment states the PR "intentionally carries no LOC id" — so the record a human acts on is wrong about the diff. Details inline.

[07souravkunda]

[blocking] Internal tracker ids are being published to a public repo.

This repository is public (gh api repos/browserstack/browserstack-local-nodejs --jq .visibility → public), and this hunk commits two internal identifiers into the source tree:

• test/local.js:467 — // Regression tests for LOC-6804 (C-007): …
• test/local.js:472 — describe('Binary download state isolation (LOC-6804)', …)

LOC-6804 is the internal Jira key and C-007 is the internal scan-report chain id. Both ship to npm inside the package tarball and are permanently in the public git history.

Evidence this was meant to be avoided: the PR title, body, and commit message are all clean — the id only leaks through the test file. And the locsec-fix-done comment posted on the Jira ticket asserts the opposite of what actually shipped:

PR: …/pull/178 (Draft; public repo, so the PR intentionally carries no LOC id).

So besides the leak itself, the record a human will act on is inaccurate about this PR's contents.

Fix: describe the tests by the behaviour they lock in, and drop the tracker/chain ids from both the comment and the describe title. The ticket↔PR link belongs on the Jira side only.

// Regression tests: the binary-download fallback signalling used to live on
// process.env, so (a) a value planted in process.env steered the download to an
// arbitrary host with no validation, and (b) a failure on one Local instance bled
// into every sibling instance in the same process.
describe('Binary download state isolation', function () {

Then correct the posted Jira comment (edit in place) so the "carries no LOC id" claim matches reality.

One caveat for the reviewer, so this is a fair call rather than a mechanical one: master already carries Fixes: LCNC-15821 in the commit message of 0d29261, so the team's practice here is evidently not strict. If you'd rather accept internal ids in this repo, that's a legitimate call to make explicitly — but then the Jira comment's claim still needs fixing.

[07souravkunda]

Fixed in 9b158d4 — renamed the describe to Binary download state isolation and stripped the internal ids from the leading comment. The diff (and the file that ships in the npm tarball) now carries no internal reference; title, body, and commit messages were already clean. eslint + the two regression tests still pass.

[07souravkunda]

[for-human] This closes the chain by the ticket's alternative remediation, not its stated closure path — worth an explicit security sign-off rather than a silent merge.

LOC-6804's description opens with:

To close this ticket: Fix the chain-breaker finding [LOC-6777] — Downloaded Native Binary Executed Without Integrity Verification. Resolving it alone breaks the entire attack chain — no separate fix is needed for the other component findings.

This PR instead implements the ticket's remediations #2 and #3 (remove the BINARY_DOWNLOAD_SOURCE_URL env-shortcut; replace the process.env signalling with per-instance state). The ticket itself characterises that path as:

a cheaper localized fix that breaks this specific chain but does not defend against C-001 or C-003.

I verified the env-mediated chain really is gone: git grep 'process\.env' pr-178 -- lib/ index.js leaves no environment variable that can influence the download source (only BROWSERSTACK_ACCESS_KEY, BROWSERSTACK_LOCAL_DEBUG_GZIP, and USER_AGENT). So C-007 as written in the ticket — steps 1-3 all name process.env — is genuinely closed.

What remains, and why LOC-6777 is still the ticket's preferred breaker: LocalBinary.binaryPath() reuses an already-executable binary out of the shared ~/.browserstack directory (checkPath(binaryPath, fs.X_OK)), so one instance's downloaded bytes are still consumed by sibling instances with no integrity check. That channel is filesystem-mediated rather than env-mediated, and it is exactly what LOC-6777's pinned SHA-256 covers.

The reasoning is disclosed openly in the Jira comment and the fix summary, so nothing here is misrepresented — it is a scope/policy decision, not a defect. What needs a human: does security accept C-007 as closed on the cheaper path, with LOC-6777 tracked separately for the residual download-to-RCE surface (C-001/C-003)? LOC-6777 needs the endpoint API to publish a pinned digest, which is server-side work owned by another team, so it cannot ship from this repo.

Not a merge blocker for this diff — flagging it so the closure decision is recorded rather than assumed.

[07souravkunda]

[nit] LocalBinary now carries two fields for each piece of download state, which is easy to write to the wrong one later.

After this change the constructor holds both:

this.sourceURL = null;                 // :22
this.downloadErrorMessage = null;      // :23
this.downloadState = { fallbackEnabled: false, errorMessage: null, sourceURL: null };  // :33

The lifetimes genuinely differ — this.sourceURL is the memo for this LocalBinary (read at :38 under ![4, 9].includes(retries)), while downloadState.sourceURL is the cache that survives the LocalBinary objects Local recreates across its retry loop — so this is not a bug, and :70 correctly writes both. But nothing in the field names conveys that split, and a future edit that touches only one of the pair will produce a subtle retry-behaviour change that no test would catch.

Worth a one-line comment at :22 distinguishing the two scopes (e.g. /* per-LocalBinary memo; the cross-retry cache is downloadState.sourceURL */). Same applies to downloadErrorMessage vs downloadState.errorMessage.

Non-blocking.

[07souravkunda]

Automated security-fix review, round 1 (head 9b158d4). The blocking finding from round 0 is resolved. 0 blocking, 1 nit (carried, optional), 1 for-human decision (carried). Still a Draft, still no approval from me — a human owns that.

I re-read the diff from scratch rather than assuming the fix landed. What I verified:

Blocking finding (gate S0 — internal ids in a public repo): RESOLVED, both halves.

• Commit 9b158d4 renames the suite to describe('Binary download state isolation') and rewrites the leading comment without ids. Re-grepped from scratch across the added diff lines, the whole branch tree (lib/ test/ index.js index.d.ts README.md), and every commit message on the branch for LOC-\d+ | SC-\d+ | C-0\d\d | APPSEC-\d+ | LCNC-\d+ — all three clean. Title and body were already clean.
• The second half was the Jira record, and that was fixed too: comment 2268713 was edited in place (updated 22:52 vs created 22:26) and now states the id history accurately, including an explicit "(Correction: an earlier version of this comment said the PR carried no id while the test file still did — that leak is now removed.)". The machine regression_test pointer was also updated to test/local.js::Binary download state isolation, so it no longer points at a describe name that doesn't exist. I checked that specifically because a rename like this usually leaves the Jira pointer stale.

Re-verified on this head, not carried over on trust:

• mocha --grep "Binary download state isolation" → 2 passing on this branch, and 2 failing when origin/master's lib/Local.js + lib/LocalBinary.js are checked out under the new test file (expected 'https://attacker.example.com/evil' to not equal …, and TypeError: Cannot set properties of undefined). The rename did not weaken the tests.
• Repo pretest gate (eslint lib/* index.js) exits 0.
• No runtime process.env.BINARY_DOWNLOAD* access remains — the only hits are one code comment at lib/Local.js:28 and the test file's own deliberate planting, cleanup, and assertions.
• lib/Local.js:272 is still the sole new LocalBinary() in lib/, so the shared-state wiring has no gap.
• git diff 6964d17 9b158d4 -- lib/ is empty — lib/ is byte-identical to the head I fully verified in round 0, so the parity analysis (sourceURL != null ≡ old !== undefined; boolean fallbackEnabled ≡ old == 'true'; unconditional sourceURL write vs failure-only fallbackEnabled) holds unchanged. No new drive-by changes: the round-1 commit touches only test/local.js.
• No CI/tooling coupling to the old test name (.github/, .travis.yml, package.json carry no test-name filter).

Still open, neither blocking:

1. (nit, not applied — fine to leave) LocalBinary still carries this.sourceURL/this.downloadErrorMessage alongside downloadState.sourceURL/.errorMessage. Not a bug — the lifetimes genuinely differ and :70 writes both correctly — just easy to write to the wrong one later. See the inline thread.
2. (for-human) C-007 is being closed via the ticket's cheaper alternative remediations #2+#3 rather than its stated chain-breaker LOC-6777 (pinned SHA-256), which needs a server-side digest owned by another team. The env-mediated chain is genuinely closed; the shared ~/.browserstack binary-reuse channel is not, and that's what LOC-6777 covers. Worth a recorded security sign-off rather than a silent merge. See the inline thread.

No further fixer round needed from my side.