From abb152bcff3b9f4c274ab1816ef246e575c8f2ea Mon Sep 17 00:00:00 2001 From: Maksim Zemskov Date: Sun, 1 Sep 2024 12:50:21 +0400 Subject: [PATCH 1/4] Fix auth reset logic during redirects to different origin when _base_url is set --- CHANGES/6764.feature.rst | 1 + aiohttp/client.py | 11 ++- docs/client_reference.rst | 7 +- tests/test_client_functional.py | 132 ++++++++++++++++++++++++++++++++ 4 files changed, 147 insertions(+), 4 deletions(-) create mode 100644 CHANGES/6764.feature.rst diff --git a/CHANGES/6764.feature.rst b/CHANGES/6764.feature.rst new file mode 100644 index 00000000000..5a6790add3d --- /dev/null +++ b/CHANGES/6764.feature.rst @@ -0,0 +1 @@ +Updated auth logic to clear authorization during redirects to a different origin when _base_url is defined -- by :user:`MaximZemskov`. diff --git a/aiohttp/client.py b/aiohttp/client.py index e3e9b29d0b8..c3fda0ff8a3 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -524,6 +524,7 @@ async def _request( with timer: # https://www.rfc-editor.org/rfc/rfc9112.html#name-retrying-requests retry_persistent_connection = method in IDEMPOTENT_METHODS + is_redirect_to_different_origin = False while True: url, auth_from_url = strip_auth_from_url(url) if not url.raw_host: @@ -543,7 +544,13 @@ async def _request( if auth is None: auth = auth_from_url - if auth is None: + + # If the session has a _base_url set and this is a redirect to a different origin + # we should not set the auth + is_auth_reset_needed = ( + is_redirect_to_different_origin and self._base_url is not None + ) + if auth is None and not is_auth_reset_needed: auth = self._default_auth # It would be confusing if we support explicit # Authorization header with auth argument @@ -716,12 +723,14 @@ async def _request( "Invalid redirect URL origin", ) from origin_val_err + is_redirect_to_different_origin = False if ( url.origin() != redirect_origin and not is_same_host_https_redirect ): auth = None headers.pop(hdrs.AUTHORIZATION, None) + is_redirect_to_different_origin = True url = parsed_redirect_url params = {} diff --git a/docs/client_reference.rst b/docs/client_reference.rst index 4117ab43e96..ba6739e13ac 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -100,9 +100,10 @@ The client session supports the context manager protocol for self closing. :param aiohttp.BasicAuth auth: an object that represents HTTP Basic Authorization (optional). It will be included - with any request to any origin and will not be - removed, event during redirect to a different - origin. + with any request. However, if a redirect to a + different origin occurs and the ``_base_url`` + parameter is set, the authorization will be + removed. :param version: supported HTTP version, ``HTTP 1.1`` by default. diff --git a/tests/test_client_functional.py b/tests/test_client_functional.py index d9b7f82abbf..2e36e03b9f1 100644 --- a/tests/test_client_functional.py +++ b/tests/test_client_functional.py @@ -2961,6 +2961,138 @@ async def close(self) -> None: assert resp.status == 200 +async def test_auth_persist_on_redirect_to_other_host_with_global_auth( + create_server_for_url_and_handler: Callable[[URL, Handler], Awaitable[TestServer]], +) -> None: + url_from = URL("http://host1.com/path1") + url_to = URL("http://host2.com/path2") + + async def srv_from(request: web.Request) -> NoReturn: + assert request.host == url_from.host + assert request.headers["Authorization"] == "Basic dXNlcjpwYXNz" + raise web.HTTPFound(url_to) + + async def srv_to(request: web.Request) -> web.Response: + assert request.host == url_to.host + assert "Authorization" in request.headers, "Header was dropped" + return web.Response() + + server_from = await create_server_for_url_and_handler(url_from, srv_from) + server_to = await create_server_for_url_and_handler(url_to, srv_to) + + assert ( + url_from.host != url_to.host or server_from.scheme != server_to.scheme + ), "Invalid test case, host or scheme must differ" + + protocol_port_map = { + "http": 80, + "https": 443, + } + etc_hosts = { + (url_from.host, protocol_port_map[server_from.scheme]): server_from, + (url_to.host, protocol_port_map[server_to.scheme]): server_to, + } + + class FakeResolver(AbstractResolver): + async def resolve( + self, + host: str, + port: int = 0, + family: socket.AddressFamily = socket.AF_INET, + ) -> List[ResolveResult]: + server = etc_hosts[(host, port)] + assert server.port is not None + + return [ + { + "hostname": host, + "host": server.host, + "port": server.port, + "family": socket.AF_INET, + "proto": 0, + "flags": socket.AI_NUMERICHOST, + } + ] + + async def close(self) -> None: + """Dummy""" + + connector = aiohttp.TCPConnector(resolver=FakeResolver(), ssl=False) + + async with aiohttp.ClientSession( + connector=connector, auth=aiohttp.BasicAuth("user", "pass") + ) as client: + resp = await client.get(url_from) + assert resp.status == 200 + + +async def test_drop_auth_on_redirect_to_other_host_with_global_auth_and_base_url( + create_server_for_url_and_handler: Callable[[URL, Handler], Awaitable[TestServer]], +) -> None: + url_from = URL("http://host1.com/path1") + url_to = URL("http://host2.com/path2") + + async def srv_from(request: web.Request) -> NoReturn: + assert request.host == url_from.host + assert request.headers["Authorization"] == "Basic dXNlcjpwYXNz" + raise web.HTTPFound(url_to) + + async def srv_to(request: web.Request) -> web.Response: + assert request.host == url_to.host + assert "Authorization" not in request.headers, "Header was not dropped" + return web.Response() + + server_from = await create_server_for_url_and_handler(url_from, srv_from) + server_to = await create_server_for_url_and_handler(url_to, srv_to) + + assert ( + url_from.host != url_to.host or server_from.scheme != server_to.scheme + ), "Invalid test case, host or scheme must differ" + + protocol_port_map = { + "http": 80, + "https": 443, + } + etc_hosts = { + (url_from.host, protocol_port_map[server_from.scheme]): server_from, + (url_to.host, protocol_port_map[server_to.scheme]): server_to, + } + + class FakeResolver(AbstractResolver): + async def resolve( + self, + host: str, + port: int = 0, + family: socket.AddressFamily = socket.AF_INET, + ) -> List[ResolveResult]: + server = etc_hosts[(host, port)] + assert server.port is not None + + return [ + { + "hostname": host, + "host": server.host, + "port": server.port, + "family": socket.AF_INET, + "proto": 0, + "flags": socket.AI_NUMERICHOST, + } + ] + + async def close(self) -> None: + """Dummy""" + + connector = aiohttp.TCPConnector(resolver=FakeResolver(), ssl=False) + + async with aiohttp.ClientSession( + connector=connector, + base_url="http://host1.com", + auth=aiohttp.BasicAuth("user", "pass"), + ) as client: + resp = await client.get("/path1") + assert resp.status == 200 + + async def test_async_with_session() -> None: async with aiohttp.ClientSession() as session: pass From 9d28ad620d2ccc9eec1b247aac9cab86c31e8fd9 Mon Sep 17 00:00:00 2001 From: Maksim Zemskov Date: Sun, 1 Sep 2024 13:03:11 +0400 Subject: [PATCH 2/4] Rename 6764.feature.rst to 8966.feature.rst --- CHANGES/{6764.feature.rst => 8966.feature.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename CHANGES/{6764.feature.rst => 8966.feature.rst} (100%) diff --git a/CHANGES/6764.feature.rst b/CHANGES/8966.feature.rst similarity index 100% rename from CHANGES/6764.feature.rst rename to CHANGES/8966.feature.rst From e75c33297994870c6840632e31aa0d57c7976200 Mon Sep 17 00:00:00 2001 From: Maksim Zemskov Date: Sun, 1 Sep 2024 17:33:31 +0400 Subject: [PATCH 3/4] Set default auth only if request url origin match base_url if base_url set --- CHANGES/8966.feature.rst | 2 +- aiohttp/client.py | 13 ++++--------- docs/client_reference.rst | 9 +++++---- 3 files changed, 10 insertions(+), 14 deletions(-) diff --git a/CHANGES/8966.feature.rst b/CHANGES/8966.feature.rst index 5a6790add3d..ab1dc45b60e 100644 --- a/CHANGES/8966.feature.rst +++ b/CHANGES/8966.feature.rst @@ -1 +1 @@ -Updated auth logic to clear authorization during redirects to a different origin when _base_url is defined -- by :user:`MaximZemskov`. +Updated ClientSession's auth logic to include default auth only if the request URL's origin matches _base_url; otherwise, the auth will not be included -- by :user:`MaximZemskov` diff --git a/aiohttp/client.py b/aiohttp/client.py index c3fda0ff8a3..405964364c4 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -524,7 +524,6 @@ async def _request( with timer: # https://www.rfc-editor.org/rfc/rfc9112.html#name-retrying-requests retry_persistent_connection = method in IDEMPOTENT_METHODS - is_redirect_to_different_origin = False while True: url, auth_from_url = strip_auth_from_url(url) if not url.raw_host: @@ -545,13 +544,11 @@ async def _request( if auth is None: auth = auth_from_url - # If the session has a _base_url set and this is a redirect to a different origin - # we should not set the auth - is_auth_reset_needed = ( - is_redirect_to_different_origin and self._base_url is not None - ) - if auth is None and not is_auth_reset_needed: + if auth is None and ( + not self._base_url or self._base_url.origin() == url.origin() + ): auth = self._default_auth + # It would be confusing if we support explicit # Authorization header with auth argument if auth is not None and hdrs.AUTHORIZATION in headers: @@ -723,14 +720,12 @@ async def _request( "Invalid redirect URL origin", ) from origin_val_err - is_redirect_to_different_origin = False if ( url.origin() != redirect_origin and not is_same_host_https_redirect ): auth = None headers.pop(hdrs.AUTHORIZATION, None) - is_redirect_to_different_origin = True url = parsed_redirect_url params = {} diff --git a/docs/client_reference.rst b/docs/client_reference.rst index ba6739e13ac..b0bbe769ec6 100644 --- a/docs/client_reference.rst +++ b/docs/client_reference.rst @@ -100,10 +100,11 @@ The client session supports the context manager protocol for self closing. :param aiohttp.BasicAuth auth: an object that represents HTTP Basic Authorization (optional). It will be included - with any request. However, if a redirect to a - different origin occurs and the ``_base_url`` - parameter is set, the authorization will be - removed. + with any request. However, if the + ``_base_url`` parameter is set, the request + URL's origin must match the base URL's origin; + otherwise, the default auth will not be + included. :param version: supported HTTP version, ``HTTP 1.1`` by default. From f854a30406421f043c3b8e06bf22a86b1adc7c6b Mon Sep 17 00:00:00 2001 From: Maksim Zemskov Date: Sun, 1 Sep 2024 17:34:40 +0400 Subject: [PATCH 4/4] Remove redundant empty line --- aiohttp/client.py | 1 - 1 file changed, 1 deletion(-) diff --git a/aiohttp/client.py b/aiohttp/client.py index 405964364c4..4e2faa1a3b6 100644 --- a/aiohttp/client.py +++ b/aiohttp/client.py @@ -548,7 +548,6 @@ async def _request( not self._base_url or self._base_url.origin() == url.origin() ): auth = self._default_auth - # It would be confusing if we support explicit # Authorization header with auth argument if auth is not None and hdrs.AUTHORIZATION in headers: