From 9cf91339b708a245762fa38ffeec9702b381e0db Mon Sep 17 00:00:00 2001
From: Matt Miller <mattmiller@comfy.org>
Date: Wed, 29 Jul 2026 23:29:36 -0700
Subject: [PATCH] Fix SVG previews broken by the stored-XSS forced-download
 (#15149)

* Fix SVG previews broken by the stored-XSS forced-download

/view and the assets download route force every SVG to
application/octet-stream + attachment. That blocks the stored XSS from
GHSA-779p-m5rp-r4h4, but it also breaks the SVG node output and Media
Assets previews, which request the file with a plain <img>.

Exempt only that case. An SVG referenced by an <img> loads in secure
static mode with scripting and external references disabled, so the
payload cannot fire. The attack needs the SVG to become a document,
which arrives with a different Sec-Fetch-Dest. Browsers set that header
themselves and page script cannot override it. A missing header, from a
non-browser client or a proxy that strips it, fails closed.

The blocklist itself is unchanged; this is a call-site gate.

* Don't let a cache replay the inline SVG into document context

The Sec-Fetch-Dest exemption makes /view and the assets content route vary
their Content-Type and Content-Disposition on a request header, but neither
response said so. FileResponse emits Last-Modified/ETag and the cache_control
middleware skips /view (the filename is in the query string, not the path), so
the inline image/svg+xml variant is heuristically cacheable. A cache keyed on
the URL alone could hand an entry primed by an <img> load to a later top-level
navigation of the same URL, turning the SVG back into a document and
re-enabling the stored XSS the forced download blocks.

Set Vary: Sec-Fetch-Dest and Cache-Control: no-store on both branches, not just
the exempt one: a cached attachment replayed to an <img> would re-break the
preview this fix exists to restore.

Also strip parameters from content_type before building the assets response.
mime_type there is uploader-supplied and unvalidated, and aiohttp rejects a
charset in the content_type argument with ValueError, so a stored
"image/svg+xml; charset=utf-8" turned a valid inline SVG into a 500.

Route-level guards now pin the headers on both branches and the parameterised
mime type; all three fail against the previous commit.
---
 app/assets/api/routes.py                      |  29 ++-
 folder_paths.py                               |  17 ++
 server.py                                     |  34 ++--
 ...test_ghsa_779p_06_inline_svg_image_dest.py | 191 ++++++++++++++++++
 4 files changed, 252 insertions(+), 19 deletions(-)
 create mode 100644 tests-unit/security_test/test_ghsa_779p_06_inline_svg_image_dest.py

diff --git a/app/assets/api/routes.py b/app/assets/api/routes.py
index 43e60094c..e25b8a57f 100644
--- a/app/assets/api/routes.py
+++ b/app/assets/api/routes.py
@@ -315,15 +315,29 @@ async def download_asset_content(request: web.Request) -> web.Response:
             404, "FILE_NOT_FOUND", "Underlying file not found on disk."
         )
 
-    # User-controlled asset content must never render inline in the app origin
+    # User-controlled asset content must not render inline in the app origin
     # (stored XSS via SVG/HTML/XML). Force dangerous types to download and
-    # override any requested inline disposition. Centralised through
-    # folder_paths.is_dangerous_content_type so this can't drift from /view and
-    # /userdata (the previous inline set here omitted image/svg+xml and missed
-    # the charset/casing/+xml-dialect bypasses).
+    # override any requested inline disposition; SVG loaded into an <img> is
+    # exempt, see renders_safely_as_image. Centralised through folder_paths so
+    # this can't drift from /view and /userdata (the previous inline set here
+    # omitted image/svg+xml and missed the charset/casing/+xml-dialect bypasses).
+    extra_headers = {}
+    sec_fetch_dest = request.headers.get("Sec-Fetch-Dest")
     if folder_paths.is_dangerous_content_type(content_type):
-        content_type = "application/octet-stream"
-        disposition = "attachment"
+        # This response now depends on a request header, so it must not be
+        # reused across destinations by a browser or intermediary cache: an
+        # inline SVG primed by an <img> fetch and replayed to a document
+        # navigation of the same URL would re-enable the stored XSS.
+        extra_headers["Vary"] = "Sec-Fetch-Dest"
+        extra_headers["Cache-Control"] = "no-store"
+        if not folder_paths.renders_safely_as_image(content_type, sec_fetch_dest):
+            content_type = "application/octet-stream"
+            disposition = "attachment"
+
+    # mime_type is uploader-supplied and unvalidated, so it can carry
+    # parameters. aiohttp rejects a charset in the content_type argument with
+    # ValueError, which would turn a valid inline SVG into a 500.
+    content_type = content_type.split(";", 1)[0].strip() or "application/octet-stream"
 
     safe_name = (filename or "").replace("\r", "").replace("\n", "")
     encoded = urllib.parse.quote(safe_name)
@@ -356,6 +370,7 @@ async def download_asset_content(request: web.Request) -> web.Response:
             "Content-Disposition": cd,
             "Content-Length": str(file_size),
             "X-Content-Type-Options": "nosniff",
+            **extra_headers,
         },
     )
 
diff --git a/folder_paths.py b/folder_paths.py
index bd3f25095..df53542dc 100644
--- a/folder_paths.py
+++ b/folder_paths.py
@@ -306,6 +306,23 @@ def is_dangerous_content_type(content_type: str | None) -> bool:
     return normalized.endswith('+xml') or normalized.endswith('/xml')
 
 
+def renders_safely_as_image(content_type: str | None, sec_fetch_dest: str | None) -> bool:
+    """Return True if a dangerous `content_type` is safe to serve inline anyway.
+
+    An SVG referenced by an ``<img>`` is loaded in secure static mode: scripts
+    and external references are disabled, so the stored XSS that
+    ``is_dangerous_content_type`` guards against cannot fire. The attack needs
+    the SVG to become a document, which is a separate ``Sec-Fetch-Dest``.
+    Browsers set that header themselves and script cannot override it (the
+    ``Sec-`` prefix makes it a forbidden header name), so it is trustworthy for
+    this decision. Anything else, including a missing header from a non-browser
+    client or a proxy that strips it, fails closed.
+    """
+    if sec_fetch_dest != 'image':
+        return False
+    return (content_type or '').split(';', 1)[0].strip().lower() == 'image/svg+xml'
+
+
 def is_within_directory(directory: str, target: str) -> bool:
     """Return True if `target` resolves to a path inside `directory`.
 
diff --git a/server.py b/server.py
index e28fe2d22..c9ffcaa0d 100644
--- a/server.py
+++ b/server.py
@@ -624,8 +624,9 @@ class PromptServer():
                         # For security, force renderable/active types (HTML, JS,
                         # CSS, SVG, XML — anything that can carry inline <script>
                         # and execute in the page origin) to download instead of
-                        # displaying inline, preventing stored XSS. The
-                        # attachment disposition is the load-bearing guard: a
+                        # displaying inline, preventing stored XSS. SVG loaded
+                        # into an <img> is exempt, see renders_safely_as_image.
+                        # The attachment disposition is the load-bearing guard: a
                         # bare filename= hint does not force a download per
                         # RFC 6266, so we only attach it on the dangerous branch
                         # to avoid breaking inline display of legitimate images.
@@ -635,18 +636,27 @@ class PromptServer():
                         # header's quoted-string and malform the disposition.
                         safe_filename = filename.replace("\\", "\\\\").replace('"', '\\"')
                         disposition = f"filename=\"{safe_filename}\""
+                        headers = {"X-Content-Type-Options": "nosniff"}
+                        sec_fetch_dest = request.headers.get('Sec-Fetch-Dest')
                         if folder_paths.is_dangerous_content_type(content_type):
-                            content_type = 'application/octet-stream'
-                            disposition = f"attachment; filename=\"{safe_filename}\""
+                            # This response now depends on a request header, so
+                            # it must not be reused across destinations.
+                            # FileResponse emits Last-Modified/ETag and nothing
+                            # sets Cache-Control on /view, which makes it
+                            # heuristically cacheable: without these headers a
+                            # cache could replay the inline SVG served to an
+                            # <img> to a later document navigation of the same
+                            # URL and re-enable the stored XSS, or replay the
+                            # attachment to an <img> and re-break the preview.
+                            headers["Vary"] = "Sec-Fetch-Dest"
+                            headers["Cache-Control"] = "no-store"
+                            if not folder_paths.renders_safely_as_image(content_type, sec_fetch_dest):
+                                content_type = 'application/octet-stream'
+                                disposition = f"attachment; filename=\"{safe_filename}\""
 
-                        return web.FileResponse(
-                            file,
-                            headers={
-                                "Content-Disposition": disposition,
-                                "Content-Type": content_type,
-                                "X-Content-Type-Options": "nosniff"
-                            }
-                        )
+                        headers["Content-Disposition"] = disposition
+                        headers["Content-Type"] = content_type
+                        return web.FileResponse(file, headers=headers)
 
             return web.Response(status=404)
 
diff --git a/tests-unit/security_test/test_ghsa_779p_06_inline_svg_image_dest.py b/tests-unit/security_test/test_ghsa_779p_06_inline_svg_image_dest.py
new file mode 100644
index 000000000..e0964e0db
--- /dev/null
+++ b/tests-unit/security_test/test_ghsa_779p_06_inline_svg_image_dest.py
@@ -0,0 +1,191 @@
+"""CI unit guard for the Sec-Fetch-Dest exemption to FIX #5 of GHSA-779p-m5rp-r4h4.
+
+FIX #5 forced every SVG served by /view and the assets download route to
+application/octet-stream + Content-Disposition: attachment. That blocked the
+stored XSS, but it also stopped SVG node outputs and Media Assets thumbnails
+from rendering, because the frontend requests them with a plain <img>.
+
+An SVG referenced by an <img> is loaded in secure static mode: scripting and
+external references are disabled, so the payload from vuln #5 cannot execute.
+The attack needs the SVG to load as a document, which arrives with a different
+Sec-Fetch-Dest. Browsers set that header themselves and page script cannot
+override it, so renders_safely_as_image() uses it to re-allow only the <img>
+case. Everything else, including a missing header, keeps the forced download.
+
+Because the decision reads a request header, the response must also carry
+Vary: Sec-Fetch-Dest and Cache-Control: no-store. Without them a cache keyed on
+the URL alone can replay the inline SVG served to an <img> to a later document
+navigation of the same URL, which re-enables the very XSS the exemption is
+built around. The route tests at the bottom pin those headers.
+
+server.py cannot be imported in a unit test (importing it spins up the full
+PromptServer/aiohttp app and its global side effects), so the /view side is
+covered by pinning the helper its closure calls.
+"""
+
+import pytest
+from aiohttp import web
+
+import folder_paths
+from app.assets.api import routes as asset_routes
+from app.assets.services.schemas import DownloadResolutionResult
+
+
+# Every Sec-Fetch-Dest that must keep the forced download. 'document' is the
+# vuln #5 attack itself; the rest either cannot execute an SVG or have no
+# reason to receive one inline. None covers curl and proxies that strip the
+# header.
+UNSAFE_DESTS = [
+    'document',
+    'iframe',
+    'object',
+    'embed',
+    'frame',
+    'script',
+    'style',
+    'empty',
+    '',
+    None,
+]
+
+
+def test_svg_renders_inline_for_image_dest():
+    assert folder_paths.renders_safely_as_image('image/svg+xml', 'image')
+
+
+def test_svg_forced_to_download_for_every_other_dest():
+    for dest in UNSAFE_DESTS:
+        assert not folder_paths.renders_safely_as_image('image/svg+xml', dest), (
+            f"SVG must not be served inline for Sec-Fetch-Dest={dest!r}. Only "
+            "an <img> load is safe, everything else can reach document context"
+        )
+
+
+def test_missing_header_fails_closed():
+    assert not folder_paths.renders_safely_as_image('image/svg+xml', None)
+
+
+def test_exemption_normalises_parameters_and_casing():
+    for content_type in ('IMAGE/SVG+XML', 'image/svg+xml; charset=utf-8', '  image/svg+xml  '):
+        assert folder_paths.renders_safely_as_image(content_type, 'image'), (
+            f"{content_type!r} is an SVG and must not be excluded from the "
+            "exemption by casing or a charset parameter"
+        )
+
+
+def test_other_dangerous_types_are_never_exempt():
+    # Only SVG is safe as an <img>. Nothing else in the blocklist may ride the
+    # image dest back into an inline response.
+    for content_type in ('text/html', 'text/javascript', 'text/css',
+                         'application/xml', 'text/xml', 'application/xhtml+xml',
+                         'image/svg+xml.html', 'application/rss+xml'):
+        assert not folder_paths.renders_safely_as_image(content_type, 'image'), (
+            f"{content_type!r} must stay forced-download regardless of Sec-Fetch-Dest"
+        )
+
+
+def test_exemption_does_not_widen_the_blocklist():
+    # The exemption is a call-site gate, not a change to what counts as
+    # dangerous. is_dangerous_content_type must still flag SVG on its own.
+    assert folder_paths.is_dangerous_content_type('image/svg+xml')
+
+
+# --- Response-level guards on the assets content route -----------------------
+#
+# The helper above decides correctly, but the exemption is only safe if the
+# response cannot be reused across Sec-Fetch-Dest values. These mount the real
+# route and assert on the headers that actually ship.
+
+SVG_PAYLOAD = b'<svg xmlns="http://www.w3.org/2000/svg"><script>alert(1)</script></svg>'
+ASSET_ID = "00000000-0000-4000-8000-000000000001"
+CONTENT_URL = f"/api/assets/{ASSET_ID}/content"
+
+
+class _StubUserManager:
+    def get_request_user_id(self, request):
+        return "test-user"
+
+
+@pytest.fixture
+def asset_app(monkeypatch, tmp_path):
+    """Mount the real /api/assets/{id}/content route over a stored SVG."""
+
+    def _factory(stored_mime_type):
+        svg = tmp_path / "thumb.svg"
+        svg.write_bytes(SVG_PAYLOAD)
+
+        monkeypatch.setattr(asset_routes, "_ASSETS_ENABLED", True)
+        monkeypatch.setattr(asset_routes, "USER_MANAGER", _StubUserManager())
+        monkeypatch.setattr(
+            asset_routes,
+            "resolve_asset_for_download",
+            lambda reference_id, owner_id: DownloadResolutionResult(
+                abs_path=str(svg),
+                content_type=stored_mime_type,
+                download_name="thumb.svg",
+            ),
+        )
+
+        app = web.Application()
+        app.add_routes(asset_routes.ROUTES)
+        return app
+
+    return _factory
+
+
+@pytest.mark.asyncio
+async def test_inline_svg_response_is_not_cacheable_across_destinations(
+    aiohttp_client, asset_app
+):
+    client = await aiohttp_client(asset_app("image/svg+xml"))
+    resp = await client.get(
+        CONTENT_URL, params={"disposition": "inline"}, headers={"Sec-Fetch-Dest": "image"}
+    )
+
+    assert resp.status == 200
+    assert "image/svg+xml" in resp.headers.get("Content-Type", "").lower()
+    # The load-bearing assertion: a cache must not be able to hand this inline
+    # SVG to a later document navigation of the same URL.
+    assert "sec-fetch-dest" in resp.headers.get("Vary", "").lower(), (
+        "The response varies on Sec-Fetch-Dest but does not say so, so a cache "
+        "keyed on the URL alone can replay the inline SVG into document context."
+    )
+    assert "no-store" in resp.headers.get("Cache-Control", "").lower()
+
+
+@pytest.mark.asyncio
+async def test_forced_download_response_also_declares_the_variance(
+    aiohttp_client, asset_app
+):
+    # The attachment branch needs the same headers, in both directions: a
+    # cached octet-stream replayed to an <img> re-breaks the preview this fix
+    # exists to restore.
+    client = await aiohttp_client(asset_app("image/svg+xml"))
+    resp = await client.get(
+        CONTENT_URL,
+        params={"disposition": "inline"},
+        headers={"Sec-Fetch-Dest": "document"},
+    )
+
+    assert resp.status == 200
+    assert "application/octet-stream" in resp.headers.get("Content-Type", "").lower()
+    assert "attachment" in resp.headers.get("Content-Disposition", "").lower()
+    assert "sec-fetch-dest" in resp.headers.get("Vary", "").lower()
+    assert "no-store" in resp.headers.get("Cache-Control", "").lower()
+
+
+@pytest.mark.asyncio
+async def test_parameterised_svg_mime_type_does_not_500(aiohttp_client, asset_app):
+    # mime_type is uploader-supplied and unvalidated. aiohttp rejects a charset
+    # in the content_type argument with ValueError, so the exempt branch must
+    # strip parameters before building the response.
+    client = await aiohttp_client(asset_app("image/svg+xml; charset=utf-8"))
+    resp = await client.get(
+        CONTENT_URL, params={"disposition": "inline"}, headers={"Sec-Fetch-Dest": "image"}
+    )
+
+    assert resp.status == 200, (
+        "A charset parameter on the stored mime type must not turn a valid "
+        "inline SVG request into a 500."
+    )
+    assert "image/svg+xml" in resp.headers.get("Content-Type", "").lower()