From 6964d172c95df78cfa0da0b435cf609a4150f150 Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Thu, 6 Aug 2026 22:21:42 +0530 Subject: [PATCH 1/4] 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/4] 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'); From 7852a152b8674c9f4594d7b43363f1c7f58d8a6e Mon Sep 17 00:00:00 2001 From: 07souravkunda Date: Fri, 14 Aug 2026 16:21:49 +0530 Subject: [PATCH 3/4] Keep the access key out of child argv; honour useCaCertificate without a proxy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four hardening fixes to the binary download path, all in the same area: - The access key was passed to lib/fetchDownloadSourceUrl.js as a positional argv element, so it was readable by any local user via `ps` or /proc//cmdline for the lifetime of the spawn. It now travels in the child's environment (/proc//environ is restricted to the owning user) and the remaining argv slots shift down by one accordingly. (CWE-214) - lib/download.js only applied `useCaCertificate` inside the `if (proxyHost && proxyPort)` branch, so a caller-supplied TLS trust anchor was ignored whenever no proxy was configured. Worse, the parent passes the literal `undefined` placeholders for the proxy slots in exactly that case, which arrive as the truthy *string* "undefined" — so the sync download built a proxy agent for host "undefined" and failed outright with `getaddrinfo ENOTFOUND undefined`. Both are fixed: the CA is applied unconditionally, and the proxy slots are compared with the existing `isUndefined` helper (as lib/fetchDownloadSourceUrl.js already does). (CWE-295) - retryBinaryDownload did an async fs.stat followed by a synchronous fs.unlinkSync inside the callback. Collapsed to a single ENOENT-tolerant fs.unlink, removing the window between the two and the uncatchable throw a failing unlinkSync raised from within the stat callback. (CWE-362) - getAvailableDirs fell back to os.tmpdir() itself — /tmp on Linux, which is world-writable — under a fixed, predictable binary name. It now uses a per-uid subdirectory created 0700, and that fallback is rejected unless it is a real directory owned by us and not group/world-writable, so a pre-created symlink or shared directory cannot be used as the destination for a binary we are about to execute. Only the temp fallback is subjected to this check; $HOME/.browserstack and cwd are unchanged. (CWE-377) Also pins the Semgrep CI container to an immutable digest so a mutated tag cannot redirect the workflow to a different image. (CWE-829) --- .github/workflows/Semgrep.yml | 5 +++- lib/LocalBinary.js | 54 ++++++++++++++++++++++++++++------- lib/download.js | 25 ++++++++++------ lib/fetchDownloadSourceUrl.js | 5 +++- 4 files changed, 69 insertions(+), 20 deletions(-) diff --git a/.github/workflows/Semgrep.yml b/.github/workflows/Semgrep.yml index 95c5710..c5e4e21 100644 --- a/.github/workflows/Semgrep.yml +++ b/.github/workflows/Semgrep.yml @@ -27,7 +27,10 @@ jobs: container: # A Docker image with Semgrep installed. Do not change this. - image: returntocorp/semgrep:1.166.0 + # Pinned to an immutable digest so a mutated tag cannot redirect CI to a + # different image. Refresh with: + # docker manifest inspect returntocorp/semgrep: + image: returntocorp/semgrep:1.166.0@sha256:c180f0c93a17b420c0af5006214a29d3c747c5459c732b740191adf657dd0068 # Skip any PR created by dependabot to avoid permission issues: if: (github.actor != 'dependabot[bot]') diff --git a/lib/LocalBinary.js b/lib/LocalBinary.js index 8d694b2..0ac0a65 100644 --- a/lib/LocalBinary.js +++ b/lib/LocalBinary.js @@ -34,7 +34,9 @@ function LocalBinary(){ let cmd, opts; cmd = 'node'; - opts = [path.join(__dirname, 'fetchDownloadSourceUrl.js'), this.key, this.bsHost]; + /* The auth token is handed to the child through its environment, not argv — + argv is readable by any local user via `ps` / /proc//cmdline. */ + opts = [path.join(__dirname, 'fetchDownloadSourceUrl.js'), 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); @@ -53,6 +55,9 @@ function LocalBinary(){ const userAgent = [packageName, version].join('/'); const env = Object.assign({ 'USER_AGENT': userAgent }, process.env); + if (this.key) { + env.BROWSERSTACK_LOCAL_AUTH_TOKEN = this.key; + } const obj = childProcess.spawnSync(cmd, opts, { env: env }); if(obj.stdout.length > 0) { this.sourceURL = obj.stdout.toString().replace(/\n+$/, ''); @@ -135,10 +140,11 @@ function LocalBinary(){ var that = this; if(retries > 0) { console.log('Retrying Download. Retries left', retries); - fs.stat(binaryPath, function(err) { - if(err == null) { - fs.unlinkSync(binaryPath); - } + /* Single unlink instead of stat-then-unlinkSync: the gap between the two + let a concurrent writer swap the file, and a failing unlinkSync threw + out of the stat callback where it could not be caught. A missing file + is the expected case here, so any error is ignored. */ + fs.unlink(binaryPath, function() { if(!callback) { return that.downloadSync(conf, destParentDir, retries - 1); } @@ -310,18 +316,38 @@ function LocalBinary(){ this.getAvailableDirs = function(){ for(var i=0; i < this.orderedPaths.length; i++){ var path = this.orderedPaths[i]; - if(this.makePath(path)) + // the last entry lives under the shared temp dir — it must be ours alone + var requirePrivate = (i === this.orderedPaths.length - 1); + if(this.makePath(path, requirePrivate)) return path; } throw new LocalError('Error trying to download BrowserStack Local binary'); }; - this.makePath = function(path){ + this.makePath = function(path, requirePrivate){ try { if(!this.checkPath(path)){ - fs.mkdirSync(path); + fs.mkdirSync(path, { mode: 0o700 }); } - return true; + return requirePrivate ? this.isUserPrivateDir(path) : true; + } catch(e){ + return false; + } + }; + + /* Only applied to the shared-temp fallback. The binary is written there and + then executed, so that directory must not be writable by anyone but us — + otherwise another local user can swap the binary between the download and + the exec, or pre-create the path as a symlink. Windows has no POSIX mode + bits; there this is a no-op. */ + this.isUserPrivateDir = function(dirPath){ + if(process.platform === 'win32' || typeof process.getuid !== 'function') return true; + try { + var stats = fs.lstatSync(dirPath); + if(!stats.isDirectory()) return false; + if(stats.uid !== process.getuid()) return false; + // reject group- or world-writable + return (stats.mode & 0o022) === 0; } catch(e){ return false; } @@ -349,10 +375,18 @@ function LocalBinary(){ return home || null; }; + /* The last entry is a per-user subdirectory of the temp dir rather than the + temp dir itself: os.tmpdir() is /tmp on Linux, which is world-writable, and + the binary name below it is fixed and predictable. */ + this.tmpDirPath = function(){ + var suffix = (typeof process.getuid === 'function') ? String(process.getuid()) : 'user'; + return path.join(os.tmpdir(), 'browserstack-local-' + suffix); + }; + this.orderedPaths = [ path.join(this.homedir(), '.browserstack'), process.cwd(), - os.tmpdir() + this.tmpDirPath() ]; } diff --git a/lib/download.js b/lib/download.js index 0b0e094..dde74a2 100644 --- a/lib/download.js +++ b/lib/download.js @@ -2,24 +2,33 @@ const https = require('https'), fs = require('fs'), HttpsProxyAgent = require('https-proxy-agent'), url = require('url'), - zlib = require('zlib'); + zlib = require('zlib'), + { isUndefined } = require('./util'); const binaryPath = process.argv[2], httpPath = process.argv[3], proxyHost = process.argv[4], proxyPort = process.argv[5], useCaCertificate = process.argv[6]; var fileStream = fs.createWriteStream(binaryPath); var options = url.parse(httpPath); -if(proxyHost && proxyPort) { +/* isUndefined, not plain truthiness: the parent passes literal `undefined` + placeholders for the proxy slots when only a CA is configured, and those + arrive here as the *string* "undefined" — which is truthy, and previously + built a proxy agent pointing at the host "undefined". */ +if(!isUndefined(proxyHost) && !isUndefined(proxyPort)) { options.agent = new HttpsProxyAgent({ host: proxyHost, port: proxyPort }); - if (useCaCertificate) { - try { - options.ca = fs.readFileSync(useCaCertificate); - } catch(err) { - console.log('failed to read cert file', err); - } +} + +/* Applied regardless of whether a proxy is configured: this is the caller's TLS + trust anchor, and silently falling back to the system store when no proxy is + set ignored what they asked for. Mirrors LocalBinary.js's async download path. */ +if (!isUndefined(useCaCertificate)) { + try { + options.ca = fs.readFileSync(useCaCertificate); + } catch(err) { + console.log('failed to read cert file', err); } } diff --git a/lib/fetchDownloadSourceUrl.js b/lib/fetchDownloadSourceUrl.js index df5c8f2..6b5d37c 100644 --- a/lib/fetchDownloadSourceUrl.js +++ b/lib/fetchDownloadSourceUrl.js @@ -3,7 +3,10 @@ const https = require('https'), HttpsProxyAgent = require('https-proxy-agent'), { isUndefined } = require('./util'); -const authToken = process.argv[2], bsHost = process.argv[3], proxyHost = process.argv[6], proxyPort = process.argv[7], useCaCertificate = process.argv[8], downloadFallback = process.argv[4], downloadErrorMessage = process.argv[5]; +/* The auth token is read from the environment, never from argv: argv is world-readable + via `ps` / /proc//cmdline, whereas /proc//environ is restricted to the + owning user. Keep it out of this argument list. */ +const authToken = process.env.BROWSERSTACK_LOCAL_AUTH_TOKEN, bsHost = process.argv[2], proxyHost = process.argv[5], proxyPort = process.argv[6], useCaCertificate = process.argv[7], downloadFallback = process.argv[3], downloadErrorMessage = process.argv[4]; let body = '', data = {'auth_token': authToken}; const options = { From c2bb7901ea0716555da008d566ab0d202e2a3d4f Mon Sep 17 00:00:00 2001 From: Vivian Vijay Ludrick <116781909+vivianludrick@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:17:41 +0530 Subject: [PATCH 4/4] LOC-7325: stop uncatchable TypeError on empty binary output in Local.start MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `start()` handles the binary's output inside an `execFile` callback. The empty-output branch called back with 'No output received' but did not return, so control fell through to `data['message']['message']` on `data = {}`. That threw a TypeError, and because the throw happens inside a callback invoked by node's internal exithandler, no try/catch around `local.start(...)` could intercept it — it surfaced as an uncaughtException in the host process. Three paths reached the same unguarded deref: - empty stdout and stderr (the reported one) — now returns after the callback, so it fires exactly once - the terminal branch of the `error` handler, which also fell through - any non-connected payload with no `message` key Also guards `JSON.parse`: non-JSON output threw a SyntaxError from the same uncatchable position, and is now reported through the callback with the raw output attached as `extra`. `startSync` shared the unguarded deref and now uses the same helper. Its empty-output branch already returned, so it was not exposed to the fall-through. Adds regression tests driving start() with stub binaries for each output shape, asserting the callback fires exactly once and nothing escapes as an uncaughtException. They need no credentials or network. Three of the four fail on master with the TypeError from the ticket. --- lib/Local.js | 31 +++++++-- test/local_start_output_handling.js | 104 ++++++++++++++++++++++++++++ 2 files changed, 128 insertions(+), 7 deletions(-) create mode 100644 test/local_start_output_handling.js diff --git a/lib/Local.js b/lib/Local.js index 8f783d7..9d482b6 100644 --- a/lib/Local.js +++ b/lib/Local.js @@ -57,7 +57,7 @@ function Local(){ else return new LocalError('No output received'); if(data['state'] != 'connected'){ - return new LocalError(data['message']['message']); + return new LocalError(that.getErrorMessage(data)); } else { that.pid = data['pid']; that.isProcessRunning = true; @@ -112,19 +112,25 @@ function Local(){ return; } else { callback(new LocalError(error.toString())); + return; } } var data = {}; - if(stdout) - data = JSON.parse(stdout); - else if(stderr) - data = JSON.parse(stderr); - else + var output = stdout || stderr; + if(!output) { callback(new LocalError('No output received')); + return; + } + try { + data = JSON.parse(output); + } catch(parseError) { + callback(new LocalError('Invalid output received: ' + parseError.message, output)); + return; + } if(data['state'] != 'connected'){ - callback(new LocalError(data['message']['message'])); + callback(new LocalError(that.getErrorMessage(data))); } else { that.pid = data['pid']; that.isProcessRunning = true; @@ -134,6 +140,17 @@ function Local(){ }, options['bs-host']); }; + // The binary reports failures as {"state": "...", "message": {"message": "..."}}, + // but not every non-connected payload carries a message key. Dereferencing it + // blindly throws, and inside the execFile callback that throw is an + // uncaughtException the caller cannot catch. See LOC-7325. + this.getErrorMessage = function(data){ + var message = data && data['message']; + if(message && typeof message === 'object') + message = message['message']; + return message || 'Failed to start BrowserStack Local'; + }; + this.isRunning = function(){ return this.pid && running(this.pid) && this.isProcessRunning; }; diff --git a/test/local_start_output_handling.js b/test/local_start_output_handling.js new file mode 100644 index 0000000..458759d --- /dev/null +++ b/test/local_start_output_handling.js @@ -0,0 +1,104 @@ +var expect = require('expect.js'), + fs = require('fs'), + os = require('os'), + path = require('path'), + browserstack = require('../index'); + +// Regression tests for LOC-7325. +// +// `Local.start` handles the binary's output inside an `execFile` callback. A +// throw there is raised by node's internal exithandler, so no try/catch around +// `start()` can intercept it — it surfaces as an uncaughtException and the +// blast radius is set by the host process's exception policy. These tests drive +// `start()` with stub binaries that reproduce each output shape and assert the +// callback fires exactly once with an error, and that nothing throws. +// +// Stubs are shell scripts, so these are skipped on Windows. +describe('Local.start output handling', function () { + var stubDir, bsLocal; + + function stub(name, body) { + var stubPath = path.join(stubDir, name); + fs.writeFileSync(stubPath, '#!/bin/sh\n' + body + '\n', { mode: 0o755 }); + return stubPath; + } + + // Drives start() with the given stub and collects every callback invocation + // plus any uncaughtException raised out of the execFile callback. + function run(stubPath, done) { + var calls = [], uncaught = []; + var existing = process.listeners('uncaughtException'); + process.removeAllListeners('uncaughtException'); + process.on('uncaughtException', function (err) { uncaught.push(err); }); + + bsLocal.binaryPath = stubPath; + bsLocal.start({ key: 'dummy-key', localIdentifier: 'loc-7325' }, function (error) { + calls.push(error); + }); + + // Settle past the execFile callback before asserting, so a second + // (throwing) invocation would have happened by now if it were going to. + setTimeout(function () { + process.removeAllListeners('uncaughtException'); + existing.forEach(function (listener) { process.on('uncaughtException', listener); }); + done(calls, uncaught); + }, 1000); + } + + before(function () { + stubDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bs-local-7325-')); + }); + + beforeEach(function () { + bsLocal = new browserstack.Local(); + // Keep the stubs from clobbering ./local.log in the repo root. + bsLocal.logfile = path.join(stubDir, 'local.log'); + }); + + if (os.platform().match(/win32/i)) { + it.skip('skipped on Windows (stub binaries are shell scripts)'); + return; + } + + it('reports an error exactly once when the binary exits with no output', function (done) { + this.timeout(10000); + run(stub('empty-output.sh', 'exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0]).to.be.an('object'); + expect(calls[0].message).to.equal('No output received'); + done(); + }); + }); + + it('reports an error exactly once when the binary emits non-JSON output', function (done) { + this.timeout(10000); + run(stub('garbage-output.sh', 'echo "segmentation fault"; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.match(/^Invalid output received: /); + expect(calls[0].extra).to.match(/segmentation fault/); + done(); + }); + }); + + it('reports a fallback message when a non-connected payload has no message key', function (done) { + this.timeout(10000); + run(stub('no-message-key.sh', 'echo \'{"state":"disconnected"}\'; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.equal('Failed to start BrowserStack Local'); + done(); + }); + }); + + it('surfaces the binary message when a non-connected payload carries one', function (done) { + this.timeout(10000); + run(stub('with-message.sh', 'echo \'{"state":"disconnected","message":{"message":"Invalid key"}}\'; exit 0'), function (calls, uncaught) { + expect(uncaught).to.eql([]); + expect(calls.length).to.equal(1); + expect(calls[0].message).to.equal('Invalid key'); + done(); + }); + }); +});