From 3a4e9c6c69a0571e20a9ae48885ad23c29200109 Mon Sep 17 00:00:00 2001 From: xzyfer Date: Fri, 23 Sep 2016 01:51:47 +1000 Subject: [PATCH 1/2] Respect NO_PROXY when downloading the binary Turns our handling proxies is complicated. The sanest thing to do is try matching [npm's behaviour][1], and liable [to change][1] to our benefit. The TLDR; of which is to let `request` do whatever it wants unless npm has been explicitly configured otherwise. Fixes #1724 [1]: https://github.com/npm/npm/commit/40afd6aaf34 [2]: https://github.com/npm/npm/issues/7168 --- scripts/install.js | 26 +++++++++++++++----------- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/scripts/install.js b/scripts/install.js index 09a115673..58e90c008 100644 --- a/scripts/install.js +++ b/scripts/install.js @@ -96,21 +96,25 @@ function download(url, dest, cb) { } /** - * Determine local proxy settings + * Determine the proxy settings configured by npm * - * @param {Object} options - * @param {Function} cb + * It's possible to configure npm to use a proxy different + * from the system defined proxy. This can be done via the + * `npm config` CLI or the `.npmrc` config file. + * + * If a proxy has been configured in this way we must + * tell request explicitly to use it. + * + * Otherwise we can trust request to the right thing. + * + * @return {String} the proxy configured by npm or an empty string * @api private */ - function getProxy() { - return process.env.npm_config_https_proxy || - process.env.npm_config_proxy || - process.env.npm_config_http_proxy || - process.env.HTTPS_PROXY || - process.env.https_proxy || - process.env.HTTP_PROXY || - process.env.http_proxy; + return '' || + process.env.npm_config_https_proxy || + process.env.npm_config_proxy || + process.env.npm_config_http_proxy; } /** From 5dba770e05ea37fb833091b8f6d560a8683db9ae Mon Sep 17 00:00:00 2001 From: xzyfer Date: Mon, 26 Sep 2016 20:15:32 +1000 Subject: [PATCH 2/2] Factor our proxy resolution for testability --- package.json | 2 +- scripts/install.js | 25 +------------ scripts/util/proxy.js | 22 +++++++++++ test/scripts/util/proxy.js | 76 ++++++++++++++++++++++++++++++++++++++ 4 files changed, 101 insertions(+), 24 deletions(-) create mode 100644 scripts/util/proxy.js create mode 100644 test/scripts/util/proxy.js diff --git a/package.json b/package.json index 95b25373d..beeba4ccd 100644 --- a/package.json +++ b/package.json @@ -31,7 +31,7 @@ "install": "node scripts/install.js", "postinstall": "node scripts/build.js", "lint": "node_modules/.bin/eslint bin/node-sass lib scripts test", - "test": "node_modules/.bin/mocha test", + "test": "node_modules/.bin/mocha test/{*,**/**}.js", "build": "node scripts/build.js --force", "prepublish": "not-in-install && node scripts/prepublish.js || in-install" }, diff --git a/scripts/install.js b/scripts/install.js index 58e90c008..10414b9f8 100644 --- a/scripts/install.js +++ b/scripts/install.js @@ -10,6 +10,7 @@ var fs = require('fs'), request = require('request'), log = require('npmlog'), pkg = require('../package.json'), + proxy = require('./util/proxy'), userAgent = require('./util/useragent'); /** @@ -51,7 +52,7 @@ function download(url, dest, cb) { var options = { rejectUnauthorized: false, - proxy: getProxy(), + proxy: proxy(), timeout: 60000, headers: { 'User-Agent': userAgent(), @@ -95,28 +96,6 @@ function download(url, dest, cb) { } } -/** - * Determine the proxy settings configured by npm - * - * It's possible to configure npm to use a proxy different - * from the system defined proxy. This can be done via the - * `npm config` CLI or the `.npmrc` config file. - * - * If a proxy has been configured in this way we must - * tell request explicitly to use it. - * - * Otherwise we can trust request to the right thing. - * - * @return {String} the proxy configured by npm or an empty string - * @api private - */ -function getProxy() { - return '' || - process.env.npm_config_https_proxy || - process.env.npm_config_proxy || - process.env.npm_config_http_proxy; -} - /** * Check and download binary * diff --git a/scripts/util/proxy.js b/scripts/util/proxy.js new file mode 100644 index 000000000..e65eac5d4 --- /dev/null +++ b/scripts/util/proxy.js @@ -0,0 +1,22 @@ + +/** + * Determine the proxy settings configured by npm + * + * It's possible to configure npm to use a proxy different + * from the system defined proxy. This can be done via the + * `npm config` CLI or the `.npmrc` config file. + * + * If a proxy has been configured in this way we must + * tell request explicitly to use it. + * + * Otherwise we can trust request to the right thing. + * + * @return {String} the proxy configured by npm or an empty string + * @api private + */ +module.exports = function() { + return process.env.npm_config_https_proxy || + process.env.npm_config_proxy || + process.env.npm_config_http_proxy || + ''; +}; diff --git a/test/scripts/util/proxy.js b/test/scripts/util/proxy.js new file mode 100644 index 000000000..d829bdbc3 --- /dev/null +++ b/test/scripts/util/proxy.js @@ -0,0 +1,76 @@ +var assert = require('assert'), + proxy = require('../../../scripts/util/proxy'); + +describe('proxy', function() { + var oldEnvironment; + + beforeEach(function() { + oldEnvironment = process.env; + }); + + afterEach(function() { + process.env = oldEnvironment; + }); + + describe('without an npm proxy config', function() { + delete process.env.npm_config_https_proxy; + delete process.env.npm_config_proxy; + delete process.env.npm_config_http_proxy; + + it('should return an empty string', function() { + assert.strictEqual('', proxy()); + }); + + it('should ignore system proxy environment variables', function() { + process.env.HTTPS_PROXY = 'http://https_proxy.com'; + process.env.PROXY = 'http://proxy.com'; + process.env.HTTP_PROXY = 'http://http_proxy.com'; + + assert.strictEqual('', proxy()); + }); + }); + + describe('with an npm proxy config', function() { + beforeEach(function() { + process.env.npm_config_https_proxy = 'http://https_proxy.com'; + process.env.npm_config_proxy = 'http://proxy.com'; + process.env.npm_config_http_proxy = 'http://http_proxy.com'; + }); + + describe('https_proxy', function() { + it('should have the highest precedence', function() { + assert.strictEqual(process.env.npm_config_https_proxy, proxy()); + }); + }); + + describe('proxy', function() { + it('should have the higher precedence than https_proxy', function() { + assert.strictEqual(process.env.npm_config_https_proxy, proxy()); + delete process.env.npm_config_https_proxy; + + assert.strictEqual(process.env.npm_config_proxy, proxy()); + }); + + it('should have the lower precedence than http_proxy', function() { + delete process.env.npm_config_https_proxy; + + assert.strictEqual(process.env.npm_config_proxy, proxy()); + delete process.env.npm_config_proxy; + + assert.strictEqual(process.env.npm_config_http_proxy, proxy()); + }); + }); + + describe('http_proxy', function() { + it('should have the lowest precedence', function() { + assert.strictEqual(process.env.npm_config_https_proxy, proxy()); + delete process.env.npm_config_https_proxy; + + assert.strictEqual(process.env.npm_config_proxy, proxy()); + delete process.env.npm_config_proxy; + + assert.strictEqual(process.env.npm_config_http_proxy, proxy()); + }); + }); + }); +}); \ No newline at end of file