Skip to content

Commit

Permalink
Handle 403 and 404 issues in FileResponse class (#8538)
Browse files Browse the repository at this point in the history
  • Loading branch information
steverep authored Jul 26, 2024
1 parent 7108d64 commit 4f834b6
Show file tree
Hide file tree
Showing 6 changed files with 138 additions and 32 deletions.
10 changes: 10 additions & 0 deletions CHANGES/8182.bugfix.rst
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
Adjusted ``FileResponse`` to check file existence and access when preparing the response -- by :user:`steverep`.

The :py:class:`~aiohttp.web.FileResponse` class was modified to respond with
403 Forbidden or 404 Not Found as appropriate. Previously, it would cause a
server error if the path did not exist or could not be accessed. Checks for
existence, non-regular files, and permissions were expected to be done in the
route handler. For static routes, this now permits a compressed file to exist
without its uncompressed variant and still be served. In addition, this
changes the response status for files without read permission to 403, and for
non-regular files from 404 to 403 for consistency.
27 changes: 22 additions & 5 deletions aiohttp/web_fileresponse.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import sys
from contextlib import suppress
from mimetypes import MimeTypes
from stat import S_ISREG
from types import MappingProxyType
from typing import (
IO,
Expand All @@ -22,6 +23,8 @@
from .helpers import ETAG_ANY, ETag, must_be_empty_body
from .typedefs import LooseHeaders, PathLike
from .web_exceptions import (
HTTPForbidden,
HTTPNotFound,
HTTPNotModified,
HTTPPartialContent,
HTTPPreconditionFailed,
Expand Down Expand Up @@ -177,13 +180,22 @@ def _get_file_path_stat_encoding(
return file_path, file_path.stat(), None

async def prepare(self, request: "BaseRequest") -> Optional[AbstractStreamWriter]:
loop = asyncio.get_event_loop()
loop = asyncio.get_running_loop()
# Encoding comparisons should be case-insensitive
# https://www.rfc-editor.org/rfc/rfc9110#section-8.4.1
accept_encoding = request.headers.get(hdrs.ACCEPT_ENCODING, "").lower()
file_path, st, file_encoding = await loop.run_in_executor(
None, self._get_file_path_stat_encoding, accept_encoding
)
try:
file_path, st, file_encoding = await loop.run_in_executor(
None, self._get_file_path_stat_encoding, accept_encoding
)
except FileNotFoundError:
self.set_status(HTTPNotFound.status_code)
return await super().prepare(request)

# Forbid special files like sockets, pipes, devices, etc.
if not S_ISREG(st.st_mode):
self.set_status(HTTPForbidden.status_code)
return await super().prepare(request)

etag_value = f"{st.st_mtime_ns:x}-{st.st_size:x}"
last_modified = st.st_mtime
Expand Down Expand Up @@ -320,7 +332,12 @@ async def prepare(self, request: "BaseRequest") -> Optional[AbstractStreamWriter
if count == 0 or must_be_empty_body(request.method, self.status):
return await super().prepare(request)

fobj = await loop.run_in_executor(None, file_path.open, "rb")
try:
fobj = await loop.run_in_executor(None, file_path.open, "rb")
except PermissionError:
self.set_status(HTTPForbidden.status_code)
return await super().prepare(request)

if start: # be aware that start could be None or int=0 here.
offset = start
else:
Expand Down
5 changes: 1 addition & 4 deletions aiohttp/web_urldispatcher.py
Original file line number Diff line number Diff line change
Expand Up @@ -690,10 +690,7 @@ def _resolve_path_to_response(self, unresolved_path: Path) -> StreamResponse:
except PermissionError as error:
raise HTTPForbidden() from error

# Not a regular file or does not exist.
if not file_path.is_file():
raise HTTPNotFound()

# Return the file response, which handles all other checks.
return FileResponse(file_path, chunk_size=self._chunk_size)

def _directory_as_html(self, dir_path: Path) -> str:
Expand Down
9 changes: 9 additions & 0 deletions tests/test_web_sendfile.py
Original file line number Diff line number Diff line change
@@ -1,11 +1,14 @@
from pathlib import Path
from stat import S_IFREG, S_IRUSR, S_IWUSR
from typing import Any
from unittest import mock

from aiohttp import hdrs
from aiohttp.test_utils import make_mocked_coro, make_mocked_request
from aiohttp.web_fileresponse import FileResponse

MOCK_MODE = S_IFREG | S_IRUSR | S_IWUSR


def test_using_gzip_if_header_present_and_file_available(loop: Any) -> None:
request = make_mocked_request(
Expand All @@ -18,6 +21,7 @@ def test_using_gzip_if_header_present_and_file_available(loop: Any) -> None:
gz_filepath = mock.create_autospec(Path, spec_set=True)
gz_filepath.stat.return_value.st_size = 1024
gz_filepath.stat.return_value.st_mtime_ns = 1603733507222449291
gz_filepath.stat.return_value.st_mode = MOCK_MODE

filepath = mock.create_autospec(Path, spec_set=True)
filepath.name = "logo.png"
Expand All @@ -39,12 +43,14 @@ def test_gzip_if_header_not_present_and_file_available(loop: Any) -> None:
gz_filepath = mock.create_autospec(Path, spec_set=True)
gz_filepath.stat.return_value.st_size = 1024
gz_filepath.stat.return_value.st_mtime_ns = 1603733507222449291
gz_filepath.stat.return_value.st_mode = MOCK_MODE

filepath = mock.create_autospec(Path, spec_set=True)
filepath.name = "logo.png"
filepath.with_suffix.return_value = gz_filepath
filepath.stat.return_value.st_size = 1024
filepath.stat.return_value.st_mtime_ns = 1603733507222449291
filepath.stat.return_value.st_mode = MOCK_MODE

file_sender = FileResponse(filepath)
file_sender._path = filepath
Expand All @@ -67,6 +73,7 @@ def test_gzip_if_header_not_present_and_file_not_available(loop: Any) -> None:
filepath.with_suffix.return_value = gz_filepath
filepath.stat.return_value.st_size = 1024
filepath.stat.return_value.st_mtime_ns = 1603733507222449291
filepath.stat.return_value.st_mode = MOCK_MODE

file_sender = FileResponse(filepath)
file_sender._path = filepath
Expand All @@ -91,6 +98,7 @@ def test_gzip_if_header_present_and_file_not_available(loop: Any) -> None:
filepath.with_suffix.return_value = gz_filepath
filepath.stat.return_value.st_size = 1024
filepath.stat.return_value.st_mtime_ns = 1603733507222449291
filepath.stat.return_value.st_mode = MOCK_MODE

file_sender = FileResponse(filepath)
file_sender._path = filepath
Expand All @@ -109,6 +117,7 @@ def test_status_controlled_by_user(loop: Any) -> None:
filepath.name = "logo.png"
filepath.stat.return_value.st_size = 1024
filepath.stat.return_value.st_mtime_ns = 1603733507222449291
filepath.stat.return_value.st_mode = MOCK_MODE

file_sender = FileResponse(filepath, status=203)
file_sender._path = filepath
Expand Down
2 changes: 1 addition & 1 deletion tests/test_web_sendfile_functional.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def hello_txt(request, tmp_path_factory) -> pathlib.Path:
"br": txt.with_suffix(f"{txt.suffix}.br"),
"bzip2": txt.with_suffix(f"{txt.suffix}.bz2"),
}
hello[None].write_bytes(HELLO_AIOHTTP)
# Uncompressed file is not actually written to test it is not required.
hello["gzip"].write_bytes(gzip.compress(HELLO_AIOHTTP))
hello["br"].write_bytes(brotli.compress(HELLO_AIOHTTP))
hello["bzip2"].write_bytes(bz2.compress(HELLO_AIOHTTP))
Expand Down
117 changes: 95 additions & 22 deletions tests/test_web_urldispatcher.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import asyncio
import functools
import os
import pathlib
import socket
import sys
from typing import Generator, Optional
from unittest import mock
from unittest.mock import MagicMock
from stat import S_IFIFO, S_IMODE
from typing import Any, Generator, Optional

import pytest
import yarl
Expand Down Expand Up @@ -445,6 +446,56 @@ def mock_is_dir(self: pathlib.Path) -> bool:
assert r.status == 403


@pytest.mark.skipif(
sys.platform.startswith("win32"), reason="Cannot remove read access on Windows"
)
async def test_static_file_without_read_permission(
tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
) -> None:
"""Test static file without read permission receives forbidden response."""
my_file = tmp_path / "my_file.txt"
my_file.write_text("secret")
my_file.chmod(0o000)

app = web.Application()
app.router.add_static("/", str(tmp_path))
client = await aiohttp_client(app)

r = await client.get(f"/{my_file.name}")
assert r.status == 403


async def test_static_file_with_mock_permission_error(
monkeypatch: pytest.MonkeyPatch,
tmp_path: pathlib.Path,
aiohttp_client: AiohttpClient,
) -> None:
"""Test static file with mock permission errors receives forbidden response."""
my_file = tmp_path / "my_file.txt"
my_file.write_text("secret")
my_readable = tmp_path / "my_readable.txt"
my_readable.write_text("info")

real_open = pathlib.Path.open

def mock_open(self: pathlib.Path, *args: Any, **kwargs: Any) -> Any:
if my_file.samefile(self):
raise PermissionError()
return real_open(self, *args, **kwargs)

monkeypatch.setattr("pathlib.Path.open", mock_open)

app = web.Application()
app.router.add_static("/", str(tmp_path))
client = await aiohttp_client(app)

# Test the mock only applies to my_file, then test the permission error.
r = await client.get(f"/{my_readable.name}")
assert r.status == 200
r = await client.get(f"/{my_file.name}")
assert r.status == 403


async def test_access_symlink_loop(
tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
) -> None:
Expand All @@ -464,32 +515,54 @@ async def test_access_symlink_loop(


async def test_access_special_resource(
tmp_path: pathlib.Path, aiohttp_client: AiohttpClient
tmp_path_factory: pytest.TempPathFactory, aiohttp_client: AiohttpClient
) -> None:
# Tests the access to a resource that is neither a file nor a directory.
# Checks that if a special resource is accessed (f.e. named pipe or UNIX
# domain socket) then 404 HTTP status returned.
"""Test access to non-regular files is forbidden using a UNIX domain socket."""
if not getattr(socket, "AF_UNIX", None):
pytest.skip("UNIX domain sockets not supported")

tmp_path = tmp_path_factory.mktemp("special")
my_special = tmp_path / "sock"
my_socket = socket.socket(socket.AF_UNIX)
my_socket.bind(str(my_special))
assert my_special.is_socket()

app = web.Application()
app.router.add_static("/", str(tmp_path))

with mock.patch("pathlib.Path.__new__") as path_constructor:
special = MagicMock()
special.is_dir.return_value = False
special.is_file.return_value = False
client = await aiohttp_client(app)
r = await client.get(f"/{my_special.name}")
assert r.status == 403
my_socket.close()

path = MagicMock()
path.joinpath.side_effect = lambda p: (special if p == "special" else path)
path.resolve.return_value = path
special.resolve.return_value = special

path_constructor.return_value = path
async def test_access_mock_special_resource(
monkeypatch: pytest.MonkeyPatch,
tmp_path: pathlib.Path,
aiohttp_client: AiohttpClient,
) -> None:
"""Test access to non-regular files is forbidden using a mock FIFO."""
my_special = tmp_path / "my_special"
my_special.touch()

real_result = my_special.stat()
real_stat = pathlib.Path.stat

def mock_stat(self: pathlib.Path) -> os.stat_result:
s = real_stat(self)
if os.path.samestat(s, real_result):
mock_mode = S_IFIFO | S_IMODE(s.st_mode)
s = os.stat_result([mock_mode] + list(s)[1:])
return s

# Register global static route:
app.router.add_static("/", str(tmp_path), show_index=True)
client = await aiohttp_client(app)
monkeypatch.setattr("pathlib.Path.stat", mock_stat)

# Request the root of the static directory.
r = await client.get("/special")
assert r.status == 403
app = web.Application()
app.router.add_static("/", str(tmp_path))
client = await aiohttp_client(app)

r = await client.get(f"/{my_special.name}")
assert r.status == 403


async def test_partially_applied_handler(aiohttp_client: AiohttpClient) -> None:
Expand Down

0 comments on commit 4f834b6

Please sign in to comment.