Replace lib/getProxyFromURI.js with proxy-from-env #2560

Rob--W · 2017-02-23T00:24:42Z

PR Checklist:

I have run npm test locally and all tests are passing.
I have added/updated tests for any new behavior.
If this is a significant change, an issue has already been created where the problem / solution was discussed: N/A

PR Description

This is identical to PR #2088 (see the detailed description over there), except rebased on master. Somehow the previous PR cannot be re-opened, so here is a new PR.

/cc @FredKSchott

FredKSchott · 2017-02-23T01:17:12Z

tests/test-proxy.js

@@ -138,13 +138,13 @@ function addTests() {
      url    : 'https://google.com',
      tunnel : false,
      pool   : false
-    }, true)
+    }, false)


why are we no longer using a proxy for these requests? Shouldn't this PR require no change to testing?

Because http_proxy is only supposed to affect http-requests, not https. Use all_proxy to get the original behavior.

FredKSchott · 2017-02-23T01:40:45Z

Please update the README to document the new support for ALL_PROXY (and maybe link to https://github.com/Rob--W/proxy-from-env#environment-variables for more information.)

We're trying to prune down our number of dependencies, but since this has no dependencies of its own, seems more reliable and better tested than our implementation, and reduces the amount of code we need to maintain... this seems like a reasonable improvement.

proxy-from-env: https://www.npmjs.com/package/proxy-from-env Differences: - `HTTP_PROXY` is no longer used for https requests, use `HTTPS_PROXY` or `ALL_PROXY` instead (this is like curl, wget, Python, ...) - The check whether `NO_PROXY` matches now takes the default port into account. E.g. with `NO_PROXY=example.com:443`, `https://example.com` will now be a direct request instead of a proxied request. - Proxy host matching is no longer a suffix match. E.g. given `http://www.example.com/`: * `NO_PROXY=example.com` does not match any more. * `NO_PROXY=notexample.com` does not match any more. * `NO_PROXY=.example.com` did and does match. * `NO_PROXY=*example.com` now matches `noexample.com` * `NO_PROXY=*.example.com` now matches `www.example.com` - IPv6 addresses in `NO_PROXY` are now supported.

Rob--W · 2017-02-24T10:46:39Z

I have edited the version in the first commit (bump to proxy-from-env@1.0.0) and added a separate commit for the README change. Let me know if I should squash the commits.

The change may technically be a breaking change, because previously http_proxy was also used for https_proxy (and now it is not any more). request's README did not mention this behavior (and the de-facto standard is to not use http_proxy as a fallback to https_proxy), so hopefully nobody relied on it.

Note that I did not introduce new unit tests with this PR because the proxy-from-env package already has an extensive sets of unit tests.

FredKSchott · 2017-02-24T19:19:43Z

README.md

 * `NO_PROXY` / `no_proxy`

 When `HTTP_PROXY` / `http_proxy` are set, they will be used to proxy non-SSL requests that do not have an explicit `proxy` configuration option present. Similarly, `HTTPS_PROXY` / `https_proxy` will be respected for SSL requests that do not have an explicit `proxy` configuration option. It is valid to define a proxy in one of the environment variables, but then override it for a specific request, using the `proxy` configuration option. Furthermore, the `proxy` configuration option can be explicitly set to false / null to opt out of proxying altogether for that request.
+If neither `http_proxy` nor `https_proxy` is set, then `all_proxy` is used if available.


Describe what happens if none of the three are set/available

Do you really see value in stating the obvious that if no proxy config is set, that the requests are not proxied?

FredKSchott · 2017-02-24T19:20:59Z

One small comment, but otherwise lgtm.

Thanks for reopening and working with us to get this merged.

FredKSchott · 2017-02-24T19:54:18Z

Can you provide some links/docs on "and the de-facto standard is to not use http_proxy as a fallback to https_proxy"? I'm seeing a lot of people who are doing what request is currently doing so I'm a little skeptical: https://github.com/search?utf8=%E2%9C%93&q=process.env.HTTP_PROXY+https&type=Code&ref=searchresults

The breaking change is my one concern with merging this in. While it wasn't documented, it was fairly easy to rely on and the amount that this would break for those users is significant. With ~1M downloads a day that's something we need to consider.

Would you support an option in proxy-from-env to use the HTTP fallback if HTTPS is not available? If we could use that I would feel comfortable merging.

Rob--W · 2017-02-25T22:57:57Z

Can you provide some links/docs on "and the de-facto standard is to not use http_proxy as a fallback to https_proxy"?

See https://github.com/Rob--W/proxy-from-env#external-resources. I consider curl and wget as canonical reference implementations.

The breaking change is my one concern with merging this in. While it wasn't documented, it was fairly easy to rely on and the amount that this would break for those users is significant. With ~1M downloads a day that's something we need to consider.

Would you support an option in proxy-from-env to use the HTTP fallback if HTTPS is not available? If we could use that I would feel comfortable merging.

How about this:

diff --git a/request.js b/request.js
index dca5427..a6c2f2f 100644
--- a/request.js
+++ b/request.js
@@ -277,6 +277,10 @@ Request.prototype.init = function (options) {
 
   if (!self.hasOwnProperty('proxy')) {
     self.proxy = getProxyForUrl(self.uri) || null
+    if (!self.proxy && self.uri.protocol === 'https:') {
+      // Fall back to http_proxy when https_proxy is not set, for backwards-compatibility.
+      self.proxy = getProxyForUrl(self.uri.href.replace('s', '')) || null
+    }
   }
 
   self.tunnel = self._tunnel.isEnabled()
diff --git a/tests/test-proxy.js b/tests/test-proxy.js
index fe93cde..dd5cefb 100644
--- a/tests/test-proxy.js
+++ b/tests/test-proxy.js
@@ -138,13 +138,13 @@ function addTests() {
       url    : 'https://google.com',
       tunnel : false,
       pool   : false
-    }, false)
+    }, true)
 
     runTest('http_proxy environment variable and https: url', {
       env    : { http_proxy : s.url },
       url    : 'https://google.com',
       tunnel : false
-    }, false)
+    }, true)
 
     runTest('HTTPS_PROXY environment variable and https: url', {
       env    : { HTTPS_PROXY : s.url },

Rob--W · 2017-02-25T23:10:50Z

I understand the desire for backwards-compatibility, but do note that automatically using http_proxy as a fallback implies that it is not possible to only proxy http requests and allow https requests to go through with a direct request.

A HTTP proxy is not necessarily capable of proxying HTTPS. The former looks similar to a regular HTTP request (the request is visible in plain text to the proxy server), whereas the latter requires a tunnel (usually with a CONNECT request) (the original request is encrypted and can only be decrypted by the operator of the website who shared the certificate).

Are you okay with showing a deprecation warning when the fallback is used?

FredKSchott · 2017-03-16T23:32:51Z

@Rob--W sorry for the delay, the last two weeks have been too busy and I hadn't been able to find time to respond.

I completely agree that what you are proposing is "more correct" than request's current behavior (thanks for the curl/wget examples to confirm). But regardless, our current behavior is out there, and lots of people are now relying on it. To break those user's libraries/applications and tell them it was their fault would be doing a disservice to them.

I am a -1 on merging anything that causes a potentially serious breaking change to request. If your library won't be able to support the fallback, I would suggest we table this until it's time to for a v3.0 release.

stale · 2018-11-23T02:29:47Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

reconbot · 2018-11-23T03:18:28Z

@FredKSchott I think it's time for a 3.0 release, what do you think?

Rob--W · 2018-11-23T10:13:50Z

I'll update the PR (to resolve merge conflicts) once this is approved.

reconbot · 2019-04-01T13:42:38Z

Request is now in maintenance mode and wont be merging any new features. Please see #3142 for more information.

FredKSchott reviewed Feb 23, 2017

View reviewed changes

Rob--W added 2 commits February 24, 2017 11:27

Mention ALL_PROXY and proxy-from-env in README

82bd184

Rob--W force-pushed the use-proxy-from-env branch from abb7452 to 82bd184 Compare February 24, 2017 10:41

FredKSchott reviewed Feb 24, 2017

View reviewed changes

stale bot added the stale label Nov 23, 2018

stale bot removed the stale label Nov 23, 2018

reconbot closed this Apr 1, 2019

honzajavorek mentioned this pull request Apr 2, 2019

Refactor logging of proxy settings apiaryio/dredd#1302

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace lib/getProxyFromURI.js with proxy-from-env #2560

Replace lib/getProxyFromURI.js with proxy-from-env #2560

Rob--W commented Feb 23, 2017

FredKSchott Feb 23, 2017 •

edited

Rob--W Feb 24, 2017

FredKSchott Feb 24, 2017

FredKSchott commented Feb 23, 2017

Rob--W commented Feb 24, 2017

FredKSchott Feb 24, 2017

Rob--W Feb 25, 2017

FredKSchott commented Feb 24, 2017

FredKSchott commented Feb 24, 2017

Rob--W commented Feb 25, 2017

Rob--W commented Feb 25, 2017

FredKSchott commented Mar 16, 2017

stale bot commented Nov 23, 2018

reconbot commented Nov 23, 2018

Rob--W commented Nov 23, 2018

reconbot commented Apr 1, 2019

Replace lib/getProxyFromURI.js with proxy-from-env #2560

Replace lib/getProxyFromURI.js with proxy-from-env #2560

Conversation

Rob--W commented Feb 23, 2017

PR Checklist:

PR Description

FredKSchott Feb 23, 2017 • edited

Choose a reason for hiding this comment

Rob--W Feb 24, 2017

Choose a reason for hiding this comment

FredKSchott Feb 24, 2017

Choose a reason for hiding this comment

FredKSchott commented Feb 23, 2017

Rob--W commented Feb 24, 2017

FredKSchott Feb 24, 2017

Choose a reason for hiding this comment

Rob--W Feb 25, 2017

Choose a reason for hiding this comment

FredKSchott commented Feb 24, 2017

FredKSchott commented Feb 24, 2017

Rob--W commented Feb 25, 2017

Rob--W commented Feb 25, 2017

FredKSchott commented Mar 16, 2017

stale bot commented Nov 23, 2018

reconbot commented Nov 23, 2018

Rob--W commented Nov 23, 2018

reconbot commented Apr 1, 2019

FredKSchott Feb 23, 2017 •

edited