Apply the value check to every option key, not just passthrough ones

Review round 1. The flag-like-value guard lived in addUserArg(), which
only runs for keys reaching the default: branch, so all 21 options with
an explicit switch case bypassed it. Because the binary's parser accepts
the '--flag=value' form, the smuggled flag carries its own value and
needs no following argv slot:

  {localIdentifier: '--log-file=/tmp/attacker-owned'}
    -> [..., '--local-identifier', '--log-file=/tmp/attacker-owned']

which the parser reads as a second --log-file. Reachable the same way
through only, folder, proxyHost/Port/User/Pass, parallelRuns,
useCaCertificate, logFile, key and verbose — i.e. through exactly the
keys the README documents. That also defeated RESERVED_OPTIONS, since
--daemon= and --log-file= could ride in as values.

Hoist the check into addArgs, above the switch, so it applies to every
key. List-valued options (--include-hosts, --exclude-hosts) take an
array, so every element is checked, not just the first.

Also from review:
- skip a null/undefined passthrough value, matching the `if(value)`
  guard every explicit case already uses, and push a coerced string so
  no non-string argv element reaches execFile/spawnSync; array values
  are pushed as separate elements, which is what a list flag expects
- note in PASSTHROUGH_OPTIONS that the entries shadowed by an explicit
  case are listed for completeness against the binary's CLI, and make
  the log-file/logFile split explicit rather than contradictory
- drop the internal tracker ids from the test comment; this repo is
  public and no such id currently ships in the tree

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: 07souravkunda

lib/Local.js

@@ -14,6 +14,12 @@ var childProcess = require('child_process'),
 // (COMMAND_CONFIGURATION in browserStackTunnel, extensions/node/config/constants.js)
 // — long names and their aliases — so every documented modifier keeps working
 // while an unrecognised key can no longer reach the daemon argv.
+//
+// Deliberately a COMPLETE mirror, so it can be diffed against the binary's CLI
+// when that gains a flag. Some entries (key, folder, force, only, forcelocal,
+// verbose, onlyAutomate, proxyHost/Port/User/Pass, localIdentifier, forceproxy,
+// logFile, parallelRuns) are handled by an explicit case in addArgs and so never
+// reach this list at runtime; they are listed for completeness, not effect.
 var PASSTHROUGH_OPTIONS = [
   'key', 'folder', 'help', 'version', 'force', 'only',
   'forcelocal', 'force-local',
@@ -56,8 +62,10 @@ var PASSTHROUGH_OPTIONS = [
 
 // Flags getBinaryArgs() always puts on the argv itself. Accepting them from
 // the options object too would let a caller append a second, conflicting copy
-// — e.g. '--daemon stop' after our '--daemon start'. ('logFile' has its own
-// supported option; only the raw binary alias is reserved.)
+// — e.g. '--daemon stop' after our '--daemon start'. The log file is settable,
+// but only through the wrapper's own 'logfile'/'logFile' case above, which
+// routes it into getBinaryArgs' single '--log-file'; the binary's raw
+// 'log-file' alias is reserved so it cannot add a second one.
 var RESERVED_OPTIONS = ['daemon', 'log-file', 'source'];
 
 // Keys consumed by this wrapper and never meant for the binary.
@@ -208,6 +216,15 @@ function Local(){
     for(var key in options){
       var value = options[key];
 
+      // Runs for EVERY key, including the ones with an explicit case below.
+      // A value is only ever safe as a value: the binary's parser will not
+      // consume one that begins with '-', it reads it as another flag — and it
+      // accepts the '--flag=value' form, so a value like '--log-file=/tmp/x'
+      // smuggles a complete flag in through an otherwise legitimate option.
+      var valueError = this.rejectFlagLikeValue(key, value);
+      if(valueError)
+        return valueError;
+
       switch(key){
       case 'key':
         if(value)
@@ -313,10 +330,24 @@ function Local(){
     }
   };
 
+  // Returns a LocalError if any part of the value would be read as a flag
+  // rather than as this option's value. List-typed options (--include-hosts,
+  // --exclude-hosts) take an array, so every element is checked.
+  this.rejectFlagLikeValue = function(key, value){
+    var values = Array.isArray(value) ? value : [value];
+    for(var i = 0; i < values.length; i++){
+      if(values[i] === undefined || values[i] === null)
+        continue;
+      if(values[i].toString().charAt(0) === '-')
+        return new LocalError('Invalid value for option \'' + key + '\': values starting with \'-\' are not allowed');
+    }
+  };
+
   // Forwards one caller-supplied option to the daemon argv, or returns a
   // LocalError describing why it was refused. Only documented modifiers get
   // through: an unknown key used to be prefixed with '--' and pushed blindly,
   // which let any caller inject arbitrary flags into the native binary.
+  // The value has already been checked by rejectFlagLikeValue in addArgs.
   this.addUserArg = function(key, value){
     if(INTERNAL_OPTIONS.indexOf(key) !== -1)
       return;
@@ -327,20 +358,24 @@ function Local(){
     if(PASSTHROUGH_OPTIONS.indexOf(key) === -1)
       return new LocalError('Unknown option \'' + key + '\'. Only documented BrowserStack Local modifiers are forwarded to the binary, see https://www.browserstack.com/local-testing#modifiers');
 
-    var stringValue = value === undefined || value === null ? '' : value.toString();
+    // Match the explicit cases above, which all guard with `if(value)`.
+    if(value === undefined || value === null)
+      return;
 
-    if(stringValue.toLowerCase() == 'true'){
+    if(value.toString().toLowerCase() == 'true'){
       this.userArgs.push('--' + key);
       return;
     }
 
-    // The binary's argv parser will not consume a value that begins with '-';
-    // it reads it as another flag instead. Refuse rather than smuggle one in.
-    if(stringValue.charAt(0) === '-')
-      return new LocalError('Invalid value for option \'' + key + '\': values starting with \'-\' are not allowed');
-
+    // argv elements must be strings — execFile/spawnSync reject anything else.
     this.userArgs.push('--' + key);
-    this.userArgs.push(value);
+    if(Array.isArray(value)){
+      for(var i = 0; i < value.length; i++){
+        this.userArgs.push(value[i].toString());
+      }
+    } else {
+      this.userArgs.push(value.toString());
+    }
   };
 
   this.getBinaryPath = function(callback, bsHost){

test/local.js

@@ -124,11 +124,11 @@ describe('Local', function () {
     });
   });
 
-  // LOC-6805 / LOC-6783 (F-007, CWE-88): addArgs used to prefix ANY unknown
-  // option key with '--' and push it onto the daemon argv, letting a caller
-  // — or upstream code merging untrusted input into `options` — inject
-  // arbitrary flags into the native binary. Only documented BrowserStackLocal
-  // modifiers may be forwarded now.
+  // Argument injection (CWE-88): addArgs used to prefix ANY unknown option key
+  // with '--' and push it onto the daemon argv, letting a caller — or upstream
+  // code merging untrusted input into `options` — inject arbitrary flags into
+  // the native binary. Only documented BrowserStackLocal modifiers may be
+  // forwarded now, and no value may pose as a flag.
 
   it('should reject unknown boolean args', function (done) {
     bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'boolArg1': true, 'boolArg2': true }, function(error){
@@ -182,6 +182,62 @@ describe('Local', function () {
     });
   });
 
+  // The value check must cover keys that have an explicit case too, not just
+  // the ones reaching the allowlist. bs-minimist accepts '--flag=value', so a
+  // smuggled flag carries its own value and needs no following argv slot.
+  it('should reject a flag-like value on an explicitly handled option', function (done) {
+    bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'localIdentifier': '--log-file=/tmp/attacker-owned' }, function(error){
+      expect(error).to.be.an(Error);
+      expect(error.toString()).to.contain('values starting with \'-\' are not allowed');
+      const args = bsLocal.getBinaryArgs();
+      expect(args.indexOf('--log-file=/tmp/attacker-owned')).to.equal(-1);
+      expect(args.indexOf('--local-identifier')).to.equal(-1);
+      done();
+    });
+  });
+
+  it('should reject a daemon-lifecycle smuggle through an explicitly handled option', function (done) {
+    bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'only': '--daemon=stop' }, function(error){
+      expect(error).to.be.an(Error);
+      expect(bsLocal.getBinaryArgs().indexOf('--daemon=stop')).to.equal(-1);
+      done();
+    });
+  });
+
+  it('should reject a flag-like element inside a list-valued option', function (done) {
+    bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'include-hosts': ['localhost', '--config-file=/tmp/attacker.yml'] }, function(error){
+      expect(error).to.be.an(Error);
+      expect(bsLocal.getBinaryArgs().indexOf('--config-file=/tmp/attacker.yml')).to.equal(-1);
+      done();
+    });
+  });
+
+  it('should forward a list-valued option as separate argv elements', function (done) {
+    bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'include-hosts': ['localhost', '127.0.0.1'] }, function(error){
+      expect(error).to.equal(undefined);
+      const args = bsLocal.getBinaryArgs();
+      expect(args.indexOf('--include-hosts')).to.not.equal(-1);
+      expect(args.indexOf('localhost')).to.not.equal(-1);
+      expect(args.indexOf('127.0.0.1')).to.not.equal(-1);
+      done();
+    });
+  });
+
+  it('should skip a null or undefined passthrough value instead of pushing it raw', function (done) {
+    // execFile/spawnSync reject a non-string argv element, so a raw null here
+    // used to throw ERR_INVALID_ARG_TYPE out of start() instead of erroring.
+    bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'region': null, 'connect-timeout': 30 }, function(error){
+      expect(error).to.equal(undefined);
+      const args = bsLocal.getBinaryArgs();
+      expect(args.indexOf('--region')).to.equal(-1);
+      expect(args.indexOf(null)).to.equal(-1);
+      // numbers are coerced, so every argv element is a string
+      expect(args.indexOf('30')).to.not.equal(-1);
+      expect(args.every(function(a){ return typeof a === 'string'; })).to.equal(true);
+      done();
+    });
+  });
+
   it('should not forward wrapper-internal keys to the binary', function (done) {
     bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true }, function(error){
       expect(error).to.equal(undefined);