Skip to content

Commit b8ccb47

Browse files
committed
fix: honor proxyUser/proxyPass when downloading the Local binary
proxyHost/proxyPort were threaded through to every binary-download request (the async in-process path, and the two spawned child scripts), but proxyUser/proxyPass were only ever forwarded to the already-running BrowserStackLocal binary's own --proxy-user/--proxy-pass flags - Local.js's conf object never carried them, so an authenticating proxy accepted the binary's own traffic but rejected the download of the binary itself. Thread proxyUser/proxyPass through the same four call sites that already handle proxyHost/proxyPort, building an HttpsProxyAgent `auth` option ("user:pass", matching what https-proxy-agent expects for the Proxy-Authorization header) from them. For the two spawned children (download.js, fetchDownloadSourceUrl.js), pass the credentials via env instead of argv, mirroring this codebase's existing BROWSERSTACK_LOCAL_AUTH_TOKEN pattern - argv is readable via `ps`/ /proc/<pid>/cmdline, env is not. Added 5 regression tests. Confirmed each fails against the pre-fix code and passes with the fix. Fixes #164
1 parent 8096a53 commit b8ccb47

6 files changed

Lines changed: 171 additions & 22 deletions

File tree

‎lib/Local.js‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -295,6 +295,10 @@ function Local(){
295295
if(this.proxyHost && this.proxyPort){
296296
conf.proxyHost = this.proxyHost;
297297
conf.proxyPort = this.proxyPort;
298+
if (this.proxyUser && this.proxyPass) {
299+
conf.proxyUser = this.proxyUser;
300+
conf.proxyPass = this.proxyPass;
301+
}
298302
}
299303
if (this.useCaCertificate) {
300304
conf.useCaCertificate = this.useCaCertificate;

‎lib/LocalBinary.js‎

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,12 @@ function LocalBinary(){
7070
if (this.key) {
7171
env.BROWSERSTACK_LOCAL_AUTH_TOKEN = this.key;
7272
}
73+
/* Same reasoning as the auth token above: proxy credentials go through the
74+
environment, not argv. */
75+
if (conf.proxyUser && conf.proxyPass) {
76+
env.BROWSERSTACK_LOCAL_PROXY_USER = conf.proxyUser;
77+
env.BROWSERSTACK_LOCAL_PROXY_PASS = conf.proxyPass;
78+
}
7379
const obj = childProcess.spawnSync(cmd, opts, { env: env });
7480
if(obj.stdout.length > 0) {
7581
this.sourceURL = obj.stdout.toString().replace(/\n+$/, '');
@@ -100,7 +106,7 @@ function LocalBinary(){
100106
downloadErrorMessage = this.downloadErrorMessage || this.downloadState.errorMessage;
101107
}
102108

103-
fetchDownloadSourceUrlAsync(this.key, this.bsHost, downloadFallback, downloadErrorMessage, conf.proxyHost, conf.proxyPort, conf.useCaCertificate, (err, sourceURL) => {
109+
fetchDownloadSourceUrlAsync(this.key, this.bsHost, downloadFallback, downloadErrorMessage, conf.proxyHost, conf.proxyPort, conf.useCaCertificate, conf.proxyUser, conf.proxyPass, (err, sourceURL) => {
104110
if (err) return callback(err);
105111
this.sourceURL = sourceURL;
106112
this.downloadState.sourceURL = sourceURL;
@@ -197,6 +203,10 @@ function LocalBinary(){
197203
try{
198204
const userAgent = [packageName, version].join('/');
199205
const env = Object.assign({ 'USER_AGENT': userAgent }, process.env);
206+
if (conf.proxyUser && conf.proxyPass) {
207+
env.BROWSERSTACK_LOCAL_PROXY_USER = conf.proxyUser;
208+
env.BROWSERSTACK_LOCAL_PROXY_PASS = conf.proxyPass;
209+
}
200210
const obj = childProcess.spawnSync(cmd, opts, { env: env });
201211
let output;
202212
if(obj.stdout.length > 0) {
@@ -236,10 +246,11 @@ function LocalBinary(){
236246

237247
var options = url.parse(this.httpPath);
238248
if(conf.proxyHost && conf.proxyPort) {
239-
options.agent = new HttpsProxyAgent({
240-
host: conf.proxyHost,
241-
port: conf.proxyPort
242-
});
249+
var proxyOpts = { host: conf.proxyHost, port: conf.proxyPort };
250+
if (conf.proxyUser && conf.proxyPass) {
251+
proxyOpts.auth = `${conf.proxyUser}:${conf.proxyPass}`;
252+
}
253+
options.agent = new HttpsProxyAgent(proxyOpts);
243254
}
244255
if (conf.useCaCertificate) {
245256
try {

‎lib/download.js‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,9 @@ const https = require('https'),
66
{ isUndefined } = require('./util');
77

88
const binaryPath = process.argv[2], httpPath = process.argv[3], proxyHost = process.argv[4], proxyPort = process.argv[5], useCaCertificate = process.argv[6];
9+
/* Proxy credentials are read from the environment, never argv: argv is
10+
world-readable via `ps` / /proc/<pid>/cmdline. Mirrors fetchDownloadSourceUrl.js. */
11+
const proxyUser = process.env.BROWSERSTACK_LOCAL_PROXY_USER, proxyPass = process.env.BROWSERSTACK_LOCAL_PROXY_PASS;
912

1013
var fileStream = fs.createWriteStream(binaryPath);
1114

@@ -15,10 +18,11 @@ var options = url.parse(httpPath);
1518
arrive here as the *string* "undefined" — which is truthy, and previously
1619
built a proxy agent pointing at the host "undefined". */
1720
if(!isUndefined(proxyHost) && !isUndefined(proxyPort)) {
18-
options.agent = new HttpsProxyAgent({
19-
host: proxyHost,
20-
port: proxyPort
21-
});
21+
const proxyOpts = { host: proxyHost, port: proxyPort };
22+
if (!isUndefined(proxyUser) && !isUndefined(proxyPass)) {
23+
proxyOpts.auth = `${proxyUser}:${proxyPass}`;
24+
}
25+
options.agent = new HttpsProxyAgent(proxyOpts);
2226
}
2327

2428
/* Applied regardless of whether a proxy is configured: this is the caller's TLS

‎lib/fetchDownloadSourceUrl.js‎

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,10 +3,11 @@ const https = require('https'),
33
HttpsProxyAgent = require('https-proxy-agent'),
44
{ isUndefined } = require('./util');
55

6-
/* The auth token is read from the environment, never from argv: argv is world-readable
7-
via `ps` / /proc/<pid>/cmdline, whereas /proc/<pid>/environ is restricted to the
8-
owning user. Keep it out of this argument list. */
9-
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];
6+
/* The auth token and proxy credentials are read from the environment, never from
7+
argv: argv is world-readable via `ps` / /proc/<pid>/cmdline, whereas
8+
/proc/<pid>/environ is restricted to the owning user. Keep them out of this
9+
argument list. */
10+
const authToken = process.env.BROWSERSTACK_LOCAL_AUTH_TOKEN, proxyUser = process.env.BROWSERSTACK_LOCAL_PROXY_USER, proxyPass = process.env.BROWSERSTACK_LOCAL_PROXY_PASS, bsHost = process.argv[2], proxyHost = process.argv[5], proxyPort = process.argv[6], useCaCertificate = process.argv[7], downloadFallback = process.argv[3], downloadErrorMessage = process.argv[4];
1011

1112
let body = '', data = {'auth_token': authToken};
1213
const options = {
@@ -25,10 +26,11 @@ if (downloadFallback == 'true') {
2526
}
2627

2728
if(!isUndefined(proxyHost) && !isUndefined(proxyPort)) {
28-
options.agent = new HttpsProxyAgent({
29-
host: proxyHost,
30-
port: proxyPort
31-
});
29+
const proxyOpts = { host: proxyHost, port: proxyPort };
30+
if (!isUndefined(proxyUser) && !isUndefined(proxyPass)) {
31+
proxyOpts.auth = `${proxyUser}:${proxyPass}`;
32+
}
33+
options.agent = new HttpsProxyAgent(proxyOpts);
3234
}
3335
if (!isUndefined(useCaCertificate)) {
3436
try {

‎lib/fetchDownloadSourceUrlAsync.js‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ const https = require('https'),
66

77
const packageName = 'browserstack-local-nodejs';
88

9-
function fetchDownloadSourceUrlAsync(authToken, bsHost, downloadFallback, downloadErrorMessage, proxyHost, proxyPort, useCaCertificate, callback) {
9+
function fetchDownloadSourceUrlAsync(authToken, bsHost, downloadFallback, downloadErrorMessage, proxyHost, proxyPort, useCaCertificate, proxyUser, proxyPass, callback) {
1010
let body = '', data = {'auth_token': authToken};
1111
const userAgent = [packageName, version].join('/');
1212
const options = {
@@ -25,10 +25,11 @@ function fetchDownloadSourceUrlAsync(authToken, bsHost, downloadFallback, downlo
2525
}
2626

2727
if(!isUndefined(proxyHost) && !isUndefined(proxyPort)) {
28-
options.agent = new HttpsProxyAgent({
29-
host: proxyHost,
30-
port: proxyPort
31-
});
28+
const proxyOpts = { host: proxyHost, port: proxyPort };
29+
if (!isUndefined(proxyUser) && !isUndefined(proxyPass)) {
30+
proxyOpts.auth = `${proxyUser}:${proxyPass}`;
31+
}
32+
options.agent = new HttpsProxyAgent(proxyOpts);
3233
}
3334
if (!isUndefined(useCaCertificate)) {
3435
try {

‎test/local.js‎

Lines changed: 127 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -491,6 +491,133 @@ describe('LocalBinary', function () {
491491
});
492492
});
493493

494+
// Regression tests for https://github.com/browserstack/browserstack-local-nodejs/issues/164:
495+
// proxyUser/proxyPass were accepted by Local's config and forwarded to the
496+
// BrowserStackLocal *binary* itself (--proxy-user/--proxy-pass), but never reached
497+
// the node-side binary download, so an authenticating proxy rejected the download
498+
// even though the same config worked for everything the binary does afterwards.
499+
describe('Proxy authentication for binary download', function () {
500+
var https = require('https');
501+
var childProcess = require('child_process');
502+
var Local = require('../lib/Local');
503+
var binary, sandBox, tempDownloadPath;
504+
505+
beforeEach(function () {
506+
binary = new LocalBinary();
507+
sandBox = sinon.sandbox.create();
508+
tempDownloadPath = path.join(process.cwd(), 'download-proxy-auth');
509+
});
510+
511+
afterEach(function () {
512+
sandBox.restore();
513+
rimraf.sync(tempDownloadPath);
514+
});
515+
516+
it('sends proxy credentials on the CONNECT agent used by the async download', function (done) {
517+
sandBox.stub(binary, 'getDownloadPath', function (conf, retries, callback) {
518+
callback(null, 'https://example.invalid/fake-binary');
519+
});
520+
sandBox.stub(https, 'get', function (options) {
521+
check(done, function () {
522+
expect(options.agent.proxy.host).to.equal('127.0.0.1');
523+
expect(String(options.agent.proxy.port)).to.equal('8080');
524+
expect(options.agent.proxy.auth).to.equal('proxyuser:proxypass');
525+
});
526+
return { on: function () { return this; } };
527+
});
528+
529+
binary.download({
530+
proxyHost: '127.0.0.1',
531+
proxyPort: 8080,
532+
proxyUser: 'proxyuser',
533+
proxyPass: 'proxypass'
534+
}, tempDownloadPath, function () {});
535+
});
536+
537+
it('does not set agent auth when no proxy credentials are configured', function (done) {
538+
sandBox.stub(binary, 'getDownloadPath', function (conf, retries, callback) {
539+
callback(null, 'https://example.invalid/fake-binary');
540+
});
541+
sandBox.stub(https, 'get', function (options) {
542+
check(done, function () {
543+
expect(options.agent.proxy.auth).to.equal(undefined);
544+
});
545+
return { on: function () { return this; } };
546+
});
547+
548+
binary.download({ proxyHost: '127.0.0.1', proxyPort: 8080 }, tempDownloadPath, function () {});
549+
});
550+
551+
it('passes proxy credentials to the spawned download.js child via env, not argv', function () {
552+
var spawnStub = sandBox.stub(childProcess, 'spawnSync', function () {
553+
return { stdout: Buffer.from('ok'), stderr: Buffer.from('') };
554+
});
555+
sandBox.stub(fs, 'existsSync', function () { return true; });
556+
sandBox.stub(fs, 'chmodSync', function () {});
557+
558+
binary.downloadSync({
559+
proxyHost: '127.0.0.1',
560+
proxyPort: 8080,
561+
proxyUser: 'proxyuser',
562+
proxyPass: 'proxypass'
563+
}, tempDownloadPath, 0);
564+
565+
var call = spawnStub.getCall(0);
566+
expect(call.args[1]).to.not.contain('proxyuser');
567+
expect(call.args[1]).to.not.contain('proxypass');
568+
expect(call.args[2].env.BROWSERSTACK_LOCAL_PROXY_USER).to.equal('proxyuser');
569+
expect(call.args[2].env.BROWSERSTACK_LOCAL_PROXY_PASS).to.equal('proxypass');
570+
});
571+
572+
it('passes proxy credentials to the spawned fetchDownloadSourceUrl.js child via env, not argv', function () {
573+
var spawnStub = sandBox.stub(childProcess, 'spawnSync', function () {
574+
return { stdout: Buffer.from('https://example.invalid'), stderr: Buffer.from('') };
575+
});
576+
577+
binary.getSourceUrlSync({
578+
proxyHost: '127.0.0.1',
579+
proxyPort: 8080,
580+
proxyUser: 'proxyuser',
581+
proxyPass: 'proxypass'
582+
}, 0);
583+
584+
var call = spawnStub.getCall(0);
585+
expect(call.args[1]).to.not.contain('proxyuser');
586+
expect(call.args[1]).to.not.contain('proxypass');
587+
expect(call.args[2].env.BROWSERSTACK_LOCAL_PROXY_USER).to.equal('proxyuser');
588+
expect(call.args[2].env.BROWSERSTACK_LOCAL_PROXY_PASS).to.equal('proxypass');
589+
});
590+
591+
it('forwards proxyUser/proxyPass from Local config through to the download child process env', function () {
592+
var spawnStub = sandBox.stub(childProcess, 'spawnSync', function () {
593+
return { stdout: Buffer.from('https://example.invalid'), stderr: Buffer.from('') };
594+
});
595+
// Make the sync path succeed on the first attempt instead of retrying:
596+
// downloadSync's spawnSync is stubbed and never actually writes a binary,
597+
// so without this the real retry loop (async via fs.unlink) keeps firing
598+
// child processes after the test has already finished and its stubs are
599+
// restored.
600+
sandBox.stub(fs, 'existsSync', function () { return true; });
601+
sandBox.stub(fs, 'chmodSync', function () {});
602+
603+
var bsLocal = new Local();
604+
bsLocal.proxyHost = '127.0.0.1';
605+
bsLocal.proxyPort = 8080;
606+
bsLocal.proxyUser = 'proxyuser';
607+
bsLocal.proxyPass = 'proxypass';
608+
609+
// No callback -> the sync path, which ends in the same spawnSync used by
610+
// getSourceUrlSync/downloadSync, whichever this hits first with an empty
611+
// binary directory.
612+
bsLocal.getBinaryPath();
613+
614+
expect(spawnStub.called).to.equal(true);
615+
var call = spawnStub.getCall(0);
616+
expect(call.args[2].env.BROWSERSTACK_LOCAL_PROXY_USER).to.equal('proxyuser');
617+
expect(call.args[2].env.BROWSERSTACK_LOCAL_PROXY_PASS).to.equal('proxypass');
618+
});
619+
});
620+
494621
// Regression tests: the binary-download fallback signalling used to live on
495622
// process.env, so (a) a value planted in process.env steered the download to an
496623
// arbitrary host with no validation, and (b) a failure on one Local instance bled

0 commit comments

Comments
 (0)