nodes/ComfyAssets-ComfyUI_PromptManager
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix(images): accept Show in folder only as a same-origin JSON request, one at a time

Browse Source 
- require an Origin equal to the request's own scheme://host:port and,
  when sent, Sec-Fetch-Site: same-origin; refuse every forwarding-style
  header (Via, X-Forwarded-*, X-Original-*, *-Client-IP, CF-Connecting-IP
  and the rest) as remote (security review M1, L2)
- answer a non-JSON body with 415, so a cross-origin request always needs
  a CORS preflight (L2)
- single flight plus a 1 s cooldown per process: 429 code busy (L1)
- path resolution errors such as ENAMETOOLONG are a JSON 404, and neither
  that nor a start failure logs the client's path at error level (L3)
This commit is contained in:
Vito Sansevero
2026-10-20 21:34:24 -07:00
parent daac06483d
commit 6c4c587d9e
4 changed files with 386 additions and 50 deletions
Download Patch File Download Diff File Expand all files Collapse all files
py/api/images.py
+48 -6
View File
@@ -40,6 +40,7 @@ try:
from ...utils.prompt_inspector import MetadataReadError, read_inspection_file
from ...utils.file_reveal import (
OpenerUnavailable,
RevealGate,
RevealTargetChanged,
is_local_request,
reveal_executor,
@@ -55,6 +56,7 @@ except ImportError:
from utils.prompt_inspector import MetadataReadError, read_inspection_file
from utils.file_reveal import (
OpenerUnavailable,
RevealGate,
RevealTargetChanged,
is_local_request,
reveal_executor,
@@ -2222,6 +2224,8 @@ class ImageRoutesMixin:
# Swapped by tests and the disposable browser server; never a shell.
_reveal_opener = staticmethod(reveal_in_file_manager)
# One per process (a class attribute): single flight plus a cooldown.
_reveal_gate = RevealGate()
@staticmethod
def _reveal_path(relative: str, root: Path) -> Path | None:
@@ -2235,11 +2239,22 @@ class ImageRoutesMixin:
async def reveal_image(self, request):
"""POST ``{root_index, root_scope, path}``: show the file in its folder.
Local-only (no proxy header, loopback client and Host): the browser
must run on the computer whose file manager opens.
Local-only (a same-origin browser request from a loopback client, no
forwarding header) and JSON-only: the browser must run on the
computer whose file manager opens. One reveal at a time (429 busy).
"""
if not is_local_request(request.remote, request.headers):
if not is_local_request(request.remote, request.headers, request.scheme):
return _not_local()
if request.content_type != "application/json":
# A non-JSON POST is a CORS "simple" request with no preflight.
return web.json_response(
{
"success": False,
"code": "unsupported-media-type",
"error": "Send the request as application/json.",
},
status=415,
)
data, error = await _json_body_or_error(request)
if error is not None:
return error
@@ -2256,7 +2271,31 @@ class ImageRoutesMixin:
root = roots[index]
if Path(relative).suffix.lower() not in SERVABLE_EXTENSIONS:
return _forbidden("Only media files can be shown")
path = await self._run_in_executor(self._reveal_path, relative, root)
gate = self._reveal_gate
if not gate.try_enter():
return web.json_response(
{
"success": False,
"code": "busy",
"error": "The folder is already opening.",
},
status=429,
)
try:
return await self._reveal(relative, root)
finally:
gate.leave()
async def _reveal(self, relative: str, root: Path):
"""Resolve and open, inside the reveal gate."""
try:
path = await self._run_in_executor(self._reveal_path, relative, root)
except (OSError, ValueError) as exc:
# ENAMETOOLONG and the like: the client's path is not logged.
self.logger.debug(
"Show in folder could not resolve: %s", type(exc).__name__
)
return _reveal_not_found()
if path is None:
return _reveal_not_found()
try:
@@ -2274,8 +2313,11 @@ class ImageRoutesMixin:
},
status=501,
)
except Exception:
self.logger.exception("Show in folder failed")
except Exception as exc:
# The exception text can hold the path: only its type at error
# level, the traceback at debug.
self.logger.error("Show in folder failed: %s", type(exc).__name__)
self.logger.debug("Show in folder failure", exc_info=True)
return web.json_response(
{"success": False, "error": "The folder could not be opened."},
status=500,
tests/browser/server.py
+3 -1
View File
@@ -86,7 +86,7 @@ def register_reveal_stub(routes, api, root: Path):
The API's opener records the resolved path; ``POST /__test__/reveal``
sets the next outcome and ``GET /__test__/reveal`` lists the opened paths.
"""
from utils.file_reveal import OpenerUnavailable
from utils.file_reveal import OpenerUnavailable, RevealGate
state = {"outcome": "opened", "opened": []}
@@ -98,6 +98,8 @@ def register_reveal_stub(routes, api, root: Path):
state["opened"].append(Path(path).relative_to(root.resolve()).as_posix())
api._reveal_opener = opener
# Each click gets an answer here; the cooldown is a unit-tested default.
api._reveal_gate = RevealGate(cooldown=0)
@routes.post("/__test__/reveal")
async def set_reveal_outcome(request: web.Request) -> web.Response:
tests/test_image_reveal.py
+224 -24
View File
@@ -5,6 +5,8 @@ No test launches a file manager: ``subprocess.Popen``, ``sys.platform`` and
"""
import ctypes
import json
import logging
import os
import subprocess
import sys
@@ -30,53 +32,132 @@ from utils.file_reveal import (
)
REVEAL = "/prompt_manager/images/reveal"
PROXY_HEADERS = ("Forwarded", "X-Forwarded-For", "X-Real-IP", "X-Forwarded-Host")
PROXY_HEADERS = (
"Forwarded",
"Via",
"X-Forwarded-For",
"X-Forwarded-Host",
"X-Forwarded-Proto",
"X-Forwarded-Port",
"X-Forwarded-Server",
"X-Real-IP",
"X-Original-Forwarded-For",
"X-Original-Host",
"CF-Connecting-IP",
"True-Client-IP",
"X-Client-IP",
"X-Cluster-Client-IP",
"Fastly-Client-IP",
)
ORIGIN = "http://127.0.0.1:8188"
def headers(**values):
base = {"Host": "127.0.0.1:8188"}
base = {"Host": "127.0.0.1:8188", "Origin": ORIGIN}
base.update({key.replace("_", "-"): value for key, value in values.items()})
return CIMultiDict(base)
return CIMultiDict({key: value for key, value in base.items() if value is not None})
class TestIsLocalRequest(unittest.TestCase):
def test_loopback_clients_by_loopback_name_are_local(self):
for remote, host in (
("127.0.0.1", "127.0.0.1:8188"),
("127.8.9.10", "localhost:8188"),
("::1", "[::1]:8188"),
("::ffff:127.0.0.1", "comfy.localhost"),
def test_same_origin_loopback_clients_by_loopback_name_are_local(self):
for remote, host, origin in (
("127.0.0.1", "127.0.0.1:8188", ORIGIN),
("127.8.9.10", "localhost:8188", "http://localhost:8188"),
("::1", "[::1]:8188", "http://[::1]:8188"),
("::ffff:127.0.0.1", "comfy.localhost", "http://comfy.localhost"),
("127.0.0.1", "LOCALHOST:8188", "http://localhost:8188"),
("127.0.0.1", "localhost", "http://localhost:80"),
):
with self.subTest(remote=remote, host=host):
self.assertTrue(is_local_request(remote, headers(Host=host)))
self.assertTrue(
is_local_request(remote, headers(Host=host, Origin=origin))
)
self.assertTrue(
is_local_request(
"127.0.0.1",
headers(Host="localhost", Origin="https://localhost:443"),
"https",
)
)
def test_non_loopback_or_unknown_remote_is_not_local(self):
for remote in ("192.168.1.5", "10.0.0.1", "::2", "fe80::1%eth0", "", None):
with self.subTest(remote=remote):
self.assertFalse(is_local_request(remote, headers()))
def test_any_proxy_header_makes_loopback_remote(self):
for name in PROXY_HEADERS:
def test_any_forwarding_header_makes_loopback_remote(self):
for name in (*PROXY_HEADERS, "X-Forwarded-Anything", "X-Original-URI"):
for spelling in (name, name.lower()):
with self.subTest(header=spelling):
request_headers = headers()
request_headers[spelling] = "127.0.0.1"
self.assertFalse(is_local_request("127.0.0.1", request_headers))
def test_foreign_host_or_origin_is_not_local(self):
# A DNS-rebinding page reaches 127.0.0.1 under its own name.
for host in ("evil.example:8188", "", "[bad"):
with self.subTest(host=host):
self.assertFalse(is_local_request("127.0.0.1", headers(Host=host)))
self.assertFalse(
is_local_request("127.0.0.1", CIMultiDict({"Origin": "http://localhost"}))
)
for origin in ("http://evil.example:8188", "null", "http://[bad"):
def test_origin_is_required_and_must_be_the_request_origin(self):
# A header-less proxy or curl leaves Origin out; a page on another
# loopback port or scheme sends its own (security review M1, L2).
self.assertFalse(is_local_request("127.0.0.1", headers(Origin=None)))
for origin in (
"",
"null",
"http://localhost:3000",
"http://127.0.0.1:3000",
"http://127.0.0.1",
"https://127.0.0.1:8188",
"http://localhost:8188",
"http://foo.localhost:8188",
"http://evil.example:8188",
"http://[bad",
"http://127.0.0.1:8188/path",
"http://user@127.0.0.1:8188",
"file://127.0.0.1:8188",
):
with self.subTest(origin=origin):
self.assertFalse(is_local_request("127.0.0.1", headers(Origin=origin)))
self.assertFalse(is_local_request("127.0.0.1", headers(Origin=ORIGIN), "https"))
def test_sec_fetch_site_must_be_same_origin_when_sent(self):
self.assertTrue(
is_local_request("127.0.0.1", headers(Origin="http://127.0.0.1:8188"))
is_local_request("127.0.0.1", headers(Sec_Fetch_Site="same-origin"))
)
for site in ("same-site", "cross-site", "none", ""):
with self.subTest(site=site):
self.assertFalse(
is_local_request("127.0.0.1", headers(Sec_Fetch_Site=site))
)
def test_foreign_host_is_not_local(self):
# A DNS-rebinding page reaches 127.0.0.1 under its own name.
for host in ("evil.example:8188", "", "[bad", "127.0.0.1:bad"):
with self.subTest(host=host):
self.assertFalse(
is_local_request(
"127.0.0.1", headers(Host=host, Origin=f"http://{host}")
)
)
self.assertFalse(is_local_request("127.0.0.1", CIMultiDict({"Origin": ORIGIN})))
class TestRevealGate(unittest.TestCase):
def test_one_reveal_at_a_time_then_a_cooldown(self):
now = [100.0]
gate = file_reveal.RevealGate(cooldown=1.0, clock=lambda: now[0])
self.assertTrue(gate.try_enter())
self.assertFalse(gate.try_enter())
now[0] += 5
self.assertFalse(gate.try_enter())
gate.leave()
now[0] += 0.5
self.assertFalse(gate.try_enter())
now[0] += 0.5
self.assertTrue(gate.try_enter())
def test_zero_cooldown_only_blocks_while_busy(self):
gate = file_reveal.RevealGate(cooldown=0)
self.assertTrue(gate.try_enter())
self.assertFalse(gate.try_enter())
gate.leave()
self.assertTrue(gate.try_enter())
class TestRevealInFileManager(unittest.TestCase):
@@ -335,6 +416,15 @@ class TestRevealInFileManager(unittest.TestCase):
shell.assert_not_called()
self.popen.assert_not_called()
def test_a_path_on_another_drive_is_not_contained(self):
# Windows: commonpath raises ValueError across drives.
with (
patch.object(file_reveal.os.path, "commonpath", side_effect=ValueError),
self.assertRaises(file_reveal.RevealTargetChanged),
):
self.reveal()
self.popen.assert_not_called()
class TestReaping(unittest.TestCase):
def test_reap_waits_on_a_daemon_thread(self):
@@ -472,14 +562,21 @@ class TestRevealRoute(ImageAPITestCase):
self.roots.append(root)
self.api._reveal_opener = opener
self.api._reveal_gate = file_reveal.RevealGate(cooldown=0)
return app
def origin(self):
return str(self.client.make_url("/").origin())
def scope(self, index=0):
roots = self.api._get_all_output_dirs()
return self.api._gallery_root_scope(roots[index], index)
async def post(self, body, **request_headers):
response = await self.client.post(REVEAL, json=body, headers=request_headers)
# Like the workspace's fetch: a same-origin JSON POST.
sent = {"Origin": self.origin(), **request_headers}
sent = {key: value for key, value in sent.items() if value is not None}
response = await self.client.post(REVEAL, json=body, headers=sent)
return response.status, await response.json()
def body(self, path="sub/a.png", **changes):
@@ -523,6 +620,105 @@ class TestRevealRoute(ImageAPITestCase):
self.assertEqual((status, data["code"]), (403, "not-local"))
self.assertEqual(self.opened, [])
async def test_missing_or_foreign_origin_is_not_local(self):
make_png(self.output_dir / "sub" / "a.png")
port = self.client.make_url("/").port
for origin in (None, f"http://127.0.0.1:{port + 1}", "null"):
with self.subTest(origin=origin):
status, data = await self.post(self.body(), Origin=origin)
self.assertEqual((status, data["code"]), (403, "not-local"))
status, data = await self.post(self.body(), **{"Sec-Fetch-Site": "same-site"})
self.assertEqual((status, data["code"]), (403, "not-local"))
status, _ = await self.post(self.body(), **{"Sec-Fetch-Site": "same-origin"})
self.assertEqual(status, 200)
self.assertEqual(len(self.opened), 1)
async def test_only_json_bodies_are_accepted(self):
# text/plain would be a CORS "simple" request with no preflight (L2).
make_png(self.output_dir / "sub" / "a.png")
body = json.dumps(self.body())
for content_type in ("text/plain", "application/x-www-form-urlencoded", None):
with self.subTest(content_type=content_type):
sent = {"Origin": self.origin()}
if content_type:
sent["Content-Type"] = content_type
response = await self.client.post(
REVEAL, data=body.encode(), headers=sent
)
self.assertEqual(response.status, 415)
data = await response.json()
self.assertEqual(
(data["success"], data["code"]), (False, "unsupported-media-type")
)
self.assertEqual(self.opened, [])
async def test_a_reveal_in_progress_or_cooling_down_is_busy(self):
make_png(self.output_dir / "sub" / "a.png")
gate = self.api._reveal_gate
self.assertTrue(gate.try_enter())
status, data = await self.post(self.body())
self.assertEqual(status, 429)
self.assertEqual(data, {**data, "success": False, "code": "busy"})
gate.leave()
now = [50.0]
self.api._reveal_gate = file_reveal.RevealGate(
cooldown=1.0, clock=lambda: now[0]
)
statuses = [(await self.post(self.body()))[0] for _ in range(2)]
now[0] += 1.0
statuses.append((await self.post(self.body()))[0])
self.assertEqual(statuses, [200, 429, 200])
self.assertEqual(len(self.opened), 2)
async def test_a_failed_reveal_releases_the_gate(self):
make_png(self.output_dir / "sub" / "a.png")
self.outcome = OSError("cannot start")
status, _ = await self.post(self.body())
self.assertEqual(status, 500)
self.outcome = None
status, _ = await self.post(self.body())
self.assertEqual(status, 200)
async def test_default_gate_is_one_per_process_with_a_cooldown(self):
gate = type(self.api)._reveal_gate
self.assertIsInstance(gate, file_reveal.RevealGate)
self.assertEqual(gate.cooldown, file_reveal.REVEAL_COOLDOWN)
self.assertGreaterEqual(file_reveal.REVEAL_COOLDOWN, 1.0)
async def test_resolution_errors_are_404_and_never_log_the_path(self):
# ENAMETOOLONG and friends must not escape as a non-JSON 500 (L3).
make_png(self.output_dir / "sub" / "a.png")
secret = "sub/" + "n" * 300 + "/a.png"
with self.assertLogs(self.api.logger, level="DEBUG") as logs:
self.api.logger.debug("start")
status, data = await self.post(self.body(path=secret))
self.assertEqual(status, 404)
self.assertEqual(data, {"success": False, "error": "Image file not found"})
with patch.object(
type(self.api),
"_reveal_path",
staticmethod(MagicMock(side_effect=OSError(36, "too long", secret))),
):
status, data = await self.post(self.body(path=secret))
self.assertEqual(status, 404)
self.assertIs(data["success"], False)
for record in logs.records:
if record.levelno >= logging.WARNING:
self.assertNotIn("n" * 300, record.getMessage())
self.assertEqual(self.opened, [])
async def test_start_failure_never_logs_the_path_at_error_level(self):
make_png(self.output_dir / "sub" / "a.png")
self.outcome = OSError(2, "No such file", str(self.output_dir / "sub"))
with self.assertLogs(self.api.logger, level="DEBUG") as logs:
status, _ = await self.post(self.body())
self.assertEqual(status, 500)
errors = [r for r in logs.records if r.levelno >= logging.ERROR]
self.assertTrue(errors)
for record in errors:
self.assertNotIn(str(self.output_dir), record.getMessage())
self.assertIsNone(record.exc_info)
async def test_changed_or_missing_scope_is_rejected(self):
make_png(self.output_dir / "sub" / "a.png")
status, data = await self.post(self.body(root_scope="0" * 64))
@@ -554,7 +750,11 @@ class TestRevealRoute(ImageAPITestCase):
with self.subTest(changes=changes):
status, data = await self.post(self.body(**changes))
self.assertEqual(status, 400, data)
response = await self.client.post(REVEAL, data="[1]")
response = await self.client.post(
REVEAL,
data="[1]",
headers={"Origin": self.origin(), "Content-Type": "application/json"},
)
self.assertEqual(response.status, 400)
self.assertEqual(self.opened, [])
utils/file_reveal.py
+111 -19
View File
@@ -1,6 +1,7 @@
"""Show one gallery file in this computer's file manager (TESTING.md D38).
The route that calls this is local-only (``is_local_request``) and hands in a
The route that calls this is local-only (``is_local_request``), single-flight
(``RevealGate``) and hands in a
path it has already resolved inside a gallery root. Windows goes through the
shell API (no command line exists to parse); macOS and Linux openers run as an
argument list with an absolute executable, no shell and no inherited streams,
@@ -16,14 +17,32 @@ import shutil
import subprocess # nosec B404
import sys
import threading
import time
from collections.abc import Mapping
from concurrent.futures import ThreadPoolExecutor
from pathlib import Path
from urllib.parse import urlsplit
# A local reverse proxy makes every client look like loopback, so a request
# that carries any of these counts as remote.
PROXY_HEADERS = ("Forwarded", "X-Forwarded-For", "X-Real-IP", "X-Forwarded-Host")
# that carries any forwarding-style header counts as remote: these names, and
# every X-Forwarded-*, X-Original-* and *-Client-IP header.
FORWARDING_HEADERS = frozenset(
(
"forwarded",
"via",
"x-real-ip",
"cf-connecting-ip",
"true-client-ip",
"x-client-ip",
"x-cluster-client-ip",
"fastly-client-ip",
)
)
FORWARDING_PREFIXES = ("x-forwarded-", "x-original-")
FORWARDING_SUFFIXES = ("-client-ip",)
DEFAULT_PORTS = {"http": 80, "https": 443}
# One reveal at a time per process, then a pause before the next.
REVEAL_COOLDOWN = 1.0
# dbus-send waits this long for the FileManager1 reply, so its exit status
# says whether a file manager really took the request.
DBUS_REPLY_TIMEOUT_MS = 2000
@@ -88,43 +107,116 @@ def _loopback_address(value: str | None) -> bool:
return (mapped or address).is_loopback
def _loopback_host(host: str | None) -> bool:
"""True for a Host or Origin host naming this computer.
def _loopback_host(host: str) -> bool:
"""True for a Host name naming this computer.
Rejects every other name, so a DNS-rebinding page whose name now resolves
to 127.0.0.1 still reads as remote.
"""
if not host:
return False
host = host.rstrip(".").lower()
if host == "localhost" or host.endswith(".localhost"):
return True
return _loopback_address(host.strip("[]"))
def _authority_host(authority: str) -> str | None:
def _authority(authority: str) -> tuple[str, int | None] | None:
"""``(host, port)`` of a Host value, or None when it does not parse."""
try:
return urlsplit(f"//{authority}").hostname
parts = urlsplit(f"//{authority}")
host, port = parts.hostname, parts.port
except ValueError:
return None
return (host, port) if host else None
def is_local_request(remote: str | None, headers: Mapping) -> bool:
"""True only for a direct loopback client addressing this computer by a
loopback name, with no proxy header and no foreign Origin."""
if any(name in headers for name in PROXY_HEADERS):
def _forwarded(headers: Mapping) -> bool:
for name in headers.keys():
name = name.lower()
if (
name in FORWARDING_HEADERS
or name.startswith(FORWARDING_PREFIXES)
or name.endswith(FORWARDING_SUFFIXES)
):
return True
return False
def _same_origin(origin: str, scheme: str, host: tuple[str, int | None]) -> bool:
"""True when *origin* is exactly this request's scheme://host:port."""
default = DEFAULT_PORTS.get(scheme)
if default is None:
return False
try:
parts = urlsplit(origin)
origin_host, origin_port = parts.hostname, parts.port
except ValueError:
return False
if (
parts.scheme != scheme
or not origin_host
or parts.username is not None
or parts.password is not None
or parts.path
or parts.query
or parts.fragment
):
return False
host_name, host_port = host
return (origin_host.rstrip("."), origin_port or default) == (
host_name.rstrip("."),
host_port or default,
)
def is_local_request(remote: str | None, headers: Mapping, scheme="http") -> bool:
"""True only for a same-origin browser request from this computer.
That is: no forwarding-style header, a loopback client, a loopback name
in Host, an ``Origin`` that is exactly this request's scheme://host:port
(browsers send it on every fetch POST, so a missing one is refused), and
``Sec-Fetch-Site: same-origin`` whenever that header is sent.
"""
if _forwarded(headers):
return False
if not _loopback_address(remote):
return False
if not _loopback_host(_authority_host(headers.get("Host", ""))):
host = _authority(headers.get("Host", ""))
if host is None or not _loopback_host(host[0]):
return False
origin = headers.get("Origin")
if origin is None:
return True
try:
return _loopback_host(urlsplit(origin).hostname)
except ValueError:
if not origin or not _same_origin(origin, scheme, host):
return False
site = headers.get("Sec-Fetch-Site")
return site is None or site == "same-origin"
class RevealGate:
"""Single flight plus a cooldown: at most one reveal runs per process,
and the next may start only *cooldown* seconds after it finished."""
def __init__(self, cooldown: float = REVEAL_COOLDOWN, clock=time.monotonic):
self.cooldown = cooldown
self._clock = clock
self._lock = threading.Lock()
self._busy = False
self._finished: float | None = None
def try_enter(self) -> bool:
with self._lock:
if self._busy:
return False
if (
self._finished is not None
and self._clock() - self._finished < self.cooldown
):
return False
self._busy = True
return True
def leave(self) -> None:
with self._lock:
self._busy = False
self._finished = self._clock()
def _executable(name: str) -> str | None: