From 66f52e3c4dd92c203c929875338ccf207bf381ec Mon Sep 17 00:00:00 2001 From: Sam Bull Date: Thu, 11 Apr 2024 15:03:23 +0100 Subject: [PATCH] Escape filenames and paths in HTML when generating index pages (#8317) Co-authored-by: J. Nick Koston (cherry picked from commit ffbc43233209df302863712b511a11bdb6001b0f) --- CHANGES/8317.bugfix.rst | 1 + aiohttp/web_urldispatcher.py | 12 ++-- tests/test_web_urldispatcher.py | 124 ++++++++++++++++++++++++++++---- 3 files changed, 118 insertions(+), 19 deletions(-) create mode 100644 CHANGES/8317.bugfix.rst diff --git a/CHANGES/8317.bugfix.rst b/CHANGES/8317.bugfix.rst new file mode 100644 index 00000000000..b24ef2aeb81 --- /dev/null +++ b/CHANGES/8317.bugfix.rst @@ -0,0 +1 @@ +Escaped filenames in static view -- by :user:`bdraco`. diff --git a/aiohttp/web_urldispatcher.py b/aiohttp/web_urldispatcher.py index 99696533444..954291f6449 100644 --- a/aiohttp/web_urldispatcher.py +++ b/aiohttp/web_urldispatcher.py @@ -1,7 +1,9 @@ import abc import asyncio import base64 +import functools import hashlib +import html import inspect import keyword import os @@ -90,6 +92,8 @@ _ExpectHandler = Callable[[Request], Awaitable[Optional[StreamResponse]]] _Resolve = Tuple[Optional["UrlMappingMatchInfo"], Set[str]] +html_escape = functools.partial(html.escape, quote=True) + class _InfoDict(TypedDict, total=False): path: str @@ -708,7 +712,7 @@ def _directory_as_html(self, filepath: Path) -> str: assert filepath.is_dir() relative_path_to_dir = filepath.relative_to(self._directory).as_posix() - index_of = f"Index of /{relative_path_to_dir}" + index_of = f"Index of /{html_escape(relative_path_to_dir)}" h1 = f"

{index_of}

" index_list = [] @@ -716,7 +720,7 @@ def _directory_as_html(self, filepath: Path) -> str: for _file in sorted(dir_index): # show file url as relative to static path rel_path = _file.relative_to(self._directory).as_posix() - file_url = self._prefix + "/" + rel_path + quoted_file_url = _quote_path(f"{self._prefix}/{rel_path}") # if file is a directory, add '/' to the end of the name if _file.is_dir(): @@ -725,9 +729,7 @@ def _directory_as_html(self, filepath: Path) -> str: file_name = _file.name index_list.append( - '
  • {name}
    • '.format( - url=file_url, name=file_name - ) + f'
  • {html_escape(file_name)}
    • ' ) ul = "".format("\n".join(index_list)) body = f"\n{h1}\n{ul}\n" diff --git a/tests/test_web_urldispatcher.py b/tests/test_web_urldispatcher.py index 76e533e473a..0441890c10b 100644 --- a/tests/test_web_urldispatcher.py +++ b/tests/test_web_urldispatcher.py @@ -1,6 +1,7 @@ import asyncio import functools import pathlib +import sys from typing import Optional from unittest import mock from unittest.mock import MagicMock @@ -14,31 +15,38 @@ @pytest.mark.parametrize( - "show_index,status,prefix,data", + "show_index,status,prefix,request_path,data", [ - pytest.param(False, 403, "/", None, id="index_forbidden"), + pytest.param(False, 403, "/", "/", None, id="index_forbidden"), pytest.param( True, 200, "/", - b"\n\nIndex of /.\n" - b"\n\n

    Index of /.

    \n\n\n", - id="index_root", + "/", + b"\n\nIndex of /.\n\n\n

    Index of" + b' /.

    \n\n\n", ), pytest.param( True, 200, "/static", - b"\n\nIndex of /.\n" - b"\n\n

    Index of /.

    \n\n\n", + "/static", + b"\n\nIndex of /.\n\n\n

    Index of" + b' /.

    \n\n\n', id="index_static", ), + pytest.param( + True, + 200, + "/static", + "/static/my_dir", + b"\n\nIndex of /my_dir\n\n\n

    " + b'Index of /my_dir

    \n\n\n", + id="index_subdir", + ), ], ) async def test_access_root_of_static_handler( @@ -47,6 +55,7 @@ async def test_access_root_of_static_handler( show_index: bool, status: int, prefix: str, + request_path: str, data: Optional[bytes], ) -> None: # Tests the operation of static file server. @@ -72,7 +81,94 @@ async def test_access_root_of_static_handler( client = await aiohttp_client(app) # Request the root of the static directory. - async with await client.get(prefix) as r: + async with await client.get(request_path) as r: + assert r.status == status + + if data: + assert r.headers["Content-Type"] == "text/html; charset=utf-8" + read_ = await r.read() + assert read_ == data + + +@pytest.mark.internal # Dependent on filesystem +@pytest.mark.skipif( + not sys.platform.startswith("linux"), + reason="Invalid filenames on some filesystems (like Windows)", +) +@pytest.mark.parametrize( + "show_index,status,prefix,request_path,data", + [ + pytest.param(False, 403, "/", "/", None, id="index_forbidden"), + pytest.param( + True, + 200, + "/", + "/", + b"\n\nIndex of /.\n\n\n

    Index of" + b' /.

    \n\n\n", + ), + pytest.param( + True, + 200, + "/static", + "/static", + b"\n\nIndex of /.\n\n\n

    Index of" + b' /.

    \n\n\n", + id="index_static", + ), + pytest.param( + True, + 200, + "/static", + "/static/.dir", + b"\n\nIndex of /<img src=0 onerror=alert(1)>.dir</t" + b"itle>\n</head>\n<body>\n<h1>Index of /<img src=0 onerror=alert(1)>.di" + b'r</h1>\n<ul>\n<li><a href="/static/%3Cimg%20src=0%20onerror=alert(1)%3E.di' + b'r/my_file_in_dir">my_file_in_dir</a></li>\n</ul>\n</body>\n</html>', + id="index_subdir", + ), + ], +) +async def test_access_root_of_static_handler_xss( + tmp_path: pathlib.Path, + aiohttp_client: AiohttpClient, + show_index: bool, + status: int, + prefix: str, + request_path: str, + data: Optional[bytes], +) -> None: + # Tests the operation of static file server. + # Try to access the root of static file server, and make + # sure that correct HTTP statuses are returned depending if we directory + # index should be shown or not. + # Ensure that html in file names is escaped. + # Ensure that links are url quoted. + my_file = tmp_path / "<img src=0 onerror=alert(1)>.txt" + my_dir = tmp_path / "<img src=0 onerror=alert(1)>.dir" + my_dir.mkdir() + my_file_in_dir = my_dir / "my_file_in_dir" + + with my_file.open("w") as fw: + fw.write("hello") + + with my_file_in_dir.open("w") as fw: + fw.write("world") + + app = web.Application() + + # Register global static route: + app.router.add_static(prefix, str(tmp_path), show_index=show_index) + client = await aiohttp_client(app) + + # Request the root of the static directory. + async with await client.get(request_path) as r: assert r.status == status if data: