From 1befdca2e797dc3766c002ece3a5e81e543e7cd0 Mon Sep 17 00:00:00 2001 From: Xiaodoudou Date: Thu, 8 Oct 2026 16:35:36 +0800 Subject: [PATCH] RigShare 1.3.1: refuse folder paths outside the workspace (Windows) * RigShare 1.3.1: refuse folder paths outside the workspace (Windows) * Tests: escape test no longer assumes Windows path semantics --- CHANGELOG.md | 14 +++++++ pyproject.toml | 2 +- rigshare_core/store.py | 10 +++-- rigshare_core/workspace.py | 70 +++++++++++++++++++++++++++++----- tests/python/test_jwt.py | 60 +++++++++++++++++++++++++++++ tests/python/test_server.py | 20 ++++++++++ tests/python/test_stats.py | 45 ++++++++++++++++++++++ tests/python/test_workspace.py | 59 ++++++++++++++++++++++++++++ 8 files changed, 267 insertions(+), 13 deletions(-) create mode 100644 tests/python/test_jwt.py create mode 100644 tests/python/test_stats.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4228a7f..9d1d784 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # Changelog +## 1.3.1 (2026-10-08) + +Security hardening of the workspace folder routes, mainly for Windows servers. + +### Fixed + +- Folder rename, move, delete and create (and the workspace listing) could be pointed outside the workflows folder with a Windows drive prefix (`C:`), which let `rmtree` / `rename` act on folders such as models or custom_nodes. Every path is now checked before it touches the disk: segments with a colon, backslash tricks or `..` are refused, and the resolved path must stay inside the workflows folder +- On Windows, case and aliases of a path were not seen by the access checks: `Users/alice`, `users./alice` or `USERS~1/alice` opened Alice's private folder as if it were common, and `Shared/` skipped its access list. Paths are now matched to the name on disk and trailing dots, spaces and 8.3 short names are dropped before any permission check +- New usernames and folder names can no longer end in a dot or space, start with a dot, or be a Windows device name (`CON`, `NUL`, `COM1`...). Usernames that differ only by case count as the same name, since they share one folder on Windows. Existing accounts and folders are untouched + +### Added + +- Tests for the JWT module, the server stats, and the folder routes refusing paths outside the workspace + ## 1.3.0 (2026-10-04) A Performance panel, a floating monitor that docks, and Manager in its own tab. diff --git a/pyproject.toml b/pyproject.toml index 193f030..ae71c23 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,7 +1,7 @@ [project] name = "comfyui-rigshare" description = "Turn one ComfyUI server into a shared team workspace. Sign-in with accounts, permissions (edit, queue, Manager, admin) and personal API keys, enforced by the server. A private folder for each person plus shared folders with access lists, with Save asking where. Live co-editing of every shared workflow, with cursors, follow mode and conflict-free merges, in the graph or App view. Per-workflow history with a snapshot on every save, team chat with its whole history, and a CPU, RAM, GPU and queue monitor." -version = "1.3.0" +version = "1.3.1" license = { file = "LICENSE" } dependencies = [] diff --git a/rigshare_core/store.py b/rigshare_core/store.py index f157fa4..6e3a519 100644 --- a/rigshare_core/store.py +++ b/rigshare_core/store.py @@ -24,7 +24,7 @@ SESSION_TTL = 30 * 24 * 3600 # a login lasts at most this long TOKEN_TTL = 12 * 3600 # each JWT is short-lived... TOKEN_REFRESH = 6 * 3600 # ...and re-issued while in use once older than this ISSUER = "rigshare" -USERNAME_RE = re.compile(r"^[A-Za-z0-9_.-]{2,32}$") +USERNAME_RE = re.compile(r"^(?!(?i:con|prn|aux|nul|com\d|lpt\d)(?:\.|$))[A-Za-z0-9_][A-Za-z0-9_.-]{0,30}[A-Za-z0-9_-]\Z") # no leading/trailing dot, no Windows device names PERMS = ("edit", "queue", "manager", "admin") API_KEY_PREFIX = "rs_" @@ -216,10 +216,14 @@ class Store: _verify_password(password, _hash_password("x", b"0" * 16)) return None + def _name_taken(self, name, ignore=None): + """Names differing only by case share one folder on Windows, so they count as the same.""" + return any(u.casefold() == name.casefold() for u in self.users if u != ignore) + def create_user(self, username, password, display_name=None, perms=None): if not USERNAME_RE.match(username or ""): raise ValueError("Username must be 2-32 characters: letters, digits, _ . -") - if username in self.users: + if self._name_taken(username): raise ValueError("Username already exists") if len(password or "") < 8: raise ValueError("Password must be at least 8 characters") @@ -239,7 +243,7 @@ class Store: raise KeyError(username) if not USERNAME_RE.match(new_username): raise ValueError("Username must be 2-32 characters: letters, digits, _ . -") - if new_username in self.users: + if self._name_taken(new_username, ignore=username): raise ValueError("Username already exists") with self._lock: user = self.users.pop(username) diff --git a/rigshare_core/workspace.py b/rigshare_core/workspace.py index 7a8b04b..32bcccf 100644 --- a/rigshare_core/workspace.py +++ b/rigshare_core/workspace.py @@ -20,7 +20,7 @@ import time from urllib.parse import unquote WORKFLOWS = "workflows" -FOLDER_NAME_RE = re.compile(r"^[\w][\w .()-]{0,63}$") +FOLDER_NAME_RE = re.compile(r"^(?!(?i:con|prn|aux|nul|com\d|lpt\d)(?:\.|$))\w(?:[\w .()-]{0,62}[\w()-])?\Z") # no trailing dot/space, no Windows device names ROLE_RANK = {None: 0, "view": 1, "edit": 2} @@ -35,10 +35,60 @@ def full_unquote(value): return value +_WIN = os.name == "nt" +_SHORT_NAME_RE = re.compile(r"~\d") # Windows 8.3 alias of a long name (USERS~1) +_case_root = None # workflows directory used to restore on-disk casing (Windows only) + + +def _on_disk_case(parts, base): + """Windows ignores case: map each segment to the casing it has on disk, so 'Users/Bob' is + judged as the same path it will open.""" + out = [] + for part in parts: + try: + names = os.listdir(base) if base else [] + except OSError: + names = [] + match = part if part in names else next((n for n in names if n.casefold() == part.casefold()), part) + out.append(match) + base = os.path.join(base, match) if base and match in names else None + return out + + def norm(path): - """Decode and normalise a userdata-relative path ('' for the root).""" + """Decode and normalise a userdata-relative path ('' for the root). + + Segments Windows would read as something else are dropped, like '..': a colon (drive prefix + 'C:', NTFS stream), a 8.3 short name, and trailing dots or spaces (Windows strips them, so + 'users./bob' would open 'users/bob'). Casing is restored from disk on Windows. Anything that + still reaches the disk goes through safe_join(). + """ path = full_unquote(str(path or "")).replace("\\", "/") - return posixpath.normpath("/" + path).lstrip("/") if path else "" + if not path: + return "" + clean = posixpath.normpath("/" + path).lstrip("/") + parts = [p for p in clean.split("/") + if p and ":" not in p and not _SHORT_NAME_RE.search(p) and (not _WIN or p == p.rstrip(" ."))] + if _WIN and _case_root and parts: + lead = [WORKFLOWS] if parts[0].casefold() == WORKFLOWS else [] + parts = lead + _on_disk_case(parts[len(lead):], _case_root) + return "/".join(parts) + + +def safe_join(root, *parts): + """Join relative parts under root; refuse anything that resolves outside it.""" + segments = [p for part in parts for p in str(part).replace("\\", "/").split("/") if p] + if any(":" in p or p in (".", "..") or (_WIN and p != p.rstrip(" .")) for p in segments): + raise PermissionError("Invalid path") + full = os.path.join(root, *segments) + base = os.path.realpath(root) + try: + inside = os.path.commonpath([base, os.path.realpath(full)]) == base + except ValueError: # different drives + inside = False + if not inside: + raise PermissionError("Invalid path") + return full def has_app(doc): @@ -56,6 +106,8 @@ class Workspace: def __init__(self, store, root): self.store = store self.root = root # absolute path of the workflows directory + global _case_root + _case_root = root self.folders = store._read("folders.json", {}) # "shared/" -> {owner, acl, created} os.makedirs(os.path.join(root, "users"), exist_ok=True) os.makedirs(os.path.join(root, "shared"), exist_ok=True) @@ -147,7 +199,7 @@ class Workspace: return self.role(rel, kind, key, perms) is not None def _files(self, rel_dir): - base = os.path.join(self.root, *rel_dir.split("/")) if rel_dir else self.root + base = safe_join(self.root, rel_dir) if rel_dir else self.root out = [] if not os.path.isdir(base): return out @@ -198,7 +250,7 @@ class Workspace: # ----- browsing (Files tab) ------------------------------------------- def _abs(self, rel): - return os.path.join(self.root, *[p for p in rel.split("/") if p]) + return safe_join(self.root, rel) def _file_has_app(self, full): """Read a workflow file only when it changed since the last check.""" @@ -372,7 +424,7 @@ class Workspace: name = (name or "").strip() if not FOLDER_NAME_RE.match(name) or name in (".", ".."): raise ValueError("Folder names: up to 64 letters, digits, spaces and . _ - ( )") - path = os.path.join(self.root, "shared", name) + path = safe_join(self.root, "shared", name) if os.path.exists(path): raise ValueError("A folder with that name already exists") os.makedirs(path) @@ -385,8 +437,8 @@ class Workspace: if not FOLDER_NAME_RE.match(new_name): raise ValueError("Folder names: up to 64 letters, digits, spaces and . _ - ( )") new_folder = f"shared/{new_name}" - src = os.path.join(self.root, *folder.split("/")) - dst = os.path.join(self.root, "shared", new_name) + src = safe_join(self.root, folder) + dst = safe_join(self.root, "shared", new_name) if os.path.exists(dst): raise ValueError("A folder with that name already exists") os.rename(src, dst) @@ -395,7 +447,7 @@ class Workspace: return new_folder def delete_folder(self, folder): - path = os.path.join(self.root, *folder.split("/")) + path = safe_join(self.root, folder) if self._files(folder): raise ValueError("Move or delete the workflows inside it first") shutil.rmtree(path, ignore_errors=True) diff --git a/tests/python/test_jwt.py b/tests/python/test_jwt.py new file mode 100644 index 0000000..c6386d9 --- /dev/null +++ b/tests/python/test_jwt.py @@ -0,0 +1,60 @@ +"""HS256 tokens: round trip, tampering, expiry, issuer, algorithm confusion.""" + +import base64 +import json +import time + +import pytest + +from rigshare_core import jwt + +SECRET = b"s3cret" + + +def claims(**extra): + return {"iss": "rig", "exp": time.time() + 60, **extra} + + +def b64(obj): + return base64.urlsafe_b64encode(json.dumps(obj).encode()).rstrip(b"=").decode() + + +def test_round_trip(): + token = jwt.encode(claims(sub="bob"), SECRET) + assert jwt.decode(token, SECRET, "rig")["sub"] == "bob" + + +def test_wrong_secret_and_tampered_payload(): + token = jwt.encode(claims(sub="bob"), SECRET) + with pytest.raises(jwt.InvalidToken): + jwt.decode(token, b"other", "rig") + head, _, sig = token.split(".") + forged = f"{head}.{b64(claims(sub='admin'))}.{sig}" + with pytest.raises(jwt.InvalidToken): + jwt.decode(forged, SECRET, "rig") + + +def test_expiry_leeway_and_not_before(): + with pytest.raises(jwt.InvalidToken): + jwt.decode(jwt.encode({"iss": "rig", "exp": time.time() - 3600}, SECRET), SECRET, "rig") + jwt.decode(jwt.encode({"iss": "rig", "exp": time.time() - 5}, SECRET), SECRET, "rig") # inside leeway + assert jwt.decode(jwt.encode({"iss": "rig", "exp": time.time() - 3600}, SECRET), SECRET, "rig", verify_exp=False) + with pytest.raises(jwt.InvalidToken): + jwt.decode(jwt.encode(claims(nbf=time.time() + 3600), SECRET), SECRET, "rig") + + +def test_wrong_issuer(): + with pytest.raises(jwt.InvalidToken): + jwt.decode(jwt.encode(claims(), SECRET), SECRET, "someone-else") + + +def test_alg_none_is_refused(): + token = f"{b64({'alg': 'none', 'typ': 'JWT'})}.{b64(claims())}." + with pytest.raises(jwt.InvalidToken): + jwt.decode(token, SECRET, "rig") + + +@pytest.mark.parametrize("junk", ["", "a.b", "a.b.c.d", "!!!.???.***", "e30.e30.e30"]) +def test_malformed_tokens(junk): + with pytest.raises(jwt.InvalidToken): + jwt.decode(junk, SECRET, "rig") diff --git a/tests/python/test_server.py b/tests/python/test_server.py index 0b69d63..a9ee305 100644 --- a/tests/python/test_server.py +++ b/tests/python/test_server.py @@ -646,3 +646,23 @@ def test_performance_run_log_eta_history_and_clear(run): assert (await r.json())["cleared"] == 3 assert hub.etas() == {"video": {"ms": 90000, "runs": 1}}, "the ETA starts over" run(scenario()) + + +def test_folder_routes_refuse_paths_outside_the_workspace(run, tmp_path): + async def scenario(): + async with rig_server() as rig: + await rig.setup_admin() + await rig.add_user("alice", edit=True) + outside = tmp_path / "models" + outside.mkdir() + (outside / "keep.bin").write_bytes(b"x") + H, h = rig.headers, rig.http + targets = [str(outside), "C:" + str(outside)[2:], r"..\..\models", "shared/C:/evil"] + for route, extra in (("delete-folder", {}), ("rename-folder", {"name": "x"}), ("move-folder", {"dest": "@shared"})): + for target in targets: + r = await h.post(rig.url(f"/rigshare/api/workspace/{route}"), json={"path": target, **extra}, headers=H["alice"]) + assert r.status in (400, 403, 404), (route, target, r.status) + r = await h.post(rig.url("/rigshare/api/workspace/mkdir"), json={"parent": "C:/", "name": "x"}, headers=H["alice"]) + assert r.status in (400, 403, 404) + assert (outside / "keep.bin").exists() + run(scenario()) diff --git a/tests/python/test_stats.py b/tests/python/test_stats.py new file mode 100644 index 0000000..2c734b0 --- /dev/null +++ b/tests/python/test_stats.py @@ -0,0 +1,45 @@ +"""Server stats: every metric is optional and never raises.""" + +from rigshare_core import stats + + +class Queue: + def __init__(self, running, pending): + self.running, self.pending = running, pending + + def get_current_queue_volatile(self): + return self.running, self.pending + + +class Server: + def __init__(self, queue): + self.prompt_queue = queue + + +def test_queue_items_order_and_node_count(): + server = Server(Queue([(1, "r1", {"1": {}, "2": {}})], [(5, "p5", {"1": {}}), (3, "p3", {})])) + items = stats.queue_items(server) + assert [(i["id"], i["status"], i["nodes"]) for i in items] == [("r1", "running", 2), ("p3", "pending", 0), ("p5", "pending", 1)] + + +def test_queue_items_survive_odd_entries_and_missing_queue(): + assert stats.queue_items(Server(Queue([(1,)], [(9,)]))) == [] + assert stats.queue_items(object()) is None + + +def test_queue_counts_with_fallback(): + assert stats._queue(Server(Queue([1], [2, 3]))) == {"running": 1, "pending": 2} + + class Old: + def get_tasks_remaining(self): + return 4 + assert stats._queue(Server(Old())) == {"running": 0, "pending": 4} + assert stats._queue(object()) is None + + +def test_collect_stats_without_psutil_or_gpu(monkeypatch): + monkeypatch.setattr(stats, "psutil", None) + monkeypatch.setattr(stats, "pynvml", None) + out = stats.collect_stats(Server(Queue([], []))) + assert out["gpus"] == [] and "cpu" not in out and out["queue"] == {"running": 0, "pending": 0} + assert stats.collect_stats() == {"gpus": []} diff --git a/tests/python/test_workspace.py b/tests/python/test_workspace.py index 58db88f..0918211 100644 --- a/tests/python/test_workspace.py +++ b/tests/python/test_workspace.py @@ -131,3 +131,62 @@ def test_rename_user_moves_private_folder(ws): ws.rename_user("alice", "alicia") assert os.path.exists(os.path.join(ws.root, "users", "alicia", "keep.json")) assert ws.folders["shared/Team"]["owner"] == "alicia" + + +@pytest.mark.parametrize("raw", ["C:/models", "C:", "a/D:x/b", r"..\..\x", "c%3A%5Cmodels"]) +def test_norm_drops_drive_segments(raw): + assert ":" not in norm(raw) and ".." not in norm(raw).split("/") + + +@pytest.mark.parametrize("bad", ["C:/models", "C:", "..", "a/../..", "x:y"]) +def test_safe_join_refuses_escapes(ws, bad): + from rigshare_core.workspace import safe_join + with pytest.raises(PermissionError): + safe_join(ws.root, bad) + + +def test_folder_ops_cannot_leave_the_workspace(ws, tmp_path): + outside = tmp_path / "models" + outside.mkdir() + (outside / "keep.txt").write_text("x") + # A colon segment (drive prefix) is refused outright; a plain absolute path is only ever + # read relative to the workflows root (a leading slash is dropped), so it can't reach `outside`. + for bad in ("C:" + str(outside)[2:], "shared/C:evil"): + with pytest.raises(PermissionError): + ws.delete_folder(bad) + with pytest.raises(PermissionError): + ws.rename_folder(bad, "renamed") + for plain in (str(outside), str(outside).replace("\\", "/")): + try: + ws.delete_folder(plain) + except (PermissionError, ValueError): + pass + assert (outside / "keep.txt").exists() + + +@pytest.mark.parametrize("raw", ["users./bob/a.json", "users /bob", "USERS~1/bob", "users/bob./a.json"]) +def test_norm_drops_windows_aliases(raw): + if os.name != "nt" and ("." in raw.split("/")[0] or " " in raw.split("/")[0]): + pytest.skip("trailing dots and spaces only alias on Windows") + assert "~1" not in norm(raw) + + +@pytest.mark.skipif(os.name != "nt", reason="case-insensitive filesystem") +def test_case_variants_cannot_bypass_private_folders(ws): + touch(ws, "users/alice/a.json") + assert norm("Users/Alice/a.json") == "users/alice/a.json" + assert norm("Workflows/USERS/ALICE") == "workflows/users/alice" + assert ws.role(norm("USERS/alice/a.json"), "user", "bob", EDITOR) is None + + +def test_name_rules(): + from rigshare_core.workspace import FOLDER_NAME_RE + from rigshare_core.store import USERNAME_RE + for bad in ("con", "NUL", "com1.txt", "art.", "art ", "bad\n"): + assert not FOLDER_NAME_RE.match(bad), bad + for bad in ("..", "bob.", ".bob", "con", "aux", "ab\n", "a"): + assert not USERNAME_RE.match(bad), bad + for ok in ("Art (v2)", "a", "my-folder"): + assert FOLDER_NAME_RE.match(ok), ok + for ok in ("bob", "al.ice", "x_y-z", "ab"): + assert USERNAME_RE.match(ok), ok