Skip to content

Scope binary-download fallback state per Local instance #178

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
07souravkunda wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Scope binary-download fallback state per Local instance #178

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6964d17
Scope binary-download fallback state per Local instance (was process.…
07souravkunda Aug 6, 2026
9b158d4
test: drop internal tracker ids from download-state isolation tests
07souravkunda Aug 6, 2026
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
22 changes: 18 additions & 4 deletions lib/Local.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,16 @@ function Local(){
this.logfile = this.sanitizePath(path.join(process.cwd(), 'local.log'));
this.opcode = 'start';
this.exitCallback;
/*
* Binary-download fallback signalling, scoped to THIS Local instance. Replaces
* the former process.env.BINARY_DOWNLOAD_* globals, which bled retry/fallback
* state (and the cached source URL) across every concurrent Local instance in
* the process and let a pre-set env var steer the download to an arbitrary
* host. This single object is shared with each LocalBinary the retry loop
* creates, so the fallback URL is still cached across retries of THIS instance
* only.
*/
this.binaryDownloadState = { fallbackEnabled: false, errorMessage: null, sourceURL: null };

@07souravkunda 07souravkunda Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.


this.errorRegex = /\*\*\* Error: [^\r\n]*/i;
this.doneRegex = /Press Ctrl-C to exit/i;
Expand Down Expand Up @@ -71,8 +81,8 @@ function Local(){
that.retriesLeft -= 1;
fs.unlinkSync(that.binaryPath);
delete(that.binaryPath);
process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
that.binaryDownloadState.fallbackEnabled = true;
return that.startSync(options);
} else {
throw new LocalError(error.toString());
Expand Down Expand Up @@ -106,8 +116,8 @@ function Local(){
that.retriesLeft -= 1;
fs.unlinkSync(that.binaryPath);
delete(that.binaryPath);
process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
that.binaryDownloadState.fallbackEnabled = true;
that.start(options, callback);
return;
} else {
Expand Down Expand Up @@ -260,6 +270,10 @@ function Local(){
this.getBinaryPath = function(callback, bsHost){
if(typeof(this.binaryPath) == 'undefined'){
this.binary = new LocalBinary();
/* Share THIS instance's download-fallback state so it survives across the
* LocalBinary objects recreated during the retry loop, without ever
* touching process-global state. */
this.binary.downloadState = this.binaryDownloadState;
var conf = {};
if(this.proxyHost && this.proxyPort){
conf.proxyHost = this.proxyHost;
Expand Down
32 changes: 22 additions & 10 deletions lib/LocalBinary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,24 +20,36 @@ function LocalBinary(){
this.baseRetries = 9;
this.sourceURL = null;
this.downloadErrorMessage = null;
/*
* Per-instance binary-download signalling. Historically these three fields were
* carried on process.env (BINARY_DOWNLOAD_FALLBACK_ENABLED / _ERROR_MESSAGE /
* _SOURCE_URL), which is a process-global mutable store: a failure on one Local
* instance bled into every other instance in the same process, and an attacker
* who could set the env before boot could force this instance to download from
* an arbitrary host. Keep the state on the instance instead. The owning Local
* object shares ONE downloadState object across the LocalBinary instances it
* recreates during a retry loop, so the fallback URL is still cached within a
* single Local instance without leaking across sibling instances.
*/

@07souravkunda 07souravkunda Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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.

this.downloadState = { fallbackEnabled: false, errorMessage: null, sourceURL: null };

this.getSourceUrlSync = function(conf, retries) {
/* Request for an endpoint to download the local binary from Rails no more than twice with 5 retries each */
if (![4, 9].includes(retries) && this.sourceURL != null) {
return this.sourceURL;
}

if (process.env.BINARY_DOWNLOAD_SOURCE_URL !== undefined && process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries != 4) {
if (this.downloadState.sourceURL != null && this.downloadState.fallbackEnabled && this.parentRetries != 4) {
/* This is triggered from Local.js if there's an error executing the downloaded binary */
return process.env.BINARY_DOWNLOAD_SOURCE_URL;
return this.downloadState.sourceURL;
}

let cmd, opts;
cmd = 'node';
opts = [path.join(__dirname, 'fetchDownloadSourceUrl.js'), this.key, this.bsHost];

if (retries == 4 || (process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries == 4)) {
opts.push(true, this.downloadErrorMessage || process.env.BINARY_DOWNLOAD_ERROR_MESSAGE);
if (retries == 4 || (this.downloadState.fallbackEnabled && this.parentRetries == 4)) {
opts.push(true, this.downloadErrorMessage || this.downloadState.errorMessage);
} else {
opts.push(false, null);
}
Expand All @@ -56,7 +68,7 @@ function LocalBinary(){
const obj = childProcess.spawnSync(cmd, opts, { env: env });
if(obj.stdout.length > 0) {
this.sourceURL = obj.stdout.toString().replace(/\n+$/, '');
process.env.BINARY_DOWNLOAD_SOURCE_URL = this.sourceURL;
this.downloadState.sourceURL = this.sourceURL;
return this.sourceURL;
} else if(obj.stderr.length > 0) {
let output = Buffer.from(JSON.parse(JSON.stringify(obj.stderr)).data).toString();
Expand All @@ -70,23 +82,23 @@ function LocalBinary(){
return callback(null, this.sourceURL);
}

if (process.env.BINARY_DOWNLOAD_SOURCE_URL !== undefined && process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries != 4) {
if (this.downloadState.sourceURL != null && this.downloadState.fallbackEnabled && this.parentRetries != 4) {
/* This is triggered from Local.js if there's an error executing the downloaded binary */
return callback(null, process.env.BINARY_DOWNLOAD_SOURCE_URL);
return callback(null, this.downloadState.sourceURL);
}

let downloadFallback = false;
let downloadErrorMessage = null;

if (retries == 4 || (process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries == 4)) {
if (retries == 4 || (this.downloadState.fallbackEnabled && this.parentRetries == 4)) {
downloadFallback = true;
downloadErrorMessage = this.downloadErrorMessage || process.env.BINARY_DOWNLOAD_ERROR_MESSAGE;
downloadErrorMessage = this.downloadErrorMessage || this.downloadState.errorMessage;
}

fetchDownloadSourceUrlAsync(this.key, this.bsHost, downloadFallback, downloadErrorMessage, conf.proxyHost, conf.proxyPort, conf.useCaCertificate, (err, sourceURL) => {
if (err) return callback(err);
this.sourceURL = sourceURL;
process.env.BINARY_DOWNLOAD_SOURCE_URL = sourceURL;
this.downloadState.sourceURL = sourceURL;
callback(null, sourceURL);
});
};
Expand Down
71 changes: 71 additions & 0 deletions test/local.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,3 +463,74 @@ describe('LocalBinary', function () {
});
});
});

// 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. Both flip from FAIL on the
// pre-fix code to PASS once the state is per-instance.
describe('Binary download state isolation', function () {
var sandBox, childProcess;
var Local = require('../lib/Local');

beforeEach(function () {
sandBox = sinon.sandbox.create();
childProcess = require('child_process');
});

afterEach(function () {
sandBox.restore();
delete process.env.BINARY_DOWNLOAD_SOURCE_URL;
delete process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED;
delete process.env.BINARY_DOWNLOAD_ERROR_MESSAGE;
});

it('does not honor a BINARY_DOWNLOAD_SOURCE_URL planted in process.env', function () {
// An attacker (CI secret injection, malicious dep, shared-workspace .env) or a
// sibling instance leaves these two vars set.
process.env.BINARY_DOWNLOAD_SOURCE_URL = 'https://attacker.example.com/evil';
process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = 'true';

// Stub the endpoint API child process so nothing hits the network; the stub
// stands in for a legitimate BrowserStack endpoint response.
var spawnStub = sandBox.stub(childProcess, 'spawnSync', function () {
return {
stdout: Buffer.from('https://legit.browserstack.com/bs\n'),
stderr: Buffer.from('')
};
});

var binary = new LocalBinary();
binary.key = 'DUMMY';
binary.bsHost = 'local.browserstack.com';
binary.parentRetries = 9;

var url = binary.getSourceUrlSync({}, 9);

// Pre-fix: the env-shortcut returns the attacker URL and spawnSync is never
// reached. Post-fix: the shortcut is gone, so the real endpoint call runs.
expect(url).to.not.equal('https://attacker.example.com/evil');
expect(url).to.equal('https://legit.browserstack.com/bs');
expect(spawnStub.called).to.equal(true);
});

it('keeps download-fallback state per Local instance (no cross-instance bleed)', function () {
var a = new Local();
var b = new Local();

// Instance A records a download failure (as its retry catch block does).
a.binaryDownloadState.fallbackEnabled = true;
a.binaryDownloadState.errorMessage = 'A private error: key=A_SECRET';
a.binaryDownloadState.sourceURL = 'https://a-context.example/bs';

// Instance B, which never failed, must be unaffected.
expect(b.binaryDownloadState.fallbackEnabled).to.equal(false);
expect(b.binaryDownloadState.errorMessage).to.equal(null);
expect(b.binaryDownloadState.sourceURL).to.equal(null);

// And nothing leaked to the process-global env.
expect(process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED).to.equal(undefined);
expect(process.env.BINARY_DOWNLOAD_SOURCE_URL).to.equal(undefined);
expect(process.env.BINARY_DOWNLOAD_ERROR_MESSAGE).to.equal(undefined);
});
});
Loading