From 3f8c63f16eec959894f3dd8b886c1ff2ce2f34df Mon Sep 17 00:00:00 2001 From: Vito Sansevero Date: Fri, 23 Oct 2026 12:08:43 -0700 Subject: [PATCH] fix(security): decide source containment as text before any filesystem access Every caller (validate-source, POST /settings, config.json at load) now refuses a source through one check, sources.source_problem: the form check first, then GalleryConfig.gallery_root_problem, which compares the normalised spelling with the allowed folders before anything resolves, stats or opens the path. A UNC share, the \\?\ and \\.\ namespaces, a device or any outside folder is refused with no SMB request, no executor stall and no existence or timing oracle; only then is the path resolved and contained again, which still catches a symlink out. The filesystem root and the home directory answer outside-allowed with the same reason as any outside path, so no caller learns the home path. Also: - POST /settings runs the form checks and parses the roots off the loop; an unusable source is a coded 400 invalid-source naming its public path (as text) and its index. - ComfyUI folders outside the base directory are path anchors, so their sources round-trip and never collide by folder name. - A per-source scan sends each index with its public path; a stale or shifted index is 409 sources-changed and starts nothing. - A source still watched after monitoring was switched off is "stopping". - Roots are compared as folders, not spellings, so an unchanged legacy resend takes no source-change hold. - Tests: no realpath/stat/lstat of an outside path (native and Windows forms with ntpath fakes), the alike answers with /, ~ and a link to ~, an event instead of a timer wait, and a teardown that drains jobs before the temporary database goes. --- py/api/job_routes.py | 55 +++-- py/api/settings_routes.py | 100 +++++++-- py/api/sources.py | 76 +++++-- py/config.py | 132 ++++++++--- tests/test_api_admin.py | 18 ++ tests/test_gallery_roots.py | 21 +- tests/test_scan_scope.py | 83 +++++-- tests/test_settings_sources.py | 390 ++++++++++++++++++++++++++++++++- 8 files changed, 763 insertions(+), 112 deletions(-) diff --git a/py/api/job_routes.py b/py/api/job_routes.py index b0c6b013..83caa851 100644 --- a/py/api/job_routes.py +++ b/py/api/job_routes.py @@ -21,8 +21,10 @@ def _not_found(): MAX_SCAN_ROOTS = 64 _SCOPE_ERROR = 'The scan scope must be "new" or "all"' _ROOTS_ERROR = ( - f"roots must be a non-empty list of up to {MAX_SCAN_ROOTS} source indexes" + f"roots must be a non-empty list of up to {MAX_SCAN_ROOTS} source indexes, " + "with paths naming the public path of each" ) +SOURCES_CHANGED_ERROR = "The sources changed since they were loaded. Reload them." def _bad_request(message, code=None): @@ -33,20 +35,29 @@ def _bad_request(message, code=None): def _scan_roots(body): - """``(roots, error)``: the sorted, deduplicated source indexes, or None. + """``(roots, error)``: ``{index: public path}`` of the selected sources, or None. Absent ``roots`` means every source. When present it is a non-empty list - of at most MAX_SCAN_ROOTS non-negative ints (never bools or floats). + of at most MAX_SCAN_ROOTS non-negative ints (never bools or floats), and + ``paths`` the same-length list of the public path the caller read for + each (GET /settings), so a stale index never scans another folder. """ if "roots" not in body: return None, None raw = body["roots"] + paths = body.get("paths") if not isinstance(raw, list) or not raw or len(raw) > MAX_SCAN_ROOTS: return None, _ROOTS_ERROR - for value in raw: + if not isinstance(paths, list) or len(paths) != len(raw): + return None, _ROOTS_ERROR + selected = {} + for value, path in zip(raw, paths): if isinstance(value, bool) or not isinstance(value, int) or value < 0: return None, _ROOTS_ERROR - return sorted(set(raw)), None + if not isinstance(path, str) or selected.get(value, path) != path: + return None, _ROOTS_ERROR + selected[value] = path + return dict(sorted(selected.items())), None async def _scan_request(request): @@ -98,6 +109,13 @@ def _summary_event(job): class JobRoutesMixin: """Mixin providing the /prompt_manager/jobs endpoints.""" + def _roots_still_listed(self, selected, output_dirs): + """Whether each ``{index: public path}`` still names that source.""" + return all( + index < len(output_dirs) and self._public_path(output_dirs[index]) == path + for index, path in selected.items() + ) + def _register_job_routes(self, routes): @routes.get("/prompt_manager/jobs") async def list_jobs_route(request): @@ -167,24 +185,35 @@ class JobRoutesMixin: The body's ``scope`` is "new" (the default: new and unresolved files) or "all". ``roots``, when given, selects sources by their index in - the effective sources (``_get_all_output_dirs``); an index past them - is a 400 ``unknown-source`` and starts nothing. The job walks the - sources resolved here, so a later source change cannot move it. + the effective sources (``_get_all_output_dirs``), and ``paths`` names + the public path read for each. An index past them, or one whose + source now has another public path (the list changed since it was + read), is a 409 ``sources-changed`` and starts nothing. The job walks + the sources resolved here, so a later source change cannot move it. """ scan, error = await _scan_request(request) if error: return _bad_request(error) output_dirs = None + roots = None if scan["roots"] is not None: output_dirs = await self._run_in_executor(self._get_all_output_dirs) - if scan["roots"][-1] >= len(output_dirs): - return _bad_request( - "That source is no longer configured. Reload the sources.", - code="unknown-source", + listed = await self._run_in_executor( + self._roots_still_listed, scan["roots"], output_dirs + ) + if not listed: + return web.json_response( + { + "success": False, + "code": "sources-changed", + "error": SOURCES_CHANGED_ERROR, + }, + status=409, ) + roots = list(scan["roots"]) try: job = self._start_scan_job( - scan["files"], roots=scan["roots"], output_dirs=output_dirs + scan["files"], roots=roots, output_dirs=output_dirs ) except JobBusy as busy: return busy_response(busy) diff --git a/py/api/settings_routes.py b/py/api/settings_routes.py index 8faa4e83..4760e083 100644 --- a/py/api/settings_routes.py +++ b/py/api/settings_routes.py @@ -65,6 +65,45 @@ def _stat_identity(path): return (stat.st_dev, stat.st_ino) if stat.st_ino else None +# Refusals that a reconnected drive or folder can lift. +_RECONNECTABLE = frozenset({"not-found", "not-directory"}) + + +def _root_key(root): + """The directory a configured root names (resolved), else its spelling.""" + from ..config import GalleryConfig + + try: + return GalleryConfig.resolve_gallery_root(root) + except (OSError, TypeError, ValueError, AttributeError): + return root + + +def _request_error(message): + return {"code": "invalid-request", "error": message} + + +def _invalid_source(index, entry, code, reason): + """The 400 body for the source at ``index`` that cannot be used. + + Names it by its public spelling (computed as text: a refused path is + never resolved), so the page can mark that row and say why. + """ + source = sources.public_spelling(entry) + advice = ( + "Remove it or reconnect it, then save." + if code in _RECONNECTABLE + else "Remove it, then save." + ) + return { + "code": "invalid-source", + "source": source, + "index": index, + "reason": reason, + "error": f"{source} can't be used: {reason}. {advice}", + } + + def _section(config, name): """``config[name]`` as a fresh dict (a non-object section starts empty).""" value = config.get(name) @@ -255,9 +294,15 @@ class SettingsRoutesMixin: {"success": False, "error": str(ve)}, status=400 ) - new_roots, error = self._parse_gallery_roots(data) + # Off the loop: an accepted root is resolved (filesystem access). + new_roots, error = await self._run_in_executor( + self._parse_gallery_roots, data + ) if error: - return web.json_response({"success": False, "error": error}, status=400) + return web.json_response({"success": False, **error}, status=400) + roots_change = new_roots is not None and await self._run_in_executor( + self._roots_change, new_roots + ) published = {} @@ -268,7 +313,7 @@ class SettingsRoutesMixin: async with contextlib.AsyncExitStack() as stack: hold = None - if self._roots_change(new_roots): + if roots_change: try: hold = await stack.enter_async_context( self._jobs.hold( @@ -344,12 +389,19 @@ class SettingsRoutesMixin: @staticmethod def _roots_change(new_roots): - """Whether a save changes the sources (the legacy page resends them).""" + """Whether a save changes the sources (blocking: it resolves them). + + Compared as directories, not spellings: the legacy page resends the + roots resolved, while config.json may hold them as hand-written + (relative, another case), and an unchanged resend is no change. + """ from ..config import GalleryConfig - return new_roots is not None and new_roots != list( - GalleryConfig.MONITORING_DIRECTORIES - ) + if new_roots is None: + return False + return [_root_key(root) for root in new_roots] != [ + _root_key(root) for root in GalleryConfig.MONITORING_DIRECTORIES + ] @staticmethod def _observer_alive(): @@ -441,15 +493,21 @@ class SettingsRoutesMixin: Accepts ``gallery_root_paths`` (list, preferred) or the legacy single ``gallery_root_path`` string. Blank entries are dropped. + Each entry passes the same checks as validate-source + (``sources.source_problem``): the form, then containment as text, + before anything is resolved. Blocking (an accepted root is resolved). + Returns: (roots, error): ``roots`` is the validated list, or ``None`` when - the payload carries no gallery root key; ``error`` is a message - when validation failed (and ``roots`` is then ``None``). + the payload carries no gallery root key; ``error`` is the coded + body of the 400 when validation failed (``roots`` is then None). + An unusable source is ``invalid-source`` naming its public path + and its position in the list. """ if "gallery_root_paths" in data: raw = data["gallery_root_paths"] if not isinstance(raw, list): - return None, "gallery_root_paths must be a list" + return None, _request_error("gallery_root_paths must be a list") elif "gallery_root_path" in data: raw = [data["gallery_root_path"]] else: @@ -459,18 +517,15 @@ class SettingsRoutesMixin: roots = [] seen = set() - for entry in raw: + for index, entry in enumerate(raw): if not isinstance(entry, str): - return None, "Gallery root paths must be strings" + return None, _request_error("Gallery root paths must be strings") entry = entry.strip() if not entry: continue - ok, reason = GalleryConfig.validate_gallery_root(entry) - if not ok: - return ( - None, - f"Invalid gallery root '{os.path.basename(entry)}': {reason}", - ) + code, reason = sources.source_problem(entry) + if code is not None: + return None, _invalid_source(index, entry, code, reason) root = GalleryConfig.resolve_gallery_root(entry) keys = {("path", _source_identity(root)), ("file", _stat_identity(root))} keys.discard(("file", None)) @@ -481,11 +536,18 @@ class SettingsRoutesMixin: return roots, None def _apply_gallery_roots(self, roots): - """Install validated gallery roots; returns True when they changed.""" + """Install validated gallery roots; returns True when they changed. + + Another spelling of the same directories installs the resolved + spelling and is no change (nothing to restart for). + """ from ..config import GalleryConfig if roots == list(GalleryConfig.MONITORING_DIRECTORIES): return False + if not self._roots_change(roots): + GalleryConfig.MONITORING_DIRECTORIES = roots + return False GalleryConfig.MONITORING_DIRECTORIES = roots self._cached_output_dir = None self.reset_gallery_roots_cache() diff --git a/py/api/sources.py b/py/api/sources.py index bc326c48..90fb18c2 100644 --- a/py/api/sources.py +++ b/py/api/sources.py @@ -6,15 +6,21 @@ ``\\\\?\\`` and ``\\\\.\\`` namespaces, a drive-relative path, an alternate data stream, a device name or a part Windows would silently trim; on POSIX no Windows drive or backslash form). +- ``source_problem`` is the one refusal every caller answers (validate-source + and the settings save): the form check, then + ``GalleryConfig.gallery_root_problem``, which decides containment as text + before it touches the filesystem. - ``describe_sources`` (blocking: it resolves paths) reports the configured sources with their validity, the effective sources with their listing index and watch state, and the image monitor's state. Every path in it is a public path (``_public_path``), never an absolute one. -A source's watch state is ``watching`` when the monitor's observer is alive -and scheduled that directory; ``manual`` when monitoring is off or this -process has no monitor; else ``restart_pending`` (a source added since the -monitor started, monitoring switched on, or an observer that stopped). +A source's watch state is ``watching`` when the monitor's observer is alive, +scheduled that directory and monitoring is on; ``stopping`` when it still +watches it but monitoring was switched off (it stops at the next restart); +``manual`` when monitoring is off or this process has no monitor; else +``restart_pending`` (a source added since the monitor started, monitoring +switched on, or an observer that stopped). """ import logging @@ -41,13 +47,7 @@ _COMFYUI_LABELS = ( ("get_temp_directory", "ComfyUI temp"), ("get_user_directory", "ComfyUI user"), ) -# Codes validate-source folds into "outside-allowed" (see validate_source). -_OUTSIDE_CODES = frozenset({"filesystem-root", "home-directory", "outside-allowed"}) -OUTSIDE_REASON = ( - "Gallery root must be inside a ComfyUI directory (output, input, temp, user) " - "or one listed in PROMPT_MANAGER_EXTRA_GALLERY_ROOTS" -) -WATCH_STATES = ("watching", "manual", "restart_pending") +WATCH_STATES = ("watching", "stopping", "manual", "restart_pending") def source_form_error(text, *, windows=None): @@ -100,6 +100,45 @@ def _windows_form_error(text): return None +def source_problem(text): + """``(code, reason)`` that refuses ``text`` as a source, else ``(None, "")``. + + The form check runs first and touches nothing; then containment is + decided as text before any filesystem access (see + ``GalleryConfig.gallery_root_problem``). Every caller answers alike. + """ + from ..config import GalleryConfig + + code = source_form_error(text) + if code is not None: + return code, FORM_REASONS[code] + return GalleryConfig.gallery_root_problem(text) + + +def public_spelling(text): + """How a refused source is named in a response, computed as text only. + + Relative to the path anchor it lies under (its public path), else its + last part: the caller sent the path, so naming it reveals nothing, and a + refused path is never resolved to name it. + """ + from ..config import GalleryConfig, _lexical_path, _lexically_inside + + text = str(text).strip() + try: + lexical = os.path.normpath(text) + key = _lexical_path(text) # the same length as ``lexical`` + if not os.path.isabs(text): + return lexical.replace("\\", "/") + for anchor in GalleryConfig.path_anchors(): + if _lexically_inside(key, [anchor]): + rest = lexical[len(anchor.rstrip(os.sep)) :].lstrip(os.sep) + return rest.replace(os.sep, "/") or "." + return os.path.basename(lexical) or lexical + except (TypeError, ValueError): + return "" + + def identity(path): """The comparison key of a directory: resolved and case-folded (Windows).""" return os.path.normcase(os.path.realpath(str(path))) @@ -162,9 +201,9 @@ def monitor_state(monitor): def watch_state(path, *, present, alive, scheduled_keys, enabled): - """``watching``, ``manual`` or ``restart_pending`` (see the module docstring).""" + """One of ``WATCH_STATES`` (see the module docstring).""" if alive and identity(path) in scheduled_keys: - return "watching" + return "watching" if enabled else "stopping" if not present or not enabled: return "manual" return "restart_pending" @@ -222,16 +261,7 @@ def validate_source(text): from ..config import GalleryConfig from . import _public_path - code = source_form_error(text) - reason = FORM_REASONS.get(code) - if code is None: - code, reason = GalleryConfig.gallery_root_problem(text) - if code in _OUTSIDE_CODES: - # One answer for every path outside the allowed folders: naming - # the filesystem root or the home directory would let a caller - # probe the server's layout. The detail goes to the log only. - logger.info(f"Source refused: {code}") - code, reason = "outside-allowed", OUTSIDE_REASON + code, reason = source_problem(text) if code is not None: return { "valid": False, diff --git a/py/config.py b/py/config.py index facc2d43..c6fa7197 100644 --- a/py/config.py +++ b/py/config.py @@ -31,7 +31,6 @@ routes = server_instance.routes extension_uri = None # Will be set in __init__.py import os # noqa: E402 -from pathlib import Path # noqa: E402 from typing import Any, Dict, List, Optional, Tuple # noqa: E402 # Import logging system @@ -51,6 +50,14 @@ config_logger = get_logger("prompt_manager.config") # may be used as gallery roots in addition to ComfyUI's own directories. EXTRA_GALLERY_ROOTS_ENV = "PROMPT_MANAGER_EXTRA_GALLERY_ROOTS" +# The one answer for every path outside the allowed folders. The filesystem +# root and the home directory answer it too, so no caller learns the server's +# layout (home path, user name) from a refusal (#249 S4). +OUTSIDE_ALLOWED_REASON = ( + "Gallery root must be inside a ComfyUI directory (output, input, temp, user) " + f"or one listed in {EXTRA_GALLERY_ROOTS_ENV}" +) + # Environment variable overriding where config.json is read from and saved to. CONFIG_PATH_ENV = "PROMPT_MANAGER_CONFIG_PATH" @@ -89,6 +96,29 @@ def write_private_json(path: str, data: Any) -> None: os.chmod(path, 0o600) +def _lexical_path(path: str) -> str: + """``path`` normalised for comparison without touching the filesystem. + + normpath collapses ``.`` and ``..`` as text and normcase folds case on + Windows; neither opens, stats nor resolves anything (no SMB request for a + UNC path). A symlink is only seen by :func:`_canonical_path` later. + """ + return os.path.normcase(os.path.normpath(path)) + + +def _lexically_inside(path: str, parents: List[str]) -> bool: + """Whether the normalised ``path`` is one of ``parents`` or below one. + + Pure text on normalised spellings (:func:`_lexical_path` or + :func:`_canonical_path`): no filesystem access. + """ + sep = os.path.sep + return any( + path == parent or path.startswith(parent.rstrip(sep) + sep) + for parent in parents + ) + + def _is_filesystem_root(path: str) -> bool: """True for '/' on POSIX and drive roots such as 'C:\\' on Windows.""" return os.path.dirname(path) == path @@ -342,31 +372,56 @@ class GalleryConfig: Filesystem roots are never allowed as parents, even when listed in the environment, because that would re-open the whole disk. """ - parents = [] + return [canonical for canonical, _spelling in cls._allowed_parent_pairs()] + + @classmethod + def _allowed_parent_pairs(cls) -> List[Tuple[str, str]]: + """``(canonical, lexical spelling)`` of each allowed parent. + + Only trusted directories (ComfyUI's own and the environment's) are + resolved here; the spelling is how ComfyUI or the environment wrote + the directory, so a submitted path can be compared without resolving it. + """ + pairs = [] + seen = set() for candidate in _comfyui_directories() + _extra_gallery_roots(): try: canonical = _canonical_path(candidate) + spelling = _lexical_path(os.path.abspath(candidate)) except (OSError, ValueError): continue if _is_filesystem_root(canonical) or not os.path.isdir(canonical): continue - if canonical not in parents: - parents.append(canonical) - return parents + if canonical not in seen: + seen.add(canonical) + pairs.append((canonical, spelling)) + return pairs @classmethod def path_anchors(cls) -> List[str]: """Canonical directories that relative gallery paths are resolved against. The ComfyUI base directory comes first, followed by the parent of each - directory listed in PROMPT_MANAGER_EXTRA_GALLERY_ROOTS, so an extra - root is addressed by its own name (``gallery/sub``) rather than by an - absolute path. The same anchors drive the public (relative) form of - paths in API responses. + ComfyUI directory outside it (``--output-directory D:\\Renders``) and of + each directory listed in PROMPT_MANAGER_EXTRA_GALLERY_ROOTS, so such a + directory is addressed by its own name (``Renders/sub``) rather than by + an absolute path, and two folders of one name stay apart. The same + anchors drive the public (relative) form of paths in API responses. """ anchors = [] base = _comfyui_base_directory() candidates = [base] if base else [] + try: + base_key = _canonical_path(base) if base else None + except (OSError, ValueError): + base_key = None + for directory in _comfyui_directories(): + try: + canonical = _canonical_path(directory) + except (OSError, ValueError): + continue + if base_key is None or not _lexically_inside(canonical, [base_key]): + candidates.append(os.path.dirname(canonical)) candidates.extend(os.path.dirname(root) for root in _extra_gallery_roots()) for candidate in candidates: try: @@ -411,42 +466,63 @@ class GalleryConfig: def gallery_root_problem(cls, path: Any) -> Tuple[Optional[str], str]: """``(code, reason)`` why ``path`` may not be a gallery root, else (None, ""). - Codes: ``empty``, ``unresolvable``, ``filesystem-root``, - ``home-directory``, ``outside-allowed``, ``not-found`` and - ``not-directory``. No reason names the path. Containment is decided - before existence, so a path outside the allowed folders answers - ``outside-allowed`` whether or not it exists (no existence oracle). + Codes: ``empty``, ``unresolvable``, ``outside-allowed``, ``not-found`` + and ``not-directory``. No reason names the path. + + Containment is decided twice. First lexically, with no filesystem + access at all: a path (UNC, ``\\\\?\\``, a device, anything) whose + normalised spelling lies outside every allowed parent answers + ``outside-allowed`` before it is resolved, so the server never opens + or stats it (no SMB connection, no existence or timing oracle). Only + then is it resolved and checked again, which catches a symlink out. + The filesystem root and the home directory answer ``outside-allowed`` + too: one answer for every caller (validate-source, the settings save, + config.json), and the detail goes to the log only. """ if not isinstance(path, str) or not path.strip(): return "empty", "Gallery root must be a non-empty path" + pairs = cls._allowed_parent_pairs() + spellings = [spelling for pair in pairs for spelling in pair] + try: + lexical = cls._lexical_candidates(path) + except (TypeError, ValueError): + return "unresolvable", "Gallery root could not be resolved" + if not any(_lexically_inside(candidate, spellings) for candidate in lexical): + return "outside-allowed", OUTSIDE_ALLOWED_REASON + try: canonical = cls.resolve_gallery_root(path) except (OSError, ValueError): return "unresolvable", "Gallery root could not be resolved" if _is_filesystem_root(canonical): - return "filesystem-root", "Gallery root cannot be a filesystem root" + config_logger.info("Gallery root refused: a filesystem root") + return "outside-allowed", OUTSIDE_ALLOWED_REASON if canonical == _canonical_path(os.path.expanduser("~")): - return "home-directory", "Gallery root cannot be the home directory" - - candidate = Path(canonical) - if not any( - candidate.is_relative_to(Path(parent)) - for parent in cls.allowed_gallery_parents() - ): - return ( - "outside-allowed", - "Gallery root must be inside a ComfyUI directory " - f"(output, input, temp, user) or one listed in " - f"{EXTRA_GALLERY_ROOTS_ENV}", - ) + config_logger.info("Gallery root refused: the home directory") + return "outside-allowed", OUTSIDE_ALLOWED_REASON + if not _lexically_inside(canonical, [canonical for canonical, _ in pairs]): + return "outside-allowed", OUTSIDE_ALLOWED_REASON if not os.path.exists(canonical): return "not-found", "Gallery root does not exist" if not os.path.isdir(canonical): return "not-directory", "Gallery root is not a directory" return None, "" + @classmethod + def _lexical_candidates(cls, path: str) -> List[str]: + """Every spelling ``path`` may resolve to, normalised as text (no IO). + + An absolute path is itself; a relative one is joined to each + :meth:`path_anchors` entry, as :meth:`resolve_gallery_root` tries them. + """ + path = path.strip() + if os.path.isabs(path): + return [_lexical_path(path)] + anchors = cls.path_anchors() + return [_lexical_path(os.path.join(anchor, path)) for anchor in anchors] + class IntegrationConfig: """Configuration for third-party extension integrations. diff --git a/tests/test_api_admin.py b/tests/test_api_admin.py index b34c0df9..87181997 100644 --- a/tests/test_api_admin.py +++ b/tests/test_api_admin.py @@ -52,6 +52,7 @@ class AdminAPITestCase(AioHTTPTestCase): self.outside_dir = Path(self.tmpdir) / "outside" self.outside_dir.mkdir() + self.addAsyncCleanup(self._drain_and_remove) self.config_path = os.path.join(self.tmpdir, "config.json") self._orig_env = { key: os.environ.pop(key, None) for key in (CONFIG_PATH_ENV, EXTRA_ROOTS_ENV) @@ -89,6 +90,23 @@ class AdminAPITestCase(AioHTTPTestCase): sys.modules.pop("folder_paths", None) else: sys.modules["folder_paths"] = self._orig_folder_paths + + async def _drain_and_remove(self): + """The last cleanup: no job outlives the temporary database. + + Registered first, so it runs after every cleanup a test added (which + may release its own gated jobs). A job still running then, such as a + scan a failed assertion never awaited, is cancelled and awaited + before the database and the tree are removed. + """ + api = getattr(self, "api", None) + jobs = api._jobs.list() if api is not None else [] + tasks = [ + job.task for job in jobs if job.task is not None and not job.task.done() + ] + for task in tasks: + task.cancel() + await asyncio.gather(*tasks, return_exceptions=True) PromptModel.close_all_instances() # restore may have swapped api.db assert_closed(self, self.tmpdir) shutil.rmtree(self.tmpdir, ignore_errors=True) diff --git a/tests/test_gallery_roots.py b/tests/test_gallery_roots.py index 5ae871b4..b16ffed0 100644 --- a/tests/test_gallery_roots.py +++ b/tests/test_gallery_roots.py @@ -23,7 +23,11 @@ sys.modules.setdefault("server", _mock_server) sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) -from py.config import GalleryConfig, PromptManagerConfig # noqa: E402 +from py.config import ( # noqa: E402 + OUTSIDE_ALLOWED_REASON, + GalleryConfig, + PromptManagerConfig, +) EXTRA_ROOTS_ENV = "PROMPT_MANAGER_EXTRA_GALLERY_ROOTS" @@ -158,9 +162,10 @@ class GalleryRootTestCase(unittest.TestCase): class TestValidateGalleryRootRejects(GalleryRootTestCase): def test_filesystem_root_rejected(self): + """Refused as outside the allowed folders: no reason names the layout.""" ok, reason = GalleryConfig.validate_gallery_root(os.path.abspath(os.sep)) self.assertFalse(ok) - self.assertIn("root", reason.lower()) + self.assertEqual(reason, OUTSIDE_ALLOWED_REASON) @unittest.skipUnless(os.name == "nt", "drive roots only exist on Windows") def test_windows_drive_root_rejected(self): @@ -170,7 +175,17 @@ class TestValidateGalleryRootRejects(GalleryRootTestCase): def test_home_directory_rejected(self): ok, reason = GalleryConfig.validate_gallery_root(os.path.expanduser("~")) self.assertFalse(ok) - self.assertIn("home", reason.lower()) + self.assertEqual(reason, OUTSIDE_ALLOWED_REASON) + + def test_home_directory_inside_an_allowed_parent_is_still_refused(self): + """Even when the environment opens the home's parent, home answers + like any outside path.""" + home = os.path.expanduser("~") + os.environ[EXTRA_ROOTS_ENV] = os.path.dirname(home) + self.assertEqual( + GalleryConfig.gallery_root_problem(home), + ("outside-allowed", OUTSIDE_ALLOWED_REASON), + ) def test_directory_outside_comfyui_rejected(self): outside = Path(self.tmpdir) / "elsewhere" diff --git a/tests/test_scan_scope.py b/tests/test_scan_scope.py index fbd48b9e..f5f085f5 100644 --- a/tests/test_scan_scope.py +++ b/tests/test_scan_scope.py @@ -275,8 +275,13 @@ class TestScanScope(ScanScopeTestCase): self.assertEqual(await _scan_request(Request("")), default) self.assertEqual(await _scan_request(Request(" \n")), default) self.assertEqual( - await _scan_request(Request('{"scope": "all", "roots": [2, 0, 2]}')), - ({"files": "all", "roots": [0, 2]}, None), + await _scan_request( + Request( + '{"scope": "all", "roots": [2, 0, 2],' + ' "paths": ["output/c", "output/a", "output/c"]}' + ) + ), + ({"files": "all", "roots": {0: "output/a", 2: "output/c"}}, None), ) request, error = await _scan_request(Request("{")) self.assertIsNone(request) @@ -332,7 +337,7 @@ class TestPerSourceScope(ScanScopeTestCase): self.api._collect_output_media_sync = spy reads = self._spy_reads() - job = await self._scan({"scope": "all", "roots": [1]}) + job = await self._scan({"scope": "all", "roots": [1], "paths": ["output/b"]}) self.assertEqual(walked, [["b"]]) self.assertEqual(reads, ["in_b.png"]) @@ -354,13 +359,15 @@ class TestPerSourceScope(ScanScopeTestCase): self.assertEqual(self.api._scan_job.last_event["directories"], ["output/b"]) async def test_duplicate_roots_are_one_selection(self): - job = await self._scan({"roots": [1, 0, 1]}) + job = await self._scan( + {"roots": [1, 0, 1], "paths": ["output/b", "output/a", "output/b"]} + ) self.assertEqual(job["scope"]["roots"], [0, 1]) self.assertEqual(job["scope"]["output_folders"], 2) self.assertEqual(job["counters"]["added"], 2) async def test_scope_line_counts_selected_folders(self): - job = await self._scan({"roots": [0]}) + job = await self._scan({"roots": [0], "paths": ["output/a"]}) self.assertTrue( job["log"][0].endswith( "Scan started · 1 output folder · new and unresolved files" @@ -375,11 +382,43 @@ class TestPerSourceScope(ScanScopeTestCase): ) self.assertIn("Scan started · 2 output folders · all files", unscoped["log"][0]) - async def test_invalid_root_index_is_400_without_job(self): - data = await self._refused({"roots": [2]}) - self.assertEqual(data["code"], "unknown-source") - data = await self._refused({"roots": [0, 64]}) - self.assertEqual(data["code"], "unknown-source") + async def _stale(self, body): + resp = await self.client.request("POST", "/prompt_manager/jobs/scan", json=body) + data = await resp.json() + self.assertEqual(resp.status, 409, data) + self.assertEqual(data["code"], "sources-changed") + self.assertFalse(data["success"]) + self.assertNotIn(self.tmpdir, json.dumps(data)) + self.assertIsNone(self.api._scan_job) + self.assertEqual(self.api._jobs.list(), []) + return data + + async def test_index_past_the_sources_is_409_without_job(self): + await self._stale({"roots": [2], "paths": ["output/c"]}) + await self._stale({"roots": [0, 64], "paths": ["output/a", "output/x"]}) + + async def test_stale_index_never_scans_another_source(self): + """The list changed after the page read it: source 0 was removed, so + old index 1 (b) is past the end and old index 0 now names b.""" + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.b)] + await self._stale({"roots": [1], "paths": ["output/b"]}) + await self._stale({"roots": [0], "paths": ["output/a"]}) + + job = await self._scan({"roots": [0], "paths": ["output/b"]}) + self.assertEqual(job["outcomes"]["added"][0]["root"], "output/b") + + async def test_roots_need_their_paths(self): + for body in ( + {"roots": [0]}, + {"roots": [0], "paths": None}, + {"roots": [0], "paths": "output/a"}, + {"roots": [0], "paths": []}, + {"roots": [0, 1], "paths": ["output/a"]}, + {"roots": [0], "paths": [0]}, + {"roots": [0, 0], "paths": ["output/a", "output/b"]}, + ): + with self.subTest(body=body): + await self._refused(body) async def test_bool_and_float_roots_rejected(self): for roots in ( @@ -399,7 +438,8 @@ class TestPerSourceScope(ScanScopeTestCase): list(range(65)), ): with self.subTest(roots=roots): - await self._refused({"roots": roots}) + paths = ["output/a"] * len(roots) if isinstance(roots, list) else [] + await self._refused({"roots": roots, "paths": paths}) async def test_legacy_scan_unscoped_unchanged(self): from py.api.admin import SCAN_COUNT_KEYS @@ -429,16 +469,29 @@ class TestPerSourceScope(ScanScopeTestCase): return original(output_dirs) self.api._collect_output_media_sync = gated + # The legacy route has decided to attach once it streams the job. + attached = asyncio.Event() + stream = self.api._stream_scan_job + + async def attaching(request, job): + attached.set() + return await stream(request, job) + + self.api._stream_scan_job = attaching resp = await self.client.request( - "POST", "/prompt_manager/jobs/scan", json={"roots": [0]} + "POST", + "/prompt_manager/jobs/scan", + json={"roots": [0], "paths": ["output/a"]}, ) self.assertEqual(resp.status, 202) started = (await resp.json())["job"] legacy = asyncio.ensure_future( self.client.request("POST", "/prompt_manager/scan", data="{}") ) - await asyncio.sleep(0.05) - gate.set() + try: + await asyncio.wait_for(attached.wait(), 10) + finally: + gate.set() payloads = _sse_payloads(await (await legacy).text()) await asyncio.wait_for(self.api._scan_job.task, 10) @@ -459,7 +512,7 @@ class TestPerSourceScope(ScanScopeTestCase): return original() self.api._get_all_output_dirs = counting - job = await self._scan({"roots": [1]}) + job = await self._scan({"roots": [1], "paths": ["output/b"]}) self.assertEqual(len(resolved), 1) # the request's own resolution only self.assertEqual(job["outcomes"]["added"][0]["root_index"], 1) diff --git a/tests/test_settings_sources.py b/tests/test_settings_sources.py index 23456058..6998c82b 100644 --- a/tests/test_settings_sources.py +++ b/tests/test_settings_sources.py @@ -8,10 +8,14 @@ checks one path and writes nothing. No response carries an absolute path. """ import asyncio +import functools import json import ntpath import os +import sys +import types import unittest +from contextlib import contextmanager from unittest.mock import patch try: @@ -19,10 +23,40 @@ try: except ImportError: # discovered with tests/ as the top-level directory from test_api_admin import AdminAPITestCase +from py import config as config_module # noqa: E402 from py.api import settings_routes, sources # noqa: E402 from py.config import GalleryConfig # noqa: E402 VALIDATE = "/prompt_manager/settings/validate-source" +SETTINGS = "/prompt_manager/settings" + + +@contextmanager +def no_filesystem_access_for(marker, test): + """Fail ``test`` if anything resolves, stats or opens a path holding ``marker``. + + Trusted directories (ComfyUI's, the anchors) may still be resolved. + """ + real = { + "realpath": os.path.realpath, + "stat": os.stat, + "lstat": os.lstat, + } + + def guard(name): + def call(path, *args, **kwargs): + if isinstance(path, (str, bytes)) and marker in os.fsdecode(path): + test.fail(f"{name}({path!r}) touched an outside path") + return real[name](path, *args, **kwargs) + + return call + + with ( + patch("os.path.realpath", guard("realpath")), + patch("os.stat", guard("stat")), + patch("os.lstat", guard("lstat")), + ): + yield class FakeMonitor: @@ -188,12 +222,12 @@ class TestSourceStatus(SourcesTestCase): ) # Monitoring switched off: the live observer still watches until the - # restart, and nothing else is pending. + # restart ("stopping"), and nothing else is pending. GalleryConfig.MONITORING_ENABLED = False data = await self._get() self.assertFalse(data["settings"]["monitoring_enabled"]) self.assertEqual( - self._states(data), {"output/a": "watching", "output/b": "manual"} + self._states(data), {"output/a": "stopping", "output/b": "manual"} ) async def test_scheduled_root_matches_through_its_spelling(self): @@ -343,9 +377,7 @@ class TestSourceChangeHold(SourcesTestCase): async def test_mixed_case_windows_paths_dedupe(self): """Two spellings of one Windows folder are saved once (normcase).""" with ( - patch.object( - GalleryConfig, "validate_gallery_root", return_value=(True, "") - ), + patch.object(sources, "source_problem", return_value=(None, "")), patch.object( GalleryConfig, "resolve_gallery_root", side_effect=lambda path: path ), @@ -457,15 +489,139 @@ class TestValidateSource(SourcesTestCase): self.assertEqual([answer for answer in answers], [answers[0]] * len(answers)) async def test_settings_save_refuses_outside_paths_alike(self): + """The legacy save answers every outside path alike too: the + filesystem root, the home directory and a link to it included.""" (self.outside_dir / "x").mkdir() + link = os.path.join(self.tmpdir, "link-home") + try: + os.symlink(os.path.expanduser("~"), link, target_is_directory=True) + except (OSError, NotImplementedError): + link = None + paths = [ + str(self.outside_dir / "x"), + self.tmpdir + "/nothing/x", + os.path.abspath(os.sep), + os.path.expanduser("~"), + ] + if link: + paths.append(link) answers = [] - for path in (self.outside_dir / "x", self.tmpdir + "/nothing/x"): - status, data = await self._post( - "/prompt_manager/settings", {"gallery_root_paths": [str(path)]} + for path in paths: + resp = await self.client.request( + "POST", SETTINGS, json={"gallery_root_paths": [path]} ) - answers.append((status, data)) - self.assertEqual(answers[0][0], 400) - self.assertEqual(answers[0], answers[1]) + body = await resp.text() + self.assertNotIn(os.path.expanduser("~"), body) + data = json.loads(body) + # Only the name the caller sent may differ. + answers.append( + ( + resp.status, + {k: v for k, v in data.items() if k not in ("source", "error")}, + ) + ) + self.assertEqual( + data["error"], + f"{data['source']} can't be used: " + f"{config_module.OUTSIDE_ALLOWED_REASON}. Remove it, then save.", + ) + self.assertEqual( + answers[0], + ( + 400, + { + "success": False, + "code": "invalid-source", + "index": 0, + "reason": config_module.OUTSIDE_ALLOWED_REASON, + }, + ), + ) + self.assertEqual(answers, [answers[0]] * len(answers)) + self.assertEqual(GalleryConfig.MONITORING_DIRECTORIES, []) + + def test_every_caller_folds_root_and_home_alike(self): + for path in (os.path.abspath(os.sep), os.path.expanduser("~")): + with self.subTest(path=path): + self.assertEqual( + GalleryConfig.gallery_root_problem(path), + ("outside-allowed", config_module.OUTSIDE_ALLOWED_REASON), + ) + self.assertEqual( + GalleryConfig.validate_gallery_root(path), + (False, config_module.OUTSIDE_ALLOWED_REASON), + ) + self.assertEqual( + sources.validate_source(path)["code"], "outside-allowed" + ) + + async def test_outside_path_is_never_resolved_or_stat(self): + """Containment is decided as text first: no realpath, stat or lstat of + an outside path, existing or not, by any caller (I2).""" + probe = self.outside_dir / "probe-x" + probe.mkdir() + (probe / "deep").mkdir() + cases = [str(probe), str(probe / "deep"), str(probe / "missing")] + with no_filesystem_access_for("probe-x", self): + for path in cases: + with self.subTest(path=path): + self.assertEqual( + GalleryConfig.gallery_root_problem(path)[0], "outside-allowed" + ) + self.assertEqual( + sources.validate_source(path)["code"], "outside-allowed" + ) + _roots, error = self.api._parse_gallery_roots( + {"gallery_root_paths": [path]} + ) + self.assertEqual(error["code"], "invalid-source") + status, data = await self._post(VALIDATE, {"path": path}) + self.assertEqual(data["code"], "outside-allowed") + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [path]} + ) + self.assertEqual((status, data["code"]), (400, "invalid-source")) + # A traversal that normalises outside is refused as text too. + sneaky = os.path.join( + str(self.output_dir), "..", "..", "outside", "probe-x" + ) + self.assertEqual( + GalleryConfig.gallery_root_problem(sneaky)[0], "outside-allowed" + ) + + async def test_settings_save_applies_the_form_checks(self): + """POST /settings refuses what validate-source refuses, by its form.""" + for path, code in { + str(self.a) + "\x00": "invalid-form", + os.path.join(str(self.output_dir), "b", "..", "a"): "traversal", + }.items(): + with self.subTest(code=code): + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [path]} + ) + self.assertEqual((status, data["code"]), (400, "invalid-source")) + self.assertEqual(data["reason"], sources.FORM_REASONS[code]) + windows = functools.partial(sources.source_form_error, windows=True) + with ( + patch.object(sources, "source_form_error", windows), + no_filesystem_access_for("C:", self), + ): + for path in ( + "\\\\?\\C:\\ComfyUI\\output", + "\\\\.\\pipe\\x", + "C:\\ComfyUI\\output\\CON", + "C:\\ComfyUI\\output\\a:stream", + "C:ComfyUI", + ): + with self.subTest(path=path): + _roots, error = self.api._parse_gallery_roots( + {"gallery_root_paths": ["output/a", path]} + ) + self.assertEqual(error["code"], "invalid-source") + self.assertEqual(error["index"], 1) + self.assertEqual( + error["reason"], sources.FORM_REASONS["invalid-form"] + ) async def test_inside_paths_say_why_they_are_not_usable(self): a_file = self.output_dir / "file.png" @@ -535,6 +691,218 @@ class TestSourceEdges(SourcesTestCase): self.assertEqual(sources.source_label(self.output_dir), "output") +class TestOutsideBaseDirectories(SourcesTestCase): + """ComfyUI folders outside the base directory (``--output-directory``): + their sources round-trip and two folders of one name stay apart (I3).""" + + async def asyncSetUp(self): + await super().asyncSetUp() + self.renders = self.outside_dir / "renders" + self.input_dir = self.comfy_dir / "input" + (self.renders / "portraits").mkdir(parents=True) + (self.input_dir / "portraits").mkdir(parents=True) + sys.modules["folder_paths"] = types.SimpleNamespace( + base_path=str(self.comfy_dir), + get_output_directory=lambda: str(self.renders), + get_input_directory=lambda: str(self.input_dir), + ) + + async def test_source_outside_base_round_trips(self): + status, rendered = await self._post( + VALIDATE, {"path": str(self.renders / "portraits")} + ) + self.assertEqual( + (rendered["valid"], rendered["path"]), (True, "renders/portraits") + ) + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [rendered["path"]]} + ) + self.assertEqual(status, 200, data) + self.assertEqual( + GalleryConfig.MONITORING_DIRECTORIES, + [self._root(self.renders / "portraits")], + ) + configured = (await self._get())["sources"]["configured"] + self.assertEqual( + [(row["path"], row["valid"]) for row in configured], + [("renders/portraits", True)], + ) + # The page resends what it read: still the same folder, no change. + status, data = await self._post( + SETTINGS, {"gallery_root_paths": ["renders/portraits"]} + ) + self.assertEqual((status, data["restart_required_fields"]), (200, [])) + + async def test_same_name_folders_stay_apart(self): + answers = [] + for path in (self.renders / "portraits", self.input_dir / "portraits"): + status, data = await self._post(VALIDATE, {"path": str(path)}) + answers.append(data["path"]) + self.assertEqual(answers, ["renders/portraits", "input/portraits"]) + status, data = await self._post(SETTINGS, {"gallery_root_paths": answers}) + self.assertEqual(status, 200, data) + sources_ = (await self._get())["sources"] + self.assertEqual([row["path"] for row in sources_["configured"]], answers) + self.assertEqual([row["path"] for row in sources_["effective"]], answers) + + +class TestUnusableSourceSave(SourcesTestCase): + async def test_unusable_source_is_named_and_saves_nothing(self): + """A configured source that went missing blocks the save and is named.""" + gone = self.output_dir / "gone" + gone.mkdir() + GalleryConfig.MONITORING_DIRECTORIES = [self._root(self.a), self._root(gone)] + self._write_config({"custom": 1}) + before = self._config_bytes() + gone.rmdir() # an unplugged drive + + status, data = await self._post( + SETTINGS, + { + "gallery_root_paths": ["output/a", "output/gone", "output/b"], + "worker_threads": 2, + }, + ) + + self.assertEqual(status, 400, data) + self.assertEqual( + data, + { + "success": False, + "code": "invalid-source", + "source": "output/gone", + "index": 1, + "reason": "Gallery root does not exist", + "error": "output/gone can't be used: Gallery root does not exist. " + "Remove it or reconnect it, then save.", + }, + ) + self.assertEqual(self._config_bytes(), before) + + async def test_malformed_roots_are_coded_invalid_request(self): + for roots in ("output/a", [1]): + with self.subTest(roots=roots): + status, data = await self._post(SETTINGS, {"gallery_root_paths": roots}) + self.assertEqual((status, data["code"]), (400, "invalid-request")) + + +class TestUnchangedRootsTakeNoHold(SourcesTestCase): + async def test_resent_spelling_of_configured_roots_is_no_change(self): + """config.json holds a hand-written spelling; the legacy page resends + it resolved: no source change, so no hold and no 409 during a scan (M1).""" + GalleryConfig.MONITORING_DIRECTORIES = ["output/a"] + gate = asyncio.Event() + + async def runner(job): + await gate.wait() + job.finish("finished") + + self.api._jobs.start( + "scan", "Metadata scan", record_changing=True, runner=runner + ) + try: + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [str(self.a)], "result_timeout": 30} + ) + self.assertEqual(status, 200, data) + self.assertEqual(data["restart_required_fields"], []) + self.assertEqual([j.kind for j in self.api._jobs.list()], ["scan"]) + status, data = await self._post( + SETTINGS, {"gallery_root_paths": [str(self.b)]} + ) + self.assertEqual((status, data["code"]), (409, "busy")) + finally: + gate.set() + + +class TestWindowsContainment(unittest.TestCase): + """Windows path forms, with ntpath fakes: anything outside the allowed + folders (a UNC share, the verbatim and device namespaces, another drive) + is refused as text, before any path is resolved or opened (I2).""" + + ALLOWED = "c:\\comfyui\\output" + NAS = "\\\\nas\\share\\renders" + + def setUp(self): + self.touched = [] + + def realpath(path, *args, **kwargs): + self.touched.append(path) + return ntpath.normcase(ntpath.normpath(path)) + + def probe(path): + self.touched.append(path) + return True + + names = ("normcase", "normpath", "isabs", "join", "dirname", "basename", "sep") + fake_path = types.SimpleNamespace( + **{name: getattr(ntpath, name) for name in names}, + realpath=realpath, + exists=probe, + isdir=probe, + expanduser=lambda path: "c:\\users\\someone", + ) + fake_os = types.SimpleNamespace( + path=fake_path, environ={}, pathsep=";", name="nt" + ) + for target, value in ( + (patch.object(config_module, "os", fake_os), None), + ( + patch.object( + GalleryConfig, + "_allowed_parent_pairs", + classmethod( + lambda cls: [(self.ALLOWED, self.ALLOWED), (self.NAS, self.NAS)] + ), + ), + None, + ), + ( + patch.object( + GalleryConfig, + "path_anchors", + classmethod(lambda cls: ["c:\\comfyui", "\\\\nas\\share"]), + ), + None, + ), + ): + target.start() + self.addCleanup(target.stop) + + def test_outside_forms_are_refused_untouched(self): + for path in ( + "\\\\attacker\\share\\x", + "//attacker/share/x", + "\\\\?\\C:\\ComfyUI\\output\\a", + "\\\\?\\UNC\\attacker\\share", + "\\\\.\\pipe\\x", + "\\\\.\\C:\\ComfyUI\\output", + "D:\\Renders", + "C:\\Windows", + "C:\\ComfyUI\\output\\..\\..\\Windows", + "C:\\ComfyUI\\output2", + "\\\\nas\\share\\other", + "..\\..\\Windows", + ): + with self.subTest(path=path): + self.touched.clear() + self.assertEqual( + GalleryConfig.gallery_root_problem(path), + ("outside-allowed", config_module.OUTSIDE_ALLOWED_REASON), + ) + self.assertEqual(self.touched, []) + + def test_allowed_spellings_are_accepted(self): + for path in ( + "C:\\ComfyUI\\output\\a", + "c:/comfyui/OUTPUT/a", + "output\\a", + "\\\\NAS\\share\\renders\\a", # a UNC spelling of an allowed extra root + ): + with self.subTest(path=path): + self.assertEqual(GalleryConfig.gallery_root_problem(path), (None, "")) + + class TestSourceFormError(unittest.TestCase): """The lexical check that runs before any filesystem access."""