requests icon indicating copy to clipboard operation
requests copied to clipboard

Re-order proxy precedence.

Open ouroborus opened this issue 11 years ago • 17 comments
trafficstars

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.)

ouroborus avatar Apr 25 '14 03:04 ouroborus

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'?

Lukasa avatar Apr 25 '14 08:04 Lukasa

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 avatar Apr 25 '14 23:04 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.

ouroborus avatar Apr 26 '14 10:04 ouroborus

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.

Lukasa avatar Apr 26 '14 10:04 Lukasa

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 avatar Apr 26 '14 10:04 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)

ouroborus avatar Apr 26 '14 22:04 ouroborus

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?

Lukasa avatar Apr 26 '14 22:04 Lukasa

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.

sigmavirus24 avatar Apr 29 '14 01:04 sigmavirus24

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

Lukasa avatar Apr 29 '14 09:04 Lukasa

Then consider me in favor of the reordering.

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

sigmavirus24 avatar Apr 29 '14 12:04 sigmavirus24

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.

ouroborus avatar May 04 '14 17:05 ouroborus

@Lukasa wasn't this already fixed?

sigmavirus24 avatar Aug 13 '14 03:08 sigmavirus24

@sigmavirus24 Not that I can see. =)

Lukasa avatar Aug 13 '14 06:08 Lukasa

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.

commonism avatar Aug 24 '15 14:08 commonism

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.

sigmavirus24 avatar Aug 24 '15 14:08 sigmavirus24

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 https://github.com/psf/requests/issues/3506#issuecomment-239304816

dmich2 avatar Feb 17 '22 19:02 dmich2