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.
This commit is contained in:
Vito Sansevero
2026-10-27 00:34:23 -07:00
parent f6df8a39b3
commit 6b5b50d4d1
6 changed files with 149 additions and 18 deletions
+22 -10
View File
@@ -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):
+12 -4
View File
@@ -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()
+20 -1
View File
@@ -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/<user>/...".
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",
+31
View File
@@ -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 = []
+31
View File
@@ -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"))
+33 -3
View File
@@ -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"])