Fix local asset file actions

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

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,

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",

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
 
 

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()

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
     ):

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):

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