diff --git a/CHANGELOG.md b/CHANGELOG.md index 67907b0..51e0319 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,19 @@ # Changelog +## 1.3.3 (2026-10-08) + +Login and request hardening, mainly for servers behind a reverse proxy. + +### Fixed + +- A reverse proxy on the same machine (nginx, Caddy, a tunnel) made every visitor look like `127.0.0.1`, which is a trusted address by default, so login and API protection were skipped for everyone. Behind a trusted proxy, trust now follows the visitor's own address (the last `X-Forwarded-For` / `X-Real-IP` entry), and a `Forwarded` header RigShare cannot read means no trust +- Login throttling counted only the address, so users behind one proxy locked each other out, one account could be guessed from many addresses, and the counters never shrank. It now limits per visitor address (5 failures a minute) and per account (10 a minute across all addresses), answers with `Retry-After`, and forgets old entries. People already signed in are never affected. Failed logins are logged + +### Added + +- Requests that carry the session cookie and an `Origin` from another site are refused: writes and the live connection (hostnames compared, so a proxy that drops the port or sets `X-Forwarded-Host` still works). API clients without a cookie are not affected +- The login page is sent with `X-Frame-Options: DENY`, a `frame-ancestors 'none'` policy, `nosniff` and a same-origin referrer policy, so another site cannot frame it + ## 1.3.2 (2026-10-08) Access rules tightened, following the review of the ComfyUI Manager listing. diff --git a/pyproject.toml b/pyproject.toml index f7c0c60..ba67011 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.2" +version = "1.3.3" license = { file = "LICENSE" } dependencies = [] diff --git a/rigshare_core/routes.py b/rigshare_core/routes.py index 114e335..785f3f7 100644 --- a/rigshare_core/routes.py +++ b/rigshare_core/routes.py @@ -9,7 +9,7 @@ import logging import os import posixpath import time -from urllib.parse import unquote +from urllib.parse import unquote, urlsplit from collections import defaultdict from aiohttp import web @@ -94,6 +94,9 @@ BROADCAST_EVENTS = {"execution_start", "execution_cached", "executing", "execute COOKIE = "rigshare_session" +# The login page must not be framed by another site (clickjacking) or sniffed into another type. +SECURITY_HEADERS = {"X-Frame-Options": "DENY", "Content-Security-Policy": "frame-ancestors 'none'", + "X-Content-Type-Options": "nosniff", "Referrer-Policy": "same-origin"} LOGIN_PAGE = os.path.join(os.path.dirname(__file__), "login.html") ICON_SVG = os.path.join(os.path.dirname(os.path.dirname(__file__)), "web", "rigshare", "assets", "icon.svg") # Reachable without logging in. @@ -112,7 +115,8 @@ def token_from(request): def setup(server, store, hub): routes = web.RouteTableDef() - failures = defaultdict(list) + failures = defaultdict(list) # client address -> times of failed logins + failures_by_name = defaultdict(list) # username -> times of failed logins def current_user(request): return store.user_for_token(token_from(request)) @@ -191,19 +195,30 @@ def setup(server, store, hub): except OSError: icon = "" page = page.replace("{{ICON_SVG}}", icon).replace("{{ICON_B64}}", base64.b64encode(icon.encode()).decode()) - return web.Response(text=page, content_type="text/html", headers={"Cache-Control": "no-store"}) + return web.Response(text=page, content_type="text/html", headers={"Cache-Control": "no-store", **SECURITY_HEADERS}) @routes.post("/rigshare/api/login") async def login(request): - ip = request.remote or "?" - now = time.time() - failures[ip] = [t for t in failures[ip] if now - t < 60] - if len(failures[ip]) >= 5: - return error("Too many attempts, wait a minute", 429) + ip, now = client_ip(request) or "?", time.time() body = await request.json() - user = store.authenticate(str(body.get("username", "")).strip(), str(body.get("password", ""))) + name = str(body.get("username", "")).strip()[:64].casefold() + for table in (failures, failures_by_name): # forget old failures, so the tables stay small + for key in [k for k, times in table.items() if not times or now - times[-1] >= 60]: + del table[key] + recent = [t for t in failures[ip] if now - t < 60] + recent_name = [t for t in failures_by_name[name] if now - t < 60] + failures[ip], failures_by_name[name] = recent, recent_name + # Per address: 5 a minute. Per account: 10 a minute across all addresses, which slows + # guessing from many places without locking the owner out for long. Signed-in people + # never come through here. + if len(recent) >= 5 or len(recent_name) >= 10: + return web.json_response({"error": "Too many attempts, wait a minute"}, status=429, + headers={"Retry-After": "60"}) + user = store.authenticate(name_raw := str(body.get("username", "")).strip(), str(body.get("password", ""))) if not user: failures[ip].append(now) + failures_by_name[name].append(now) + log.warning(f"[RigShare] Failed login for '{name_raw[:32]}' from {ip}") return error("Invalid username or password", 401) token = store.create_session(user["username"]) response = web.json_response({"token": token, "user": store.public_user(user)}) @@ -651,6 +666,50 @@ def setup(server, store, hub): continue return False + def client_ip(request): + """The caller's address. Behind a trusted proxy (one of trusted_ips), the address that + proxy appended to X-Forwarded-For / X-Real-IP; otherwise the socket's. A proxy on the + same machine used to make every visitor look like 127.0.0.1, so everyone was trusted.""" + remote = request.remote or "" + if not trusted(remote): + return remote # an untrusted peer's forwarding headers are just claims + forwarded = request.headers.get("X-Forwarded-For") + if forwarded: + return forwarded.split(",")[-1].strip() + real = request.headers.get("X-Real-IP") + if real: + return real.strip() + if request.headers.get("Forwarded"): + return "" # a proxy we cannot read: do not trust the visitor + return remote + + def cross_site(request): + """A browser sending its session cookie from another site. Browsers always send Origin on + cross-site writes and websockets; API clients (no cookie, usually no Origin) are not + affected.""" + origin = request.headers.get("Origin") + if not origin or COOKIE not in request.cookies: + return False + if request.method in ("GET", "HEAD", "OPTIONS") and request.path != "/rigshare/ws": + return False + # Hostnames only: a proxy often passes Host without the port the browser used. + def hostname(value): + try: + return (urlsplit(value if "//" in value else "//" + value).hostname or "").lower() + except ValueError: + return "" + origin_host = hostname(origin) + allowed = {hostname(request.host)} + allowed.update(hostname(h.strip()) for h in (request.headers.get("X-Forwarded-Host") or "").split(",") if h.strip()) + if origin_host and origin_host in allowed: + return False + log.warning(f"[RigShare] Refused cross-site {request.method} {request.path}: Origin {origin[:80]!r} " + f"does not match Host {request.host[:80]!r}") + return True + + def trusted_request(request): + return trusted(client_ip(request)) + def parse_userdata(request): """('list'|'get'|'save'|'delete'|'move', path, dest) for ComfyUI userdata calls under workflows/.""" raw = request.raw_path.split("?", 1)[0] @@ -706,7 +765,7 @@ def setup(server, store, hub): """Cancelling a queued prompt or stopping the running one: your own, unless admin.""" if request.method != "POST" or api_path(request) not in ("/queue", "/interrupt"): return None - if trusted(request.remote or "") or perms_for(request).get("admin"): + if trusted_request(request) or perms_for(request).get("admin"): return None user = current_user(request) me = user["username"] if user else None @@ -805,7 +864,9 @@ def setup(server, store, hub): @web.middleware async def protect(request, handler): path = request.path - if store.config.get("require_login") and path not in PUBLIC_PATHS and not trusted(request.remote or ""): + if cross_site(request): + return web.json_response({"error": "RigShare: cross-site request refused"}, status=403) + if store.config.get("require_login") and path not in PUBLIC_PATHS and not trusted_request(request): if not current_user(request): wants_page = request.method == "GET" and ( path in ("/", "/index.html") or "text/html" in request.headers.get("Accept", "")) @@ -816,20 +877,20 @@ def setup(server, store, hub): "'Authorization: Bearer '."}, status=401, headers={"X-RigShare-Login": "required"}) userdata = parse_userdata(request) - if userdata and not trusted(request.remote or ""): + if userdata and not trusted_request(request): refusal = check_userdata(request, *userdata) if refusal: return web.json_response({"error": f"RigShare: {refusal}"}, status=403) cfg = store.config.get("api_protection", {}) if cfg.get("enabled"): path = request.path[4:] if request.path.startswith("/api/") else request.path - if needs_manager(request.method, path) and not trusted(request.remote or ""): + if needs_manager(request.method, path) and not trusted_request(request): if not perms_for(request).get("manager"): return web.json_response({"error": "RigShare: you need the 'manager' permission to use ComfyUI Manager"}, status=403) for method, prefix, need in PROTECTED: if (not method or request.method == method) and (path == prefix or path.startswith(prefix)): - if trusted(request.remote or ""): + if trusted_request(request): break perms = perms_for(request) if not any(perms.get(p) for p in need.split("|")): @@ -841,7 +902,7 @@ def setup(server, store, hub): break for method, prefix, need, why in PLUGIN_RULES: if (method is None or request.method == method) and api_path(request).startswith(prefix): - if not trusted(request.remote or "") and not perms_for(request).get(need) and not perms_for(request).get("admin"): + if not trusted_request(request) and not perms_for(request).get(need) and not perms_for(request).get("admin"): return web.json_response({"error": f"RigShare: {why}"}, status=403) break refusal = await check_queue_ownership(request) @@ -862,7 +923,7 @@ def setup(server, store, hub): snapshot_saved_file(request, userdata[1], await request.read()) await hub.files_changed(userdata[1]) # Workflow listings only show what this person may open. - if userdata and userdata[0] == "list" and not trusted(request.remote or "") and hub.workspace: + if userdata and userdata[0] == "list" and not trusted_request(request) and hub.workspace: response = filter_listing(request, response, userdata[1]) # A file moved on disk: its live room follows it. if userdata and userdata[0] == "move" and getattr(response, "status", 0) == 200: diff --git a/tests/python/test_server.py b/tests/python/test_server.py index 2a6e57d..ea51d56 100644 --- a/tests/python/test_server.py +++ b/tests/python/test_server.py @@ -5,6 +5,8 @@ import json import os from urllib.parse import quote +from aiohttp import ClientSession + from helpers import PASSWORD, enc, rig_server BASE_DOC = {"last_node_id": 2, "last_link_id": 0, "nodes": [ @@ -705,3 +707,85 @@ def test_access_is_checked_without_a_live_room(run): assert "editing" in (await viewer_sock.until("room_denied"))["reason"] assert open_key not in rig.hub.rooms run(scenario()) + + +def test_a_local_proxy_does_not_make_visitors_trusted(run): + async def scenario(): + async with rig_server() as rig: + await rig.setup_admin() + rig.store.update_config({"api_protection": {"enabled": True, "trusted_ips": ["127.0.0.1"]}}) + anon = {"Accept": "application/json"} + async with ClientSession() as plain: + # Straight from the machine itself: trusted, no login asked. + assert (await plain.get(rig.url("/api/prompt"), headers=anon)).status != 401 + # The same machine relaying an outside visitor (nginx, Caddy, a tunnel): not trusted. + for header in ({"X-Forwarded-For": "203.0.113.9"}, + {"X-Forwarded-For": "10.0.0.1, 203.0.113.9"}, + {"X-Real-IP": "203.0.113.9"}, {"Forwarded": "for=203.0.113.9"}): + r = await plain.get(rig.url("/api/prompt"), headers={**anon, **header}) + assert r.status == 401, header + # A proxy relaying someone on the trusted network keeps that trust. + r = await plain.get(rig.url("/api/prompt"), headers={**anon, "X-Forwarded-For": "127.0.0.1"}) + assert r.status != 401 + run(scenario()) + + +def test_login_attempts_are_throttled_per_address_and_per_account(run): + async def scenario(): + async with rig_server() as rig: + await rig.setup_admin() + await rig.add_user("alice", edit=True) + h, login = rig.http, rig.url("/rigshare/api/login") + bad = {"username": "alice", "password": "wrong-password"} + for _ in range(5): + assert (await h.post(login, json=bad)).status == 401 + r = await h.post(login, json=bad) + assert r.status == 429 and r.headers["Retry-After"] == "60" + assert (await h.post(login, json={"username": "alice", "password": PASSWORD})).status == 429, "even the right password waits" + # Someone already signed in is not affected: no login involved. + assert (await h.get(rig.url("/rigshare/api/status"), headers=rig.headers["alice"])).status == 200 + + # Many addresses (a trusted proxy relaying them) against one account. + rig.store.update_config({"api_protection": {"enabled": True, "trusted_ips": ["127.0.0.1"]}}) + statuses = [] + for n in range(12): + r = await h.post(login, json={"username": "Alice", "password": "wrong-password"}, + headers={"X-Forwarded-For": f"203.0.113.{n + 1}"}) + statuses.append(r.status) + # The 5 failures above already count against the account: 5 more fit, then it waits. + assert statuses[:5] == [401] * 5 and statuses[5:] == [429] * 7 + r = await h.post(login, json={"username": "someone-else", "password": "x"}, headers={"X-Forwarded-For": "198.51.100.7"}) + assert r.status == 401, "other accounts are not slowed down" + run(scenario()) + + +def test_cross_site_requests_with_a_session_cookie_are_refused(run): + async def scenario(): + async with rig_server() as rig: + await rig.setup_admin() + h = rig.http + assert any(c.key == "rigshare_session" for c in h.cookie_jar), "logged in by cookie" + own = {"Origin": rig.url("")} + evil = {"Origin": "https://evil.example"} + r = await h.post(rig.url("/rigshare/api/snapshots"), json={}, headers=evil) + assert r.status == 403 and "cross-site" in (await r.json())["error"] + assert (await h.post(rig.url("/rigshare/api/snapshots"), json={}, headers=own)).status != 403 + assert (await h.post(rig.url("/rigshare/api/snapshots"), json={})).status != 403, "no Origin: API client" + assert (await h.get(rig.url("/rigshare/api/status"), headers=evil)).status == 200, "reads are not writes" + r = await h.get(rig.url("/rigshare/ws"), headers={**evil, "Upgrade": "websocket", "Connection": "Upgrade"}) + assert r.status == 403 + # Behind a proxy that rewrites Host, X-Forwarded-Host names the public host. + r = await h.post(rig.url("/rigshare/api/snapshots"), json={}, + headers={"Origin": "https://comfy.example.com", "X-Forwarded-Host": "comfy.example.com"}) + assert r.status != 403 + run(scenario()) + + +def test_login_page_cannot_be_framed(run): + async def scenario(): + async with rig_server() as rig: + r = await rig.http.get(rig.url("/rigshare/login")) + assert r.headers["X-Frame-Options"] == "DENY" + assert "frame-ancestors 'none'" in r.headers["Content-Security-Policy"] + assert r.headers["X-Content-Type-Options"] == "nosniff" + run(scenario())