nodes/ComfyAssets-ComfyUI_PromptManager
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(images): re-check, isolate and reap the Show in folder opener

Browse Source 
- re-check that the real path is still a regular file inside its root
  right before launching; a changed target answers 404 (review L4)
- run openers on a dedicated single worker, not ComfyUI's default
  executor, and make dbus-send wait for the FileManager1 reply
  (--print-reply --reply-timeout=2000) so a missing service falls back
  to xdg-open (L1)
- reap launched processes on a daemon thread, give them a minimal
  environment with a fixed system PATH, and use /usr/bin/open on macOS
  and dbus-send/xdg-open resolved only on that PATH (L5)
This commit is contained in:
Vito Sansevero
2026-10-20 21:29:56 -07:00
parent 59fb9a99c9
commit daac06483d
4 changed files with 287 additions and 62 deletions
Download Patch File Download Diff File Expand all files Collapse all files
py/api/images.py
+16 -4
View File
@@ -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(
{
tests/browser/server.py
+1 -1
View File
@@ -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":
tests/test_image_reveal.py
+165 -42
View File
@@ -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")
utils/file_reveal.py
+105 -15
View File
@@ -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)]))