diff --git a/SECURITY.md b/SECURITY.md index 299b0067b..d88d7115f 100644 --- a/SECURITY.md +++ b/SECURITY.md @@ -9,6 +9,11 @@ ComfyUI is designed to run locally. By default, the server binds to `127.0.0.1`, - Anyone with access to the ComfyUI URL is trusted (a direct consequence of the localhost-only default). - PyTorch and other dependencies are at the versions we ship or recommend in the README. +The asset `open-location` action trusts the direct transport peer's loopback +address and opens the host file manager. Do not forward that endpoint through a +same-host reverse proxy unless an additional access control restricts it to the +host operator. + A report is in scope only if it affects a user operating within this threat model. ## What We Consider a Vulnerability diff --git a/app/assets/api/routes.py b/app/assets/api/routes.py index 9f1fc9340..737f898b6 100644 --- a/app/assets/api/routes.py +++ b/app/assets/api/routes.py @@ -25,6 +25,7 @@ from app.assets.api.upload import ( ) from app.assets.seeder import ScanInProgressError, asset_seeder from app.assets.services import ( + AssetFileDeleteForbiddenError, DependencyMissingError, HashMismatchError, apply_tags, @@ -641,10 +642,11 @@ async def update_asset_route(request: web.Request) -> web.Response: @_require_assets_feature_enabled async def delete_asset_route(request: web.Request) -> web.Response: reference_id = str(uuid.UUID(request.match_info["id"])) - owner_id = USER_MANAGER.get_request_user_id(request) delete_content = request.query.get("delete_content") == "true" + owner_id = None try: + owner_id = USER_MANAGER.get_request_user_id(request) if delete_content: detail = get_asset_detail(reference_id=reference_id, owner_id=owner_id) if detail is None: @@ -684,7 +686,7 @@ async def delete_asset_route(request: web.Request) -> web.Response: owner_id=owner_id, delete_content_if_orphan=False, ) - except PermissionError as error: + except AssetFileDeleteForbiddenError as error: return _build_error_response( 403, "ASSET_DELETE_FORBIDDEN", @@ -709,6 +711,12 @@ async def delete_asset_route(request: web.Request) -> web.Response: @ROUTES.post(f"/api/assets/{{id:{UUID_RE}}}/open-location") @_require_assets_feature_enabled async def open_asset_location_route(request: web.Request) -> web.Response: + """Open an output in the host file manager for a direct local client. + + The transport peer is the trust boundary. Operators must not forward this + endpoint through a same-host reverse proxy without an additional access + control that restricts it to the host operator. + """ reference_id = str(uuid.UUID(request.match_info["id"])) if not is_loopback_address(request.remote): return _build_error_response( @@ -724,10 +732,17 @@ async def open_asset_location_route(request: web.Request) -> web.Response: "Cross-site requests cannot open local file locations.", ) - detail = get_asset_detail( - reference_id=reference_id, - owner_id=USER_MANAGER.get_request_user_id(request), - ) + owner_id = None + try: + owner_id = USER_MANAGER.get_request_user_id(request) + detail = get_asset_detail(reference_id=reference_id, owner_id=owner_id) + except Exception: + logging.exception( + "get_asset_detail failed for reference_id=%s, owner_id=%s", + reference_id, + owner_id, + ) + return _build_error_response(500, "INTERNAL", "Unexpected server error.") if detail is None: return _build_error_response( 404, @@ -749,7 +764,7 @@ async def open_asset_location_route(request: web.Request) -> web.Response: ) try: - reveal_file_in_file_manager(file_path) + await asyncio.to_thread(reveal_file_in_file_manager, file_path) except FileNotFoundError: return _build_error_response( 404, diff --git a/app/assets/services/__init__.py b/app/assets/services/__init__.py index a7bdbc25b..cf5c20cb1 100644 --- a/app/assets/services/__init__.py +++ b/app/assets/services/__init__.py @@ -1,4 +1,5 @@ from app.assets.services.asset_management import ( + AssetFileDeleteForbiddenError, asset_exists, delete_asset_reference, delete_asset_reference_with_file, @@ -53,6 +54,7 @@ from app.assets.services.tagging import ( __all__ = [ "AddTagsResult", + "AssetFileDeleteForbiddenError", "AssetData", "AssetDetailResult", "AssetSummaryData", diff --git a/app/assets/services/asset_management.py b/app/assets/services/asset_management.py index 93cf6ba87..4e3b78fe7 100644 --- a/app/assets/services/asset_management.py +++ b/app/assets/services/asset_management.py @@ -1,7 +1,9 @@ import contextlib +import errno import logging import mimetypes import os +import shutil import uuid from datetime import timezone from typing import Sequence @@ -57,6 +59,45 @@ from app.assets.services.schemas import ( from app.database.db import create_session +class AssetFileDeleteForbiddenError(PermissionError): + pass + + +def _copy_file_with_fsync(source_path: str, destination_path: str) -> None: + with open(source_path, "rb") as source_file, open( + destination_path, "wb" + ) as destination_file: + shutil.copyfileobj(source_file, destination_file) + destination_file.flush() + os.fsync(destination_file.fileno()) + shutil.copystat(source_path, destination_path) + + +def _stage_file_across_devices(source_path: str, staged_path: str) -> None: + try: + _copy_file_with_fsync(source_path, staged_path) + os.remove(source_path) + except OSError: + with contextlib.suppress(OSError): + os.remove(staged_path) + raise + + +def _restore_file_across_devices(staged_path: str, original_path: str) -> None: + restore_path = os.path.join( + os.path.dirname(original_path), f".comfy-restore-{uuid.uuid4().hex}.tmp" + ) + try: + _copy_file_with_fsync(staged_path, restore_path) + os.replace(restore_path, original_path) + except OSError: + with contextlib.suppress(OSError): + os.remove(restore_path) + raise + with contextlib.suppress(OSError): + os.remove(staged_path) + + def get_asset_detail( reference_id: str, owner_id: str = "", @@ -234,6 +275,20 @@ def delete_asset_reference_with_file( allowed_directories: Sequence[str], allow_ownerless: bool = False, ) -> bool: + """Delete a reference and permanently remove its source file. + + ``expected_file_path`` must still match the stored path, and the caller must + restrict ``allowed_directories`` to managed roots. The source is staged in + ``staging_directory`` before the database commit and restored if the commit + fails. ``allow_ownerless`` permits deleting legacy ownerless references. + The asset row is removed only when no references of any state remain. + The filesystem move and database commit are failure-compensated but are not + a single atomic transaction; abrupt process termination between them is not + recoverable by this function. + + Raises ``AssetFileDeleteForbiddenError`` when ownership, path identity, or + containment checks fail. + """ with create_session() as session: ref = get_reference_by_id(session, reference_id=reference_id) if ref is None or ref.deleted_at is not None: @@ -243,7 +298,7 @@ def delete_asset_reference_with_file( owns_reference = ref.owner_id == caller_owner_id may_delete_ownerless = allow_ownerless and ref.owner_id == "" if not (owns_reference or may_delete_ownerless): - raise PermissionError( + raise AssetFileDeleteForbiddenError( "Only the owning user can delete an asset's source file." ) @@ -252,27 +307,27 @@ def delete_asset_reference_with_file( if not file_path or os.path.realpath(file_path) != os.path.realpath( expected_file_path ): - raise PermissionError( + raise AssetFileDeleteForbiddenError( "The asset source path changed before it could be deleted." ) if not any( folder_paths.is_within_directory(directory, file_path) for directory in allowed_directories ): - raise PermissionError( + raise AssetFileDeleteForbiddenError( "The asset source file is outside the directories allowed for deletion." ) deleted = delete_reference_by_id( - session, reference_id=reference_id, owner_id=owner_id + session, reference_id=reference_id, owner_id=caller_owner_id ) if not deleted: session.rollback() return False staged_file_path: str | None = None + cross_device_staging = False if file_path and os.path.isfile(file_path): - source_directory = os.path.dirname(file_path) os.makedirs(staging_directory, exist_ok=True) staged_file_path = os.path.join( staging_directory, f".comfy-delete-{uuid.uuid4().hex}.tmp" @@ -281,23 +336,23 @@ def delete_asset_reference_with_file( folder_paths.is_within_directory(directory, file_path) for directory in allowed_directories ): - raise PermissionError( + raise AssetFileDeleteForbiddenError( "The asset source file moved outside the directories allowed for deletion." ) try: os.replace(file_path, staged_file_path) - except OSError: - staged_file_path = os.path.join( - source_directory, f".comfy-delete-{uuid.uuid4().hex}.tmp" - ) + except OSError as staging_error: + if staging_error.errno != errno.EXDEV: + raise if not any( folder_paths.is_within_directory(directory, file_path) for directory in allowed_directories ): - raise PermissionError( + raise AssetFileDeleteForbiddenError( "The asset source file moved outside the directories allowed for deletion." ) - os.replace(file_path, staged_file_path) + _stage_file_across_devices(file_path, staged_file_path) + cross_device_staging = True try: session.flush() @@ -315,7 +370,10 @@ def delete_asset_reference_with_file( and not os.path.exists(file_path) ): try: - os.replace(staged_file_path, file_path) + if cross_device_staging: + _restore_file_across_devices(staged_file_path, file_path) + else: + os.replace(staged_file_path, file_path) except OSError: logging.exception( "Failed to restore staged asset file after rollback: %s", @@ -326,24 +384,11 @@ def delete_asset_reference_with_file( if staged_file_path: try: os.remove(staged_file_path) - except OSError as cleanup_error: + except OSError: logging.exception( "Failed to remove staged asset file after commit: %s", staged_file_path, ) - if not folder_paths.is_within_directory( - staging_directory, staged_file_path - ): - retry_path = os.path.join( - staging_directory, f".comfy-delete-{uuid.uuid4().hex}.tmp" - ) - try: - os.replace(staged_file_path, retry_path) - except OSError: - os.replace(staged_file_path, file_path) - raise RuntimeError( - "Cleanup could not be queued; the source file was restored." - ) from cleanup_error return True diff --git a/app/assets/services/file_location.py b/app/assets/services/file_location.py index 3b17e4e72..6246cdbc5 100644 --- a/app/assets/services/file_location.py +++ b/app/assets/services/file_location.py @@ -2,6 +2,7 @@ import ipaddress import os import subprocess import sys +import threading def is_loopback_address(address: str | None) -> bool: @@ -38,9 +39,10 @@ def reveal_file_in_file_manager(file_path: str) -> None: if not os.path.isfile(normalized_path): raise FileNotFoundError(normalized_path) - subprocess.Popen( + process = subprocess.Popen( build_file_manager_command(normalized_path), close_fds=True, stdout=subprocess.DEVNULL, stderr=subprocess.DEVNULL, ) + threading.Thread(target=process.wait, daemon=True).start() diff --git a/tests-unit/assets_test/services/test_asset_management.py b/tests-unit/assets_test/services/test_asset_management.py index e2e64b3bf..10382ed0b 100644 --- a/tests-unit/assets_test/services/test_asset_management.py +++ b/tests-unit/assets_test/services/test_asset_management.py @@ -1,4 +1,7 @@ """Tests for asset_management services.""" +import errno +import os + import folder_paths import pytest from sqlalchemy.orm import Session @@ -7,12 +10,14 @@ from app.assets.database.models import Asset, AssetReference from app.assets.database.queries import ensure_tags_exist, add_tags_to_reference from app.assets.helpers import get_utc_now from app.assets.services import ( + AssetFileDeleteForbiddenError, delete_asset_reference_with_file, get_asset_detail, update_asset_metadata, delete_asset_reference, set_asset_preview, ) +from app.assets.services import asset_management from app.assets.services.asset_management import resolve_hash_to_path @@ -260,7 +265,7 @@ class TestDeleteAssetReferenceWithFile: selected_ref_id = selected_ref.id session.commit() - with pytest.raises(PermissionError, match="owning user"): + with pytest.raises(AssetFileDeleteForbiddenError, match="owning user"): delete_asset_reference_with_file( reference_id=selected_ref_id, owner_id="another-user", @@ -297,6 +302,38 @@ class TestDeleteAssetReferenceWithFile: assert not selected_file.exists() assert session.get(AssetReference, selected_ref_id) is None + def test_uses_normalized_owner_for_deletion( + self, mock_create_session, session: Session, temp_dir, monkeypatch + ): + selected_file = temp_dir / "selected.bin" + selected_file.write_bytes(b"content") + asset = _make_asset(session) + selected_ref = _make_reference(session, asset, owner_id="user1") + selected_ref.file_path = str(selected_file) + selected_ref_id = selected_ref.id + session.commit() + owner_ids = [] + real_delete = asset_management.delete_reference_by_id + + def capture_owner_id(session, reference_id, owner_id): + owner_ids.append(owner_id) + return real_delete(session, reference_id, owner_id) + + monkeypatch.setattr( + asset_management, "delete_reference_by_id", capture_owner_id + ) + + result = delete_asset_reference_with_file( + reference_id=selected_ref_id, + owner_id=" user1 ", + staging_directory=str(temp_dir / "staging"), + expected_file_path=str(selected_file), + allowed_directories=[str(temp_dir)], + ) + + assert result is True + assert owner_ids == ["user1"] + def test_preserves_soft_deleted_shared_reference( self, mock_create_session, session: Session, temp_dir ): @@ -359,7 +396,7 @@ class TestDeleteAssetReferenceWithFile: assert selected_file.read_bytes() == b"content" assert session.get(AssetReference, selected_ref_id) is not None - def test_queues_final_cleanup_failure_in_managed_temp( + def test_retains_final_cleanup_failure_in_managed_temp( self, mock_create_session, session: Session, temp_dir, monkeypatch ): selected_file = temp_dir / "managed" / "selected.bin" @@ -392,6 +429,289 @@ class TestDeleteAssetReferenceWithFile: assert queued_files[0].read_bytes() == b"content" assert session.get(AssetReference, selected_ref_id) is None + def test_stages_cross_device_file_in_managed_temp( + self, mock_create_session, session: Session, temp_dir, monkeypatch + ): + selected_file = temp_dir / "managed" / "selected.bin" + selected_file.parent.mkdir() + selected_file.write_bytes(b"content") + staging_directory = temp_dir / "staging" + asset = _make_asset(session) + selected_ref = _make_reference(session, asset, name=selected_file.name) + selected_ref.file_path = str(selected_file) + selected_ref_id = selected_ref.id + session.commit() + copy_calls = [] + real_copy = asset_management._copy_file_with_fsync + + def fail_cross_device_replace(_source, _destination): + raise OSError(errno.EXDEV, "Cross-device link") + + def capture_copy(source, destination): + copy_calls.append((os.fspath(source), os.fspath(destination))) + return real_copy(source, destination) + + monkeypatch.setattr( + asset_management.os, "replace", fail_cross_device_replace + ) + monkeypatch.setattr( + asset_management, "_copy_file_with_fsync", capture_copy + ) + + result = delete_asset_reference_with_file( + reference_id=selected_ref_id, + owner_id="", + staging_directory=str(staging_directory), + expected_file_path=str(selected_file), + allowed_directories=[str(selected_file.parent)], + ) + + session.expire_all() + assert result is True + assert len(copy_calls) == 1 + assert copy_calls[0][0] == str(selected_file) + assert os.path.dirname(copy_calls[0][1]) == str(staging_directory) + assert not selected_file.exists() + assert list(staging_directory.glob(".comfy-delete-*.tmp")) == [] + assert session.get(AssetReference, selected_ref_id) is None + + def test_retains_cross_device_cleanup_failure_in_managed_temp( + self, mock_create_session, session: Session, temp_dir, monkeypatch + ): + selected_file = temp_dir / "managed" / "selected.bin" + selected_file.parent.mkdir() + selected_file.write_bytes(b"content") + staging_directory = temp_dir / "staging" + asset = _make_asset(session) + selected_ref = _make_reference(session, asset, name=selected_file.name) + selected_ref.file_path = str(selected_file) + selected_ref_id = selected_ref.id + session.commit() + real_copy = asset_management._copy_file_with_fsync + real_remove = os.remove + copy_calls = [] + + def fail_cross_device_replace(_source, _destination): + raise OSError(errno.EXDEV, "Cross-device link") + + def capture_copy(source, destination): + copy_calls.append((os.fspath(source), os.fspath(destination))) + return real_copy(source, destination) + + def fail_final_cleanup(path): + if os.fspath(path) == os.fspath(selected_file): + return real_remove(path) + raise PermissionError("file is busy") + + monkeypatch.setattr( + asset_management.os, "replace", fail_cross_device_replace + ) + monkeypatch.setattr( + asset_management, "_copy_file_with_fsync", capture_copy + ) + monkeypatch.setattr(asset_management.os, "remove", fail_final_cleanup) + + result = delete_asset_reference_with_file( + reference_id=selected_ref_id, + owner_id="", + staging_directory=str(staging_directory), + expected_file_path=str(selected_file), + allowed_directories=[str(selected_file.parent)], + ) + + session.expire_all() + queued_files = list(staging_directory.glob(".comfy-delete-*.tmp")) + assert result is True + assert len(copy_calls) == 1 + assert not selected_file.exists() + assert len(queued_files) == 1 + assert queued_files[0].read_bytes() == b"content" + assert session.get(AssetReference, selected_ref_id) is None + + def test_cleans_cross_device_copy_when_source_unlink_fails( + self, mock_create_session, session: Session, temp_dir, monkeypatch + ): + selected_file = temp_dir / "managed" / "selected.bin" + selected_file.parent.mkdir() + selected_file.write_bytes(b"content") + staging_directory = temp_dir / "staging" + asset = _make_asset(session) + selected_ref = _make_reference(session, asset, name=selected_file.name) + selected_ref.file_path = str(selected_file) + selected_ref_id = selected_ref.id + session.commit() + real_remove = os.remove + + def fail_cross_device_replace(_source, _destination): + raise OSError(errno.EXDEV, "Cross-device link") + + def fail_source_unlink(path): + if os.fspath(path) == os.fspath(selected_file): + raise PermissionError(errno.EACCES, "Permission denied") + return real_remove(path) + + monkeypatch.setattr( + asset_management.os, "replace", fail_cross_device_replace + ) + monkeypatch.setattr(asset_management.os, "remove", fail_source_unlink) + + with pytest.raises(PermissionError, match="Permission denied"): + delete_asset_reference_with_file( + reference_id=selected_ref_id, + owner_id="", + staging_directory=str(staging_directory), + expected_file_path=str(selected_file), + allowed_directories=[str(selected_file.parent)], + ) + + session.expire_all() + assert selected_file.read_bytes() == b"content" + assert list(staging_directory.glob(".comfy-delete-*.tmp")) == [] + assert session.get(AssetReference, selected_ref_id) is not None + + def test_restores_cross_device_stage_when_commit_fails( + self, mock_create_session, session: Session, temp_dir, monkeypatch + ): + selected_file = temp_dir / "managed" / "selected.bin" + selected_file.parent.mkdir() + selected_file.write_bytes(b"content") + staging_directory = temp_dir / "staging" + asset = _make_asset(session) + selected_ref = _make_reference(session, asset, name=selected_file.name) + selected_ref.file_path = str(selected_file) + selected_ref_id = selected_ref.id + session.commit() + real_replace = os.replace + real_copy = asset_management._copy_file_with_fsync + replace_calls = 0 + copy_calls = [] + + def fail_first_replace(source, destination): + nonlocal replace_calls + replace_calls += 1 + if replace_calls == 1: + raise OSError(errno.EXDEV, "Cross-device link") + return real_replace(source, destination) + + def capture_copy(source, destination): + copy_calls.append((os.fspath(source), os.fspath(destination))) + return real_copy(source, destination) + + def fail_commit(_session): + raise RuntimeError("commit failed") + + monkeypatch.setattr(asset_management.os, "replace", fail_first_replace) + monkeypatch.setattr( + asset_management, "_copy_file_with_fsync", capture_copy + ) + monkeypatch.setattr(Session, "commit", fail_commit) + + with pytest.raises(RuntimeError, match="commit failed"): + delete_asset_reference_with_file( + reference_id=selected_ref_id, + owner_id="", + staging_directory=str(staging_directory), + expected_file_path=str(selected_file), + allowed_directories=[str(selected_file.parent)], + ) + + session.expire_all() + assert len(copy_calls) == 2 + assert copy_calls[0][0] == str(selected_file) + assert copy_calls[1][1].startswith( + str(selected_file.parent / ".comfy-restore-") + ) + assert selected_file.read_bytes() == b"content" + assert list(staging_directory.glob(".comfy-delete-*.tmp")) == [] + assert session.get(AssetReference, selected_ref_id) is not None + + def test_does_not_fallback_for_non_cross_device_staging_error( + self, mock_create_session, session: Session, temp_dir, monkeypatch + ): + selected_file = temp_dir / "managed" / "selected.bin" + selected_file.parent.mkdir() + selected_file.write_bytes(b"content") + asset = _make_asset(session) + selected_ref = _make_reference(session, asset, name=selected_file.name) + selected_ref.file_path = str(selected_file) + selected_ref_id = selected_ref.id + session.commit() + copy_calls = [] + + def fail_replace(_source, _destination): + raise PermissionError(errno.EACCES, "Permission denied") + + monkeypatch.setattr(asset_management.os, "replace", fail_replace) + monkeypatch.setattr( + asset_management, + "_copy_file_with_fsync", + lambda *args: copy_calls.append(args), + ) + + with pytest.raises(PermissionError, match="Permission denied"): + delete_asset_reference_with_file( + reference_id=selected_ref_id, + owner_id="", + staging_directory=str(temp_dir / "staging"), + expected_file_path=str(selected_file), + allowed_directories=[str(selected_file.parent)], + ) + + session.expire_all() + assert copy_calls == [] + assert selected_file.read_bytes() == b"content" + assert session.get(AssetReference, selected_ref_id) is not None + + def test_rechecks_containment_before_cross_device_fallback( + self, mock_create_session, session: Session, temp_dir, monkeypatch + ): + selected_file = temp_dir / "managed" / "selected.bin" + selected_file.parent.mkdir() + selected_file.write_bytes(b"content") + asset = _make_asset(session) + selected_ref = _make_reference(session, asset, name=selected_file.name) + selected_ref.file_path = str(selected_file) + selected_ref_id = selected_ref.id + session.commit() + containment_results = iter([True, True, False]) + copy_calls = [] + + def fail_cross_device_replace(_source, _destination): + raise OSError(errno.EXDEV, "Cross-device link") + + def capture_copy(*args): + copy_calls.append(args) + + monkeypatch.setattr( + folder_paths, + "is_within_directory", + lambda *_args: next(containment_results), + ) + monkeypatch.setattr( + asset_management.os, + "replace", + fail_cross_device_replace, + ) + monkeypatch.setattr( + asset_management, + "_copy_file_with_fsync", + capture_copy, + ) + + with pytest.raises(AssetFileDeleteForbiddenError, match="moved outside"): + delete_asset_reference_with_file( + reference_id=selected_ref_id, + owner_id="", + staging_directory=str(temp_dir / "staging"), + expected_file_path=str(selected_file), + allowed_directories=[str(selected_file.parent)], + ) + + session.expire_all() + assert copy_calls == [] + assert selected_file.read_bytes() == b"content" + assert session.get(AssetReference, selected_ref_id) is not None + def test_rejects_a_source_path_changed_after_authorization( self, mock_create_session, session: Session, temp_dir ): diff --git a/tests-unit/assets_test/services/test_file_location.py b/tests-unit/assets_test/services/test_file_location.py index 5380df36e..afa5e08e1 100644 --- a/tests-unit/assets_test/services/test_file_location.py +++ b/tests-unit/assets_test/services/test_file_location.py @@ -1,4 +1,6 @@ import subprocess +import threading +from unittest.mock import Mock import pytest @@ -19,7 +21,17 @@ def test_accepts_loopback_addresses(address): @pytest.mark.parametrize( "address", - [None, "", "localhost", "0.0.0.0", "192.168.1.20", "::", "fe80::1"], + [ + None, + "", + "localhost", + "0.0.0.0", + "192.168.1.20", + "::", + "fe80::1", + "::ffff:192.168.1.20", + "fe80::1%eth0", + ], ) def test_rejects_non_loopback_addresses(address): assert not is_loopback_address(address) @@ -48,20 +60,22 @@ def test_builds_platform_commands(tmp_path): def test_reveal_launches_without_a_shell(tmp_path, monkeypatch): source = tmp_path / "render.png" source.write_bytes(b"image") - calls = [] - - def capture_popen(*args, **kwargs): - calls.append((args, kwargs)) - - monkeypatch.setattr(subprocess, "Popen", capture_popen) + process = Mock() + popen = Mock(return_value=process) + reaper = Mock() + thread = Mock(return_value=reaper) + monkeypatch.setattr(subprocess, "Popen", popen) + monkeypatch.setattr(threading, "Thread", thread) reveal_file_in_file_manager(str(source)) - assert len(calls) == 1 - args, kwargs = calls[0] + popen.assert_called_once() + args, kwargs = popen.call_args assert args[0] assert kwargs["close_fds"] is True assert "shell" not in kwargs + thread.assert_called_once_with(target=process.wait, daemon=True) + reaper.start.assert_called_once_with() def test_reveal_rejects_missing_files(tmp_path): diff --git a/tests-unit/assets_test/test_asset_location_route.py b/tests-unit/assets_test/test_asset_location_route.py index 9ea715593..3a81b8fc5 100644 --- a/tests-unit/assets_test/test_asset_location_route.py +++ b/tests-unit/assets_test/test_asset_location_route.py @@ -77,8 +77,21 @@ def test_open_location_reveals_managed_generated_file(tmp_path, monkeypatch): reveal.assert_called_once_with(str(source)) +def test_open_location_returns_structured_error_for_unknown_user(monkeypatch): + monkeypatch.setattr( + routes, + "USER_MANAGER", + SimpleNamespace(get_request_user_id=Mock(side_effect=KeyError("unknown"))), + ) + + response = run_route(make_request()) + + assert response.status == 500 + assert json.loads(response.text)["error"]["code"] == "INTERNAL" + + def test_open_location_rejects_non_output_assets(tmp_path, monkeypatch): - source = tmp_path / "input" / "image.png" + source = tmp_path / "output" / "image.png" source.parent.mkdir() source.write_bytes(b"image") @@ -99,11 +112,48 @@ def test_open_location_rejects_non_output_assets(tmp_path, monkeypatch): "get_output_directory", lambda: str(tmp_path / "output"), ) + reveal = Mock() + monkeypatch.setattr(routes, "reveal_file_in_file_manager", reveal) response = run_route(make_request()) assert response.status == 403 assert json.loads(response.text)["error"]["code"] == "ASSET_LOCATION_FORBIDDEN" + reveal.assert_not_called() + + +def test_open_location_rejects_output_asset_outside_output_root( + tmp_path, monkeypatch +): + source = tmp_path / "outside" / "image.png" + source.parent.mkdir() + source.write_bytes(b"image") + + monkeypatch.setattr( + routes, + "USER_MANAGER", + SimpleNamespace(get_request_user_id=lambda _request: "default"), + ) + monkeypatch.setattr( + routes, + "get_asset_detail", + lambda **_kwargs: SimpleNamespace( + tags=["output"], ref=SimpleNamespace(file_path=str(source)) + ), + ) + monkeypatch.setattr( + routes.folder_paths, + "get_output_directory", + lambda: str(tmp_path / "output"), + ) + reveal = Mock() + monkeypatch.setattr(routes, "reveal_file_in_file_manager", reveal) + + response = run_route(make_request()) + + assert response.status == 403 + assert json.loads(response.text)["error"]["code"] == "ASSET_LOCATION_FORBIDDEN" + reveal.assert_not_called() def test_delete_content_rejects_paths_outside_managed_root(tmp_path, monkeypatch): @@ -181,3 +231,58 @@ def test_delete_content_uses_guarded_file_service(tmp_path, monkeypatch): allowed_directories=[str(tmp_path / "output")], allow_ownerless=True, ) + + +def test_delete_returns_structured_error_for_unknown_user(monkeypatch): + delete_reference = Mock() + monkeypatch.setattr( + routes, + "USER_MANAGER", + SimpleNamespace(get_request_user_id=Mock(side_effect=KeyError("unknown"))), + ) + monkeypatch.setattr(routes, "delete_asset_reference", delete_reference) + + response = run_delete_route(make_request()) + + assert response.status == 500 + assert json.loads(response.text)["error"]["code"] == "INTERNAL" + delete_reference.assert_not_called() + + +def test_delete_does_not_expose_os_permission_error_path(tmp_path, monkeypatch): + source = tmp_path / "output" / "private" / "render.png" + source.parent.mkdir(parents=True) + source.write_bytes(b"image") + delete_with_file = Mock( + side_effect=PermissionError(13, "Permission denied", str(source)) + ) + monkeypatch.setattr( + routes, + "USER_MANAGER", + SimpleNamespace(get_request_user_id=lambda _request: "default"), + ) + monkeypatch.setattr( + routes, + "get_asset_detail", + lambda **_kwargs: SimpleNamespace( + tags=["output"], ref=SimpleNamespace(file_path=str(source)) + ), + ) + monkeypatch.setattr( + routes.folder_paths, + "get_output_directory", + lambda: str(tmp_path / "output"), + ) + monkeypatch.setattr( + routes.folder_paths, + "get_temp_directory", + lambda: str(tmp_path / "temp"), + ) + monkeypatch.setattr(routes.user_manager.args, "multi_user", False) + monkeypatch.setattr(routes, "delete_asset_reference_with_file", delete_with_file) + + response = run_delete_route(make_request(query={"delete_content": "true"})) + + assert response.status == 500 + assert json.loads(response.text)["error"]["code"] == "INTERNAL" + assert str(source) not in response.text