From c68789b0fbcb849949bbd96b68d94fe4d7f98095 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Thu, 2 Jul 2026 20:15:08 -0700 Subject: [PATCH] revert(security): drop CVE-2026-56674 Origin: null CSRF change Per maintainer review, the reported CSRF is already mitigated by the pre-existing Sec-Fetch-Site: cross-site check for current browsers, and the null-origin rejection risked breaking legitimate sandboxed-iframe embeds. Restores origin_only_middleware and is_loopback in server.py to their prior state (the Sec-Fetch-Site check is retained) and removes utils/origin_check.py and its regression test. The other four GHSA-779p fixes are unaffected. --- server.py | 54 +++++++-- .../test_ghsa_779p_01_origin_csrf.py | 114 ------------------ utils/origin_check.py | 91 -------------- 3 files changed, 46 insertions(+), 213 deletions(-) delete mode 100644 tests-unit/security_test/test_ghsa_779p_01_origin_csrf.py delete mode 100644 utils/origin_check.py diff --git a/server.py b/server.py index 3ab33ad14..461ebe2f6 100644 --- a/server.py +++ b/server.py @@ -23,6 +23,8 @@ import json import glob import struct import ssl +import socket +import ipaddress from PIL import Image, ImageOps from PIL.PngImagePlugin import PngInfo from io import BytesIO @@ -38,7 +40,6 @@ import comfy.utils import comfy.model_management from comfy_api import feature_flags import node_helpers -from utils.origin_check import is_cross_origin_forbidden from comfyui_version import __version__ from app.frontend_management import FrontendManager, parse_version from comfy_api.internal import _ComfyNodeInternal @@ -126,6 +127,33 @@ def create_cors_middleware(allowed_origin: str): return cors_middleware + +def is_loopback(host): + if host is None: + return False + try: + if ipaddress.ip_address(host).is_loopback: + return True + else: + return False + except: + pass + + loopback = False + for family in (socket.AF_INET, socket.AF_INET6): + try: + r = socket.getaddrinfo(host, None, family, socket.SOCK_STREAM) + for family, _, _, _, sockaddr in r: + if not ipaddress.ip_address(sockaddr[0]).is_loopback: + return loopback + else: + loopback = True + except socket.gaierror: + pass + + return loopback + + def create_origin_only_middleware(): @web.middleware async def origin_only_middleware(request: web.Request, handler): @@ -139,13 +167,23 @@ def create_origin_only_middleware(): if 'Host' in request.headers and 'Origin' in request.headers: host = request.headers['Host'] origin = request.headers['Origin'] - # The host/origin CSRF decision (incl. the Origin: null bypass fix - # for GHSA-779p-m5rp-r4h4 #1) lives in utils.origin_check so it can - # be unit-tested without standing up the full server. See - # tests-unit/security_test/test_ghsa_779p_01_origin_csrf.py. - if is_cross_origin_forbidden(host, origin): - logging.warning("WARNING: request with non matching host and origin, returning 403 (host={!r} origin={!r})".format(host, origin)) - return web.Response(status=403) + host_domain = host.lower() + parsed = urllib.parse.urlparse(origin) + origin_domain = parsed.netloc.lower() + host_domain_parsed = urllib.parse.urlsplit('//' + host_domain) + + #limit the check to when the host domain is localhost, this makes it slightly less safe but should still prevent the exploit + loopback = is_loopback(host_domain_parsed.hostname) + + if parsed.port is None: #if origin doesn't have a port strip it from the host to handle weird browsers, same for host + host_domain = host_domain_parsed.hostname + if host_domain_parsed.port is None: + origin_domain = parsed.hostname + + if loopback and host_domain is not None and origin_domain is not None and len(host_domain) > 0 and len(origin_domain) > 0: + if host_domain != origin_domain: + logging.warning("WARNING: request with non matching host and origin {} != {}, returning 403".format(host_domain, origin_domain)) + return web.Response(status=403) if request.method == "OPTIONS": response = web.Response() diff --git a/tests-unit/security_test/test_ghsa_779p_01_origin_csrf.py b/tests-unit/security_test/test_ghsa_779p_01_origin_csrf.py deleted file mode 100644 index 38ad0768f..000000000 --- a/tests-unit/security_test/test_ghsa_779p_01_origin_csrf.py +++ /dev/null @@ -1,114 +0,0 @@ -"""CI unit guard for FIX #1 of GHSA-779p-m5rp-r4h4 — the Origin: null CSRF bypass. - -Vuln #1 was a CSRF bypass: the loopback host/origin guard in -``server.create_origin_only_middleware`` skipped the comparison entirely whenever -the origin host parsed to an empty string. An opaque ``Origin: null`` (sent by a -sandboxed iframe or a ``data:``/``file:`` document) parses to exactly that, so an -attacker could forge cross-origin state-mutating requests against a victim's -loopback ComfyUI — the entry primitive for the [CISA-1] CSRF -> stored-XSS -> -client-side RCE chain. - -The decision logic was extracted verbatim from the middleware closure into -``utils.origin_check`` precisely so it can be exercised here: ``server.py`` cannot -be imported in a unit test (importing it spins up the full PromptServer/aiohttp -app and its global side effects), which is why finding #1 previously had no -server-free CI coverage and only a live-server POC -(``.security/pocs/test_security_ghsa_779p.py::TestOriginNullCsrf``, skipped -unless a server is running on 127.0.0.1:8188). This file is the fast, hermetic -guard so the Origin: null bypass cannot silently reopen. - -Cases use IP literals so the loopback determination does not depend on DNS; the -one name-based case ("localhost") relies only on standard loopback resolution. -""" - -from utils.origin_check import is_cross_origin_forbidden, is_loopback - - -# --------------------------------------------------------------------------- -# The regression: an opaque/empty Origin against a loopback Host MUST be a 403. -# Each of these returned False (allowed) before the fix — that is the bug. -# --------------------------------------------------------------------------- -OPAQUE_ORIGIN_ON_LOOPBACK = [ - ("127.0.0.1:8188", "null"), # the exact reported bypass - ("127.0.0.1:8188", ""), # empty Origin header, same empty-host path - ("127.0.0.1", "null"), # host without an explicit port - ("[::1]:8188", "null"), # IPv6 loopback host -] - - -def test_origin_null_is_forbidden(): - """Origin: null against a loopback host must be rejected (the #1 fix).""" - assert is_cross_origin_forbidden("127.0.0.1:8188", "null") is True, ( - "Origin: null was treated as allowed — this is exactly the " - "GHSA-779p-m5rp-r4h4 #1 CSRF bypass reopening." - ) - - -def test_all_opaque_origins_on_loopback_forbidden(): - for host, origin in OPAQUE_ORIGIN_ON_LOOPBACK: - assert is_cross_origin_forbidden(host, origin) is True, (host, origin) - - -# --------------------------------------------------------------------------- -# False-positive guards: legitimate same-origin requests must stay allowed, or -# the fix would break the dev server. The port-stripping cases preserve the -# original "handle weird browsers" behaviour (origin without a port matches a -# host with one, and vice versa). -# --------------------------------------------------------------------------- -MATCHING_ORIGINS = [ - ("127.0.0.1:8188", "http://127.0.0.1:8188"), - ("127.0.0.1:8188", "http://127.0.0.1"), # origin has no port -> host port stripped for compare - ("127.0.0.1", "http://127.0.0.1"), - ("localhost:8188", "http://localhost:8188"), -] - - -def test_matching_origins_allowed(): - for host, origin in MATCHING_ORIGINS: - assert is_cross_origin_forbidden(host, origin) is False, (host, origin) - - -# --------------------------------------------------------------------------- -# Genuine cross-origin requests against a loopback host must be forbidden. -# --------------------------------------------------------------------------- -MISMATCHED_ORIGINS_ON_LOOPBACK = [ - ("127.0.0.1:8188", "http://evil.com"), - ("127.0.0.1:8188", "https://127.0.0.1:9999"), # same host, different port - ("127.0.0.1:8188", "http://localhost.evil.com"), -] - - -def test_mismatched_origins_on_loopback_forbidden(): - for host, origin in MISMATCHED_ORIGINS_ON_LOOPBACK: - assert is_cross_origin_forbidden(host, origin) is True, (host, origin) - - -# --------------------------------------------------------------------------- -# Scope guard: the check is deliberately limited to loopback hosts. A -# non-loopback Host must NOT trip the guard (even on a mismatch / opaque -# origin) — this preserves the original behaviour and documents that the -# mitigation is localhost-only by design. -# --------------------------------------------------------------------------- -NON_LOOPBACK_HOSTS = [ - ("203.0.113.5:8188", "http://evil.com"), # public IP literal -> not loopback - ("203.0.113.5:8188", "null"), - ("10.0.0.5:8188", "null"), # private but not loopback -] - - -def test_non_loopback_host_not_subject_to_check(): - for host, origin in NON_LOOPBACK_HOSTS: - assert is_cross_origin_forbidden(host, origin) is False, (host, origin) - - -# --------------------------------------------------------------------------- -# is_loopback — the predicate the scoping above depends on. -# --------------------------------------------------------------------------- -def test_is_loopback_true_for_loopback_addresses(): - for host in ("127.0.0.1", "::1", "localhost"): - assert is_loopback(host) is True, host - - -def test_is_loopback_false_for_non_loopback_and_none(): - for host in ("203.0.113.5", "10.0.0.5", None): - assert is_loopback(host) is False, host diff --git a/utils/origin_check.py b/utils/origin_check.py deleted file mode 100644 index cff631292..000000000 --- a/utils/origin_check.py +++ /dev/null @@ -1,91 +0,0 @@ -"""Host/Origin CSRF check for the loopback dev server. - -Extracted verbatim from ``server.create_origin_only_middleware`` so the decision -logic is importable and unit-testable without standing up the full -PromptServer/aiohttp app (importing ``server`` pulls in ``nodes``/``execution``/ -torch and has global side effects). The wiring lives in ``server.py``; the -regression guard for GHSA-779p-m5rp-r4h4 finding #1 (CSRF bypass via -``Origin: null``) lives in -``tests-unit/security_test/test_ghsa_779p_01_origin_csrf.py``. - -Only ``urllib.parse``/``ipaddress``/``socket`` (stdlib) are imported here, so the -module stays cheap to import from a unit test. -""" - -import ipaddress -import socket -import urllib.parse - - -def is_loopback(host): - if host is None: - return False - try: - if ipaddress.ip_address(host).is_loopback: - return True - else: - return False - except ValueError: - # Not an IP literal (ip_address raises ValueError); fall through to DNS - # resolution below. Narrowed from a bare except so genuine interrupts - # (KeyboardInterrupt/SystemExit) aren't swallowed. - pass - - loopback = False - for family in (socket.AF_INET, socket.AF_INET6): - try: - r = socket.getaddrinfo(host, None, family, socket.SOCK_STREAM) - for family, _, _, _, sockaddr in r: - if not ipaddress.ip_address(sockaddr[0]).is_loopback: - return loopback - else: - loopback = True - except socket.gaierror: - pass - - return loopback - - -def is_cross_origin_forbidden(host, origin): - """Return True if a request with these ``Host``/``Origin`` headers must be rejected (403). - - This prevents the case where a random website can queue Comfy workflows by - making a POST to 127.0.0.1, which browsers don't prevent. In that case the - Host and Origin hostnames won't match. The check is intentionally limited to - when the Host resolves to a loopback address; for non-loopback hosts it - returns False (it is a localhost-CSRF mitigation, not a general same-origin - enforcer). - - GHSA-779p-m5rp-r4h4 #1 fix: an opaque origin (e.g. ``"null"`` sent by a - sandboxed iframe or a ``data:``/``file:`` document) parses to an empty/None - host. Previously such requests skipped the comparison entirely, which let an - attacker bypass the host/origin CSRF check with ``Origin: null``. A missing - or empty origin host is now treated as a mismatch and rejected. - """ - host_domain = host.lower() - parsed = urllib.parse.urlparse(origin) - origin_domain = parsed.netloc.lower() - host_domain_parsed = urllib.parse.urlsplit('//' + host_domain) - - # A non-numeric or out-of-range port (e.g. Origin: http://127.0.0.1:99999) - # makes urllib raise ValueError on .port access. Treat a malformed port as a - # rejected request rather than letting it surface as an uncaught 500 in the - # middleware — it fails closed, consistent with the CSRF stance. - try: - origin_port = parsed.port - host_port = host_domain_parsed.port - except ValueError: - return True - - loopback = is_loopback(host_domain_parsed.hostname) - - if origin_port is None: # if origin doesn't have a port strip it from the host to handle weird browsers, same for host - host_domain = host_domain_parsed.hostname - if host_port is None: - origin_domain = parsed.hostname - - if loopback and host_domain is not None and len(host_domain) > 0: - if origin_domain is None or len(origin_domain) == 0 or host_domain != origin_domain: - return True - - return False