From 6964d172c95df78cfa0da0b435cf609a4150f150 Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Thu, 6 Aug 2026 22:21:42 +0530 Subject: [PATCH 1/2] 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 --- lib/Local.js | 22 +++++++++++--- lib/LocalBinary.js | 32 ++++++++++++++------- test/local.js | 71 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 111 insertions(+), 14 deletions(-) diff --git a/lib/Local.js b/lib/Local.js index 8f783d7..c1b1cda 100644 --- a/lib/Local.js +++ b/lib/Local.js @@ -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 }; this.errorRegex = /\*\*\* Error: [^\r\n]*/i; this.doneRegex = /Press Ctrl-C to exit/i; @@ -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()); @@ -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 { @@ -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; diff --git a/lib/LocalBinary.js b/lib/LocalBinary.js index 8d694b2..898252e 100644 --- a/lib/LocalBinary.js +++ b/lib/LocalBinary.js @@ -20,6 +20,18 @@ 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. + */ + 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 */ @@ -27,17 +39,17 @@ function LocalBinary(){ 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); } @@ -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(); @@ -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); }); }; diff --git a/test/local.js b/test/local.js index 79c10eb..2f561e9 100644 --- a/test/local.js +++ b/test/local.js @@ -463,3 +463,74 @@ describe('LocalBinary', function () { }); }); }); + +// Regression tests for LOC-6804 (C-007): 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 (LOC-6804)', 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); + }); +}); From 9b158d47ca2815699e27ea9e0f6ec70d206fb104 Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Thu, 6 Aug 2026 22:50:30 +0530 Subject: [PATCH 2/2] test: drop internal tracker ids from download-state isolation tests Rename the describe and strip the leading comment's internal ids so the regression suite carries no internal reference (this is a public repo whose test file also ships in the published npm tarball). No test behaviour change. Co-Authored-By: Claude Opus 4.8 --- test/local.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/local.js b/test/local.js index 2f561e9..b380c6f 100644 --- a/test/local.js +++ b/test/local.js @@ -464,12 +464,12 @@ describe('LocalBinary', function () { }); }); -// Regression tests for LOC-6804 (C-007): 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 (LOC-6804)', 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');