From 6b5b50d4d1b87aa82d930d7cf03695bec1d0c741 Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Tue, 27 Oct 2026 00:34:23 -0700 Subject: [PATCH] fix(logs): never empty a renamed log or serve a swapped file; keep root anchors private Review findings on #329 PR 1: - Clear empties the active log through the handler only when the handler's open stream is the listed file (same inode); a log the user renamed while ComfyUI runs is no longer emptied. - A download is refused when the opened file is not the one listed (inode), or has another name (a hard link). - Windows reparse points that name another file (the name-surrogate bit: symlinks, junctions, WSL links) are links under Rule R; cloud placeholders stay regular files. - The report's paths use the closest anchor that is not a filesystem root, and the scrub also catches the home folder without its root. --- py/api/diagnostics_report.py | 32 +++++++++++++++++-------- py/api/logging_routes.py | 16 +++++++++---- tests/test_api_diagnostics_report.py | 21 +++++++++++++++- tests/test_api_logging.py | 31 ++++++++++++++++++++++++ tests/test_utils_logging_config.py | 31 ++++++++++++++++++++++++ utils/logging_config.py | 36 +++++++++++++++++++++++++--- 6 files changed, 149 insertions(+), 18 deletions(-) diff --git a/py/api/diagnostics_report.py b/py/api/diagnostics_report.py index 2866d80..b642895 100644 --- a/py/api/diagnostics_report.py +++ b/py/api/diagnostics_report.py @@ -9,11 +9,12 @@ the configuration keys that only ``config.json`` holds. It never copies ``PromptManagerConfig.get_config()``: no path leaf is listed, and the CivitAI key is reported only as ``has_civitai_api_key``. -Every path in the report goes through ``report_path``: relative to ComfyUI -(or another public anchor), else ``"outside ComfyUI"``, never a basename -that could be a user name. As a backstop, ``scrub`` replaces any string that -still looks like an absolute path, holds the saved key or the home folder, -and counts each replacement in ``redacted`` (0 for a correct builder). +Every path in the report goes through ``report_path``: relative to the +closest public anchor that is not a filesystem root, else ``"outside +ComfyUI"``, never a basename that could be a user name. As a backstop, +``scrub`` replaces any string that still looks like an absolute path, holds +the saved key or the home folder (with or without its root), and counts each +replacement in ``redacted`` (0 for a correct builder). The report opens the database, so it takes a registry lease: it is refused (409 ``busy``) while an ``exclusive`` job (a restore) runs, and a restore @@ -61,10 +62,14 @@ def report_path(path): canonical = Path(os.path.normcase(os.path.realpath(str(path)))) except (OSError, ValueError): return OUTSIDE - for anchor in GalleryConfig.path_anchors(): - if canonical.is_relative_to(Path(anchor)): - return canonical.relative_to(Path(anchor)).as_posix() or "." - return OUTSIDE + # The closest anchor wins; a filesystem root (an output folder directly + # under / or D:\\) never does, or every path would read as public. + anchors = [Path(anchor) for anchor in GalleryConfig.path_anchors()] + anchors = [a for a in anchors if a.parent != a and canonical.is_relative_to(a)] + if not anchors: + return OUTSIDE + anchor = max(anchors, key=lambda a: len(a.parts)) + return canonical.relative_to(anchor).as_posix() or "." def _looks_absolute(text): @@ -81,7 +86,14 @@ def _home_spellings(): home = os.path.expanduser("~") if not home or len(home) < 4 or home in ("/", "\\"): return () - return tuple({home, home.replace("\\", "/")}) + spellings = {home, home.replace("\\", "/")} + # Also without its root or drive ("home/vito"), as a path made relative + # to a filesystem root would spell it; only with two parts or more. + relative = os.path.splitdrive(home)[1].lstrip("\\/") + for spelling in (relative, relative.replace("\\", "/")): + if "/" in spelling or "\\" in spelling: + spellings.add(spelling) + return tuple(spellings) def scrub(report, key): diff --git a/py/api/logging_routes.py b/py/api/logging_routes.py index e7e9d4e..40e3780 100644 --- a/py/api/logging_routes.py +++ b/py/api/logging_routes.py @@ -95,9 +95,11 @@ def _validate_log_config(data): def _read_log_bytes(path): """The whole file at ``path``, opened without following a link (blocking). - The file is checked again through the open handle (a regular file), so a - name swapped for a link after it was listed is refused as missing. The - handle is closed before this returns. + The file is checked again through the open handle: a regular file with + one name and, when ``path`` carries it, the inode it was listed with. A + name swapped for a link or another file after it was listed, or a hard + link to a file elsewhere, is refused as missing. The handle is closed + before this returns. """ flags = os.O_RDONLY | getattr(os, "O_BINARY", 0) | getattr(os, "O_NOFOLLOW", 0) try: @@ -106,8 +108,14 @@ def _read_log_bytes(path): if getattr(os, "O_NOFOLLOW", 0) and os.path.islink(str(path)): raise FileNotFoundError("Log file not found") from error raise + listed_ino = getattr(path, "listed_ino", 0) with os.fdopen(fd, "rb") as handle: - if not stat.S_ISREG(os.fstat(handle.fileno()).st_mode): + opened = os.fstat(handle.fileno()) + if ( + not stat.S_ISREG(opened.st_mode) + or opened.st_nlink > 1 # a hard link to a file elsewhere + or (listed_ino and opened.st_ino and opened.st_ino != listed_ino) + ): raise FileNotFoundError("Log file not found") return handle.read() diff --git a/tests/test_api_diagnostics_report.py b/tests/test_api_diagnostics_report.py index ba8f595..d5ba20a 100644 --- a/tests/test_api_diagnostics_report.py +++ b/tests/test_api_diagnostics_report.py @@ -26,7 +26,7 @@ sys.modules.setdefault("server", _mock_server) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) from py.api import admin, diagnostics_report # noqa: E402 -from py.api.diagnostics_report import report_path, scrub # noqa: E402 +from py.api.diagnostics_report import OUTSIDE, report_path, scrub # noqa: E402 from py.config import ( GalleryConfig, IntegrationConfig, @@ -210,6 +210,25 @@ class TestReport(DiagnosticsReportTestCase): self.assertEqual(report_path(""), "") self.assertEqual(report_path(None), "") + def test_a_filesystem_root_anchor_never_makes_a_path_public(self): + # --output-directory /renders makes "/" an anchor; a path outside + # ComfyUI must still read "outside ComfyUI", never "home//...". + root = os.path.abspath(os.sep) + anchors = [str(self.comfy_dir), root] + with mock.patch.object(GalleryConfig, "path_anchors", return_value=anchors): + self.assertEqual( + report_path(os.path.join(str(self.outside_dir), "x")), OUTSIDE + ) + self.assertEqual( + report_path(os.path.join(str(self.comfy_dir), "output")), "output" + ) + + def test_scrub_removes_the_home_folder_without_its_root(self): + home = os.path.expanduser("~") + relative = os.path.splitdrive(home)[1].lstrip("\\/").replace("\\", "/") + cleaned = scrub({"a": f"{relative}/area/prompts.db", "redacted": 0}, None) + self.assertEqual(cleaned, {"a": "[removed]", "redacted": 1}) + def test_scrub_removes_a_leaked_path_and_counts_it(self): leaked = { "a": "/home/someone/x", diff --git a/tests/test_api_logging.py b/tests/test_api_logging.py index c6551bb..b98d9a7 100644 --- a/tests/test_api_logging.py +++ b/tests/test_api_logging.py @@ -433,6 +433,37 @@ class TestLogFilesAndDownload(LoggingAPITestCase): self.assertEqual(resp.status, 404) self.assertNotIn("the key", body) + async def test_download_refuses_a_hard_link(self): + secret = Path(self.tmpdir) / "secret.txt" + secret.write_text("the key") + try: + os.link(secret, self.log_dir / "prompt_manager.log.3") + except (OSError, NotImplementedError): # pragma: no cover + self.skipTest("this filesystem cannot create hard links") + resp = await self.client.request( + "GET", "/prompt_manager/logs/download/prompt_manager.log.3" + ) + self.assertEqual(resp.status, 404) + self.assertNotIn("the key", await resp.text()) + + async def test_download_refuses_a_file_swapped_after_it_was_listed(self): + (self.log_dir / "prompt_manager.log.2").write_text("listed") + other = Path(self.tmpdir) / "other.txt" + other.write_text("swapped in") + real_entry = self.manager.log_file_entry + + def entry_then_swap(name): + path = real_entry(name) + os.replace(other, path) # another file now has the listed name + return path + + self.manager.log_file_entry = entry_then_swap + resp = await self.client.request( + "GET", "/prompt_manager/logs/download/prompt_manager.log.2" + ) + self.assertEqual(resp.status, 404) + self.assertNotIn("swapped in", await resp.text()) + async def test_download_reads_off_the_loop(self): (self.log_dir / "prompt_manager.log").write_bytes(b"line\n") seen = [] diff --git a/tests/test_utils_logging_config.py b/tests/test_utils_logging_config.py index 37dec48..ed7d765 100644 --- a/tests/test_utils_logging_config.py +++ b/tests/test_utils_logging_config.py @@ -4,6 +4,7 @@ import logging import ntpath import os import pathlib +import stat import sys import tempfile import threading @@ -645,6 +646,36 @@ class TestRotatedCopyRule(IsolatedLoggerTestCase): with open(target, "rb") as handle: self.assertEqual(handle.read(), b"secret " * 1000) + @unittest.skipIf(os.name == "nt", "Windows cannot rename a log that is open") + def test_truncate_never_empties_a_renamed_log_the_handler_still_writes(self): + # The user renamed the active log to keep it; the handler still has it + # open. Clear must empty the new prompt_manager.log, not the kept copy. + self.logger.info("worth keeping") + kept = os.path.join(self.log_dir, "saved_copy.txt") + os.rename(self._path("prompt_manager.log"), kept) + with open(kept, "rb") as handle: + before = handle.read() + self._write("prompt_manager.log", b"fresh active log\n") + result = self.manager.truncate_logs() + self.assertEqual(result["errors"], []) + with open(kept, "rb") as handle: + self.assertTrue(handle.read().startswith(before)) + with open(self._path("prompt_manager.log"), "rb") as handle: + self.assertEqual(handle.read(), b"") + + def test_name_surrogate_reparse_points_are_links_and_placeholders_are_not(self): + regular = stat.S_IFREG | 0o644 + path = self._write("prompt_manager.log.3") + for tag, is_link in ( + (0xA000001D, True), # WSL symlink + (0xA0000003, True), # mount point (junction) + (0x9000001A, False), # OneDrive cloud placeholder: a regular file + (0, False), + ): + with self.subTest(tag=hex(tag)): + st = mock.Mock(st_mode=regular, st_reparse_tag=tag) + self.assertIs(logging_config._is_link(path, st), is_link) + def test_truncate_with_file_logging_off_never_creates_the_active_log(self): self.manager.update_config({"file_logging": False}) os.unlink(self._path("prompt_manager.log")) diff --git a/utils/logging_config.py b/utils/logging_config.py index cc23608..43931f7 100644 --- a/utils/logging_config.py +++ b/utils/logging_config.py @@ -49,10 +49,23 @@ def _rotated_number(name: str) -> Optional[int]: return int(match.group(1)) if match else None +# Windows reparse tags with the name-surrogate bit name another file or +# folder (symlinks, junctions, WSL symlinks); cloud placeholders do not. +_NAME_SURROGATE = 0x20000000 + + def _is_link(path: str, st: os.stat_result) -> bool: + if getattr(st, "st_reparse_tag", 0) & _NAME_SURROGATE: + return True return stat.S_ISLNK(st.st_mode) or os.path.isjunction(path) +class LogFilePath(type(Path())): + """A listed log file's path, with the inode it had when it was listed.""" + + listed_ino = 0 + + def _kind(name: str, path: str, st: os.stat_result) -> str: """Rule R: ``active``, ``rotated``, ``link`` or ``other`` (never deleted).""" if _is_link(path, st): @@ -124,10 +137,21 @@ def log_file_entry(log_dir, name) -> Path: if row["name"] == name and row["kind"] in ("active", "rotated", "other"): if row["_dir"]: break - return Path(log_dir) / row["name"] + entry = LogFilePath(log_dir, row["name"]) + entry.listed_ino = row["ino"] + return entry raise FileNotFoundError("Log file not found") +def _stream_is(stream, ino: int) -> bool: + """Whether ``stream`` is open on the file with inode ``ino`` (0: unknown).""" + try: + open_ino = os.fstat(stream.fileno()).st_ino + except (OSError, ValueError, AttributeError): + return False + return not (ino and open_ino and open_ino != ino) + + def _error_text(error: BaseException) -> str: """An OSError's reason and file name, without the folder it is in.""" if isinstance(error, OSError): @@ -467,7 +491,7 @@ class PromptManagerLogger: for handler in reversed(held): handler.release() - def _truncate_main_log(self, log_file: Path) -> None: + def _truncate_main_log(self, log_file: Path, expected_ino: int = 0) -> None: """Empty the main log through the handler that writes to it. Opening the file a second time with ``"w"`` leaves the handler's own @@ -476,6 +500,10 @@ class PromptManagerLogger: the next record rolls the file over. Truncating the handler's stream keeps position and size in step. Without a handler on it (file logging off) the existing file is emptied in place, never through a link. + + A handler whose open stream is not the listed file (``expected_ino``: + the user renamed the log, which the handler still writes to) is left + alone: the file under the active name is emptied instead. """ handled = False target = os.path.normcase(os.path.realpath(str(log_file))) @@ -486,6 +514,8 @@ class PromptManagerLogger: continue if os.path.normcase(os.path.realpath(base)) != target: continue + if not _stream_is(stream, expected_ino): + continue handler.acquire() try: stream.flush() @@ -575,7 +605,7 @@ class PromptManagerLogger: path = os.path.join(self.log_dir, row["name"]) try: if row["kind"] == "active": - self._truncate_main_log(Path(path)) + self._truncate_main_log(Path(path), row["ino"]) else: os.unlink(path) results["truncated"].append(row["name"])