Confine SmartBatchResize's input_folder to approved folders (1.1.4)
The registry's HIGH-severity finding was never the COM/folder-picker code we suspected in 1.1.1-1.1.3 — it's smart_batch_resize.py's input_folder widget accepting any path and writing to it, an unconfined filesystem write reachable via unauthenticated POST /prompt. Found via the undocumented ?include_status_reason=true parameter on the versions API. Adds craftkit_folder_guard.py (pattern adapted from Pixaroma's _path_guard.py): a folder is only usable if it's inside ComfyUI's own input/output/temp directories, or the user picked it in the native OS folder dialog at some point (the click-OK happens outside anything a request can reach), or allow_any is hand-edited into the config. Also fixes a PowerShell console window flashing open behind the folder dialog (missing CREATE_NO_WINDOW).
This commit is contained in:
@@ -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/
|
||||
|
||||
|
||||
+15
-1
@@ -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})
|
||||
|
||||
|
||||
@@ -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 <ComfyUI user dir>/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."
|
||||
)
|
||||
+4
-1
@@ -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"
|
||||
|
||||
@@ -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}")
|
||||
|
||||
|
||||
Reference in New Issue
Block a user