diff --git a/app/assets/api/routes.py b/app/assets/api/routes.py index 7ef462f5c..53c84eff3 100644 --- a/app/assets/api/routes.py +++ b/app/assets/api/routes.py @@ -306,12 +306,15 @@ async def download_asset_content(request: web.Request) -> web.Response: 404, "FILE_NOT_FOUND", "Underlying file not found on disk." ) - _DANGEROUS_MIME_TYPES = { - "text/html", "text/html-sandboxed", "application/xhtml+xml", - "text/javascript", "text/css", - } - if content_type in _DANGEROUS_MIME_TYPES: + # User-controlled asset content must never render inline in the app origin + # (stored XSS via SVG/HTML/XML). Force dangerous types to download and + # override any requested inline disposition. Centralised through + # folder_paths.is_dangerous_content_type so this can't drift from /view and + # /userdata (the previous inline set here omitted image/svg+xml and missed + # the charset/casing/+xml-dialect bypasses). + if folder_paths.is_dangerous_content_type(content_type): content_type = "application/octet-stream" + disposition = "attachment" safe_name = (filename or "").replace("\r", "").replace("\n", "") encoded = urllib.parse.quote(safe_name) diff --git a/app/model_manager.py b/app/model_manager.py index 8f6e34b33..b0329ce17 100644 --- a/app/model_manager.py +++ b/app/model_manager.py @@ -50,21 +50,45 @@ class ModelFileManager: @routes.get("/experiment/models/preview/{folder}/{path_index}/{filename:.*}") async def get_model_preview(request): folder_name = request.match_info.get("folder", None) - path_index = int(request.match_info.get("path_index", None)) filename = request.match_info.get("filename", None) if folder_name not in folder_paths.folder_names_and_paths: return web.Response(status=404) + # The "{filename:.*}" capture also matches the empty string, which + # would resolve to the folder itself; reject it explicitly. + if not filename: + return web.Response(status=400) + + try: + path_index = int(request.match_info.get("path_index", None)) + except (TypeError, ValueError): + return web.Response(status=400) + folders = folder_paths.folder_names_and_paths[folder_name] + if path_index < 0 or path_index >= len(folders[0]): + return web.Response(status=404) folder = folders[0][path_index] - full_filename = os.path.join(folder, filename) + full_filename = os.path.normpath(os.path.join(folder, filename)) + + # Prevent path traversal: the requested file must stay within the + # configured model folder. `filename` is an unrestricted ".*" capture, + # so values like "../../../../etc/passwd" would otherwise escape it. + if not folder_paths.is_within_directory(folder, full_filename): + return web.Response(status=403) previews = self.get_model_previews(full_filename) default_preview = previews[0] if len(previews) > 0 else None if default_preview is None or (isinstance(default_preview, str) and not os.path.isfile(default_preview)): return web.Response(status=404) + # The preview is selected by a glob inside get_model_previews, so a + # companion file (e.g. "model.preview.png") could itself be a symlink + # resolving outside the model folder. Re-validate the file actually + # opened: is_within_directory realpaths it, catching symlink escape. + if isinstance(default_preview, str) and not folder_paths.is_within_directory(folder, default_preview): + return web.Response(status=403) + try: with Image.open(default_preview) as img: img_bytes = BytesIO() diff --git a/app/user_manager.py b/app/user_manager.py index 7b11e381c..de261ad39 100644 --- a/app/user_manager.py +++ b/app/user_manager.py @@ -6,6 +6,7 @@ import glob import shutil import logging import tempfile +import mimetypes from aiohttp import web from urllib import parse from comfy.cli_args import args @@ -336,7 +337,20 @@ class UserManager(): if not isinstance(path, str): return path - return web.FileResponse(path) + # User data files are arbitrary user-supplied content and are never + # meant to render inline. Disable MIME sniffing and force a download + # so uploaded markup/scripts can't execute in the app origin (stored + # XSS). Content-Disposition: attachment is the load-bearing guard; + # the content-type override and nosniff are defence in depth. + content_type = mimetypes.guess_type(path)[0] or 'application/octet-stream' + if folder_paths.is_dangerous_content_type(content_type): + content_type = 'application/octet-stream' + + return web.FileResponse(path, headers={ + "Content-Type": content_type, + "X-Content-Type-Options": "nosniff", + "Content-Disposition": "attachment", + }) @routes.post("/userdata/{file}") async def post_userdata(request): diff --git a/folder_paths.py b/folder_paths.py index 7304e1b73..ee048b0f2 100644 --- a/folder_paths.py +++ b/folder_paths.py @@ -264,6 +264,59 @@ def annotated_filepath(name: str) -> tuple[str, str | None]: return name, base_dir +# Content types a browser may execute or render inline. File endpoints that +# serve user-controlled content must force these to download (and ideally set +# Content-Disposition: attachment) to avoid stored XSS. Centralised here so the +# /view and /userdata handlers can't drift apart. mimetypes.guess_type may +# return either the text/* or application/* spelling depending on platform, so +# both are listed. +DANGEROUS_CONTENT_TYPES = { + 'text/html', 'text/html-sandboxed', 'application/xhtml+xml', + 'text/javascript', 'application/javascript', 'application/x-javascript', + 'application/ecmascript', 'text/css', + 'image/svg+xml', 'application/xml', 'text/xml', + # message/rfc822 (.mht/.mhtml) can carry script in some browsers. + 'message/rfc822', +} + + +def is_dangerous_content_type(content_type: str | None) -> bool: + """Return True if a browser may execute or render `content_type` inline. + + Normalises before matching so the check can't be slipped past with a + charset/boundary parameter (``text/html; charset=utf-8``) or casing + (``TEXT/HTML``). Any XML dialect (``*+xml`` or ``*/xml``) is treated as + dangerous because XML can carry inline script via stylesheet/entity tricks, + which also covers the ``application/{xslt,rss,atom,rdf}+xml`` family without + enumerating each one. Endpoints serving user-controlled content should route + a dangerous type to ``application/octet-stream`` + ``Content-Disposition: + attachment`` + ``X-Content-Type-Options: nosniff``. + """ + if not content_type: + return False + normalized = content_type.split(';', 1)[0].strip().lower() + if normalized in DANGEROUS_CONTENT_TYPES: + return True + return normalized.endswith('+xml') or normalized.endswith('/xml') + + +def is_within_directory(directory: str, target: str) -> bool: + """Return True if `target` resolves to a path inside `directory`. + + Uses realpath on both operands so that a symlink placed inside `directory` + that points elsewhere cannot escape the containment check at open time. + """ + try: + directory = os.path.realpath(directory) + target = os.path.realpath(target) + return os.path.commonpath((directory, target)) == directory + except ValueError: + # ValueError is raised by realpath() on a path with an embedded null + # byte, and by commonpath() on Windows when the paths are on different + # drives. In either case the target is not safely within the directory. + return False + + def get_annotated_filepath(name: str, default_dir: str | None=None) -> str: name, base_dir = annotated_filepath(name) @@ -273,7 +326,12 @@ def get_annotated_filepath(name: str, default_dir: str | None=None) -> str: else: base_dir = get_input_directory() # fallback path - return os.path.join(base_dir, name) + filepath = os.path.abspath(os.path.join(base_dir, name)) + # Prevent path traversal: the resolved path must stay within base_dir. + # repr() the name in the message so a crafted value can't inject log lines. + if not is_within_directory(base_dir, filepath): + raise ValueError("Invalid file path: {!r}".format(name)) + return filepath def exists_annotated_filepath(name) -> bool: @@ -282,7 +340,10 @@ def exists_annotated_filepath(name) -> bool: if base_dir is None: base_dir = get_input_directory() # fallback path - filepath = os.path.join(base_dir, name) + filepath = os.path.abspath(os.path.join(base_dir, name)) + # Treat traversal attempts as non-existent rather than probing the filesystem. + if not is_within_directory(base_dir, filepath): + return False return os.path.exists(filepath) diff --git a/server.py b/server.py index 361850f38..461ebe2f6 100644 --- a/server.py +++ b/server.py @@ -127,6 +127,7 @@ def create_cors_middleware(allowed_origin: str): return cors_middleware + def is_loopback(host): if host is None: return False @@ -616,15 +617,30 @@ class PromptServer(): or 'application/octet-stream' ) - # For security, force certain mimetypes to download instead of display - if content_type in {'text/html', 'text/html-sandboxed', 'application/xhtml+xml', 'text/javascript', 'text/css'}: - content_type = 'application/octet-stream' # Forces download + # For security, force renderable/active types (HTML, JS, + # CSS, SVG, XML — anything that can carry inline ' + files = {"file": ("evil.svg", svg, "image/svg+xml")} + form_data = { + "tags": json.dumps(["models", "checkpoints", "unit-tests", "svgxss"]), + "name": "evil.svg", + } + up = http.post(api_base + "/api/assets", files=files, data=form_data, timeout=120) + body = up.json() + assert up.status_code in (200, 201), body + aid = body["id"] + try: + r = http.get(f"{api_base}/api/assets/{aid}/content?disposition=inline", timeout=120) + r.content + assert r.status_code == 200 + ct = r.headers.get("Content-Type", "").lower() + cd = r.headers.get("Content-Disposition", "").lower() + assert "svg" not in ct, f"SVG served with a renderable content type: {ct!r}" + assert ct.startswith("application/octet-stream"), f"expected octet-stream, got {ct!r}" + assert "attachment" in cd, f"inline disposition not overridden to attachment: {cd!r}" + assert r.headers.get("X-Content-Type-Options", "").lower() == "nosniff" + finally: + with contextlib.suppress(Exception): + http.delete(f"{api_base}/api/assets/{aid}", timeout=30) + + def test_download_attachment_and_inline(http: requests.Session, api_base: str, seeded_asset: dict): aid = seeded_asset["id"] diff --git a/tests-unit/comfy_test/folder_path_test.py b/tests-unit/comfy_test/folder_path_test.py index 775e15c36..3b398e60b 100644 --- a/tests-unit/comfy_test/folder_path_test.py +++ b/tests-unit/comfy_test/folder_path_test.py @@ -53,8 +53,11 @@ def test_annotated_filepath(): def test_get_annotated_filepath(): default_dir = "/default/dir" - assert folder_paths.get_annotated_filepath("test.txt", default_dir) == os.path.join(default_dir, "test.txt") - assert folder_paths.get_annotated_filepath("test.txt [output]") == os.path.join(folder_paths.get_output_directory(), "test.txt") + # get_annotated_filepath now normalizes with os.path.abspath (part of the + # GHSA-779p traversal hardening), so compare against the normalized form — + # on Windows abspath also prepends the current drive letter. + assert folder_paths.get_annotated_filepath("test.txt", default_dir) == os.path.abspath(os.path.join(default_dir, "test.txt")) + assert folder_paths.get_annotated_filepath("test.txt [output]") == os.path.abspath(os.path.join(folder_paths.get_output_directory(), "test.txt")) def test_add_model_folder_path_append(clear_folder_paths): folder_paths.add_model_folder_path("test_folder", "/default/path", is_default=True) diff --git a/tests-unit/security_test/__init__.py b/tests-unit/security_test/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests-unit/security_test/test_ghsa_779p_02_preview_traversal.py b/tests-unit/security_test/test_ghsa_779p_02_preview_traversal.py new file mode 100644 index 000000000..f17fd26ea --- /dev/null +++ b/tests-unit/security_test/test_ghsa_779p_02_preview_traversal.py @@ -0,0 +1,192 @@ +"""CI unit tests for FIX #2 of GHSA-779p-m5rp-r4h4. + +Path traversal / hardening in app/model_manager.py get_model_preview +(route /experiment/models/preview/{folder}/{path_index}/{filename:.*}). + +Reference: https://github.com/Comfy-Org/ComfyUI/security/advisories/GHSA-779p-m5rp-r4h4 +""" +import pytest +import yarl +from io import BytesIO +from PIL import Image +from aiohttp import web +from unittest.mock import patch +from app.model_manager import ModelFileManager + +pytestmark = ( + pytest.mark.asyncio +) # This applies the asyncio mark to all test functions in the module + +@pytest.fixture +def model_manager(): + return ModelFileManager() + +@pytest.fixture +def app(model_manager): + app = web.Application() + routes = web.RouteTableDef() + model_manager.add_routes(routes) + app.add_routes(routes) + return app + + +async def test_legit_preview_returns_200(aiohttp_client, app, tmp_path): + """Sanity: a real preview PNG inside the model folder is served as webp 200.""" + img = Image.new('RGB', (16, 16), color=(255, 0, 128)) + img.save(tmp_path / "test_model.png", format='PNG') + + with patch('folder_paths.folder_names_and_paths', { + 'test_folder': ([str(tmp_path)], None) + }): + client = await aiohttp_client(app) + response = await client.get('/experiment/models/preview/test_folder/0/test_model.png') + + assert response.status == 200 + assert response.content_type == 'image/webp' + + img_bytes = BytesIO(await response.read()) + served = Image.open(img_bytes) + assert served.format + assert served.format.lower() == 'webp' + served.close() + + +async def test_non_integer_path_index_returns_400(aiohttp_client, app, tmp_path): + """A non-integer path_index segment must be rejected with 400.""" + with patch('folder_paths.folder_names_and_paths', { + 'test_folder': ([str(tmp_path)], None) + }): + client = await aiohttp_client(app) + response = await client.get('/experiment/models/preview/test_folder/abc/test_model.png') + + assert response.status == 400 + + +async def test_out_of_range_path_index_returns_404(aiohttp_client, app, tmp_path): + """A path_index beyond the configured folder list must return 404.""" + with patch('folder_paths.folder_names_and_paths', { + 'test_folder': ([str(tmp_path)], None) + }): + client = await aiohttp_client(app) + response = await client.get('/experiment/models/preview/test_folder/99/test_model.png') + + assert response.status == 404 + + +async def test_empty_filename_returns_400(aiohttp_client, app, tmp_path): + """The "{filename:.*}" capture also matches the empty string (trailing + slash). It would resolve to the folder itself and must be rejected with 400.""" + with patch('folder_paths.folder_names_and_paths', { + 'test_folder': ([str(tmp_path)], None) + }): + client = await aiohttp_client(app) + response = await client.get('/experiment/models/preview/test_folder/0/') + + assert response.status == 400 + + +async def test_path_traversal_in_filename_returns_403(aiohttp_client, app, tmp_path): + """Path traversal in {filename} must be rejected with 403 and must NOT read + a file outside the configured model directory. + + GOTCHA: aiohttp/yarl collapses literal ``../`` dot-segments out of the URL + path before it reaches the handler, which would make this test vacuously + pass (the request would hit a different/non-existent route). We percent-encode + the dots and slashes (``%2e%2e%2f``) and send the URL with + ``yarl.URL(..., encoded=True)`` so the bytes survive client-side normalization + untouched; aiohttp's router then percent-decodes them into ``match_info``, + delivering the literal ``../`` traversal to the handler's ``{filename:.*}`` + capture. + + Without the fix the handler computes + ``os.path.normpath(os.path.join(folder, "../../../../etc/hosts"))``, which + escapes ``tmp_path`` and would be passed straight to get_model_previews -> + Image.open, serving bytes from outside the model dir (200/served bytes). The + is_within_directory() containment check is the load-bearing fix that turns + that escape into a 403. + """ + # Sanity-anchor: a legit preview exists inside tmp_path, so a 200 path is + # genuinely reachable — proving the 403 below is the containment check + # firing, not an unrelated 404. + img = Image.new('RGB', (16, 16), color=(255, 0, 128)) + img.save(tmp_path / "test_model.png", format='PNG') + + # Percent-encoded "../../../../etc/hosts" so yarl does not collapse the + # dot-segments before the request leaves the client. + encoded_traversal = '%2e%2e%2f' * 4 + 'etc%2fhosts' + raw_path = '/experiment/models/preview/test_folder/0/' + encoded_traversal + url = yarl.URL(raw_path, encoded=True) + + with patch('folder_paths.folder_names_and_paths', { + 'test_folder': ([str(tmp_path)], None) + }): + client = await aiohttp_client(app) + response = await client.get(url) + + # Confirm the traversal actually reached the handler intact: a 200 here + # would mean either normalization stripped the ``../`` (vacuous pass) or + # the containment check failed open and served outside-dir bytes. + assert response.status == 403, ( + f"expected 403 from is_within_directory() containment check, " + f"got {response.status}; traversal may have been normalized away " + f"or the fix failed open" + ) + body = await response.read() + assert body == b"", "403 response must not carry any file bytes" + + +async def test_symlink_companion_preview_returns_403(aiohttp_client, app, tmp_path): + """A companion preview file is selected by a glob inside get_model_previews + and then opened. If that companion is a symlink whose path is in-dir but + whose target escapes the model folder, it must be rejected with 403 — not + served. The requested path itself stays in-dir (so the first containment + check passes); the load-bearing fix is the SECOND is_within_directory check + on the file actually opened. + """ + model_dir = tmp_path / "models" + model_dir.mkdir() + secret_dir = tmp_path / "secret" + secret_dir.mkdir() + # A real image OUTSIDE the model dir — valid, so without the fix Image.open + # would succeed and its bytes would be served (200). + secret = secret_dir / "secret.png" + Image.new('RGB', (8, 8), color=(0, 0, 0)).save(secret, format='PNG') + # Companion preview, in-dir by name but a symlink escaping the model dir. + # (No real model file is needed — get_model_previews globs companions by + # basename, and omitting a .safetensors avoids the metadata-header read.) + companion = model_dir / "model.preview.png" + try: + companion.symlink_to(secret) + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported on this platform/filesystem") + + with patch('folder_paths.folder_names_and_paths', { + 'test_folder': ([str(model_dir)], None) + }): + client = await aiohttp_client(app) + response = await client.get('/experiment/models/preview/test_folder/0/model.safetensors') + + assert response.status == 403, ( + f"expected 403 — the globbed companion preview is a symlink resolving " + f"outside the model dir and must not be served; got {response.status}" + ) + assert await response.read() == b"" + + +async def test_null_byte_in_filename_no_500(aiohttp_client, app, tmp_path): + """A NUL byte in the filename must yield a clean client rejection, not a 500 + from an uncaught ValueError in is_within_directory's realpath() call.""" + raw_path = '/experiment/models/preview/test_folder/0/' + 'a%00b' + url = yarl.URL(raw_path, encoded=True) + + with patch('folder_paths.folder_names_and_paths', { + 'test_folder': ([str(tmp_path)], None) + }): + client = await aiohttp_client(app) + response = await client.get(url) + + assert response.status != 500, ( + f"NUL byte produced a 500 (uncaught ValueError); expected a clean " + f"4xx rejection, got {response.status}" + ) + assert 400 <= response.status < 500 diff --git a/tests-unit/security_test/test_ghsa_779p_03_annotated_traversal.py b/tests-unit/security_test/test_ghsa_779p_03_annotated_traversal.py new file mode 100644 index 000000000..88102760c --- /dev/null +++ b/tests-unit/security_test/test_ghsa_779p_03_annotated_traversal.py @@ -0,0 +1,165 @@ +"""Security tests for GHSA-779p-m5rp-r4h4 — FIX #3. + +Path traversal in folder_paths.get_annotated_filepath / exists_annotated_filepath, +plus the shared is_within_directory() containment helper. + +These are pure-function tests (no running server). The input/output/temp +directories are pointed at tmp_path via the folder_paths setters, so a crafted +name containing `../`, an absolute path, or a symlink that escapes the base +directory must be rejected. + +Reference: https://github.com/Comfy-Org/ComfyUI/security/advisories/GHSA-779p-m5rp-r4h4 +""" +import os + +import pytest + +import folder_paths +from comfy.options import enable_args_parsing +enable_args_parsing() + + +@pytest.fixture +def sandbox(tmp_path): + """Point folder_paths' input/output/temp dirs at a real temp sandbox. + + Yields the realpath'd base, input, output and temp directories. The original + directory values are restored afterward so tests stay isolated. + """ + base = os.path.realpath(str(tmp_path)) + input_dir = os.path.join(base, "input") + output_dir = os.path.join(base, "output") + temp_dir = os.path.join(base, "temp") + for d in (input_dir, output_dir, temp_dir): + os.makedirs(d, exist_ok=True) + + orig_input = folder_paths.get_input_directory() + orig_output = folder_paths.get_output_directory() + orig_temp = folder_paths.get_temp_directory() + + folder_paths.set_input_directory(input_dir) + folder_paths.set_output_directory(output_dir) + folder_paths.set_temp_directory(temp_dir) + + yield { + "base": base, + "input": input_dir, + "output": output_dir, + "temp": temp_dir, + } + + folder_paths.set_input_directory(orig_input) + folder_paths.set_output_directory(orig_output) + folder_paths.set_temp_directory(orig_temp) + + +# --------------------------------------------------------------------------- +# is_within_directory() — the shared containment helper +# --------------------------------------------------------------------------- + +def test_is_within_directory_legit_child(sandbox): + base = sandbox["input"] + child = os.path.join(base, "sub", "image.png") + assert folder_paths.is_within_directory(base, child) is True + + +def test_is_within_directory_dotdot_escape(sandbox): + base = sandbox["input"] + escape = os.path.join(base, "..", "..", "etc", "passwd") + assert folder_paths.is_within_directory(base, escape) is False + + +def test_is_within_directory_symlink_escape(sandbox): + """A symlink created INSIDE base that points OUTSIDE base must not pass. + + This is the key new hardening: is_within_directory realpath()s both operands, + so a symlink planted in the base directory can't be used to read files + elsewhere. We create a real on-disk symlink and a real secret target to + verify the check actually resolves the link. + """ + base = sandbox["input"] + + # A directory living outside the base, holding a secret file. + outside = os.path.join(sandbox["base"], "outside_secret_dir") + os.makedirs(outside, exist_ok=True) + secret = os.path.join(outside, "secret.txt") + with open(secret, "w") as f: + f.write("top secret") + + # Plant a symlink inside base that points at the outside directory. + # symlink creation can require elevated privileges / Developer Mode on + # Windows, so skip cleanly where it isn't available (same guard as the + # sibling test in test_ghsa_779p_02_preview_traversal.py). + link = os.path.join(base, "escape_link") + try: + os.symlink(outside, link) + except (OSError, NotImplementedError): + pytest.skip("symlinks not supported on this platform/filesystem") + + # Accessing the secret "through" the in-base symlink must be rejected. + target_via_link = os.path.join(link, "secret.txt") + assert folder_paths.is_within_directory(base, target_via_link) is False + + +# --------------------------------------------------------------------------- +# get_annotated_filepath() +# --------------------------------------------------------------------------- + +def test_get_annotated_filepath_legit_name(sandbox): + result = folder_paths.get_annotated_filepath("image.png") + assert result == os.path.join(sandbox["input"], "image.png") + assert folder_paths.is_within_directory(sandbox["input"], result) + + +def test_get_annotated_filepath_input_annotation(sandbox): + result = folder_paths.get_annotated_filepath("image.png [input]") + assert result == os.path.join(sandbox["input"], "image.png") + + +def test_get_annotated_filepath_output_annotation(sandbox): + result = folder_paths.get_annotated_filepath("image.png [output]") + assert result == os.path.join(sandbox["output"], "image.png") + + +def test_get_annotated_filepath_temp_annotation(sandbox): + result = folder_paths.get_annotated_filepath("image.png [temp]") + assert result == os.path.join(sandbox["temp"], "image.png") + + +def test_get_annotated_filepath_dotdot_raises(sandbox): + with pytest.raises(ValueError): + folder_paths.get_annotated_filepath("../etc/passwd") + + +def test_get_annotated_filepath_dotdot_with_annotation_raises(sandbox): + with pytest.raises(ValueError): + folder_paths.get_annotated_filepath("../../etc/passwd [output]") + + +def test_get_annotated_filepath_absolute_escape_raises(sandbox): + with pytest.raises(ValueError): + folder_paths.get_annotated_filepath("/etc/passwd") + + +# --------------------------------------------------------------------------- +# exists_annotated_filepath() +# --------------------------------------------------------------------------- + +def test_exists_annotated_filepath_existing_legit_file(sandbox): + real = os.path.join(sandbox["input"], "real.png") + with open(real, "w") as f: + f.write("data") + assert folder_paths.exists_annotated_filepath("real.png") is True + + +def test_exists_annotated_filepath_traversal_returns_false(sandbox): + """A traversal name must return False without raising and without probing + outside the base directory (must never reach os.path.exists for the escape). + """ + # /etc/passwd exists on POSIX; the function must still report False because + # the resolved path escapes the input directory. + assert folder_paths.exists_annotated_filepath("../../../../../../etc/passwd") is False + + +def test_exists_annotated_filepath_absolute_returns_false(sandbox): + assert folder_paths.exists_annotated_filepath("/etc/passwd") is False diff --git a/tests-unit/security_test/test_ghsa_779p_04_userdata_xss.py b/tests-unit/security_test/test_ghsa_779p_04_userdata_xss.py new file mode 100644 index 000000000..aa1250327 --- /dev/null +++ b/tests-unit/security_test/test_ghsa_779p_04_userdata_xss.py @@ -0,0 +1,147 @@ +""" +CI unit tests for FIX #4 of GHSA-779p-m5rp-r4h4. + +Stored-XSS hardening on GET /userdata/{file} in app/user_manager.py. + +User data files are arbitrary user-supplied content and must never render +inline in the app origin. The getuserdata handler: + - forces Content-Type to application/octet-stream for any type in + folder_paths.DANGEROUS_CONTENT_TYPES (text/html, image/svg+xml, + text/javascript, ...), + - sets X-Content-Type-Options: nosniff, + - sets Content-Disposition: attachment. + +These tests pre-create files in tmp_path and GET them back, asserting the +secure response headers. They mirror the aiohttp_client pattern in +tests-unit/prompt_server_test/user_manager_test.py. +""" + +import pytest +import os +from aiohttp import web +from app.user_manager import UserManager + +pytestmark = ( + pytest.mark.asyncio +) # This applies the asyncio mark to all test functions in the module + + +@pytest.fixture +def user_manager(tmp_path): + um = UserManager() + um.get_request_user_filepath = lambda req, file, **kwargs: os.path.join( + tmp_path, file + ) if file else tmp_path + return um + + +@pytest.fixture +def app(user_manager): + app = web.Application() + routes = web.RouteTableDef() + user_manager.add_routes(routes) + app.add_routes(routes) + return app + + +async def test_html_served_as_octet_stream(aiohttp_client, app, tmp_path): + (tmp_path / "evil.html").write_text( + "" + ) + + client = await aiohttp_client(app) + resp = await client.get("/userdata/evil.html") + + assert resp.status == 200 + ct = resp.headers.get("Content-Type", "") + # The load-bearing assertion: a .html file must NOT be served as text/html. + assert "text/html" not in ct.lower(), ( + f"Content-Type {ct!r} would let a browser render/execute the file (stored XSS)." + ) + assert ct == "application/octet-stream" + assert resp.headers.get("X-Content-Type-Options") == "nosniff" + assert "attachment" in resp.headers.get("Content-Disposition", "") + + +async def test_svg_served_as_octet_stream(aiohttp_client, app, tmp_path): + (tmp_path / "evil.svg").write_text( + '' + '' + '' + "" + ) + + client = await aiohttp_client(app) + resp = await client.get("/userdata/evil.svg") + + assert resp.status == 200 + ct = resp.headers.get("Content-Type", "") + # SVG can carry inline