Skip to content

Commit 44bbdb8

Browse files
authored
Resolve same-scheme redirects without an authority against the URL (aio-libs#13855)
<!-- Thank you for your contribution! --> ## What do these changes do? A redirect `Location` that has the scheme of the current URL but no `//`, such as `http:/path` or `http:path`, is now joined with the current URL, as browsers and the WHATWG URL Standard resolve it. Previously only a `Location` without a scheme was joined, so `http:/example.com` was taken as an absolute URL without a host and failed with `InvalidUrlRedirectClientError`. This is needed for aio-libs/yarl#1937, where yarl reads `http:example.com/` as `http://example.com/` in its default WHATWG mode. Without the join, aiohttp would follow `Location: http:/example.com` to the host `example.com`, while a browser stays on the current host. `Location: http:///example.com` is left as it was (still rejected): browsers read it as the host `example.com`, which `URL.join()` does not do. The check for `//` normalizes the Location as URL parsers do (surrounding C0 controls and spaces stripped, tabs and newlines dropped) and reads a backslash like a slash, as browsers do for http and https. `http:/example.com` is removed from the invalid URL test data: a request to it is invalid with the released yarl but goes to `example.com` with yarl#1937. As a redirect it now resolves on both. ## Are there changes in behavior for the user? Yes. `Location: http:/path`, `http:path` and `http:/` now redirect to that path on the current host instead of raising `InvalidUrlRedirectClientError`. ## Is it a substantial burden for the maintainers to support this? No, it is one condition in the redirect handling. ## Related issue number Needed by aio-libs/yarl#1937, whose "Aiohttp tests" job fails on `http:/example.com` without it. ## Checklist - [x] I think the code is well written - [x] Unit tests for the changes exist - [ ] Documentation reflects the changes: N/A, no documented behavior changes - [ ] If you provide code modification, please add yourself to `CONTRIBUTORS.txt`: N/A, already listed - [x] Add a new news fragment into the `CHANGES/` folder Drafted with Claude Code (Claude Opus 5.5); reviewed by @asvetlov. <details> <summary>Agent run details (optional, for reviewers)</summary> Tests: `AIOHTTP_NO_EXTENSIONS=1 pytest tests -n auto`, 4796 passed, 60 skipped, 14 xfailed, both with yarl master (f761c37) and with the yarl#1937 branch. Lint: black, isort, codespell passed; flake8 (E, F, W) passed when run directly, because the local pre-commit flake8 hook fails to load `flake8-requirements` (`pkg_resources` missing); mypy on `aiohttp/client.py` reports the same unrelated errors as on master. </details>
1 parent 31b6ebe commit 44bbdb8

4 files changed

Lines changed: 88 additions & 4 deletions

File tree

‎CHANGES/13855.bugfix.rst‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
Resolved a redirect ``Location`` with the scheme of the current URL but
2+
without ``//``, such as ``http:/path`` or ``http:path``, against the
3+
current URL, as browsers do, instead of treating it as an absolute URL
4+
without a host
5+
-- by :user:`asvetlov`.

‎aiohttp/client.py‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -168,6 +168,22 @@
168168
from typing import Unpack
169169

170170

171+
# URL parsers strip leading and trailing C0 control characters and spaces and
172+
# drop tabs and newlines before splitting a URL.
173+
_C0_CONTROL_OR_SPACE = "".join(map(chr, range(0x21)))
174+
_REMOVE_TAB_OR_NEWLINE = str.maketrans("", "", "\t\n\r")
175+
176+
177+
def _has_no_authority(location: str, scheme: str) -> bool:
178+
"""Tell if a URL with a scheme has no "//" after the scheme.
179+
180+
Browsers read a backslash like a slash there for http and https.
181+
"""
182+
location = location.strip(_C0_CONTROL_OR_SPACE).translate(_REMOVE_TAB_OR_NEWLINE)
183+
rest = location[len(scheme) + 1 : len(scheme) + 3]
184+
return rest[:1] not in ("/", "\\") or rest[1:] not in ("/", "\\")
185+
186+
171187
class _RequestOptions(TypedDict, total=False):
172188
params: Query
173189
data: Any
@@ -834,7 +850,11 @@ async def _request(
834850
await req._body.close()
835851
resp.close()
836852
raise NonHttpUrlRedirectClientError(r_url)
837-
elif not scheme:
853+
elif not scheme or (
854+
scheme == url.scheme and _has_no_authority(r_url, scheme)
855+
):
856+
# "http:/path" or "http:path" is a reference to
857+
# the current URL, as browsers resolve it.
838858
parsed_redirect_url = url.join(parsed_redirect_url)
839859

840860
try:

‎tests/test_client_functional.py‎

Lines changed: 40 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1006,6 +1006,38 @@ async def handler_ok(request: web.Request) -> web.Response:
10061006
assert resp.url.path == "/ok"
10071007

10081008

1009+
@pytest.mark.parametrize(
1010+
("location", "path", "query"),
1011+
(
1012+
("http:/ok", "/ok", {}),
1013+
("http:ok", "/ok", {}),
1014+
("http:/ok?a=b", "/ok", {"a": "b"}),
1015+
("http:/", "/", {}),
1016+
),
1017+
)
1018+
async def test_redirect_same_scheme_without_authority(
1019+
aiohttp_client: AiohttpClient, location: str, path: str, query: dict[str, str]
1020+
) -> None:
1021+
async def handler_redirect(request: web.Request) -> web.Response:
1022+
return web.Response(status=301, headers={"Location": location})
1023+
1024+
async def handler_ok(request: web.Request) -> web.Response:
1025+
assert dict(request.query) == query
1026+
return web.Response(status=200)
1027+
1028+
app = web.Application()
1029+
app.router.add_route("GET", path, handler_ok)
1030+
app.router.add_route("GET", "/redirect", handler_redirect)
1031+
client = await aiohttp_client(app)
1032+
1033+
async with client.get("/redirect") as resp:
1034+
assert resp.status == 200
1035+
assert resp.url.host == "127.0.0.1"
1036+
assert resp.url.path == path
1037+
assert dict(resp.url.query) == query
1038+
assert len(resp.history) == 1
1039+
1040+
10091041
async def test_history(aiohttp_client: AiohttpClient) -> None:
10101042
async def handler_redirect(request: web.Request) -> web.Response:
10111043
return web.Response(status=301, headers={"Location": "/ok"})
@@ -3213,7 +3245,12 @@ async def handler_redirect(request: web.Request) -> web.Response:
32133245
INVALID_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN = (
32143246
# # yarl.URL.origin raises ValueError
32153247
("http:/", "http:///"),
3216-
("http:/example.com", "http:///example.com"),
3248+
("http:///example.com", "http:///example.com"),
3249+
)
3250+
3251+
# A redirect to "http:/" resolves against the current URL, see
3252+
# test_redirect_same_scheme_without_authority().
3253+
INVALID_REDIRECT_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN = (
32173254
("http:///example.com", "http:///example.com"),
32183255
)
32193256

@@ -3259,7 +3296,7 @@ async def test_invalid_and_non_http_url(
32593296
(
32603297
*(
32613298
(url, message, InvalidUrlRedirectClientError)
3262-
for (url, message) in INVALID_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN
3299+
for (url, message) in INVALID_REDIRECT_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN
32633300
+ INVALID_URL_WITH_ERROR_MESSAGE_YARL_NEW
32643301
),
32653302
*(
@@ -3294,7 +3331,7 @@ async def generate_redirecting_response(request: web.Request) -> web.Response:
32943331
(
32953332
*(
32963333
(url, message, InvalidUrlRedirectClientError)
3297-
for (url, message) in INVALID_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN
3334+
for (url, message) in INVALID_REDIRECT_URL_WITH_ERROR_MESSAGE_YARL_ORIGIN
32983335
+ INVALID_URL_WITH_ERROR_MESSAGE_YARL_NEW
32993336
),
33003337
*(

‎tests/test_client_session.py‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1729,3 +1729,25 @@ async def test_netrc_auth_host_not_in_netrc(auth_server: TestServer) -> None:
17291729
text = await resp.text()
17301730
# Should not have auth since the host is not in netrc
17311731
assert text == "no_auth"
1732+
1733+
1734+
@pytest.mark.parametrize(
1735+
("location", "expected"),
1736+
(
1737+
("http:/ok", True),
1738+
("http:ok", True),
1739+
("http:", True),
1740+
("http:/", True),
1741+
("http://example.com/", False),
1742+
("http:///example.com", False),
1743+
(" http:///example.com", False),
1744+
("\x00http:///example.com", False),
1745+
("http:/\t/example.com", False),
1746+
("http:/\n/example.com", False),
1747+
("http:\\\\example.com", False),
1748+
("http:\\/example.com", False),
1749+
("http:/\\example.com", False),
1750+
),
1751+
)
1752+
def test_has_no_authority(location: str, expected: bool) -> None:
1753+
assert client._has_no_authority(location, "http") is expected

0 commit comments

Comments
 (0)