diff --git a/.gitignore b/.gitignore index bbc3285..aa6eda3 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,9 @@ how-to-publish-comfyui-node.md fable-review-2026-07-02.md opus-review-craftkit-2026-07-02.md +# Local dev-only tooling, not part of the published node package +check_registry_status.bat + # Claude Code session data .claude/ diff --git a/__init__.py b/__init__.py index 2d633be..70ca568 100644 --- a/__init__.py +++ b/__init__.py @@ -111,7 +111,12 @@ $r try: result = subprocess.run( [exe, "-NoProfile", "-Command", ps], - capture_output=True, text=True, timeout=300 + capture_output=True, text=True, timeout=300, + # Without this, launching powershell.exe pops up its own + # visible console window behind the folder dialog (ComfyUI + # itself doesn't reuse it) — the dialog is the only UI this + # is meant to show. + creationflags=subprocess.CREATE_NO_WINDOW, ) except FileNotFoundError: continue @@ -153,6 +158,15 @@ $r return web.json_response({"ok": False, "error": folder.removeprefix(_DIALOG_ERROR_PREFIX)}, status=500) if folder and os.path.isdir(folder): + # The dialog's click-OK just happened in the operating system, not + # in this request, so this is the one place a folder can be + # authorized for SmartBatchResize's input_folder — see + # craftkit_folder_guard.py. + try: + from .craftkit_folder_guard import remember_folder + remember_folder(folder) + except Exception as e: + print(f"[CraftKit] Could not remember approved folder ({e}); it will need re-approving.") return web.json_response({"ok": True, "path": folder}) return web.json_response({"ok": False, "cancelled": True}) diff --git a/craftkit_folder_guard.py b/craftkit_folder_guard.py new file mode 100644 index 0000000..7ec2bde --- /dev/null +++ b/craftkit_folder_guard.py @@ -0,0 +1,286 @@ +"""Containment guard for nodes that take a free-text folder path (SmartBatchResize's +`input_folder`). + +WHY THIS EXISTS +---------------- +ComfyUI's /prompt endpoint is unauthenticated, so any STRING widget value is +effectively attacker-controlled: a malicious webpage open in the same browser +can POST a crafted prompt to http://127.0.0.1:8188/prompt via CSRF. A node +that reads/writes an arbitrary user-supplied folder is therefore an +unconfined filesystem read/write reachable without any real authorization — +exactly what got CraftKit 1.0.1-1.1.1 banned from the registry (HIGH severity, +see smart_batch_resize.py:246-263 in the pre-fix version). + +THE FIX: a folder is only usable if one of these holds: + a) it's inside ComfyUI's own input/output/temp directories, or + b) the user picked it in the native OS folder dialog at some point (the + click-OK happens in the operating system, outside anything a request + can reach — that's the one signal we have that a human authorized it), or + c) "allow_any": true was hand-edited into the config file (never settable + via any route — a route that could flip it would be a one-call bypass). + +Config lives at /craftkit/allowed_folders.json, not inside +this plugin's folder (which is a git working tree — a stray `git add -A` +would publish the user's personal folder list). + +Pattern adapted from Pixaroma's ComfyUI-Pixaroma/nodes/_path_guard.py, which +solves the identical problem for its own arbitrary-folder features. +""" +from __future__ import annotations + +import json +import os +import sys +import threading + +_LOCK = threading.Lock() +_CONFIG_NAME = "allowed_folders.json" + + +def is_path_under(child: str, *parents: str) -> bool: + """True iff `child` resolves inside ANY of `parents`. + + Strict realpath-on-both-sides is the primary (and usually only) test. A + lexical (abspath) fallback runs only when the strict comparison can't even + be made because the two sides land on different drives (e.g. a models + folder junctioned onto another disk) — os.path.commonpath raises + ValueError in that case rather than just returning False. + """ + if not child or not isinstance(child, str): + return False + try: + child_real = os.path.realpath(child) + child_abs = os.path.abspath(child) + except (OSError, ValueError, TypeError): + return False + for p in parents: + if not p or not isinstance(p, str): + continue + cross_drive = False + try: + parent_real = os.path.realpath(p) + if os.path.commonpath([child_real, parent_real]) == parent_real: + return True + except ValueError: + cross_drive = True + except (OSError, TypeError): + continue + if not cross_drive: + continue + try: + parent_abs = os.path.abspath(p) + if os.path.commonpath([os.path.normcase(child_abs), os.path.normcase(parent_abs)]) == os.path.normcase(parent_abs): + return True + except (OSError, ValueError, TypeError): + continue + return False + + +def _norm_raw(p) -> str: + if not p or not isinstance(p, str): + return "" + return p.strip().strip('"').strip("'").strip() + + +def unc_like(p) -> bool: + r"""True for a Windows UNC path (\\server\share, //server/share). + + Purely lexical — must not touch the filesystem. Merely calling + os.path.realpath/isdir on a UNC path makes Windows open an SMB connection + and hand over an NTLM hash before any containment check gets to run, so a + UNC value has to be judged before it's resolved, not after. + """ + return _norm_raw(p).replace("/", "\\").startswith("\\\\") + + +def is_unc_root(p) -> bool: + r"""True when `p` is exactly a UNC share root (\\server\share, no subpath). + + os.path.commonpath can't compare such a root against a path inside it + (splitdrive gives an empty, "relative"-looking remainder), so it has to be + matched by prefix instead — see _lexically_under_any. + """ + try: + drive, rest = os.path.splitdrive(os.path.abspath(p)) + except (OSError, ValueError, TypeError): + return False + return drive.replace("/", "\\").startswith("\\\\") and rest.strip("\\/") == "" + + +def _lexically_under_any(child: str, parents) -> bool: + """Prefix test on abspath only — no realpath, so no filesystem access. + Weaker than is_path_under (a symlink could fool it); used only to accept a + UNC path under a share root the user already approved via the picker.""" + child = _norm_raw(child) + if not child: + return False + try: + c = os.path.normcase(os.path.abspath(child)) + except (OSError, ValueError, TypeError): + return False + for p in parents or (): + p = _norm_raw(p) + if not p: + continue + try: + pa = os.path.normcase(os.path.abspath(p)).rstrip("\\/") + except (OSError, ValueError, TypeError): + continue + if pa and (c == pa or c.startswith(pa + os.sep) or c.startswith(pa + "/")): + return True + return False + + +def comfy_roots() -> list: + """ComfyUI's own input/output/temp directories, whichever resolve. + + folder_paths is imported lazily so this module stays importable outside a + live ComfyUI process (e.g. under pytest). + """ + roots = [] + try: + import folder_paths + except Exception: + return roots + for getter in ("get_input_directory", "get_output_directory", "get_temp_directory"): + try: + fn = getattr(folder_paths, getter, None) + v = fn() if callable(fn) else None + if v: + roots.append(v) + except Exception: + continue + return roots + + +def _config_path() -> str: + base = None + try: + import folder_paths + base = folder_paths.get_user_directory() + except Exception: + base = None + if not base: + base = os.path.join(os.path.expanduser("~"), ".craftkit") + return os.path.join(base, "craftkit", _CONFIG_NAME) + + +def _read_config() -> dict: + """Never raises — a missing or damaged file means 'nothing extra is + allowed', never an exception mid-run. `damaged` lets remember_folder + refuse to write, rather than silently overwriting a corrupt-but-recoverable + config and wiping every previously approved folder.""" + out = {"folders": [], "allow_any": False, "damaged": False} + path = _config_path() + if not os.path.exists(path): + return out + try: + # utf-8-sig transparently strips a BOM if present (e.g. a file written + # by PowerShell's `Set-Content -Encoding utf8`, or hand-edited in an + # older Notepad) — plain utf-8 would otherwise fail to parse it and + # this file would wrongly read as "damaged". + with open(path, "r", encoding="utf-8-sig") as f: + obj = json.load(f) + except Exception: + out["damaged"] = True + return out + if not isinstance(obj, dict): + out["damaged"] = True + return out + folders = obj.get("folders") + if isinstance(folders, list): + out["folders"] = [x for x in folders if isinstance(x, str) and x.strip()] + elif folders is not None: + out["damaged"] = True + out["allow_any"] = obj.get("allow_any") is True + return out + + +def dialog_available() -> bool: + if os.name == "nt" or sys.platform == "darwin": + return True + return bool(os.environ.get("DISPLAY") or os.environ.get("WAYLAND_DISPLAY")) + + +def remember_folder(path: str) -> bool: + """Add a folder the user picked IN THE NATIVE OS DIALOG to the allowlist. + + Call this ONLY from the native-picker route (__init__.py's browse_folder), + never from anywhere an ordinary request body can reach — the dialog's + click-OK is the one signal that a human, not a request, chose this path. + """ + if not path or not isinstance(path, str): + return False + try: + real = os.path.realpath(path) + except (OSError, ValueError, TypeError): + return False + if not os.path.isdir(real): + return False + with _LOCK: + cfg = _read_config() + if cfg["damaged"]: + return False + if is_path_under(real, *cfg["folders"]): + return True # already covered by an approved parent + kept = [f for f in cfg["folders"] if not is_path_under(f, real)] + kept.append(real) + path = _config_path() + try: + os.makedirs(os.path.dirname(path), exist_ok=True) + tmp = path + ".tmp" + with open(tmp, "w", encoding="utf-8") as f: + json.dump({"folders": kept, "allow_any": cfg["allow_any"]}, f, indent=2) + os.replace(tmp, path) + except Exception: + return False + return True + + +def prescreen(raw) -> bool: + """Cheap no-filesystem screen to run BEFORE anything calls realpath/isdir + on an untrusted folder string. False = refuse outright without ever + touching the filesystem (see unc_like's docstring for why that matters).""" + cfg = _read_config() + if cfg["allow_any"]: + return True + if unc_like(raw): + return _lexically_under_any(raw, cfg["folders"]) + return True + + +def folder_allowed(path: str) -> bool: + """True when `path` is somewhere SmartBatchResize may read/write.""" + if not path or not isinstance(path, str): + return False + if not prescreen(path): + return False + cfg = _read_config() + if cfg["allow_any"]: + return True + roots = comfy_roots() + if roots and is_path_under(path, *roots): + return True + folders = cfg["folders"] + if not folders: + return False + if is_path_under(path, *folders): + return True + unc_roots = [f for f in folders if is_unc_root(f)] + return bool(unc_roots) and _lexically_under_any(path, unc_roots) + + +def denied_message(path: str) -> str: + browse = "click the Browse button on the node and pick this folder once — that approves it permanently." + manual = ( + "add it to \"folders\" (or set \"allow_any\": true) in\n {config}" + ).format(config=_config_path()) + if dialog_available(): + how = f"To approve it, {browse}\nNo folder dialog available (headless install)? Instead {manual}" + else: + how = f"No folder dialog on this machine (headless install), so approve it by hand: {manual}" + return ( + f"[SmartBatchResize] Folder not approved: {path}\n" + f"{how}\n" + "ComfyUI's own input/output/temp folders always work without approval." + ) diff --git a/pyproject.toml b/pyproject.toml index 2585ee8..cad44ae 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,13 +1,16 @@ [project] name = "comfyui-craftkit" description = "Smart resize, batch resize, resolution multiplier and prompt controller nodes for ComfyUI" -version = "1.1.3" +version = "1.1.4" readme = "README.md" requires-python = ">=3.10" license = { file = "LICENSE" } # Pillow, NumPy, and PyTorch all ship with ComfyUI itself — intentionally not # re-declared here to avoid version conflicts with ComfyUI's own pins. dependencies = [] +classifiers = [ + "Operating System :: OS Independent", +] [project.urls] Repository = "https://github.com/CraftopiaStudio/ComfyUI-CraftKit" diff --git a/smart_batch_resize.py b/smart_batch_resize.py index afe432a..b9a177d 100644 --- a/smart_batch_resize.py +++ b/smart_batch_resize.py @@ -13,6 +13,8 @@ try: except ImportError: ProgressBar = None +from .craftkit_folder_guard import folder_allowed, denied_message + INTERP_MAP = { "lanczos": PILImage.LANCZOS, @@ -208,7 +210,7 @@ class SmartBatchResize: # are added/removed on its own. Hash the listing so unrelated graph runs # can still hit the cache, while an actual folder change invalidates it. folder = input_folder.strip() - if not folder or not os.path.isdir(folder): + if not folder or not folder_allowed(folder) or not os.path.isdir(folder): return "" try: files = sorted( @@ -246,6 +248,8 @@ class SmartBatchResize: input_folder = input_folder.strip() if not input_folder: raise ValueError("[SmartBatchResize] No folder selected. Enter a path or use the Browse button.") + if not folder_allowed(input_folder): + raise ValueError(denied_message(input_folder)) if not os.path.isdir(input_folder): raise ValueError(f"[SmartBatchResize] Folder not found: {input_folder}")