restify/node-restify#878 HTTP proxy improvments (http_proxy et al) (#…

…85)



* changelog entry for http proxy improvments

* updates from yunong feedback: move TODO to a ticket, prefer 'self.'

CHANGES.md

@@ -2,6 +2,20 @@
 
 ## not yet released
 
+- restify/node-restify#878 Improved HTTP proxy handling.
+    - There is a new `options.noProxy` option to `create*Client` that overrides
+      the `NO_PROXY` envvar.
+    - `options.proxy` overrides `*_proxy` environment variables (e.g.
+      `HTTPS_PROXY`, `http_proxy`).
+    - Compatibility note: To maintain backward compatibility in 1.x, any of
+      these environment variables (in the order shown, first wins) are used to
+      proxy for http or https URLs: `HTTPS_PROXY`, `https_proxy`, `HTTP_PROXY`,
+      `http_proxy`. In a future major version, behaviour might be changed
+      to ignore `HTTPS_PROXY` and `https_proxy` environment variables when
+      proxying a *http* URL. See
+      <https://github.com/restify/node-restify/issues/878#issuecomment-249673285>
+      for discussion.
+
 ## 1.3.3
 
 - Correct usage of `assert.number` (and variants) for update from

README.md

@@ -145,6 +145,9 @@ it will be an `HttpError`.
 |version|String|semver string to set the accept-version|
 |followRedirects|Boolean|Follow redirects from server|
 |maxRedirects|Number|Maximum number of redirects to follow|
+|proxy|String|An HTTP proxy URL string (or parsed URL object) to use for requests. If not specified, then the `https_proxy` or `http_proxy` environment variables are used. Pass `proxy: false` to explicitly disable using a proxy (i.e. to ensure a proxy URL is not picked up from environment variables). See the [Proxy](#proxy) section below.|
+|noProxy|String|A comma-separated list of hosts for which to not use a proxy. If not specified, then then `NO_PROXY` environment variable is used. One can pass `noProxy: ''` to explicitly set this empty and ensure a possible environment variable is not used. See the [Proxy](#proxy) section below.|
+
 
 #### get(path, callback)
 
@@ -323,41 +326,66 @@ and note that `read` and `write` probably need to be overridden.
 
 #### Proxy
 
-There are several options for enabling a proxy for the
-http client. The following options are available to set a proxy url:
+A restify client can use an HTTP proxy, either via options to `createClient`
+or via the `http_proxy`, `https_proxy`, and `NO_PROXY` environment variables
+common in many tools (e.g., `curl`).
 
-    // Set the proxy option in the client configuration
     restify.createClient({
-        proxy: 'http://127.0.0.1'
+        proxy: <proxy url string or object>,
+        noProxy: <boolean>
     });
 
-From environment variables:
+The `proxy` option to `createClient` specifies the proxy URL, for example:
+
+    proxy: 'http://user:password@example.com:4321'
+
+Or a proxy object can be given. (Warning: the `proxyAuth` field is not what
+a simple `require('url').parse()` will produce if your proxy URL has auth
+info.)
+
+    proxy: {
+        protocol: 'http:',
+        host: 'example.com',
+        port: 4321,
+        proxyAuth: 'user:password'
+    }
 
-    $ export HTTPS_PROXY = 'https://127.0.0.1'
-    $ export HTTP_PROXY = 'http://127.0.0.1'
+Or `proxy: false` can be given to explicitly disable using a proxy -- i.e. to
+ensure a proxy URL is not picked up from environment variables.
 
-There is an option to disable the use of a proxy on a url basis or for
-all urls. This can be enabled by setting an environment variable.
+If not specified, then the following environment variables (in the given order)
+are used to pick up a proxy URL:
 
-Don't proxy requests to any urls
+    HTTPS_PROXY
+    https_proxy
+    HTTP_PROXY
+    http_proxy
 
-    $ export NO_PROXY='*'
+Note: A future major version of restify(-clients) might change this environment
+variable behaviour. See the discussion on [this issue](https://github.com/restify/node-restify/issues/878#issuecomment-249673285).
 
-Don't proxy requests to localhost
 
-    $ export NO_PROXY='127.0.0.1'
+The `noProxy` option can be used to exclude some hosts from using a given
+proxy. If it is not specified, then the `NO_PROXY` or `no_proxy` environment
+variable is used. Use `noProxy: ''` to override a possible environment variable,
+but not match any hosts.
 
-Don't proxy requests to localhost on port 8000
+The value is a string giving a comma-separated set of host, host-part suffix, or
+the special '*' to indicate all hosts. (Its definition is intended to match
+curl's `NO_PROXY` environment variable.) Some examples:
 
-    $ export NO_PROXY='localhost:8000'
 
-Don't proxy requests to multiple IPs
+    $ export NO_PROXY='*'               # don't proxy requests to any urls
+    $ export NO_PROXY='127.0.0.1'       # don't proxy requests the localhost IP
+    $ export NO_PROXY='localhost:8000'  # ... 'localhost' hostname and port 8000
+    $ export NO_PROXY='google.com'      # ... "google.com" and "*.google.com"
+    $ export NO_PROXY='www.google.com'  # ... "www.google.com"
+    $ export NO_PROXY='127.0.0.1, google.com'   # multiple hosts
 
-    $ export NO_PROXY='127.0.0.1, 8.8.8.8'
+**Note**: The url being requested must match the full hostname or hostname
+part to a '.': `NO_PROXY=oogle.com` does not match "google.com". DNS lookups are
+not performed to determine the IP address of a hostname.
 
-**Note**: The url being requested must match the full hostname in
-the proxy configuration or NO_PROXY environment variable. DNS
-lookups are not performed to determine the IP address of a hostname.
 
 #### basicAuth(username, password)
 

lib/HttpClient.js

@@ -316,10 +316,38 @@ function rawRequest(opts, cb) {
 } // end `rawRequest`
 
 
-//  Check if url is excluded by the no_proxy environment variable
-function isProxyForURL(address) {
-    var noProxy = process.env.NO_PROXY || process.env.no_proxy || null;
+function proxyOptsFromStr(str) {
+    if (!str) {
+        return (false);
+    }
+
+    var s = str;
+
+    // Normalize: host:port -> http://host:port
+    // FWIW `curl` supports using "http_proxy=host:port".
+    if (!/^[a-z0-9]+:\/\//.test(s)) {
+        s = 'http://' + s;
+    }
+    var parsed = url.parse(s);
+
+    var proxyOpts = {
+        protocol: parsed.protocol,
+        host: parsed.hostname
+    };
+
+    if (parsed.port) {
+        proxyOpts.port = Number(parsed.port);
+    }
 
+    if (parsed.auth) {
+        proxyOpts.proxyAuth = parsed.auth;
+    }
+
+    return (proxyOpts);
+}
+
+//  Check if url is excluded by the no_proxy environment variable
+function isProxyForURL(noProxy, address) {
     // wildcard
     if (noProxy === '*') {
         return (null);
@@ -367,6 +395,7 @@ function isProxyForURL(address) {
     }
     return (true);
 }
+
 ///--- API
 
 function HttpClient(options) {
@@ -377,6 +406,7 @@ function HttpClient(options) {
     assert.optionalString(options.socketPath, 'options.socketPath');
     assert.optionalString(options.url, 'options.url');
     assert.optionalBool(options.followRedirects, 'options.followRedirects');
+    assert.optionalString(options.noProxy, 'options.noProxy');
     assert.optionalNumber(options.maxRedirects, 'options.maxRedirects');
 
     EventEmitter.call(this);
@@ -414,18 +444,33 @@ function HttpClient(options) {
     this.socketPath = options.socketPath || false;
     this.url = options.url ? url.parse(options.url) : {};
 
-    if (process.env.https_proxy) {
-        this.proxy = url.parse(process.env.https_proxy);
-    } else if (process.env.http_proxy) {
-        this.proxy = url.parse(process.env.http_proxy);
+    // HTTP proxy: `options.proxy` wins, else `https_proxy`/`http_proxy` envvars
+    // (upper and lowercase) are used.
+    if (options.proxy === false) {
+        self.proxy = false;
     } else if (options.proxy) {
-        this.proxy = options.proxy;
+        if (typeof (options.proxy) === 'string') {
+            self.proxy = proxyOptsFromStr(options.proxy);
+        } else {
+            assert.object(options.proxy, 'options.proxy');
+            self.proxy = options.proxy;
+        }
     } else {
-        this.proxy = false;
+        // For backwards compat in restify 4.x and restify-clients 1.x, the
+        // `https_proxy` or `http_proxy` envvar will work for both HTTP and
+        // HTTPS. That behaviour may change in the next major version. See
+        // restify/node-restify#878 for details.
+        self.proxy = proxyOptsFromStr(process.env.https_proxy ||
+            process.env.HTTPS_PROXY ||
+            process.env.http_proxy ||
+            process.env.HTTP_PROXY);
     }
 
-    if (this.proxy && !isProxyForURL(self.url)) {
-        this.proxy = false;
+    var noProxy = (options.hasOwnProperty('noProxy') ? options.noProxy
+        : (process.env.NO_PROXY || process.env.no_proxy || null));
+
+    if (self.proxy && !isProxyForURL(noProxy, self.url)) {
+        self.proxy = false;
     }
 
     if (options.accept) {