Skip to content

Commit 6964d17

Browse files
07souravkundaclaude
andcommitted
Scope binary-download fallback state per Local instance (was process.env)
The binary-download retry/fallback state was signalled through three process.env vars (BINARY_DOWNLOAD_FALLBACK_ENABLED / _ERROR_MESSAGE / _SOURCE_URL). process.env is a process-global mutable store, which caused two problems on the binary download path: - Cross-instance state bleed (CWE-362): a download/exec failure on one Local instance set these globals for the whole process, so every other concurrent Local instance (e.g. in a parallel test runner) inherited the failed instance's fallback flag, error text, and cached source URL - instances silently downloaded from another instance's request-context URL and reported another instance's error as their own telemetry. - Unvalidated download source (CWE-494): getSourceUrl(Sync) returned process.env.BINARY_DOWNLOAD_SOURCE_URL verbatim, with no scheme/host check, before contacting the endpoint API. A value planted in the environment before the process booted therefore steered the binary download to an arbitrary host, which is then chmod 0755'd and executed. Replace the globals with a per-Local-instance state object, shared by reference across 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). Adds regression tests that fail before this change and pass after it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 0d29261 commit 6964d17

3 files changed

Lines changed: 111 additions & 14 deletions

File tree

lib/Local.js

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,16 @@ function Local(){
2323
this.logfile = this.sanitizePath(path.join(process.cwd(), 'local.log'));
2424
this.opcode = 'start';
2525
this.exitCallback;
26+
/*
27+
* Binary-download fallback signalling, scoped to THIS Local instance. Replaces
28+
* the former process.env.BINARY_DOWNLOAD_* globals, which bled retry/fallback
29+
* state (and the cached source URL) across every concurrent Local instance in
30+
* the process and let a pre-set env var steer the download to an arbitrary
31+
* host. This single object is shared with each LocalBinary the retry loop
32+
* creates, so the fallback URL is still cached across retries of THIS instance
33+
* only.
34+
*/
35+
this.binaryDownloadState = { fallbackEnabled: false, errorMessage: null, sourceURL: null };
2636

2737
this.errorRegex = /\*\*\* Error: [^\r\n]*/i;
2838
this.doneRegex = /Press Ctrl-C to exit/i;
@@ -71,8 +81,8 @@ function Local(){
7181
that.retriesLeft -= 1;
7282
fs.unlinkSync(that.binaryPath);
7383
delete(that.binaryPath);
74-
process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
75-
process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
84+
that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
85+
that.binaryDownloadState.fallbackEnabled = true;
7686
return that.startSync(options);
7787
} else {
7888
throw new LocalError(error.toString());
@@ -106,8 +116,8 @@ function Local(){
106116
that.retriesLeft -= 1;
107117
fs.unlinkSync(that.binaryPath);
108118
delete(that.binaryPath);
109-
process.env.BINARY_DOWNLOAD_ERROR_MESSAGE = binaryDownloadErrorMessage;
110-
process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = true;
119+
that.binaryDownloadState.errorMessage = binaryDownloadErrorMessage;
120+
that.binaryDownloadState.fallbackEnabled = true;
111121
that.start(options, callback);
112122
return;
113123
} else {
@@ -260,6 +270,10 @@ function Local(){
260270
this.getBinaryPath = function(callback, bsHost){
261271
if(typeof(this.binaryPath) == 'undefined'){
262272
this.binary = new LocalBinary();
273+
/* Share THIS instance's download-fallback state so it survives across the
274+
* LocalBinary objects recreated during the retry loop, without ever
275+
* touching process-global state. */
276+
this.binary.downloadState = this.binaryDownloadState;
263277
var conf = {};
264278
if(this.proxyHost && this.proxyPort){
265279
conf.proxyHost = this.proxyHost;

lib/LocalBinary.js

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,24 +20,36 @@ function LocalBinary(){
2020
this.baseRetries = 9;
2121
this.sourceURL = null;
2222
this.downloadErrorMessage = null;
23+
/*
24+
* Per-instance binary-download signalling. Historically these three fields were
25+
* carried on process.env (BINARY_DOWNLOAD_FALLBACK_ENABLED / _ERROR_MESSAGE /
26+
* _SOURCE_URL), which is a process-global mutable store: a failure on one Local
27+
* instance bled into every other instance in the same process, and an attacker
28+
* who could set the env before boot could force this instance to download from
29+
* an arbitrary host. Keep the state on the instance instead. The owning Local
30+
* object shares ONE downloadState object across the LocalBinary instances it
31+
* recreates during a retry loop, so the fallback URL is still cached within a
32+
* single Local instance without leaking across sibling instances.
33+
*/
34+
this.downloadState = { fallbackEnabled: false, errorMessage: null, sourceURL: null };
2335

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

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

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

39-
if (retries == 4 || (process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries == 4)) {
40-
opts.push(true, this.downloadErrorMessage || process.env.BINARY_DOWNLOAD_ERROR_MESSAGE);
51+
if (retries == 4 || (this.downloadState.fallbackEnabled && this.parentRetries == 4)) {
52+
opts.push(true, this.downloadErrorMessage || this.downloadState.errorMessage);
4153
} else {
4254
opts.push(false, null);
4355
}
@@ -56,7 +68,7 @@ function LocalBinary(){
5668
const obj = childProcess.spawnSync(cmd, opts, { env: env });
5769
if(obj.stdout.length > 0) {
5870
this.sourceURL = obj.stdout.toString().replace(/\n+$/, '');
59-
process.env.BINARY_DOWNLOAD_SOURCE_URL = this.sourceURL;
71+
this.downloadState.sourceURL = this.sourceURL;
6072
return this.sourceURL;
6173
} else if(obj.stderr.length > 0) {
6274
let output = Buffer.from(JSON.parse(JSON.stringify(obj.stderr)).data).toString();
@@ -70,23 +82,23 @@ function LocalBinary(){
7082
return callback(null, this.sourceURL);
7183
}
7284

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

7890
let downloadFallback = false;
7991
let downloadErrorMessage = null;
8092

81-
if (retries == 4 || (process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED == 'true' && this.parentRetries == 4)) {
93+
if (retries == 4 || (this.downloadState.fallbackEnabled && this.parentRetries == 4)) {
8294
downloadFallback = true;
83-
downloadErrorMessage = this.downloadErrorMessage || process.env.BINARY_DOWNLOAD_ERROR_MESSAGE;
95+
downloadErrorMessage = this.downloadErrorMessage || this.downloadState.errorMessage;
8496
}
8597

8698
fetchDownloadSourceUrlAsync(this.key, this.bsHost, downloadFallback, downloadErrorMessage, conf.proxyHost, conf.proxyPort, conf.useCaCertificate, (err, sourceURL) => {
8799
if (err) return callback(err);
88100
this.sourceURL = sourceURL;
89-
process.env.BINARY_DOWNLOAD_SOURCE_URL = sourceURL;
101+
this.downloadState.sourceURL = sourceURL;
90102
callback(null, sourceURL);
91103
});
92104
};

test/local.js

Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -463,3 +463,74 @@ describe('LocalBinary', function () {
463463
});
464464
});
465465
});
466+
467+
// Regression tests for LOC-6804 (C-007): the binary-download fallback signalling
468+
// used to live on process.env, so (a) a value planted in process.env steered the
469+
// download to an arbitrary host with no validation, and (b) a failure on one Local
470+
// instance bled into every sibling instance in the same process. Both flip from
471+
// FAIL on the pre-fix code to PASS once the state is per-instance.
472+
describe('Binary download state isolation (LOC-6804)', function () {
473+
var sandBox, childProcess;
474+
var Local = require('../lib/Local');
475+
476+
beforeEach(function () {
477+
sandBox = sinon.sandbox.create();
478+
childProcess = require('child_process');
479+
});
480+
481+
afterEach(function () {
482+
sandBox.restore();
483+
delete process.env.BINARY_DOWNLOAD_SOURCE_URL;
484+
delete process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED;
485+
delete process.env.BINARY_DOWNLOAD_ERROR_MESSAGE;
486+
});
487+
488+
it('does not honor a BINARY_DOWNLOAD_SOURCE_URL planted in process.env', function () {
489+
// An attacker (CI secret injection, malicious dep, shared-workspace .env) or a
490+
// sibling instance leaves these two vars set.
491+
process.env.BINARY_DOWNLOAD_SOURCE_URL = 'https://attacker.example.com/evil';
492+
process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED = 'true';
493+
494+
// Stub the endpoint API child process so nothing hits the network; the stub
495+
// stands in for a legitimate BrowserStack endpoint response.
496+
var spawnStub = sandBox.stub(childProcess, 'spawnSync', function () {
497+
return {
498+
stdout: Buffer.from('https://legit.browserstack.com/bs\n'),
499+
stderr: Buffer.from('')
500+
};
501+
});
502+
503+
var binary = new LocalBinary();
504+
binary.key = 'DUMMY';
505+
binary.bsHost = 'local.browserstack.com';
506+
binary.parentRetries = 9;
507+
508+
var url = binary.getSourceUrlSync({}, 9);
509+
510+
// Pre-fix: the env-shortcut returns the attacker URL and spawnSync is never
511+
// reached. Post-fix: the shortcut is gone, so the real endpoint call runs.
512+
expect(url).to.not.equal('https://attacker.example.com/evil');
513+
expect(url).to.equal('https://legit.browserstack.com/bs');
514+
expect(spawnStub.called).to.equal(true);
515+
});
516+
517+
it('keeps download-fallback state per Local instance (no cross-instance bleed)', function () {
518+
var a = new Local();
519+
var b = new Local();
520+
521+
// Instance A records a download failure (as its retry catch block does).
522+
a.binaryDownloadState.fallbackEnabled = true;
523+
a.binaryDownloadState.errorMessage = 'A private error: key=A_SECRET';
524+
a.binaryDownloadState.sourceURL = 'https://a-context.example/bs';
525+
526+
// Instance B, which never failed, must be unaffected.
527+
expect(b.binaryDownloadState.fallbackEnabled).to.equal(false);
528+
expect(b.binaryDownloadState.errorMessage).to.equal(null);
529+
expect(b.binaryDownloadState.sourceURL).to.equal(null);
530+
531+
// And nothing leaked to the process-global env.
532+
expect(process.env.BINARY_DOWNLOAD_FALLBACK_ENABLED).to.equal(undefined);
533+
expect(process.env.BINARY_DOWNLOAD_SOURCE_URL).to.equal(undefined);
534+
expect(process.env.BINARY_DOWNLOAD_ERROR_MESSAGE).to.equal(undefined);
535+
});
536+
});

0 commit comments

Comments
 (0)