diff --git a/py/api/images.py b/py/api/images.py index 85b7f31..aef3656 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -40,6 +40,7 @@ try: from ...utils.prompt_inspector import MetadataReadError, read_inspection_file from ...utils.file_reveal import ( OpenerUnavailable, + RevealGate, RevealTargetChanged, is_local_request, reveal_executor, @@ -55,6 +56,7 @@ except ImportError: from utils.prompt_inspector import MetadataReadError, read_inspection_file from utils.file_reveal import ( OpenerUnavailable, + RevealGate, RevealTargetChanged, is_local_request, reveal_executor, @@ -2222,6 +2224,8 @@ class ImageRoutesMixin: # Swapped by tests and the disposable browser server; never a shell. _reveal_opener = staticmethod(reveal_in_file_manager) + # One per process (a class attribute): single flight plus a cooldown. + _reveal_gate = RevealGate() @staticmethod def _reveal_path(relative: str, root: Path) -> Path | None: @@ -2235,11 +2239,22 @@ class ImageRoutesMixin: async def reveal_image(self, request): """POST ``{root_index, root_scope, path}``: show the file in its folder. - Local-only (no proxy header, loopback client and Host): the browser - must run on the computer whose file manager opens. + Local-only (a same-origin browser request from a loopback client, no + forwarding header) and JSON-only: the browser must run on the + computer whose file manager opens. One reveal at a time (429 busy). """ - if not is_local_request(request.remote, request.headers): + if not is_local_request(request.remote, request.headers, request.scheme): return _not_local() + if request.content_type != "application/json": + # A non-JSON POST is a CORS "simple" request with no preflight. + return web.json_response( + { + "success": False, + "code": "unsupported-media-type", + "error": "Send the request as application/json.", + }, + status=415, + ) data, error = await _json_body_or_error(request) if error is not None: return error @@ -2256,7 +2271,31 @@ class ImageRoutesMixin: root = roots[index] if Path(relative).suffix.lower() not in SERVABLE_EXTENSIONS: return _forbidden("Only media files can be shown") - path = await self._run_in_executor(self._reveal_path, relative, root) + gate = self._reveal_gate + if not gate.try_enter(): + return web.json_response( + { + "success": False, + "code": "busy", + "error": "The folder is already opening.", + }, + status=429, + ) + try: + return await self._reveal(relative, root) + finally: + gate.leave() + + async def _reveal(self, relative: str, root: Path): + """Resolve and open, inside the reveal gate.""" + try: + path = await self._run_in_executor(self._reveal_path, relative, root) + except (OSError, ValueError) as exc: + # ENAMETOOLONG and the like: the client's path is not logged. + self.logger.debug( + "Show in folder could not resolve: %s", type(exc).__name__ + ) + return _reveal_not_found() if path is None: return _reveal_not_found() try: @@ -2274,8 +2313,11 @@ class ImageRoutesMixin: }, status=501, ) - except Exception: - self.logger.exception("Show in folder failed") + except Exception as exc: + # The exception text can hold the path: only its type at error + # level, the traceback at debug. + self.logger.error("Show in folder failed: %s", type(exc).__name__) + self.logger.debug("Show in folder failure", exc_info=True) return web.json_response( {"success": False, "error": "The folder could not be opened."}, status=500, diff --git a/tests/browser/server.py b/tests/browser/server.py index 75a5b5e..53333bc 100644 --- a/tests/browser/server.py +++ b/tests/browser/server.py @@ -86,7 +86,7 @@ def register_reveal_stub(routes, api, root: Path): The API's opener records the resolved path; ``POST /__test__/reveal`` sets the next outcome and ``GET /__test__/reveal`` lists the opened paths. """ - from utils.file_reveal import OpenerUnavailable + from utils.file_reveal import OpenerUnavailable, RevealGate state = {"outcome": "opened", "opened": []} @@ -98,6 +98,8 @@ def register_reveal_stub(routes, api, root: Path): state["opened"].append(Path(path).relative_to(root.resolve()).as_posix()) api._reveal_opener = opener + # Each click gets an answer here; the cooldown is a unit-tested default. + api._reveal_gate = RevealGate(cooldown=0) @routes.post("/__test__/reveal") async def set_reveal_outcome(request: web.Request) -> web.Response: diff --git a/tests/test_image_reveal.py b/tests/test_image_reveal.py index abb703e..2d0af14 100644 --- a/tests/test_image_reveal.py +++ b/tests/test_image_reveal.py @@ -5,6 +5,8 @@ No test launches a file manager: ``subprocess.Popen``, ``sys.platform`` and """ import ctypes +import json +import logging import os import subprocess import sys @@ -30,53 +32,132 @@ from utils.file_reveal import ( ) REVEAL = "/prompt_manager/images/reveal" -PROXY_HEADERS = ("Forwarded", "X-Forwarded-For", "X-Real-IP", "X-Forwarded-Host") +PROXY_HEADERS = ( + "Forwarded", + "Via", + "X-Forwarded-For", + "X-Forwarded-Host", + "X-Forwarded-Proto", + "X-Forwarded-Port", + "X-Forwarded-Server", + "X-Real-IP", + "X-Original-Forwarded-For", + "X-Original-Host", + "CF-Connecting-IP", + "True-Client-IP", + "X-Client-IP", + "X-Cluster-Client-IP", + "Fastly-Client-IP", +) +ORIGIN = "http://127.0.0.1:8188" def headers(**values): - base = {"Host": "127.0.0.1:8188"} + base = {"Host": "127.0.0.1:8188", "Origin": ORIGIN} base.update({key.replace("_", "-"): value for key, value in values.items()}) - return CIMultiDict(base) + return CIMultiDict({key: value for key, value in base.items() if value is not None}) class TestIsLocalRequest(unittest.TestCase): - def test_loopback_clients_by_loopback_name_are_local(self): - for remote, host in ( - ("127.0.0.1", "127.0.0.1:8188"), - ("127.8.9.10", "localhost:8188"), - ("::1", "[::1]:8188"), - ("::ffff:127.0.0.1", "comfy.localhost"), + def test_same_origin_loopback_clients_by_loopback_name_are_local(self): + for remote, host, origin in ( + ("127.0.0.1", "127.0.0.1:8188", ORIGIN), + ("127.8.9.10", "localhost:8188", "http://localhost:8188"), + ("::1", "[::1]:8188", "http://[::1]:8188"), + ("::ffff:127.0.0.1", "comfy.localhost", "http://comfy.localhost"), + ("127.0.0.1", "LOCALHOST:8188", "http://localhost:8188"), + ("127.0.0.1", "localhost", "http://localhost:80"), ): with self.subTest(remote=remote, host=host): - self.assertTrue(is_local_request(remote, headers(Host=host))) + self.assertTrue( + is_local_request(remote, headers(Host=host, Origin=origin)) + ) + self.assertTrue( + is_local_request( + "127.0.0.1", + headers(Host="localhost", Origin="https://localhost:443"), + "https", + ) + ) def test_non_loopback_or_unknown_remote_is_not_local(self): for remote in ("192.168.1.5", "10.0.0.1", "::2", "fe80::1%eth0", "", None): with self.subTest(remote=remote): self.assertFalse(is_local_request(remote, headers())) - def test_any_proxy_header_makes_loopback_remote(self): - for name in PROXY_HEADERS: + def test_any_forwarding_header_makes_loopback_remote(self): + for name in (*PROXY_HEADERS, "X-Forwarded-Anything", "X-Original-URI"): for spelling in (name, name.lower()): with self.subTest(header=spelling): request_headers = headers() request_headers[spelling] = "127.0.0.1" self.assertFalse(is_local_request("127.0.0.1", request_headers)) - def test_foreign_host_or_origin_is_not_local(self): - # A DNS-rebinding page reaches 127.0.0.1 under its own name. - for host in ("evil.example:8188", "", "[bad"): - with self.subTest(host=host): - self.assertFalse(is_local_request("127.0.0.1", headers(Host=host))) - self.assertFalse( - is_local_request("127.0.0.1", CIMultiDict({"Origin": "http://localhost"})) - ) - for origin in ("http://evil.example:8188", "null", "http://[bad"): + def test_origin_is_required_and_must_be_the_request_origin(self): + # A header-less proxy or curl leaves Origin out; a page on another + # loopback port or scheme sends its own (security review M1, L2). + self.assertFalse(is_local_request("127.0.0.1", headers(Origin=None))) + for origin in ( + "", + "null", + "http://localhost:3000", + "http://127.0.0.1:3000", + "http://127.0.0.1", + "https://127.0.0.1:8188", + "http://localhost:8188", + "http://foo.localhost:8188", + "http://evil.example:8188", + "http://[bad", + "http://127.0.0.1:8188/path", + "http://user@127.0.0.1:8188", + "file://127.0.0.1:8188", + ): with self.subTest(origin=origin): self.assertFalse(is_local_request("127.0.0.1", headers(Origin=origin))) + self.assertFalse(is_local_request("127.0.0.1", headers(Origin=ORIGIN), "https")) + + def test_sec_fetch_site_must_be_same_origin_when_sent(self): self.assertTrue( - is_local_request("127.0.0.1", headers(Origin="http://127.0.0.1:8188")) + is_local_request("127.0.0.1", headers(Sec_Fetch_Site="same-origin")) ) + for site in ("same-site", "cross-site", "none", ""): + with self.subTest(site=site): + self.assertFalse( + is_local_request("127.0.0.1", headers(Sec_Fetch_Site=site)) + ) + + def test_foreign_host_is_not_local(self): + # A DNS-rebinding page reaches 127.0.0.1 under its own name. + for host in ("evil.example:8188", "", "[bad", "127.0.0.1:bad"): + with self.subTest(host=host): + self.assertFalse( + is_local_request( + "127.0.0.1", headers(Host=host, Origin=f"http://{host}") + ) + ) + self.assertFalse(is_local_request("127.0.0.1", CIMultiDict({"Origin": ORIGIN}))) + + +class TestRevealGate(unittest.TestCase): + def test_one_reveal_at_a_time_then_a_cooldown(self): + now = [100.0] + gate = file_reveal.RevealGate(cooldown=1.0, clock=lambda: now[0]) + self.assertTrue(gate.try_enter()) + self.assertFalse(gate.try_enter()) + now[0] += 5 + self.assertFalse(gate.try_enter()) + gate.leave() + now[0] += 0.5 + self.assertFalse(gate.try_enter()) + now[0] += 0.5 + self.assertTrue(gate.try_enter()) + + def test_zero_cooldown_only_blocks_while_busy(self): + gate = file_reveal.RevealGate(cooldown=0) + self.assertTrue(gate.try_enter()) + self.assertFalse(gate.try_enter()) + gate.leave() + self.assertTrue(gate.try_enter()) class TestRevealInFileManager(unittest.TestCase): @@ -335,6 +416,15 @@ class TestRevealInFileManager(unittest.TestCase): shell.assert_not_called() self.popen.assert_not_called() + def test_a_path_on_another_drive_is_not_contained(self): + # Windows: commonpath raises ValueError across drives. + with ( + patch.object(file_reveal.os.path, "commonpath", side_effect=ValueError), + self.assertRaises(file_reveal.RevealTargetChanged), + ): + self.reveal() + self.popen.assert_not_called() + class TestReaping(unittest.TestCase): def test_reap_waits_on_a_daemon_thread(self): @@ -472,14 +562,21 @@ class TestRevealRoute(ImageAPITestCase): self.roots.append(root) self.api._reveal_opener = opener + self.api._reveal_gate = file_reveal.RevealGate(cooldown=0) return app + def origin(self): + return str(self.client.make_url("/").origin()) + def scope(self, index=0): roots = self.api._get_all_output_dirs() return self.api._gallery_root_scope(roots[index], index) async def post(self, body, **request_headers): - response = await self.client.post(REVEAL, json=body, headers=request_headers) + # Like the workspace's fetch: a same-origin JSON POST. + sent = {"Origin": self.origin(), **request_headers} + sent = {key: value for key, value in sent.items() if value is not None} + response = await self.client.post(REVEAL, json=body, headers=sent) return response.status, await response.json() def body(self, path="sub/a.png", **changes): @@ -523,6 +620,105 @@ class TestRevealRoute(ImageAPITestCase): self.assertEqual((status, data["code"]), (403, "not-local")) self.assertEqual(self.opened, []) + async def test_missing_or_foreign_origin_is_not_local(self): + make_png(self.output_dir / "sub" / "a.png") + port = self.client.make_url("/").port + for origin in (None, f"http://127.0.0.1:{port + 1}", "null"): + with self.subTest(origin=origin): + status, data = await self.post(self.body(), Origin=origin) + self.assertEqual((status, data["code"]), (403, "not-local")) + status, data = await self.post(self.body(), **{"Sec-Fetch-Site": "same-site"}) + self.assertEqual((status, data["code"]), (403, "not-local")) + status, _ = await self.post(self.body(), **{"Sec-Fetch-Site": "same-origin"}) + self.assertEqual(status, 200) + self.assertEqual(len(self.opened), 1) + + async def test_only_json_bodies_are_accepted(self): + # text/plain would be a CORS "simple" request with no preflight (L2). + make_png(self.output_dir / "sub" / "a.png") + body = json.dumps(self.body()) + for content_type in ("text/plain", "application/x-www-form-urlencoded", None): + with self.subTest(content_type=content_type): + sent = {"Origin": self.origin()} + if content_type: + sent["Content-Type"] = content_type + response = await self.client.post( + REVEAL, data=body.encode(), headers=sent + ) + self.assertEqual(response.status, 415) + data = await response.json() + self.assertEqual( + (data["success"], data["code"]), (False, "unsupported-media-type") + ) + self.assertEqual(self.opened, []) + + async def test_a_reveal_in_progress_or_cooling_down_is_busy(self): + make_png(self.output_dir / "sub" / "a.png") + gate = self.api._reveal_gate + self.assertTrue(gate.try_enter()) + status, data = await self.post(self.body()) + self.assertEqual(status, 429) + self.assertEqual(data, {**data, "success": False, "code": "busy"}) + gate.leave() + now = [50.0] + self.api._reveal_gate = file_reveal.RevealGate( + cooldown=1.0, clock=lambda: now[0] + ) + statuses = [(await self.post(self.body()))[0] for _ in range(2)] + now[0] += 1.0 + statuses.append((await self.post(self.body()))[0]) + self.assertEqual(statuses, [200, 429, 200]) + self.assertEqual(len(self.opened), 2) + + async def test_a_failed_reveal_releases_the_gate(self): + make_png(self.output_dir / "sub" / "a.png") + self.outcome = OSError("cannot start") + status, _ = await self.post(self.body()) + self.assertEqual(status, 500) + self.outcome = None + status, _ = await self.post(self.body()) + self.assertEqual(status, 200) + + async def test_default_gate_is_one_per_process_with_a_cooldown(self): + gate = type(self.api)._reveal_gate + self.assertIsInstance(gate, file_reveal.RevealGate) + self.assertEqual(gate.cooldown, file_reveal.REVEAL_COOLDOWN) + self.assertGreaterEqual(file_reveal.REVEAL_COOLDOWN, 1.0) + + async def test_resolution_errors_are_404_and_never_log_the_path(self): + # ENAMETOOLONG and friends must not escape as a non-JSON 500 (L3). + make_png(self.output_dir / "sub" / "a.png") + secret = "sub/" + "n" * 300 + "/a.png" + with self.assertLogs(self.api.logger, level="DEBUG") as logs: + self.api.logger.debug("start") + status, data = await self.post(self.body(path=secret)) + self.assertEqual(status, 404) + self.assertEqual(data, {"success": False, "error": "Image file not found"}) + with patch.object( + type(self.api), + "_reveal_path", + staticmethod(MagicMock(side_effect=OSError(36, "too long", secret))), + ): + status, data = await self.post(self.body(path=secret)) + self.assertEqual(status, 404) + self.assertIs(data["success"], False) + for record in logs.records: + if record.levelno >= logging.WARNING: + self.assertNotIn("n" * 300, record.getMessage()) + self.assertEqual(self.opened, []) + + async def test_start_failure_never_logs_the_path_at_error_level(self): + make_png(self.output_dir / "sub" / "a.png") + self.outcome = OSError(2, "No such file", str(self.output_dir / "sub")) + with self.assertLogs(self.api.logger, level="DEBUG") as logs: + status, _ = await self.post(self.body()) + self.assertEqual(status, 500) + errors = [r for r in logs.records if r.levelno >= logging.ERROR] + self.assertTrue(errors) + for record in errors: + self.assertNotIn(str(self.output_dir), record.getMessage()) + self.assertIsNone(record.exc_info) + async def test_changed_or_missing_scope_is_rejected(self): make_png(self.output_dir / "sub" / "a.png") status, data = await self.post(self.body(root_scope="0" * 64)) @@ -554,7 +750,11 @@ class TestRevealRoute(ImageAPITestCase): with self.subTest(changes=changes): status, data = await self.post(self.body(**changes)) self.assertEqual(status, 400, data) - response = await self.client.post(REVEAL, data="[1]") + response = await self.client.post( + REVEAL, + data="[1]", + headers={"Origin": self.origin(), "Content-Type": "application/json"}, + ) self.assertEqual(response.status, 400) self.assertEqual(self.opened, []) diff --git a/utils/file_reveal.py b/utils/file_reveal.py index 912055f..916191a 100644 --- a/utils/file_reveal.py +++ b/utils/file_reveal.py @@ -1,6 +1,7 @@ """Show one gallery file in this computer's file manager (TESTING.md D38). -The route that calls this is local-only (``is_local_request``) and hands in a +The route that calls this is local-only (``is_local_request``), single-flight +(``RevealGate``) and hands in a path it has already resolved inside a gallery root. Windows goes through the shell API (no command line exists to parse); macOS and Linux openers run as an argument list with an absolute executable, no shell and no inherited streams, @@ -16,14 +17,32 @@ import shutil import subprocess # nosec B404 import sys import threading +import time from collections.abc import Mapping from concurrent.futures import ThreadPoolExecutor from pathlib import Path from urllib.parse import urlsplit # A local reverse proxy makes every client look like loopback, so a request -# that carries any of these counts as remote. -PROXY_HEADERS = ("Forwarded", "X-Forwarded-For", "X-Real-IP", "X-Forwarded-Host") +# that carries any forwarding-style header counts as remote: these names, and +# every X-Forwarded-*, X-Original-* and *-Client-IP header. +FORWARDING_HEADERS = frozenset( + ( + "forwarded", + "via", + "x-real-ip", + "cf-connecting-ip", + "true-client-ip", + "x-client-ip", + "x-cluster-client-ip", + "fastly-client-ip", + ) +) +FORWARDING_PREFIXES = ("x-forwarded-", "x-original-") +FORWARDING_SUFFIXES = ("-client-ip",) +DEFAULT_PORTS = {"http": 80, "https": 443} +# One reveal at a time per process, then a pause before the next. +REVEAL_COOLDOWN = 1.0 # dbus-send waits this long for the FileManager1 reply, so its exit status # says whether a file manager really took the request. DBUS_REPLY_TIMEOUT_MS = 2000 @@ -88,43 +107,116 @@ def _loopback_address(value: str | None) -> bool: return (mapped or address).is_loopback -def _loopback_host(host: str | None) -> bool: - """True for a Host or Origin host naming this computer. +def _loopback_host(host: str) -> bool: + """True for a Host name naming this computer. Rejects every other name, so a DNS-rebinding page whose name now resolves to 127.0.0.1 still reads as remote. """ - if not host: - return False host = host.rstrip(".").lower() if host == "localhost" or host.endswith(".localhost"): return True return _loopback_address(host.strip("[]")) -def _authority_host(authority: str) -> str | None: +def _authority(authority: str) -> tuple[str, int | None] | None: + """``(host, port)`` of a Host value, or None when it does not parse.""" try: - return urlsplit(f"//{authority}").hostname + parts = urlsplit(f"//{authority}") + host, port = parts.hostname, parts.port except ValueError: return None + return (host, port) if host else None -def is_local_request(remote: str | None, headers: Mapping) -> bool: - """True only for a direct loopback client addressing this computer by a - loopback name, with no proxy header and no foreign Origin.""" - if any(name in headers for name in PROXY_HEADERS): +def _forwarded(headers: Mapping) -> bool: + for name in headers.keys(): + name = name.lower() + if ( + name in FORWARDING_HEADERS + or name.startswith(FORWARDING_PREFIXES) + or name.endswith(FORWARDING_SUFFIXES) + ): + return True + return False + + +def _same_origin(origin: str, scheme: str, host: tuple[str, int | None]) -> bool: + """True when *origin* is exactly this request's scheme://host:port.""" + default = DEFAULT_PORTS.get(scheme) + if default is None: + return False + try: + parts = urlsplit(origin) + origin_host, origin_port = parts.hostname, parts.port + except ValueError: + return False + if ( + parts.scheme != scheme + or not origin_host + or parts.username is not None + or parts.password is not None + or parts.path + or parts.query + or parts.fragment + ): + return False + host_name, host_port = host + return (origin_host.rstrip("."), origin_port or default) == ( + host_name.rstrip("."), + host_port or default, + ) + + +def is_local_request(remote: str | None, headers: Mapping, scheme="http") -> bool: + """True only for a same-origin browser request from this computer. + + That is: no forwarding-style header, a loopback client, a loopback name + in Host, an ``Origin`` that is exactly this request's scheme://host:port + (browsers send it on every fetch POST, so a missing one is refused), and + ``Sec-Fetch-Site: same-origin`` whenever that header is sent. + """ + if _forwarded(headers): return False if not _loopback_address(remote): return False - if not _loopback_host(_authority_host(headers.get("Host", ""))): + host = _authority(headers.get("Host", "")) + if host is None or not _loopback_host(host[0]): return False origin = headers.get("Origin") - if origin is None: - return True - try: - return _loopback_host(urlsplit(origin).hostname) - except ValueError: + if not origin or not _same_origin(origin, scheme, host): return False + site = headers.get("Sec-Fetch-Site") + return site is None or site == "same-origin" + + +class RevealGate: + """Single flight plus a cooldown: at most one reveal runs per process, + and the next may start only *cooldown* seconds after it finished.""" + + def __init__(self, cooldown: float = REVEAL_COOLDOWN, clock=time.monotonic): + self.cooldown = cooldown + self._clock = clock + self._lock = threading.Lock() + self._busy = False + self._finished: float | None = None + + def try_enter(self) -> bool: + with self._lock: + if self._busy: + return False + if ( + self._finished is not None + and self._clock() - self._finished < self.cooldown + ): + return False + self._busy = True + return True + + def leave(self) -> None: + with self._lock: + self._busy = False + self._finished = self._clock() def _executable(name: str) -> str | None: