Remove blocking IO for static resources and refactor exception handli…

…ng (#8507)

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <[email protected]>
Co-authored-by: J. Nick Koston <[email protected]>

CHANGES/8507.bugfix.rst

@@ -0,0 +1,8 @@
+Removed blocking I/O in the event loop for static resources and refactored
+exception handling -- by :user:`steverep`.
+
+File system calls when handling requests for static routes were moved to a
+separate thread to potentially improve performance. Exception handling
+was tightened in order to only return 403 Forbidden or 404 Not Found responses
+for expected scenarios; 500 Internal Server Error would be returned for any
+unknown errors.

aiohttp/web_urldispatcher.py

@@ -7,6 +7,7 @@
 import keyword
 import os
 import re
+import sys
 from contextlib import contextmanager
 from pathlib import Path
 from types import MappingProxyType
@@ -75,6 +76,12 @@
 else:
     BaseDict = dict
 
+CIRCULAR_SYMLINK_ERROR = (
+    OSError
+    if sys.version_info < (3, 10) and sys.platform.startswith("win32")
+    else RuntimeError
+)
+
 YARL_VERSION: Final[Tuple[int, ...]] = tuple(map(int, yarl_version.split(".")[:2]))
 
 HTTP_METHOD_RE: Final[Pattern[str]] = re.compile(
@@ -639,59 +646,66 @@ def __iter__(self) -> Iterator[AbstractRoute]:
 
     async def _handle(self, request: Request) -> StreamResponse:
         rel_url = request.match_info["filename"]
+        filename = Path(rel_url)
+        if filename.anchor:
+            # rel_url is an absolute name like
+            # /static/\\machine_name\c$ or /static/D:\path
+            # where the static dir is totally different
+            raise HTTPForbidden()
+
+        unresolved_path = self._directory.joinpath(filename)
+        loop = asyncio.get_running_loop()
+        return await loop.run_in_executor(
+            None, self._resolve_path_to_response, unresolved_path
+        )
+
+    def _resolve_path_to_response(self, unresolved_path: Path) -> StreamResponse:
+        """Take the unresolved path and query the file system to form a response."""
+        # Check for access outside the root directory. For follow symlinks, URI
+        # cannot traverse out, but symlinks can. Otherwise, no access outside
+        # root is permitted.
         try:
-            filename = Path(rel_url)
-            if filename.anchor:
-                # rel_url is an absolute name like
-                # /static/\\machine_name\c$ or /static/D:\path
-                # where the static dir is totally different
-                raise HTTPForbidden()
-            unresolved_path = self._directory.joinpath(filename)
             if self._follow_symlinks:
                 normalized_path = Path(os.path.normpath(unresolved_path))
                 normalized_path.relative_to(self._directory)
-                filepath = normalized_path.resolve()
+                file_path = normalized_path.resolve()
             else:
-                filepath = unresolved_path.resolve()
-                filepath.relative_to(self._directory)
-        except (ValueError, FileNotFoundError) as error:
-            # relatively safe
-            raise HTTPNotFound() from error
-        except HTTPForbidden:
-            raise
-        except Exception as error:
-            # perm error or other kind!
-            request.app.logger.exception(error)
+                file_path = unresolved_path.resolve()
+                file_path.relative_to(self._directory)
+        except (ValueError, CIRCULAR_SYMLINK_ERROR) as error:
+            # ValueError for relative check; RuntimeError for circular symlink.
             raise HTTPNotFound() from error
 
-        # on opening a dir, load its contents if allowed
-        if filepath.is_dir():
-            if self._show_index:
-                try:
+        # if path is a directory, return the contents if permitted. Note the
+        # directory check will raise if a segment is not readable.
+        try:
+            if file_path.is_dir():
+                if self._show_index:
                     return Response(
-                        text=self._directory_as_html(filepath), content_type="text/html"
+                        text=self._directory_as_html(file_path),
+                        content_type="text/html",
                     )
-                except PermissionError:
+                else:
                     raise HTTPForbidden()
-            else:
-                raise HTTPForbidden()
-        elif filepath.is_file():
-            return FileResponse(filepath, chunk_size=self._chunk_size)
-        else:
-            raise HTTPNotFound
+        except PermissionError as error:
+            raise HTTPForbidden() from error
+
+        # Not a regular file or does not exist.
+        if not file_path.is_file():
+            raise HTTPNotFound()
 
-    def _directory_as_html(self, filepath: Path) -> str:
-        # returns directory's index as html
+        return FileResponse(file_path, chunk_size=self._chunk_size)
 
-        # sanity check
-        assert filepath.is_dir()
+    def _directory_as_html(self, dir_path: Path) -> str:
+        """returns directory's index as html."""
+        assert dir_path.is_dir()
 
-        relative_path_to_dir = filepath.relative_to(self._directory).as_posix()
+        relative_path_to_dir = dir_path.relative_to(self._directory).as_posix()
         index_of = f"Index of /{html_escape(relative_path_to_dir)}"
         h1 = f"<h1>{index_of}</h1>"
 
         index_list = []
-        dir_index = filepath.iterdir()
+        dir_index = dir_path.iterdir()
         for _file in sorted(dir_index):
             # show file url as relative to static path
             rel_path = _file.relative_to(self._directory).as_posix()

tests/test_web_urldispatcher.py

@@ -2,7 +2,7 @@
 import functools
 import pathlib
 import sys
-from typing import Optional
+from typing import Generator, Optional
 from unittest import mock
 from unittest.mock import MagicMock
 
@@ -388,31 +388,61 @@ async def async_handler(request: web.Request) -> web.Response:
             assert route.handler.__doc__ == "Doc"
 
 
-async def test_unauthorized_folder_access(
-    tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
+@pytest.mark.skipif(
+    sys.platform.startswith("win32"), reason="Cannot remove read access on Windows"
+)
+@pytest.mark.parametrize("file_request", ["", "my_file.txt"])
+async def test_static_directory_without_read_permission(
+    tmp_path: pathlib.Path, aiohttp_client: AiohttpClient, file_request: str
 ) -> None:
-    # Tests the unauthorized access to a folder of static file server.
-    # Try to list a folder content of static file server when server does not
-    # have permissions to do so for the folder.
+    """Test static directory without read permission receives forbidden response."""
     my_dir = tmp_path / "my_dir"
     my_dir.mkdir()
+    my_dir.chmod(0o000)
 
     app = web.Application()
+    app.router.add_static("/", str(tmp_path), show_index=True)
+    client = await aiohttp_client(app)
 
-    with mock.patch("pathlib.Path.__new__") as path_constructor:
-        path = MagicMock()
-        path.joinpath.return_value = path
-        path.resolve.return_value = path
-        path.iterdir.return_value.__iter__.side_effect = PermissionError()
-        path_constructor.return_value = path
+    r = await client.get(f"/{my_dir.name}/{file_request}")
+    assert r.status == 403
 
-        # Register global static route:
-        app.router.add_static("/", str(tmp_path), show_index=True)
-        client = await aiohttp_client(app)
 
-        # Request the root of the static directory.
-        r = await client.get("/" + my_dir.name)
-        assert r.status == 403
+@pytest.mark.parametrize("file_request", ["", "my_file.txt"])
+async def test_static_directory_with_mock_permission_error(
+    monkeypatch: pytest.MonkeyPatch,
+    tmp_path: pathlib.Path,
+    aiohttp_client: AiohttpClient,
+    file_request: str,
+) -> None:
+    """Test static directory with mock permission errors receives forbidden response."""
+    my_dir = tmp_path / "my_dir"
+    my_dir.mkdir()
+
+    real_iterdir = pathlib.Path.iterdir
+    real_is_dir = pathlib.Path.is_dir
+
+    def mock_iterdir(self: pathlib.Path) -> Generator[pathlib.Path, None, None]:
+        if my_dir.samefile(self):
+            raise PermissionError()
+        return real_iterdir(self)
+
+    def mock_is_dir(self: pathlib.Path) -> bool:
+        if my_dir.samefile(self.parent):
+            raise PermissionError()
+        return real_is_dir(self)
+
+    monkeypatch.setattr("pathlib.Path.iterdir", mock_iterdir)
+    monkeypatch.setattr("pathlib.Path.is_dir", mock_is_dir)
+
+    app = web.Application()
+    app.router.add_static("/", str(tmp_path), show_index=True)
+    client = await aiohttp_client(app)
+
+    r = await client.get("/")
+    assert r.status == 200
+    r = await client.get(f"/{my_dir.name}/{file_request}")
+    assert r.status == 403
 
 
 async def test_access_symlink_loop(