From 1964cb1d06a0f32f0a5f41e4b4fcdd8ef25c76a0 Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Thu, 6 Aug 2026 21:22:50 +0530 Subject: [PATCH 1/2] LOC-6805: allowlist option keys forwarded to the BrowserStackLocal binary MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit addArgs() prefixed ANY unrecognised key in the caller-supplied options object with '--' and pushed it, with its value, onto the daemon argv. Any caller — or upstream code merging untrusted input into options — could inject arbitrary flags into the native binary (CWE-88). Only documented BrowserStackLocal modifiers are forwarded now. The allowlist mirrors the binary's own CLI definition (COMMAND_CONFIGURATION in browserStackTunnel), long names and aliases, so every documented modifier without an explicit switch case — localProxyHost/Port/User/Pass, pac-file, custom-repeater, bs-host — keeps working. Also refused: - daemon / log-file / source, which getBinaryArgs() sets itself, so a caller cannot append a conflicting second copy (e.g. '--daemon stop' after our '--daemon start') - a value beginning with '-'. The binary's parser does not consume such a value, it reads it as another flag, so a legitimate key could still smuggle one in. These values are already mis-parsed today, so this is not a regression. - onlyCommand, a wrapper-internal key, no longer leaks into the argv. addArgs returns a LocalError, delivered through the existing paths: callback(err) for start(), returned for startSync(). Closes the entry step of chain C-008. LOC-6790 (binarypath traversal) and LOC-6777 (no binary integrity check) are unaffected and remain open — the chain description's claim that this gates binarypath is inaccurate, that value is an explicit switch case. Co-Authored-By: Claude Opus 5 (1M context) --- README.md | 2 + lib/Local.js | 104 +++++++++++++++++++++++++++++++++++++++++++++----- test/local.js | 80 +++++++++++++++++++++++++++++++++----- 3 files changed, 167 insertions(+), 19 deletions(-) diff --git a/README.md b/README.md index 6166a8f..24c30ad 100644 --- a/README.md +++ b/README.md @@ -40,6 +40,8 @@ bs_local.start(bs_local_args, function() { Apart from the key, all other BrowserStack Local modifiers are optional. For the full list of modifiers, refer [BrowserStack Local modifiers](https://www.browserstack.com/local-testing#modifiers). For examples, refer below - +Only documented modifiers are forwarded to the binary. An unrecognised option key, an option the wrapper sets itself (`daemon`, `log-file`), or a value beginning with `-` is refused with an error rather than passed through to the `BrowserStackLocal` argv — otherwise any code that merges untrusted input into the options object could inject arbitrary flags into the binary. + #### Verbose Logging To enable verbose logging - ```js diff --git a/lib/Local.js b/lib/Local.js index 8f783d7..b50692c 100644 --- a/lib/Local.js +++ b/lib/Local.js @@ -9,6 +9,60 @@ var childProcess = require('child_process'), version = require('../package.json').version, treeKill = require('tree-kill'); +// Option keys this wrapper is allowed to forward verbatim to the +// BrowserStackLocal daemon. Mirrors the binary's own CLI definition +// (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. +var PASSTHROUGH_OPTIONS = [ + 'key', 'folder', 'help', 'version', 'force', 'only', + 'forcelocal', 'force-local', + 'verbose', + 'onlyAutomate', 'only-automate', + 'proxyHost', 'proxy-host', + 'proxyPort', 'proxy-port', + 'proxyUser', 'proxy-user', + 'proxyPass', 'proxy-pass', + 'localIdentifier', 'local-identifier', + 'forceproxy', 'force-proxy', + 'region', + 'localProxyHost', 'local-proxy-host', + 'localProxyPort', 'local-proxy-port', + 'localProxyUser', 'local-proxy-user', + 'localProxyPass', 'local-proxy-pass', + 'enableLoggingForAPI', 'enable-logging-for-api', + 'logFile', + 'pacFile', 'pac-file', + 'parallelRuns', 'parallel-runs', + 'disableProxyDiscovery', 'disable-proxy-discovery', + 'enableUTCLogging', 'enable-utc-logging', + 'no-container', + 'include-hosts', 'exclude-hosts', + 'bsHost', 'bs-host', + 'debug-utility', 'debug-url', + 'customRepeater', 'custom-repeater', + 'enterprise', + 'use-system-installed-ca', 'use-ca-certificate', + 'https-ports', + 'ntlm-username', 'ntlm-password', 'ntlm-domain', 'ntlm-workstation', + 'connect-timeout', + 'public-interface-services', + 'disableDashboard', 'disable-dashboard', + 'config-file', + 'client-protocol', + 'identifier', + 'trusted-hosts' +]; + +// 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.) +var RESERVED_OPTIONS = ['daemon', 'log-file', 'source']; + +// Keys consumed by this wrapper and never meant for the binary. +var INTERNAL_OPTIONS = ['onlyCommand']; + function Local(){ this.sanitizePath = function(rawPath) { var doubleQuoteIfRequired = this.windows && !rawPath.match(/"[^"]+"/) ? '"' : ''; @@ -30,7 +84,9 @@ function Local(){ this.startSync = function(options) { this.userArgs = []; var that = this; - this.addArgs(options); + const argsError = this.addArgs(options); + if(argsError) + return argsError; if(typeof options['onlyCommand'] !== 'undefined') return; @@ -83,7 +139,9 @@ function Local(){ this.start = function(options, callback){ this.userArgs = []; var that = this; - this.addArgs(options); + const argsError = this.addArgs(options); + if(argsError) + return callback(argsError); if(typeof options['onlyCommand'] !== 'undefined') return callback(); @@ -245,18 +303,46 @@ function Local(){ this.binaryPath = value; break; - default: - if(value.toString().toLowerCase() == 'true'){ - this.userArgs.push('--' + key); - } else { - this.userArgs.push('--' + key); - this.userArgs.push(value); - } + default: { + var error = this.addUserArg(key, value); + if(error) + return error; break; } + } } }; + // 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. + this.addUserArg = function(key, value){ + if(INTERNAL_OPTIONS.indexOf(key) !== -1) + return; + + if(RESERVED_OPTIONS.indexOf(key) !== -1) + return new LocalError('Option \'' + key + '\' is set by browserstack-local itself and cannot be passed in'); + + 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(); + + if(stringValue.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'); + + this.userArgs.push('--' + key); + this.userArgs.push(value); + }; + this.getBinaryPath = function(callback, bsHost){ if(typeof(this.binaryPath) == 'undefined'){ this.binary = new LocalBinary(); diff --git a/test/local.js b/test/local.js index 79c10eb..514c4b7 100644 --- a/test/local.js +++ b/test/local.js @@ -124,20 +124,80 @@ describe('Local', function () { }); }); - it('should enable custom boolean args', function (done) { - bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'boolArg1': true, 'boolArg2': true }, function(){ - expect(bsLocal.getBinaryArgs().indexOf('--boolArg1')).to.not.equal(-1); - expect(bsLocal.getBinaryArgs().indexOf('--boolArg2')).to.not.equal(-1); + // 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. + + it('should reject unknown boolean args', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'boolArg1': true, 'boolArg2': true }, function(error){ + expect(error).to.be.an(Error); + expect(error.toString()).to.contain('Unknown option \'boolArg1\''); + expect(bsLocal.getBinaryArgs().indexOf('--boolArg1')).to.equal(-1); + expect(bsLocal.getBinaryArgs().indexOf('--boolArg2')).to.equal(-1); done(); }); }); - it('should enable custom keyval args', function (done) { - bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'customKey1': 'custom value1', 'customKey2': 'custom value2' }, function(){ - expect(bsLocal.getBinaryArgs().indexOf('--customKey1')).to.not.equal(-1); - expect(bsLocal.getBinaryArgs().indexOf('custom value1')).to.not.equal(-1); - expect(bsLocal.getBinaryArgs().indexOf('--customKey2')).to.not.equal(-1); - expect(bsLocal.getBinaryArgs().indexOf('custom value2')).to.not.equal(-1); + it('should reject unknown keyval args', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'customKey1': 'custom value1', 'customKey2': 'custom value2' }, function(error){ + expect(error).to.be.an(Error); + expect(error.toString()).to.contain('Unknown option \'customKey1\''); + expect(bsLocal.getBinaryArgs().indexOf('--customKey1')).to.equal(-1); + expect(bsLocal.getBinaryArgs().indexOf('custom value1')).to.equal(-1); + done(); + }); + }); + + it('should reject the reported flag-injection payload', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'config': '/tmp/attacker.conf', 'daemon': 'stop' }, function(error){ + expect(error).to.be.an(Error); + const args = bsLocal.getBinaryArgs(); + expect(args.indexOf('--config')).to.equal(-1); + expect(args.indexOf('/tmp/attacker.conf')).to.equal(-1); + // the wrapper's own '--daemon start' must be the only daemon flag + expect(args.indexOf('stop')).to.equal(-1); + done(); + }); + }); + + it('should reject options the wrapper sets itself', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'log-file': '/tmp/attacker-owned' }, function(error){ + expect(error).to.be.an(Error); + expect(error.toString()).to.contain('set by browserstack-local itself'); + expect(bsLocal.getBinaryArgs().indexOf('/tmp/attacker-owned')).to.equal(-1); + done(); + }); + }); + + it('should reject a value that would be parsed as another flag', function (done) { + // bs-minimist does not consume a value beginning with '-'; it reads it as + // a separate flag, so a legitimate key can still smuggle one in. + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'region': '--pac-file' }, function(error){ + expect(error).to.be.an(Error); + expect(error.toString()).to.contain('values starting with \'-\' are not allowed'); + expect(bsLocal.getBinaryArgs().indexOf('--pac-file')).to.equal(-1); + 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); + expect(bsLocal.getBinaryArgs().indexOf('--onlyCommand')).to.equal(-1); + done(); + }); + }); + + it('should still forward documented modifiers that have no explicit case', function (done) { + bsLocal.start({ 'key': process.env.BROWSERSTACK_ACCESS_KEY, onlyCommand: true, 'localProxyHost': '127.0.0.1', 'pac-file': '/tmp/proxy.pac' }, function(error){ + expect(error).to.equal(undefined); + const args = bsLocal.getBinaryArgs(); + expect(args.indexOf('--localProxyHost')).to.not.equal(-1); + expect(args.indexOf('127.0.0.1')).to.not.equal(-1); + expect(args.indexOf('--pac-file')).to.not.equal(-1); + expect(args.indexOf('/tmp/proxy.pac')).to.not.equal(-1); done(); }); }); From 4ab626247b191866938eb9ab64807fd7ff1def4c Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Thu, 6 Aug 2026 22:16:01 +0530 Subject: [PATCH 2/2] Apply the value check to every option key, not just passthrough ones MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- lib/Local.js | 55 ++++++++++++++++++++++++++++++++++-------- test/local.js | 66 +++++++++++++++++++++++++++++++++++++++++++++++---- 2 files changed, 106 insertions(+), 15 deletions(-) diff --git a/lib/Local.js b/lib/Local.js index b50692c..ea19edb 100644 --- a/lib/Local.js +++ b/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){ diff --git a/test/local.js b/test/local.js index 514c4b7..e83bf95 100644 --- a/test/local.js +++ b/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);