Merge pull request #178 from browserstack/locsec/WI-f16a1d45

Scope binary-download fallback state per Local instance

Author: 07souravkunda

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 {
@@ -277,6 +287,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;

lib/LocalBinary.js

@@ -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.
+   */
+  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);
     }
@@ -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);
     });
   };

test/local.js

@@ -490,3 +490,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);
+  });
+});