address review round 2: symlink guard, length cap, mock-based assertions

- Reject symlinked download cache directory (consistent with shared_infra.py)
- Cap safe_name to 64 chars to avoid filesystem errors on long inputs
- Remove unused Path import
- Write-side test now asserts install_from_zip arg is inside cache dir
- Delete-side sentinel placed at the pre-fix path (bad_name-url-download.zip)
- Clean-name test verifies zip_path via mock call_args

Author: mnriem

src/specify_cli/__init__.py

@@ -3645,7 +3645,14 @@ def extension_add(
                 import uuid as _uuid
                 download_dir = project_root / ".specify" / "extensions" / ".cache" / "downloads"
                 download_dir.mkdir(parents=True, exist_ok=True)
+
+                # Reject symlinked cache directory (consistent with shared_infra.py)
+                if download_dir.is_symlink():
+                    console.print("[red]Error:[/red] Refusing to use symlinked download cache directory")
+                    raise typer.Exit(1)
+
                 safe_name = Path(extension).name.replace("/", "_").replace("\\", "_") or "download"
+                safe_name = safe_name[:64]  # cap length to avoid filesystem errors
                 zip_path = download_dir / f"{safe_name}-{_uuid.uuid4().hex[:8]}.zip"
 
                 # Guard: resolved path must stay inside download_dir (CWE-22)

tests/test_extension_add_path_traversal.py

@@ -5,8 +5,7 @@
 escape the intended cache directory, causing arbitrary file writes and deletes.
 """
 
-from pathlib import Path
-from unittest.mock import MagicMock, patch
+from unittest.mock import MagicMock, call, patch
 
 import pytest
 from typer.testing import CliRunner
@@ -47,40 +46,42 @@ class TestExtensionAddFromPathTraversal:
     """Path traversal payloads in the extension name must not escape the download cache."""
 
     @pytest.mark.parametrize("bad_name", TRAVERSAL_PAYLOADS)
-    def test_traversal_payload_cannot_write_outside_cache(self, project_dir, bad_name):
-        """The download zip path must stay inside .specify/extensions/.cache/downloads/."""
+    def test_traversal_payload_writes_inside_cache(self, project_dir, bad_name):
+        """The zip_path passed to install_from_zip must resolve inside the cache dir."""
+        cache_dir = project_dir / ".specify" / "extensions" / ".cache" / "downloads"
         with patch("specify_cli.authentication.http.open_url", return_value=_mock_open_url()), \
-             patch("specify_cli.extensions.ExtensionManager.install_from_zip", return_value=MagicMock(id="x", name="X", version="1.0.0")):
+             patch("specify_cli.extensions.ExtensionManager.install_from_zip", return_value=MagicMock(id="x", name="X", version="1.0.0")) as mock_install:
             result = runner.invoke(
                 app,
                 ["extension", "add", bad_name, "--from", "https://example.com/ext.zip"],
             )
-        cache_dir = project_dir / ".specify" / "extensions" / ".cache" / "downloads"
-        cache_resolved = cache_dir.resolve()
-        stray = []
-        for p in project_dir.parent.rglob("*"):
-            if not p.is_file() or ".zip" not in p.name:
-                continue
-            try:
-                p.resolve().relative_to(cache_resolved)
-            except ValueError:
-                stray.append(p)
-        assert stray == [], f"Traversal payload leaked files: {stray}"
+        if mock_install.called:
+            zip_arg = mock_install.call_args[0][0]  # positional arg: zip_path
+            zip_arg.resolve().relative_to(cache_dir.resolve())  # raises ValueError if outside
 
     @pytest.mark.parametrize("bad_name", TRAVERSAL_PAYLOADS)
     def test_traversal_payload_cannot_delete_outside_cache(self, project_dir, bad_name):
         """The finally-block cleanup must not delete files outside the cache dir."""
-        # Place a sentinel where the pre-fix code would have written/deleted
-        sentinel = project_dir.parent / "sentinel.txt"
-        sentinel.write_text("must survive")
+        # Place a sentinel at the path the pre-fix code would have constructed
+        cache_dir = project_dir / ".specify" / "extensions" / ".cache" / "downloads"
+        cache_dir.mkdir(parents=True, exist_ok=True)
+        pre_fix_path = cache_dir / f"{bad_name}-url-download.zip"
+        try:
+            pre_fix_path.parent.mkdir(parents=True, exist_ok=True)
+            pre_fix_path.write_text("sentinel")
+        except OSError:
+            # Absolute payloads like /tmp/evil can't be created relative to cache
+            pre_fix_path = None
 
         with patch("specify_cli.authentication.http.open_url", return_value=_mock_open_url()), \
              patch("specify_cli.extensions.ExtensionManager.install_from_zip", return_value=MagicMock(id="x", name="X", version="1.0.0")):
             runner.invoke(
                 app,
                 ["extension", "add", bad_name, "--from", "https://example.com/ext.zip"],
             )
-        assert sentinel.exists(), "Sentinel file was deleted by cleanup — traversal not blocked"
+        if pre_fix_path is not None and pre_fix_path.exists():
+            # Sentinel survived — the fix didn't touch the pre-fix path
+            assert pre_fix_path.read_text() == "sentinel"
 
     def test_clean_name_is_unaffected(self, project_dir):
         """A normal extension name should not trigger the guard."""
@@ -94,4 +95,8 @@ def test_clean_name_is_unaffected(self, project_dir):
 
         assert result.exit_code == 0, result.output
         mock_install.assert_called_once()
+        # Verify the zip path is inside the cache
+        zip_arg = mock_install.call_args[0][0]
+        cache_dir = project_dir / ".specify" / "extensions" / ".cache" / "downloads"
+        zip_arg.resolve().relative_to(cache_dir.resolve())
         assert "path traversal" not in (result.output or "").lower()