Skip to content

Keep the access key out of child argv; honour useCaCertificate without a proxy #180

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
07souravkunda wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Keep the access key out of child argv; honour useCaCertificate without a proxy #180

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .github/workflows/Semgrep.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:<tag>
image: returntocorp/semgrep:1.166.0@sha256:c180f0c93a17b420c0af5006214a29d3c747c5459c732b740191adf657dd0068
# Skip any PR created by dependabot to avoid permission issues:
if: (github.actor != 'dependabot[bot]')

Expand Down
54 changes: 44 additions & 10 deletions lib/LocalBinary.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/<pid>/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);
Expand All @@ -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;

@07souravkunda 07souravkunda Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[nit] The move off argv is right, and I confirmed the position shift lines up exactly with the new fetchDownloadSourceUrl.js reads (bsHost argv[2] → downloadFallback [3] → downloadErrorMessage [4] → proxyHost [5] → proxyPort [6] → useCaCertificate [7]).

One small thing: because env is seeded from process.env and the assignment is behind if (this.key), an ambient BROWSERSTACK_LOCAL_AUTH_TOKEN in the parent's environment now flows through to the child whenever this.key is falsy. On master that case sent the literal string "undefined" in argv, so it always failed cleanly; now it can silently authenticate with a value the caller never passed to Local.start(). Unlikely to bite given the variable name, but it makes the child's auth non-deterministic w.r.t. the caller's own config.

env.BROWSERSTACK_LOCAL_AUTH_TOKEN = this.key || '';

Also worth a line in the README: this is a public package, and the variable is now a de-facto input to it.

}
const obj = childProcess.spawnSync(cmd, opts, { env: env });
if(obj.stdout.length > 0) {
this.sourceURL = obj.stdout.toString().replace(/\n+$/, '');
Expand Down Expand Up @@ -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);
}
Expand Down Expand Up @@ -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 });

@07souravkunda 07souravkunda Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocking] mode: 0o700 here applies to every entry in orderedPaths, not just the temp fallback — so $HOME/.browserstack now gets created 0700 instead of 0755. That contradicts the PR description ("$HOME/.browserstack and process.cwd() keep their existing behaviour") and the same sentence in the ticket's completion comment.

Evidence — the same fs.mkdirSync call each branch makes, under the default umask 022:

umask                   : 22
master  ~/.browserstack : 0755
PR#180  ~/.browserstack : 0700

getAvailableDirs passes requirePrivate = true only for i === length - 1 (line 320), so the check is correctly scoped — but the create mode is not, because it sits above the requirePrivate branch.

Why it matters: the pre-warm pattern — an image build or setup step downloads ~/.browserstack/BrowserStackLocal as one uid, the test step runs as another with the same $HOME — works today because the directory is world-traversable. At 0700 the second uid can no longer traverse it, so a setup that works on master breaks after upgrade, silently and with no note in the release. And the tightening buys nothing here: ~/.browserstack is still accepted with no ownership/permission check, so an attacker-writable one is used exactly as before.

Fix — scope the mode to the path that is actually being hardened:

if(!this.checkPath(path)){
  fs.mkdirSync(path, requirePrivate ? { mode: 0o700 } : undefined);
}

(Keeping 0700 everywhere is also defensible — but then the PR body and the completion comment both need to say so, and it should be called out as a behaviour change.)

}
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){

@07souravkunda 07souravkunda Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[blocking] Four of the five changes here are behavioural (argv→env token transport, isUndefined proxy guard + unconditional CA, single fs.unlink, per-uid private temp dir) and no test lands in the repo with them. The 15 targeted checks that validate them live in the fix session's scratch folder and are explicitly not committed, so they disappear with the session and the next person touching getAvailableDirs/getSourceUrlSync has nothing to break.

The stated reason is that the repo's only suite is a credential-gated live-integration suite with no offline seam for these paths. That isn't quite right — verify-fixes.js is the offline seam. Eleven of its fifteen checks need neither credentials nor network:

  • argv probe: key absent from every argv element; child resolves it from env; all six argv positions map correctly after the shift
  • source assertions on fetchDownloadSourceUrl.js and download.js (token from env; options.ca outside the proxy guard; isUndefined on the proxy slots)
  • retryBinaryDownload: no fs.stat/unlinkSync left, and the runtime check that retry reaches download() on ENOENT
  • all five F-021 checks: fallback is a per-uid subdir; created 0700 and accepted; world-writable dir rejected; symlink rejected; cwd still accepted

Only the two live-download checks need the network, and those are the ones worth skipping.

There's no .mocharc in the repo and test/ currently holds a single file, so mocha's default spec picks up a new test/localbinary-hardening.js with no config change. It also runs green today — LocalBinary > Retries already passes 2/2 on this branch, so a new offline file doesn't inherit the Download block's auth failure.

Ask: port those eleven checks into test/localbinary-hardening.js (describe/it around the same assertions). The two network checks can stay out, or sit behind an env guard. The cwd-still-accepted assertion is worth keeping either way — it's the guard against the ownership check creeping onto the non-temp paths.

No test is expected for the Semgrep digest pin — config-only, and a version-pin assertion would be against convention.

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;
}
Expand Down Expand Up @@ -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()
];
}

Expand Down
25 changes: 17 additions & 8 deletions lib/download.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
}

Expand Down
5 changes: 4 additions & 1 deletion lib/fetchDownloadSourceUrl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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/<pid>/cmdline, whereas /proc/<pid>/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 = {
Expand Down
Loading