[staging] Dynamic routes with percent-encodable fixed segments (space, non-ASCII) are unre

[loonister1]

Staging review — this PR targets your own fork. Upstream sees nothing until you approve.

Upstream	aio-libs/aiohttp
Issue	#13433 — Dynamic routes with percent-encodable fixed segments (space, non-ASCII) are unreachable and url_for() URLs 404
Tests	unknown scope
Diff	3 files, 59 lines
AI policy	unstated

Commits

1. 5ac5f592 Match dynamic route fixed parts against the unquoted path — 2 file(s), 35 lines
2. 22b2ac16 Match static resource prefixes against the unquoted path — 2 file(s), 19 lines
3. 8ec53e72 Add a change note for issue Dynamic routes with percent-encodable fixed segments (space, non-ASCII) are unreachable and url_for() URLs 404 aio-libs/aiohttp#13433 — 1 file(s), 5 lines

What this change does

DynamicResource and PrefixResource percent-encoded their fixed path segment at registration and used that encoded form for both the match pattern and canonical, while UrlDispatcher.resolve walks the decoded path_safe and derives index keys from canonical — so such resources were never probed. The fix keeps the encoded form only for URL construction (_formatter, new _quoted_prefix) and uses the as-written form for matching and canonical, which is the representation PlainResource already used and the router already matches on. _get_resource_index_key and resolve are unchanged.

Reviewer notes

verdict=merge
reason=It fixes the root cause rather than the symptom: the encoded form stops being the matching/index representation and is kept only for URL construction, which is precisely the split PlainResource already uses (raw _path for _match/canonical, URL.build(..., encoded=False) for url_for), so _get_resource_index_key and resolve need no special-casing. The behaviour-level tests cover dynamic matching, the url_for round trip from the issue, the static prefix, and canonical, and the diff is small and idiomatic (correct CHANGES/13433.bugfix.rst, comments in house style).
risk=1) canonical is a documented public property and get_info()['prefix']/['formatter'] are semi-public; this changes their value for dynamic and prefix resources with encodable characters (raw instead of percent-encoded). Third-party instrumentation (OpenTelemetry/Prometheus route labels) reads resource.canonical — that representation change is the maintainers' call, not the contributor's, and it deserves a line in the changelog if you agree with it. 2) Only tests/test_urldispatch.py and tests/test_web_urldispatcher.py were run — ask for the full suite, especially test_web_functional.py, test_route_def.py and the subapp/normalize_path_middleware tests, since PrefixedSubAppResource._add_prefix_to_resources now propagates a raw prefix into every nested resource's pattern/path/formatter. 3) Audit every remaining reader of self._prefix, not just the one url_for the diff patched: StaticResource._directory_as_html does _quote_path(f"{self._prefix}/{rel_path}"), which previously double-encoded an already-requoted prefix (% → %25) and now looks correct — but no test exercises a directory listing under a non-ASCII prefix, so that improvement is unverified. StaticResource.resolve also slices with len(self._prefix) against the decoded path_safe; that slice is only right because the prefix is now raw, and is worth a nested-subapp test. 4) Scope check: PlainResource.add_prefix now receives a raw prefix too, so subapps mounted at an encodable prefix change behaviour beyond the dynamic/static cases named in the issue — a fix of the same bug class, but confirm you want it in this PR. 5) Residual gap: a fixed part containing a literal % (e.g. /100%/{x}) is still unmatchable, because path_safe leaves %25 encoded; this matches PlainResource's existing limitation and preserves _requote_path's "existing %-sequences are pre-encodings" semantics (so /a%2Fb/{x} keeps working), but the changelog's blanket "needs percent-encoding" wording overclaims slightly. 6) Minor: _quoted_prefix is a new attribute set in PrefixResource.__init__; MatchedSubAppResource is patched because it bypasses that __init__, but any external PrefixResource subclass doing the same will now raise AttributeError in add_prefix/url_for.
concerns=The tests were never run — please run .contrib-venv/bin/pytest --no-cov -q tests/test_urldispatch.py tests/test_web_urldispatcher.py before trusting this. Beyond that: (1) the canonical/get_info()["prefix"] semantic change described above is user-visible and may deserve a docs note in docs/web_reference.rst, which I did not add; (2) _directory_as_html still passes the now-unencoded _prefix through _quote_path, which I deliberately left alone — it produces correct links for spaces/non-ASCII but still double-encodes a %2F in the prefix, exactly as it did before; (3) DynamicResource.add_prefix now requotes the incoming prefix before prepending it to _formatter, which changes url_for() output for dynamic routes inside a sub-app mounted on an encodable prefix — that is the same bug, but it is a second behaviour change in the same commit.
attempts=1

Test output

[targeted; 3 commit(s) checked individually] $ /Users/m2/Documents/PW/github-bot/work/aio-libs__aiohttp/13433/.contrib-venv/bin/python -m pytest -q --no-header tests/test_urldispatch.py tests/test_web_urldispatcher.py
============================= test session starts ==============================
collected 208 items / 4 deselected / 204 selected

tests/test_urldispatch.py .............................................. [ 22%]
........................................................................ [ 57%]
.....................................                                    [ 75%]
tests/test_web_urldispatcher.py ........................................ [ 95%]
.........                                                                [100%]

============================= slowest 10 durations =============================
0.12s call     tests/test_urldispatch.py::test_domain_valid
0.11s call     tests/test_urldispatch.py::test_static_not_match
0.10s call     tests/test_web_urldispatcher.py::test_follow_symlink_directory_traversal_after_normalization
0.09s call     tests/test_web_urldispatcher.py::test_access_root_of_static_handler[index_subdir]
0.09s call     tests/test_web_urldispatcher.py::test_access_compressed_file_as_symlink
0.08s call     tests/test_web_urldispatcher.py::test_access_root_of_static_handler[index_forbidden]
0.07s call     tests/test_web_urldispatcher.py::test_access_root_of_static_handler[True-200-/-/-<html>\n<head>\n<title>Index of /.</title>\n</head>\n<body>\n<h1>Index of /.</h1>\n<ul>\n<li><a href="/my_dir">my_dir/</a></li>\n<li><a href="/my_file">my_file</a></li>\n</ul>\n</body>\n</html>]
0.07s call     tests/test_web_urldispatcher.py::test_access_root_of_static_handler[index_subdir_trailing_slash]
0.06s call     tests/test_web_urldispatcher.py::test_access_root_of_static_handler[index_static]
0.06s call     tests/test_web_urldispatcher.py::test_static_file_with_mock_permission_error
====================== 204 passed, 4 deselected in 4.06s =======================

Lint

(no linter)

---

To act on this, add a label:
contrib:submit to open it upstream · contrib:reject to discard · contrib:hold to leave it queued.

[loonister1]

Submitted upstream: aio-libs#13498