fix(security): use abspath instead of realpath in containment checks to support symlinks (#1028)

AGENTS.md

@@ -137,7 +137,13 @@ npm run test:coverage       # Generate coverage report
 - Dual mode: ComfyUI plugin (folder_paths) vs standalone (settings.json)
 - Detection: `os.environ.get("LORA_MANAGER_STANDALONE", "0") == "1"`
 - Run `python scripts/sync_translation_keys.py` after adding UI strings to `locales/en.json`
-- Symlinks require normalized paths
+- Symlinks require normalized paths.
+  **Business paths vs real paths**: All stored paths and operation routing use the
+  original paths as they appear under configured model roots — symlinks are NOT
+  resolved. `os.path.realpath` is only for scanner dedup and the symlink cache.
+  Any path passed to `os.remove`/`os.rename`/`shutil.move` or validated by a
+  containment check MUST use the business path (i.e. `os.path.abspath`, not
+  `realpath`).
 
 ## Git / Commit Messages
 

py/services/download_manager.py

@@ -1392,8 +1392,8 @@ class DownloadManager:
                 base_save_dir = save_dir
                 save_dir = os.path.join(save_dir, relative_path)
                 # Security: validate path containment after joining
-                resolved_dir = os.path.realpath(os.path.normpath(save_dir))
+                resolved_dir = os.path.abspath(os.path.normpath(save_dir))
-                base_dir = os.path.realpath(os.path.normpath(base_save_dir))
+                base_dir = os.path.abspath(os.path.normpath(base_save_dir))
                 if not resolved_dir.startswith(base_dir + os.sep) and resolved_dir != base_dir:
                     logger.warning(
                         "Path traversal detected: %s escapes %s",

py/services/model_lifecycle_service.py

@@ -51,9 +51,10 @@ async def delete_model_artifacts(
 def _require_path_in_library_roots(file_path: str, scanner, *, label: str = "path") -> None:
     """Raise ``ValueError`` if *file_path* is not inside a configured model root.
 
-    Uses ``os.path.realpath()`` to resolve symlinks before comparing,
+    Uses ``os.path.abspath()`` (NOT ``realpath``) to resolve ``..`` and ``.``
-    so symlink-based escapes are also caught.  Skips when the scanner
+    while preserving symlinks — this keeps the check in business-path space.
-    does not expose ``get_model_roots`` or the list is empty.
+    Skips when the scanner does not expose ``get_model_roots`` or the list
+    is empty.
     """
 
     roots = None
@@ -65,10 +66,10 @@ def _require_path_in_library_roots(file_path: str, scanner, *, label: str = "pat
     if not roots:
         return
 
-    resolved = os.path.realpath(os.path.normpath(file_path))
+    resolved = os.path.abspath(os.path.normpath(file_path))
 
     for root in roots:
-        root_resolved = os.path.realpath(os.path.normpath(root))
+        root_resolved = os.path.abspath(os.path.normpath(root))
         if resolved == root_resolved or resolved.startswith(root_resolved + os.sep):
             return
 

tests/services/test_download_manager_basic.py

@@ -1248,6 +1248,50 @@ def test_relative_path_sanitizes_double_slashes():
     assert relative_path == "SDXL/no tags/Author"
 
 
+def test_download_containment_accepts_symlink_save_dir(tmp_path):
+    """Verify the download path containment check (download_manager.py:1395-1397)
+    accepts save directories reached through user-created symlinks inside the
+    library root — reproducing the symlink scenario from issue #1028."""
+    # Library root with a symlink subdirectory pointing to an external drive
+    lora_root = tmp_path / "loras"
+    lora_root.mkdir()
+
+    external_drive = tmp_path / "external" / "models"
+    external_drive.mkdir(parents=True)
+
+    symlink = lora_root / "Krea 2"
+    symlink.symlink_to(str(external_drive))
+
+    # Simulate a download: base_save_dir = library root,
+    # relative_path = "Krea 2/concept/NewModel"
+    base_save_dir = str(lora_root)
+    save_dir = os.path.join(base_save_dir, "Krea 2", "concept", "NewModel")
+
+    # Replicate the exact containment check from download_manager.py
+    resolved_dir = os.path.abspath(os.path.normpath(save_dir))
+    base_dir = os.path.abspath(os.path.normpath(base_save_dir))
+
+    # Must NOT be rejected — symlinks are legitimate business paths
+    assert resolved_dir.startswith(base_dir + os.sep)
+
+
+def test_download_containment_rejects_dot_dot_traversal(tmp_path):
+    """Verify the download path containment check still blocks ``..`` traversal
+    after the realpath → abspath change."""
+    lora_root = tmp_path / "loras"
+    lora_root.mkdir()
+
+    base_save_dir = str(lora_root)
+    save_dir = os.path.join(base_save_dir, "..", "..", "etc", "passwd")
+
+    resolved_dir = os.path.abspath(os.path.normpath(save_dir))
+    base_dir = os.path.abspath(os.path.normpath(base_save_dir))
+
+    # Must be rejected — dot-dot escapes the library root
+    assert not resolved_dir.startswith(base_dir + os.sep)
+    assert resolved_dir != base_dir
+
+
 def test_distribute_preview_to_entries_moves_and_copies(tmp_path):
     """Test that preview distribution moves file to first entry and copies to others."""
     manager = DownloadManager()

tests/services/test_model_lifecycle_service.py

@@ -1,4 +1,5 @@
 import json
+import os
 from pathlib import Path
 
 import pytest
@@ -51,11 +52,12 @@ class TestRequirePathInLibraryRoots:
         scanner = ScannerWithRoots([str(root)])
         _require_path_in_library_roots(str(root), scanner)
 
-    def test_rejects_symlink_escape(self, tmp_path):
+    def test_accepts_symlink_within_root(self, tmp_path):
+        """Symlinks under a configured root are legitimate business paths
+        and should be accepted — containment works on business-path space,
+        not resolved physical paths."""
         root = tmp_path / "loras"
         root.mkdir()
-        model = root / "model.safetensors"
-        model.write_text("")
 
         outside_dir = tmp_path / "outside"
         outside_dir.mkdir()
@@ -65,9 +67,22 @@ class TestRequirePathInLibraryRoots:
         symlink = root / "link.safetensors"
         symlink.symlink_to(outside_file)
 
+        scanner = ScannerWithRoots([str(root)])
+        # Symlink path is under root in business-path space → accepted
+        _require_path_in_library_roots(str(symlink), scanner)
+
+    def test_rejects_dot_dot_traversal(self, tmp_path):
+        """Verify that ``..`` components are still resolved and blocked —
+        ``abspath`` normalises dot-dot but does not resolve symlinks."""
+        root = tmp_path / "loras"
+        root.mkdir()
+
+        # A path that traverses up out of the root via ..
+        escaped = os.path.join(str(root), "..", "..", "etc", "passwd")
+
         scanner = ScannerWithRoots([str(root)])
         with pytest.raises(ValueError, match="outside configured library"):
-            _require_path_in_library_roots(str(symlink), scanner)
+            _require_path_in_library_roots(escaped, scanner)
 
 
 class ScannerForDelete: