diff --git a/AGENTS.md b/AGENTS.md index f7f45e29..8a1aee57 100644 --- a/AGENTS.md +++ b/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 diff --git a/py/services/download_manager.py b/py/services/download_manager.py index 85ddfdcb..fae40360 100644 --- a/py/services/download_manager.py +++ b/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)) - base_dir = os.path.realpath(os.path.normpath(base_save_dir)) + resolved_dir = os.path.abspath(os.path.normpath(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", diff --git a/py/services/model_lifecycle_service.py b/py/services/model_lifecycle_service.py index 02b0a4f7..2aba5e3a 100644 --- a/py/services/model_lifecycle_service.py +++ b/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, - so symlink-based escapes are also caught. Skips when the scanner - does not expose ``get_model_roots`` or the list is empty. + Uses ``os.path.abspath()`` (NOT ``realpath``) to resolve ``..`` and ``.`` + while preserving symlinks — this keeps the check in business-path space. + 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 diff --git a/tests/services/test_download_manager_basic.py b/tests/services/test_download_manager_basic.py index 4f717949..a0a6d861 100644 --- a/tests/services/test_download_manager_basic.py +++ b/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() diff --git a/tests/services/test_model_lifecycle_service.py b/tests/services/test_model_lifecycle_service.py index d55f8815..f421fee8 100644 --- a/tests/services/test_model_lifecycle_service.py +++ b/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: