rubz
dca216b0a2
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).
287 lines
10 KiB
Python
287 lines
10 KiB
Python
"""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."
|
|
)
|