Fix double unquoting in url dispatcher (#9267)

Co-authored-by: Sam Bull <[email protected]>

CHANGES/9267.breaking.rst

@@ -0,0 +1 @@
+Increased minimum yarl version to 1.12.0 -- by :user:`bdraco`.

CHANGES/9267.bugfix.rst

@@ -0,0 +1 @@
+8898.bugfix.rst

aiohttp/web_urldispatcher.py

@@ -352,7 +352,7 @@ def register_route(self, route: "ResourceRoute") -> None:
     async def resolve(self, request: Request) -> _Resolve:
         allowed_methods: Set[str] = set()
 
-        match_dict = self._match(request.rel_url.path)
+        match_dict = self._match(request.rel_url.path_safe)
         if match_dict is None:
             return None, allowed_methods
 
@@ -402,8 +402,7 @@ def _match(self, path: str) -> Optional[Dict[str, str]]:
         # string comparison is about 10 times faster than regexp matching
         if self._path == path:
             return {}
-        else:
-            return None
+        return None
 
     def raw_match(self, path: str) -> bool:
         return self._path == path
@@ -473,10 +472,9 @@ def _match(self, path: str) -> Optional[Dict[str, str]]:
         match = self._pattern.fullmatch(path)
         if match is None:
             return None
-        else:
-            return {
-                key: _unquote_path(value) for key, value in match.groupdict().items()
-            }
+        return {
+            key: _unquote_path_safe(value) for key, value in match.groupdict().items()
+        }
 
     def raw_match(self, path: str) -> bool:
         return self._orig_path == path
@@ -618,7 +616,7 @@ def set_options_route(self, handler: Handler) -> None:
         )
 
     async def resolve(self, request: Request) -> _Resolve:
-        path = request.rel_url.path
+        path = request.rel_url.path_safe
         method = request.method
         allowed_methods = set(self._routes)
         if not path.startswith(self._prefix2) and path != self._prefix:
@@ -627,7 +625,7 @@ async def resolve(self, request: Request) -> _Resolve:
         if method not in allowed_methods:
             return None, allowed_methods
 
-        match_dict = {"filename": _unquote_path(path[len(self._prefix) + 1 :])}
+        match_dict = {"filename": _unquote_path_safe(path[len(self._prefix) + 1 :])}
         return (UrlMappingMatchInfo(match_dict, self._routes[method]), allowed_methods)
 
     def __len__(self) -> int:
@@ -1007,7 +1005,7 @@ async def resolve(self, request: Request) -> UrlMappingMatchInfo:
         # candidates for a given url part because there are multiple resources
         # registered for the same canonical path, we resolve them in a linear
         # fashion to ensure registration order is respected.
-        url_part = request.rel_url.path
+        url_part = request.rel_url.path_safe
         while url_part:
             for candidate in resource_index.get(url_part, ()):
                 match_dict, allowed = await candidate.resolve(request)
@@ -1256,8 +1254,10 @@ def _quote_path(value: str) -> str:
     return URL.build(path=value, encoded=False).raw_path
 
 
-def _unquote_path(value: str) -> str:
-    return URL.build(path=value, encoded=True).path.replace("%2F", "/")
+def _unquote_path_safe(value: str) -> str:
+    if "%" not in value:
+        return value
+    return value.replace("%2F", "/").replace("%25", "%")
 
 
 def _requote_path(value: str) -> str:

requirements/base.txt

@@ -38,5 +38,5 @@ typing-extensions==4.12.2
     # via multidict
 uvloop==0.20.0 ; platform_system != "Windows" and implementation_name == "cpython"
     # via -r requirements/base.in
-yarl==1.11.1
+yarl==1.12.0
     # via -r requirements/runtime-deps.in

requirements/constraints.txt

@@ -283,7 +283,7 @@ webcolors==24.8.0
     # via blockdiag
 wheel==0.44.0
     # via pip-tools
-yarl==1.11.1
+yarl==1.12.0
     # via -r requirements/runtime-deps.in
 zipp==3.20.2
     # via

requirements/dev.txt

@@ -275,7 +275,7 @@ webcolors==24.8.0
     # via blockdiag
 wheel==0.44.0
     # via pip-tools
-yarl==1.11.1
+yarl==1.12.0
     # via -r requirements/runtime-deps.in
 zipp==3.20.2
     # via

requirements/runtime-deps.in

@@ -8,4 +8,4 @@ Brotli; platform_python_implementation == 'CPython'
 brotlicffi; platform_python_implementation != 'CPython'
 frozenlist >= 1.1.1
 multidict >=4.5, < 7.0
-yarl >= 1.11.0, < 2.0
+yarl >= 1.12.0, < 2.0

requirements/runtime-deps.txt

@@ -32,5 +32,5 @@ pycparser==2.22
     # via cffi
 typing-extensions==4.12.2
     # via multidict
-yarl==1.11.1
+yarl==1.12.0
     # via -r requirements/runtime-deps.in

requirements/test.txt

@@ -131,5 +131,5 @@ uvloop==0.20.0 ; platform_system != "Windows" and implementation_name == "cpytho
     # via -r requirements/base.in
 wait-for-it==2.2.2
     # via -r requirements/test.in
-yarl==1.11.1
+yarl==1.12.0
     # via -r requirements/runtime-deps.in

setup.cfg

@@ -53,7 +53,7 @@ install_requires =
   async-timeout >= 4.0, < 5.0 ; python_version < "3.11"
   frozenlist >= 1.1.1
   multidict >=4.5, < 7.0
-  yarl >= 1.11.0, < 2.0
+  yarl >= 1.12.0, < 2.0
 
 [options.exclude_package_data]
 * =

tests/test_urldispatch.py

@@ -4,7 +4,7 @@
 from collections.abc import Container, Iterable, Mapping, MutableMapping, Sized
 from functools import partial
 from typing import Awaitable, Callable, Dict, List, NoReturn, Optional, Type
-from urllib.parse import unquote
+from urllib.parse import quote, unquote
 
 import pytest
 from yarl import URL
@@ -486,7 +486,7 @@ def test_add_static_quoting(router: web.UrlDispatcher) -> None:
     )
     assert router["static"] is resource
     url = resource.url_for(filename="/1 2/файл%2F.txt")
-    assert url.path == "/пре %2Fфикс/1 2/файл%2F.txt"
+    assert url.path == "/пре /фикс/1 2/файл%2F.txt"
     assert str(url) == (
         "/%D0%BF%D1%80%D0%B5%20%2F%D1%84%D0%B8%D0%BA%D1%81"
         "/1%202/%D1%84%D0%B0%D0%B9%D0%BB%252F.txt"
@@ -664,7 +664,7 @@ def test_route_dynamic_quoting(router: web.UrlDispatcher) -> None:
     route = router.add_route("GET", r"/пре %2Fфикс/{arg}", handler)
 
     url = route.url_for(arg="1 2/текст%2F")
-    assert url.path == "/пре %2Fфикс/1 2/текст%2F"
+    assert url.path == "/пре /фикс/1 2/текст%2F"
     assert str(url) == (
         "/%D0%BF%D1%80%D0%B5%20%2F%D1%84%D0%B8%D0%BA%D1%81"
         "/1%202/%D1%82%D0%B5%D0%BA%D1%81%D1%82%252F"
@@ -777,6 +777,17 @@ async def test_dynamic_match_unquoted_path(router: web.UrlDispatcher) -> None:
     assert match_info == {"path": "path", "subpath": unquote(resource_id)}
 
 
+async def test_dynamic_match_double_quoted_path(router: web.UrlDispatcher) -> None:
+    """Verify that double-quoted path is unquoted only once."""
+    handler = make_handler()
+    router.add_route("GET", "/{path}/{subpath}", handler)
+    resource_id = quote("my/path|with!some%strange$characters", safe="")
+    double_quoted_resource_id = quote(resource_id, safe="")
+    req = make_mocked_request("GET", f"/path/{double_quoted_resource_id}")
+    match_info = await router.resolve(req)
+    assert match_info == {"path": "path", "subpath": resource_id}
+
+
 def test_add_route_not_started_with_slash(router: web.UrlDispatcher) -> None:
     with pytest.raises(ValueError):
         handler = make_handler()

tests/test_web_urldispatcher.py

@@ -5,7 +5,7 @@
 import socket
 import sys
 from stat import S_IFIFO, S_IMODE
-from typing import Any, Generator, Optional
+from typing import Any, Generator, NoReturn, Optional
 
 import pytest
 import yarl
@@ -864,6 +864,22 @@ async def handler(request: web.Request) -> web.Response:
         assert resp.status == expected_http_resp_status
 
 
+async def test_decoded_raw_match_regex(aiohttp_client: AiohttpClient) -> None:
+    """Verify that raw_match only matches decoded url."""
+    app = web.Application()
+
+    async def handler(request: web.Request) -> NoReturn:
+        assert False
+
+    app.router.add_get("/467%2C802%2C24834%2C24952%2C25362%2C40574/hello", handler)
+    client = await aiohttp_client(app)
+
+    async with client.get(
+        yarl.URL("/467%2C802%2C24834%2C24952%2C25362%2C40574/hello", encoded=True)
+    ) as resp:
+        assert resp.status == 404  # should only match decoded url
+
+
 async def test_order_is_preserved(aiohttp_client: AiohttpClient) -> None:
     """Test route order is preserved.