Remove blocking IO for static resources and refactor exception handling

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 routs were moved to a
+separate thread in order 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

@@ -639,59 +639,67 @@ 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)
+        # TODO(PY39): Remove OSError needed to appease 3.8 on Windows.
+        except (ValueError, RuntimeError, OSError) 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

@@ -388,31 +388,25 @@ async def async_handler(request: web.Request) -> web.Response:
             assert route.handler.__doc__ == "Doc"
 
 
+@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_unauthorized_folder_access(
-    tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
+    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
+    """Tests forbidden response for request with unauthorized directory."""
     # have permissions to do so for the folder.
     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
-
-        # 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
+    r = await client.get(f"/{my_dir.name}/{file_request}")
+    assert r.status == 403
 
 
 async def test_access_symlink_loop(