Merge pull request #1725 from xzyfer/fix/no-proxy

Respect NO_PROXY when downloading the binary

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"
   },

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,24 +96,6 @@ function download(url, dest, cb) {
   }
 }
 
-/**
- * Determine local proxy settings
- *
- * @param {Object} options
- * @param {Function} cb
- * @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;
-}
-
 /**
  * Check and download binary
  *

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 ||
+    '';
+};

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());
+      });
+    });
+  });
+});