diff --git a/py/api/images.py b/py/api/images.py index 452d5d4..85b7f31 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -40,7 +40,9 @@ try: from ...utils.prompt_inspector import MetadataReadError, read_inspection_file from ...utils.file_reveal import ( OpenerUnavailable, + RevealTargetChanged, is_local_request, + reveal_executor, reveal_in_file_manager, ) except ImportError: @@ -53,7 +55,9 @@ except ImportError: from utils.prompt_inspector import MetadataReadError, read_inspection_file from utils.file_reveal import ( OpenerUnavailable, + RevealTargetChanged, is_local_request, + reveal_executor, reveal_in_file_manager, ) @@ -560,6 +564,12 @@ def _reveal_target(data: Mapping): return (index, scope, relative), None +def _reveal_not_found(): + return web.json_response( + {"success": False, "error": "Image file not found"}, status=404 + ) + + def _not_local(): return web.json_response( { @@ -2248,11 +2258,13 @@ class ImageRoutesMixin: return _forbidden("Only media files can be shown") path = await self._run_in_executor(self._reveal_path, relative, root) if path is None: - return web.json_response( - {"success": False, "error": "Image file not found"}, status=404 - ) + return _reveal_not_found() try: - await self._run_in_executor(self._reveal_opener, path) + await asyncio.get_running_loop().run_in_executor( + reveal_executor(), self._reveal_opener, path, root + ) + except RevealTargetChanged: + return _reveal_not_found() except OpenerUnavailable: return web.json_response( { diff --git a/tests/browser/server.py b/tests/browser/server.py index 9c8ba02..75a5b5e 100644 --- a/tests/browser/server.py +++ b/tests/browser/server.py @@ -90,7 +90,7 @@ def register_reveal_stub(routes, api, root: Path): state = {"outcome": "opened", "opened": []} - def opener(path: Path) -> None: + def opener(path: Path, gallery_root: Path) -> None: if state["outcome"] == "unavailable": raise OpenerUnavailable() if state["outcome"] == "fail": diff --git a/tests/test_image_reveal.py b/tests/test_image_reveal.py index 14882cb..abb703e 100644 --- a/tests/test_image_reveal.py +++ b/tests/test_image_reveal.py @@ -8,6 +8,7 @@ import ctypes import os import subprocess import sys +import tempfile import types import unittest from pathlib import Path @@ -80,16 +81,25 @@ class TestIsLocalRequest(unittest.TestCase): class TestRevealInFileManager(unittest.TestCase): def setUp(self): - self.path = Path(os.path.abspath(os.path.join(os.sep, "out", "sub", "a.png"))) + temp = tempfile.TemporaryDirectory(ignore_cleanup_errors=True) + self.addCleanup(temp.cleanup) + self.root = Path(os.path.realpath(temp.name)) / "out" + self.path = self.root / "sub" / "a.png" + self.path.parent.mkdir(parents=True) + self.path.write_bytes(b"png") self.popen = patch.object(subprocess, "Popen").start() + self.reaped = patch.object(file_reveal, "_reap").start() self.addCleanup(patch.stopall) + def reveal(self, path=None): + reveal_in_file_manager(self.path if path is None else path, self.root) + def which(self, available): - return patch.object( - file_reveal.shutil, - "which", - side_effect=lambda name: available.get(name), - ) + def lookup(name, path=None): + self.assertEqual(path, file_reveal.SYSTEM_PATH) + return available.get(name) + + return patch.object(file_reveal.shutil, "which", side_effect=lookup) def argv(self, call=0): return self.popen.call_args_list[call].args[0] @@ -99,7 +109,9 @@ class TestRevealInFileManager(unittest.TestCase): for stream in ("stdin", "stdout", "stderr"): self.assertIs(options[stream], subprocess.DEVNULL) self.assertIs(options["close_fds"], True) + self.assertIs(options["start_new_session"], True) self.assertNotIn("shell", options) + self.assertEqual(options["env"]["PATH"], file_reveal.SYSTEM_PATH) def windows(self, selected=True, startfile=None): """Windows without Windows: the shell call and os.startfile injected.""" @@ -113,10 +125,12 @@ class TestRevealInFileManager(unittest.TestCase): def test_windows_selects_through_the_shell_api_without_a_process(self): # A comma path splits Explorer's own command line (security review H1); # the shell API takes the path as one value, unchanged. - path = Path(os.path.abspath(os.path.join(os.sep, "out", "a,", "b,c.png"))) + path = self.root / "a," / "b,c.png" + path.parent.mkdir() + path.write_bytes(b"png") select, start, platform = self.windows(selected=True) with select as shell, start as startfile, platform: - reveal_in_file_manager(path) + self.reveal(path) shell.assert_called_once_with(path) startfile.assert_not_called() self.popen.assert_not_called() @@ -124,7 +138,7 @@ class TestRevealInFileManager(unittest.TestCase): def test_windows_falls_back_to_opening_the_folder(self): select, start, platform = self.windows(selected=False) with select as shell, start as startfile, platform: - reveal_in_file_manager(self.path) + self.reveal() shell.assert_called_once_with(self.path) startfile.assert_called_once_with(str(self.path.parent), "explore") self.popen.assert_not_called() @@ -134,7 +148,7 @@ class TestRevealInFileManager(unittest.TestCase): selected=False, startfile=MagicMock(side_effect=OSError("denied")) ) with select, start, platform, self.assertRaises(OSError): - reveal_in_file_manager(self.path) + self.reveal() self.popen.assert_not_called() def test_windows_without_startfile_is_unavailable(self): @@ -144,41 +158,93 @@ class TestRevealInFileManager(unittest.TestCase): patch.object(file_reveal.os, "startfile", None, create=True), self.assertRaises(OpenerUnavailable), ): - reveal_in_file_manager(self.path) + self.reveal() self.popen.assert_not_called() - def test_macos_reveals_with_open(self): + def test_macos_reveals_with_the_system_open(self): with ( patch.object(file_reveal.sys, "platform", "darwin"), - self.which({"open": "/usr/bin/open"}), + patch.object(file_reveal, "_macos_open", return_value="/usr/bin/open"), + self.which({"open": "/planted/open"}), ): - reveal_in_file_manager(self.path) + self.reveal() self.assertEqual(self.argv(), ["/usr/bin/open", "-R", str(self.path)]) self.assert_detached() - self.assertIs(self.popen.call_args.kwargs["start_new_session"], True) self.popen.return_value.wait.assert_not_called() + self.reaped.assert_called_once_with(self.popen.return_value) + + def test_macos_open_is_the_fixed_system_path(self): + self.assertEqual(file_reveal.MACOS_OPEN, "/usr/bin/open") + with patch.object(file_reveal.os.path, "isfile", return_value=False): + self.assertIsNone(file_reveal._macos_open()) + with ( + patch.object(file_reveal.os.path, "isfile", return_value=True), + patch.object(file_reveal.os, "access", return_value=True), + ): + self.assertEqual(file_reveal._macos_open(), "/usr/bin/open") + with ( + patch.object(file_reveal.sys, "platform", "darwin"), + patch.object(file_reveal, "_macos_open", return_value=None), + self.assertRaises(OpenerUnavailable), + ): + self.reveal() + self.popen.assert_not_called() def test_relative_path_lookup_never_counts(self): with ( - patch.object(file_reveal.sys, "platform", "darwin"), - self.which({"open": "./open"}), + patch.object(file_reveal.sys, "platform", "linux"), + self.which({"dbus-send": "./dbus-send", "xdg-open": "bin/xdg-open"}), self.assertRaises(OpenerUnavailable), ): - reveal_in_file_manager(self.path) + self.reveal() self.popen.assert_not_called() - def test_linux_asks_the_file_manager_service_first(self): + def test_children_get_a_minimal_environment(self): + inherited = { + "PATH": "/venv/bin:/usr/bin", + "PYTHONPATH": "/comfy", + "LD_LIBRARY_PATH": "/venv/lib", + "VIRTUAL_ENV": "/venv", + "CIVITAI_API_KEY": "secret", + "HOME": "/home/u", + "DISPLAY": ":0", + "WAYLAND_DISPLAY": "wayland-0", + "XDG_RUNTIME_DIR": "/run/user/1000", + "XDG_CURRENT_DESKTOP": "KDE", + "DBUS_SESSION_BUS_ADDRESS": "unix:path=/run/user/1000/bus", + "LANG": "en_US.UTF-8", + } + with patch.dict(os.environ, inherited, clear=True): + env = file_reveal._child_env() + self.assertEqual( + env, + { + "PATH": file_reveal.SYSTEM_PATH, + "HOME": "/home/u", + "DISPLAY": ":0", + "WAYLAND_DISPLAY": "wayland-0", + "XDG_RUNTIME_DIR": "/run/user/1000", + "XDG_CURRENT_DESKTOP": "KDE", + "DBUS_SESSION_BUS_ADDRESS": "unix:path=/run/user/1000/bus", + "LANG": "en_US.UTF-8", + }, + ) + self.assertNotIn("/venv", file_reveal.SYSTEM_PATH) + + def test_linux_asks_the_file_manager_service_and_waits_for_its_reply(self): self.popen.return_value.wait.return_value = 0 with ( patch.object(file_reveal.sys, "platform", "linux"), self.which({"dbus-send": "/usr/bin/dbus-send", "xdg-open": "/x"}), ): - reveal_in_file_manager(self.path) + self.reveal() self.assertEqual( self.argv(), [ "/usr/bin/dbus-send", "--session", + "--print-reply", + "--reply-timeout=2000", "--dest=org.freedesktop.FileManager1", "--type=method_call", "/org/freedesktop/FileManager1", @@ -188,46 +254,45 @@ class TestRevealInFileManager(unittest.TestCase): ], ) self.assertEqual(self.popen.call_count, 1) + self.popen.return_value.wait.assert_called_once_with( + timeout=file_reveal.DBUS_WAIT + ) self.assert_detached() - - def test_linux_slow_service_start_counts_as_delivered(self): - self.popen.return_value.wait.side_effect = subprocess.TimeoutExpired("d", 2) - with ( - patch.object(file_reveal.sys, "platform", "linux"), - self.which({"dbus-send": "/usr/bin/dbus-send", "xdg-open": "/x"}), - ): - reveal_in_file_manager(self.path) - self.assertEqual(self.popen.call_count, 1) + self.reaped.assert_not_called() def test_linux_falls_back_to_xdg_open_on_the_folder(self): + both = {"dbus-send": "/usr/bin/dbus-send", "xdg-open": "/usr/bin/xdg-open"} cases = { "missing": ({"xdg-open": "/usr/bin/xdg-open"}, None), - "non-zero": ( - {"dbus-send": "/usr/bin/dbus-send", "xdg-open": "/usr/bin/xdg-open"}, - 1, - ), - "start error": ( - {"dbus-send": "/usr/bin/dbus-send", "xdg-open": "/usr/bin/xdg-open"}, - OSError("no bus"), - ), + "no reply": (both, 1), + "start error": (both, OSError("no bus")), + "hung": (both, subprocess.TimeoutExpired("dbus-send", 3)), } for name, (available, outcome) in cases.items(): with self.subTest(name): self.popen.reset_mock(side_effect=True, return_value=True) + self.reaped.reset_mock() + dbus = MagicMock() if isinstance(outcome, OSError): self.popen.side_effect = [outcome, MagicMock()] + elif isinstance(outcome, subprocess.TimeoutExpired): + dbus.wait.side_effect = [outcome, None] + self.popen.side_effect = [dbus, MagicMock()] else: self.popen.return_value.wait.return_value = outcome with ( patch.object(file_reveal.sys, "platform", "linux"), self.which(available), ): - reveal_in_file_manager(self.path) + self.reveal() self.assertEqual( self.popen.call_args.args[0], ["/usr/bin/xdg-open", str(self.path.parent)], ) self.assert_detached(self.popen.call_count - 1) + self.reaped.assert_called_once() + if name == "hung": + dbus.kill.assert_called_once_with() def test_linux_without_any_opener_is_unavailable(self): with ( @@ -235,14 +300,60 @@ class TestRevealInFileManager(unittest.TestCase): self.which({}), self.assertRaises(OpenerUnavailable), ): - reveal_in_file_manager(self.path) + self.reveal() self.popen.assert_not_called() def test_relative_paths_are_refused(self): with self.assertRaises(ValueError): - reveal_in_file_manager(Path("sub/a.png")) + reveal_in_file_manager(Path("sub/a.png"), self.root) self.popen.assert_not_called() + def test_containment_is_rechecked_just_before_launching(self): + # A parent swapped for a symlink after the route resolved the path + # (security review L4) must not open a folder outside the root. + outside = self.root.parent / "elsewhere" + outside.mkdir() + (outside / "a.png").write_bytes(b"png") + (self.root / "sub" / "a.png").unlink() + (self.root / "sub").rmdir() + try: + os.symlink(outside, self.root / "sub", target_is_directory=True) + except (OSError, NotImplementedError): + self.skipTest("symlinks unavailable") + missing = self.root / "gone.png" + folder = self.root / "folder.png" + folder.mkdir() + for path in (self.path, missing, folder): + for platform in ("linux", "darwin", "win32"): + with ( + self.subTest(path=path.name, platform=platform), + patch.object(file_reveal.sys, "platform", platform), + patch.object(file_reveal, "_windows_select") as shell, + self.assertRaises(file_reveal.RevealTargetChanged), + ): + self.reveal(path) + shell.assert_not_called() + self.popen.assert_not_called() + + +class TestReaping(unittest.TestCase): + def test_reap_waits_on_a_daemon_thread(self): + process = MagicMock() + with patch.object(file_reveal.threading, "Thread") as thread: + file_reveal._reap(process) + thread.assert_called_once() + kwargs = thread.call_args.kwargs + self.assertIs(kwargs["target"], process.wait) + self.assertIs(kwargs["daemon"], True) + thread.return_value.start.assert_called_once_with() + + def test_reveal_runs_on_its_own_single_worker(self): + executor = file_reveal.reveal_executor() + self.assertIs(file_reveal.reveal_executor(), executor) + self.assertEqual(executor._max_workers, 1) + name = executor.submit(lambda: file_reveal.threading.current_thread().name) + self.assertTrue(name.result(timeout=5).startswith("pm-reveal")) + class FakeShell: """ole32 and shell32 stand-ins recording the shell calls (H1).""" @@ -350,13 +461,15 @@ class TestWindowsSelect(unittest.TestCase): class TestRevealRoute(ImageAPITestCase): async def get_application(self): app = await super().get_application() - self.opened = [] + self.opened, self.roots, self.threads = [], [], [] self.outcome = None - def opener(path): + def opener(path, root): + self.threads.append(file_reveal.threading.current_thread().name) if self.outcome is not None: raise self.outcome self.opened.append(path) + self.roots.append(root) self.api._reveal_opener = opener return app @@ -377,6 +490,16 @@ class TestRevealRoute(ImageAPITestCase): status, data = await self.post(self.body()) self.assertEqual((status, data), (200, {"success": True})) self.assertEqual(self.opened, [Path(os.path.realpath(target))]) + self.assertEqual(self.roots, [self.output_dir]) + # Its own worker, never ComfyUI's shared default executor (L1). + self.assertTrue(self.threads[0].startswith("pm-reveal"), self.threads) + + async def test_target_changed_before_launch_is_404(self): + make_png(self.output_dir / "sub" / "a.png") + self.outcome = file_reveal.RevealTargetChanged() + status, data = await self.post(self.body()) + self.assertEqual(status, 404) + self.assertEqual(data, {"success": False, "error": "Image file not found"}) async def test_non_loopback_remote_is_not_local(self): make_png(self.output_dir / "sub" / "a.png") diff --git a/utils/file_reveal.py b/utils/file_reveal.py index f8729a1..912055f 100644 --- a/utils/file_reveal.py +++ b/utils/file_reveal.py @@ -15,23 +15,68 @@ import shutil # starts a process (see _windows_select). import subprocess # nosec B404 import sys +import threading 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") -# dbus-send exits at once when it cannot reach the session bus; a start that -# outlives this is treated as delivered and is not waited on further. -DBUS_START_TIMEOUT = 2.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 +# The dbus-send process itself gets a little longer before it is killed. +DBUS_WAIT = 3.0 FILE_MANAGER_DEST = "org.freedesktop.FileManager1" +# Openers are looked up here only, never on ComfyUI's (venv) PATH, and the +# children get this as their PATH. +SYSTEM_PATH = os.pathsep.join( + ("/usr/local/bin", "/usr/bin", "/bin", "/run/current-system/sw/bin") +) +MACOS_OPEN = "/usr/bin/open" +# What a desktop opener needs from ComfyUI's environment; everything else +# (venv PATH, PYTHONPATH, LD_LIBRARY_PATH, API keys) stays behind. +CHILD_ENV_NAMES = ( + "HOME", + "DISPLAY", + "WAYLAND_DISPLAY", + "XDG_RUNTIME_DIR", + "XDG_CURRENT_DESKTOP", + "XDG_SESSION_TYPE", + "XDG_DATA_DIRS", + "XDG_CONFIG_DIRS", + "DBUS_SESSION_BUS_ADDRESS", + "LANG", + "LC_ALL", +) class OpenerUnavailable(Exception): """No file manager can be started on this computer.""" +class RevealTargetChanged(Exception): + """The file is no longer a regular file inside its root.""" + + +_executor_lock = threading.Lock() +_executor: ThreadPoolExecutor | None = None + + +def reveal_executor() -> ThreadPoolExecutor: + """The one worker that runs openers, so a slow file manager or dbus reply + never holds a thread of ComfyUI's shared default executor.""" + global _executor + with _executor_lock: + if _executor is None: + _executor = ThreadPoolExecutor( + max_workers=1, thread_name_prefix="pm-reveal" + ) + return _executor + + def _loopback_address(value: str | None) -> bool: if not value: return False @@ -83,11 +128,48 @@ def is_local_request(remote: str | None, headers: Mapping) -> bool: def _executable(name: str) -> str | None: - """Absolute path of *name* on PATH; a relative PATH entry never counts.""" - found = shutil.which(name) + """Absolute path of *name* on the fixed SYSTEM_PATH, never a relative hit.""" + found = shutil.which(name, path=SYSTEM_PATH) return found if found and os.path.isabs(found) else None +def _macos_open() -> str | None: + """macOS's own ``open``, at its fixed system path.""" + if os.path.isfile(MACOS_OPEN) and os.access(MACOS_OPEN, os.X_OK): + return MACOS_OPEN + return None + + +def _child_env() -> dict[str, str]: + env = {name: os.environ[name] for name in CHILD_ENV_NAMES if name in os.environ} + return {"PATH": SYSTEM_PATH, **env} + + +def _reap(process: subprocess.Popen) -> None: + """Wait for *process* on a daemon thread, so it never lingers as a zombie + and is never waited on by the caller.""" + threading.Thread(target=process.wait, name="pm-reveal-reaper", daemon=True).start() + + +def _contained_file(path: Path, root: Path) -> Path: + """*path*'s real path when it is still a regular file inside *root*. + + Run right before launching: a parent swapped for a symlink since the + route resolved the path must not open a folder outside the root. + """ + real = os.path.realpath(path) + real_root = os.path.realpath(root) + try: + inside = os.path.commonpath( + [os.path.normcase(real), os.path.normcase(real_root)] + ) == os.path.normcase(real_root) + except ValueError: + inside = False + if not inside or real == real_root or not os.path.isfile(real): + raise RevealTargetChanged() + return Path(real) + + # HRESULTs as the signed 32-bit values ctypes.c_long returns. S_OK = 0 S_FALSE = 1 @@ -171,6 +253,7 @@ def _launch(argv: list[str]) -> subprocess.Popen: "close_fds": True, # Its own session, so stopping ComfyUI does not take the window along. "start_new_session": True, + "env": _child_env(), } # POSIX only (Windows uses the shell API): argv[0] is an absolute opener, # the rest are fixed flags and one absolute path inside a gallery root, @@ -179,13 +262,16 @@ def _launch(argv: list[str]) -> subprocess.Popen: def _show_items(path: Path) -> bool: - """Ask the freedesktop FileManager1 service to select *path*.""" + """Ask the freedesktop FileManager1 service to select *path*; True only + when the service replied (``--print-reply`` makes the exit status real).""" dbus = _executable("dbus-send") if dbus is None: return False argv = [ dbus, "--session", + "--print-reply", + f"--reply-timeout={DBUS_REPLY_TIMEOUT_MS}", f"--dest={FILE_MANAGER_DEST}", "--type=method_call", "/org/freedesktop/FileManager1", @@ -198,32 +284,36 @@ def _show_items(path: Path) -> bool: except OSError: return False try: - return process.wait(timeout=DBUS_START_TIMEOUT) == 0 + return process.wait(timeout=DBUS_WAIT) == 0 except subprocess.TimeoutExpired: - return True + process.kill() + process.wait() + return False -def reveal_in_file_manager(path: Path) -> None: - """Open the file manager at *path* (absolute, an existing regular file). +def reveal_in_file_manager(path: Path, root: Path) -> None: + """Open the file manager at *path*, a regular file inside *root*. - Raises OpenerUnavailable when no opener exists and OSError when the one - found cannot start. Never waits on the file manager itself. + Re-checks that containment first (RevealTargetChanged). Raises + OpenerUnavailable when no opener exists and OSError when the one found + cannot start. Never waits on the file manager itself. """ path = Path(path) if not path.is_absolute(): raise ValueError("path must be absolute") + path = _contained_file(path, Path(root)) if sys.platform == "win32": _reveal_windows(path) return if sys.platform == "darwin": - opener = _executable("open") + opener = _macos_open() if opener is None: raise OpenerUnavailable() - _launch([opener, "-R", str(path)]) + _reap(_launch([opener, "-R", str(path)])) return if _show_items(path): return opener = _executable("xdg-open") if opener is None: raise OpenerUnavailable() - _launch([opener, str(path.parent)]) + _reap(_launch([opener, str(path.parent)]))