Re-order proxy precedence.

[ouroborus]

Session.trust_env = False turns off the checking of environment variables for options including proxy settings (*_proxy). But urllib picks up and uses these environment proxy settings anyway. requests should pass the trust_env setting on to urllib. (Although I'm not sure if urllib has a similar override.)

(Proxy setting precedence should be sorted out here as well. They way it is now, environment proxy settings will interfere with (rather than be over-ridden by) the proxies argument in Session.request or requests.request calls and the Session.proxies config regardless of trust_env settings.)

[Lukasa]

I've taken a quick look at the code in urllib3 and I can't find any reference to the *_PROXY environment variables there. Can you point to where in the code urllib3 grabs that information?

As for proxy setting precedence, what do you mean by 'interfere with'? We don't override any settings already in the proxy dictionary as we use dict.setdefault to set those values. All that happens is you may get an additional proxy value for a scheme you didn't declare. Is this what you meant by 'interfere with'?

[ouroborus]

I'll have to write up a sample that triggers this.

So far I've got a situation where the env proxy is defined, session.proxy is defined (with a different proxy), and session.trust_env = False. The results are the initial request goes through the session-defined proxy and the redirect goes through the env-defined proxy.

[ouroborus]

It seems redirects pass through SessionRedirectMixin.resolve_redirects in sessions.py which has a line proxies = self.rebuild_proxies(prepared_request, proxies). rebuild_proxies appears to ignore the passed in proxies argument and session.trust_env settings.

[Lukasa]

It does ignore trust_env and the original proxies dict (both bugs), but you're not running that code unless you're using the requests release from GitHub. =) Fixing both of those bugs, however, should resolve the problem. I'll fix it up later today.

[ouroborus]

Ah, somehow I though the installation docs were saying to use the github source rather than pip or easy_install. I see now that it says instead to use pip instead of easy_install and then goes on to say where you can get the source.

[ouroborus]

Regarding proxy setting precedence, I think Session.request(..., proxies) should override Session.proxies which should override proxies set in the environment. Currently, environment proxies override session proxies (using Session.trust_env = True).

In pseudo-code, it'd be something like:

trust_env = request.trust_env
if trust_env == None:
    trust_env = Session.trust_env
if trust_env == None:
    trust_env = True
proxies = {}
if trust_env:
    proxies = env.proxies
proxies = proxies.update(Session.proxies).update(request.proxies)

[Lukasa]

To be clear for those who aren't sure, the way @ouroborus' suggestion differs from the current logic is that we take the proxies from the request, then apply proxies from the environment, then finally apply proxies from the Session.

I'm open to re-ordering the precedence of the priorities. @sigmavirus24, thoughts?

[sigmavirus24]

Can anyone recall the reasoning behind the current order of precedence? It is extremely odd to me that the order of precedence is contrary to the rest of the library.

[Lukasa]

No, and IIRC (I'm on my phone) it's been that way for a while. Looked like an oversight to me.

[sigmavirus24]

Then consider me in favor of the reordering.

Sent from my Android device with K-9 Mail. Please excuse my brevity.

[ouroborus]

It's been noted that this issue is poorly named. I'm not sure what to rename it or even if it should be now that it has been created. @Lukasa, feel free to rename it if and as you see fit.

[sigmavirus24]

@Lukasa wasn't this already fixed?

[Lukasa]

@sigmavirus24 Not that I can see. =)

[commonism]

This issue causes real headache when using saltstack with pip states and https_proxy set in the environment.
While saltstacks pip state allows passing a proxy, this bug ignores the proxy given and prefers the environment proxy instead, as a result the wrong proxy gets used.

A workaround is overwriting your environment variable with saltstacks env_var:

PackageX:
  pip.installed:
    - proxy: http://proxyA
    - env_vars:
      https_proxy: "http://proxyA
      http_proxy: "http://proxyA

But it is rather messy, therefore I'd appreciate if this bug would be fixed.

[sigmavirus24]

But it is rather messy, therefore I'd appreciate if this bug would be fixed.

It will be. That's why it's labeled as "Planned". Also note that we've set a milestone for it. Thanks for your interest.

[dmich2]

To quote @nateprewitt from Aug 11, 2016:

It may be worth noting that it's PR #2839 that fixes this.

Originally posted by @nateprewitt in #3506 (comment)

[fireattack]

While investigating this old proxy precedence bug/issue, I came across #6153. That bug is about a behavioral difference when passing None versus "" as HTTP/HTTPS proxies at the request level, but it only appears for redirected requests.

The bug was dismissed as expected behavior. As @nateprewitt noted, "None has a semantic of 'do not use this key' in the library, hence the difference in behavior." This explanation is valid; however, if this principle is strictly adhered to, it doesn't account for why None behaves as "disable proxy" fine for normal, non-redirected requests. There is definitely inconsistent behavior at play.

To summarize,
Expected behavior:

• If {"http": None} cannot be used to disable a proxy for HTTP (because of how we treat None in general) it should not work for any requests, including non-redirected ones.
• If {"http": None} can disable a proxy for HTTP, it should work consistently, even for redirected requests.

Observed behavior:
{"http": None} works for the original request but not for redirected ones.

I bring this up here because the behavioral discrepancy between original and redirected requests is strikingly similar to the issue described in #2018 (comment) here back in 2014, which was promptly fixed in #2021. So I'm thinking a similar fix should be made too, when we're addressing precedence issues.