Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor rebuild_proxies to separate resolution and auth handling #5924

Merged
merged 1 commit into from Nov 24, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
24 changes: 7 additions & 17 deletions requests/sessions.py
Expand Up @@ -29,7 +29,7 @@

from .utils import (
requote_uri, get_environ_proxies, get_netrc_auth, should_bypass_proxies,
get_auth_from_url, rewind_body
get_auth_from_url, rewind_body, resolve_proxies
)

from .status_codes import codes
Expand Down Expand Up @@ -269,7 +269,6 @@ def rebuild_auth(self, prepared_request, response):
if new_auth is not None:
prepared_request.prepare_auth(new_auth)


def rebuild_proxies(self, prepared_request, proxies):
"""This method re-evaluates the proxy configuration by considering the
environment variables. If we are redirected to a URL covered by
Expand All @@ -282,21 +281,9 @@ def rebuild_proxies(self, prepared_request, proxies):

:rtype: dict
"""
proxies = proxies if proxies is not None else {}
headers = prepared_request.headers
url = prepared_request.url
scheme = urlparse(url).scheme
new_proxies = proxies.copy()
no_proxy = proxies.get('no_proxy')

bypass_proxy = should_bypass_proxies(url, no_proxy=no_proxy)
if self.trust_env and not bypass_proxy:
environ_proxies = get_environ_proxies(url, no_proxy=no_proxy)

proxy = environ_proxies.get(scheme, environ_proxies.get('all'))

if proxy:
new_proxies.setdefault(scheme, proxy)
scheme = urlparse(prepared_request.url).scheme
new_proxies = resolve_proxies(prepared_request, proxies, self.trust_env)

if 'Proxy-Authorization' in headers:
del headers['Proxy-Authorization']
Expand Down Expand Up @@ -633,7 +620,10 @@ def send(self, request, **kwargs):
kwargs.setdefault('stream', self.stream)
kwargs.setdefault('verify', self.verify)
kwargs.setdefault('cert', self.cert)
kwargs.setdefault('proxies', self.rebuild_proxies(request, self.proxies))
if 'proxies' not in kwargs:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change should actually fix the performance regression in most cases (Session.request - which I assume is the most common flow - always sets proxies on kwargs).

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, I think proxies will be the escape hatch when performance is a concern here :)

kwargs['proxies'] = resolve_proxies(
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So now we're:

  1. No longer stripping the proxy-auth header. Sounds right to me.
  2. No longer setting the proxy-auth header at all. Is this the desired behavior? What if someone uses a proxy and passes the username + password in the url? (though I'm not sure if that flow is supported or not)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For 2.) we were never doing that before this change and I don't think that was an intended byproduct. We should still support our normal proxy auth flows that were available prior to #5681.

request, self.proxies, self.trust_env
)

# It's possible that users might accidentally send a Request object.
# Guard against that specific failure case.
Expand Down
28 changes: 28 additions & 0 deletions requests/utils.py
Expand Up @@ -830,6 +830,34 @@ def select_proxy(url, proxies):
return proxy


def resolve_proxies(request, proxies, trust_env=True):
"""This method takes proxy information from a request and configuration
input to resolve a mapping of target proxies. This will consider settings
such a NO_PROXY to strip proxy configurations.

:param request: Request or PreparedRequest
:param proxies: A dictionary of schemes or schemes and hosts to proxy URLs
:param trust_env: Boolean declaring whether to trust environment configs

:rtype: dict
"""
proxies = proxies if proxies is not None else {}
url = request.url
scheme = urlparse(url).scheme
no_proxy = proxies.get('no_proxy')
new_proxies = proxies.copy()

bypass_proxy = should_bypass_proxies(url, no_proxy=no_proxy)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Didn't we want to unify 850 and 851 so we don't call should_bypass_proxies if trust_env is False?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do, I was going to have @dbaxa rebase their change onto whatever we merge so they can still have a commit in the repo.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me.

if trust_env and not bypass_proxy:
environ_proxies = get_environ_proxies(url, no_proxy=no_proxy)

proxy = environ_proxies.get(scheme, environ_proxies.get('all'))

if proxy:
new_proxies.setdefault(scheme, proxy)
return new_proxies


def default_user_agent(name="python-requests"):
"""
Return a string representing the default user agent.
Expand Down
11 changes: 10 additions & 1 deletion tests/test_requests.py
Expand Up @@ -590,6 +590,15 @@ def test_respect_proxy_env_on_request(self, httpbin):
session = requests.Session()
session.request(method='GET', url=httpbin())

def test_proxy_authorization_preserved_on_request(self, httpbin):
proxy_auth_value = "Bearer XXX"
session = requests.Session()
session.headers.update({"Proxy-Authorization": proxy_auth_value})
resp = session.request(method='GET', url=httpbin('get'))
sent_headers = resp.json().get('headers', {})

assert sent_headers.get("Proxy-Authorization") == proxy_auth_value

def test_basicauth_with_netrc(self, httpbin):
auth = ('user', 'pass')
wrong_auth = ('wronguser', 'wrongpass')
Expand Down Expand Up @@ -2575,4 +2584,4 @@ def test_post_json_nan(self, httpbin):
def test_json_decode_compatibility(self, httpbin):
r = requests.get(httpbin('bytes/20'))
with pytest.raises(requests.exceptions.JSONDecodeError):
r.json()
r.json()