diff --git a/app/assets/api/routes.py b/app/assets/api/routes.py index 43e60094c29..e25b8a57fcd 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 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 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 stream_file_chunks(): "Content-Disposition": cd, "Content-Length": str(file_size), "X-Content-Type-Options": "nosniff", + **extra_headers, }, ) diff --git a/app/user_manager.py b/app/user_manager.py index de261ad3968..55e7e81e3d1 100644 --- a/app/user_manager.py +++ b/app/user_manager.py @@ -343,13 +343,22 @@ async def getuserdata(request): # XSS). Content-Disposition: attachment is the load-bearing guard; # the content-type override and nosniff are defence in depth. content_type = mimetypes.guess_type(path)[0] or 'application/octet-stream' - if folder_paths.is_dangerous_content_type(content_type): - content_type = 'application/octet-stream' + + user_root = self.get_request_user_filepath(request, None, create_dir=False) + is_user_css = path == os.path.abspath(os.path.join(user_root, "user.css")) + + if is_user_css: + content_type = "text/css" + disposition = "inline" + else: + if folder_paths.is_dangerous_content_type(content_type): + content_type = 'application/octet-stream' + disposition = "attachment" return web.FileResponse(path, headers={ "Content-Type": content_type, "X-Content-Type-Options": "nosniff", - "Content-Disposition": "attachment", + "Content-Disposition": disposition, }) @routes.post("/userdata/{file}") diff --git a/comfy_execution/jobs.py b/comfy_execution/jobs.py index f0ad59f86f7..34c06363ba3 100644 --- a/comfy_execution/jobs.py +++ b/comfy_execution/jobs.py @@ -170,6 +170,19 @@ def is_previewable(media_type: str, item: dict) -> bool: return False +def is_text_preview(media_type: str, item: dict) -> bool: + """ + Check if a previewable output item is textual rather than visual media. + + Saved text files (SaveText's .txt/.md/.json) are real outputs but must not + outrank visual media when picking the job preview. + """ + if media_type == 'text': + return True + filename = item.get('filename', '').lower() + return any(filename.endswith(ext) for ext in TEXT_EXTENSIONS) + + def normalize_queue_item(item: tuple, status: str) -> dict: """Convert queue item tuple to unified job dict. @@ -259,8 +272,13 @@ def get_outputs_summary(outputs: dict) -> tuple[int, Optional[dict]]: Returns (outputs_count, preview_output). Preview priority (matching frontend): - 1. type="output" with previewable media - 2. Any previewable media + 1. type="output" visual media (saved images/video/audio/3d) + 2. any other previewable visual media (e.g. temp/preview images) + 3. saved text file (e.g. SaveText's .txt/.md/.json) + 4. raw text (only when the job produced nothing else previewable) + + Text is kept in its own slots so node/execution order can't let a text + output mask a visual one (e.g. a text node that runs before an image). Text content entries (strings under 'text') are preview-only metadata, matching the frontend's METADATA_KEYS: they can serve as the fallback @@ -269,6 +287,8 @@ def get_outputs_summary(outputs: dict) -> tuple[int, Optional[dict]]: count = 0 preview_output = None fallback_preview = None + text_file_fallback = None + text_fallback = None for node_id, node_outputs in outputs.items(): if not isinstance(node_outputs, dict): @@ -296,8 +316,8 @@ def get_outputs_summary(outputs: dict) -> tuple[int, Optional[dict]]: 'nodeId': node_id, 'mediaType': media_type } - if fallback_preview is None: - fallback_preview = enriched + if text_fallback is None: + text_fallback = enriched continue # normalize_output_item returned a dict (e.g. 3D file) item = normalized @@ -314,12 +334,15 @@ def get_outputs_summary(outputs: dict) -> tuple[int, Optional[dict]]: } if 'mediaType' not in item: enriched['mediaType'] = media_type - if item.get('type') == 'output': + if is_text_preview(media_type, item): + if text_file_fallback is None: + text_file_fallback = enriched + elif item.get('type') == 'output': preview_output = enriched elif fallback_preview is None: fallback_preview = enriched - return count, preview_output or fallback_preview + return count, preview_output or fallback_preview or text_file_fallback or text_fallback def apply_sorting(jobs: list[dict], sort_by: str, sort_order: str) -> list[dict]: diff --git a/folder_paths.py b/folder_paths.py index bd3f250954d..df53542dc8d 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 ```` 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 e28fe2d2247..c9ffcaa0da9 100644 --- a/server.py +++ b/server.py @@ -624,8 +624,9 @@ async def view_image(request): # For security, force renderable/active types (HTML, JS, # CSS, SVG, XML — anything that can carry inline ' +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 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() diff --git a/tests/execution/test_jobs.py b/tests/execution/test_jobs.py index f7cb612e4f5..cef2b41cbe5 100644 --- a/tests/execution/test_jobs.py +++ b/tests/execution/test_jobs.py @@ -280,6 +280,86 @@ def test_string_3d_filename_used_as_fallback(self): assert preview['filename'] == 'model.glb' assert preview['mediaType'] == '3d' + def test_media_preview_preferred_over_text(self): + """A visual output wins the preview even when a text node is iterated + first (regression: text could mask a later temp/preview image).""" + outputs = { + 'text_node': {'text': ['a caption']}, + 'image_node': {'images': [{'filename': 'preview.png', 'type': 'temp'}]}, + } + count, preview = get_outputs_summary(outputs) + # Text is preview-only metadata and not counted; only the image counts. + assert count == 1 + assert preview['filename'] == 'preview.png' + assert preview['mediaType'] == 'images' + + def test_text_used_as_preview_when_no_media(self): + """Text is the preview only when the job produced no media output.""" + outputs = { + 'text_node': {'text': ['hello world']}, + } + count, preview = get_outputs_summary(outputs) + assert count == 0 # text entries are not counted as outputs + assert preview['mediaType'] == 'text' + assert preview['content'] == 'hello world' + + def test_media_preview_preferred_over_saved_text_file(self): + """A visual output wins the preview over a saved text file (SaveText), + even a temp/preview image iterated after the text node.""" + outputs = { + 'save_text': { + 'text': ['the text'], + 'files': [{'filename': 'ComfyUI_00001.txt', 'subfolder': '', 'type': 'output'}], + }, + 'preview_image': {'images': [{'filename': 'preview.png', 'type': 'temp'}]}, + } + count, preview = get_outputs_summary(outputs) + assert count == 2 # the .txt file and the image; raw text is metadata + assert preview['filename'] == 'preview.png' + assert preview['mediaType'] == 'images' + + def test_saved_media_preferred_over_saved_text_file(self): + outputs = { + 'save_text': { + 'text': ['the text'], + 'files': [{'filename': 'ComfyUI_00001.txt', 'subfolder': '', 'type': 'output'}], + }, + 'save_image': {'images': [{'filename': 'result.png', 'type': 'output'}]}, + } + count, preview = get_outputs_summary(outputs) + assert count == 2 + assert preview['filename'] == 'result.png' + + def test_mime_format_file_preferred_over_saved_text_file(self): + """Custom-node outputs previewable via MIME format (e.g. VHS videos + under arbitrary keys) rank as visual media, above saved text files.""" + outputs = { + 'save_text': { + 'files': [{'filename': 'notes.md', 'subfolder': '', 'type': 'output'}], + }, + 'video_node': { + 'files': [{'filename': 'clip.webm', 'format': 'video/webm', 'type': 'output'}], + }, + } + count, preview = get_outputs_summary(outputs) + assert count == 2 + assert preview['filename'] == 'clip.webm' + + + def test_saved_text_file_preferred_over_raw_text(self): + """With no media in the job, the saved text file (a real, counted + output) is the preview rather than the raw text metadata.""" + outputs = { + 'save_text': { + 'text': ['the text'], + 'files': [{'filename': 'ComfyUI_00001.txt', 'subfolder': '', 'type': 'output'}], + }, + } + count, preview = get_outputs_summary(outputs) + assert count == 1 + assert preview['filename'] == 'ComfyUI_00001.txt' + assert preview['mediaType'] == 'files' + class TestHas3DExtension: """Unit tests for has_3d_extension()"""