fix(extensions): validate ignore before --force removal; reject symlinked ancestors in config restore

Three review-driven hardening fixes to the keep-config reinstall, adopting the
repo's own safe-write contract (shared_infra) instead of leaf-only guards:

1. Validate the source ignore file BEFORE the --force self.remove(). It was
   loaded only after removal, so a malformed/invalid-UTF-8 ignore file uninstalled
   a working extension (files + registry entry) before validation raised. Moved
   the single _load_extensionignore call above the force-removal block.

2. Reject a symlinked ANCESTOR of the config destination. _atomic_restore_config's
   mkstemp(dir=dest.parent)+os.replace operated inside dest.parent, so if dest_dir
   was swapped to a symlink after copytree, config bytes (secrets) were created in
   and replaced into the external target. It now calls the repo's
   shared_infra._ensure_safe_shared_directory to refuse any symlinked ancestor
   under the project root before writing.

3. Rollback no longer merges through a symlinked/partial dir. It reset dest_dir
   with rmtree(ignore_errors=True) then copytree(dirs_exist_ok=True), which could
   follow a symlink. It now resets via a new _reset_dir (unlink a symlink / rmtree
   a real dir, surfacing failures) and recreates dest_dir under a verified
   non-symlink ancestor chain before copying; a rejected ancestor leaves the
   durable backup on disk.

Tests: --force reinstall with a malformed ignore leaves the extension installed
(fails before: it was removed); a symlinked dest_dir is refused and never
receives config bytes, rollback recreating a real dir (skipped without symlink
privilege, runs on CI). Existing reinstall/keep-config tests unchanged (341 pass).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: jawwad-ali

src/specify_cli/extensions/__init__.py

@@ -1344,23 +1344,56 @@ def check_compatibility(
         return True
 
     @staticmethod
+    def _reset_dir(path: Path) -> None:
+        """Remove whatever occupies *path* so a fresh directory can take its place.
+
+        A symlink is unlinked (never followed); a real directory is rmtree'd; any
+        other entry is unlinked. Unlike ``rmtree(..., ignore_errors=True)`` this
+        surfaces failures so a caller never merges into a partially-removed or
+        still-symlinked path.
+        """
+        if path.is_symlink():
+            path.unlink()
+        elif path.is_dir():
+            shutil.rmtree(path)
+        elif path.exists():
+            path.unlink()
+
     def _atomic_restore_config(
-        dest: Path, data: bytes, mode: int, atime: float, mtime: float
+        self, dest: Path, data: bytes, mode: int, atime: float, mtime: float
     ) -> None:
-        """Restore a preserved config to *dest* atomically (temp file + os.replace).
+        """Restore a preserved config to *dest* atomically and symlink-safely.
+
+        Two guards, both required because *dest*'s parent is a fresh copytree
+        output that a racing process could swap for a symlink:
+
+        1. Reject a symlinked ancestor of *dest* (``dest.parent`` and up, under
+           the project root) via the repo's shared safe-write guard
+           (``shared_infra._ensure_safe_shared_directory``). Without this,
+           ``mkstemp(dir=dest.parent)`` + ``os.replace`` would create and install
+           the config (possibly secrets) *inside a symlinked external target*.
+        2. Write to a sibling temp file, then ``os.replace`` swaps it into the
+           directory entry — never following a symlink that occupies the leaf
+           *dest*, and never leaving a partial write.
+
+        Mode/timestamps are set on the temp file (independently — either can
+        succeed alone) so the installed file carries them from the first moment.
+        """
+        from ..shared_infra import _ensure_safe_shared_directory
+
+        # (1) Refuse a symlinked ancestor of the destination. dest.parent must
+        # already exist (a fresh copytree dir, the backup dir, or a rollback dir
+        # recreated safely just above the call).
+        _ensure_safe_shared_directory(
+            self.project_root,
+            dest.parent,
+            create=False,
+            context="extension config directory",
+        )
 
-        Writes to a sibling temp file, then ``os.replace()`` swaps it into place.
-        ``os.replace`` operates on the directory entry, so it (a) never follows a
-        symlink that may occupy *dest* — closing the check-then-use race where a
-        racing process swaps in a symlink to overwrite an external target — and
-        (b) leaves either the old file or the fully-written new one, never a
-        partial write. Mirrors ``shared_infra._write_shared_bytes``. Mode and
-        timestamps are set on the temp file (independently — either can succeed
-        on its own) so the installed file carries them from the first moment.
-        """
         # os.replace cannot replace a directory with a file; clear a stray
-        # (non-symlink) directory at the path first. A symlink is handled by
-        # os.replace itself (it swaps the link entry without following it).
+        # (non-symlink) directory at the leaf first. A symlink at the leaf is
+        # handled by os.replace itself (it swaps the link entry, never follows).
         if dest.is_dir() and not dest.is_symlink():
             shutil.rmtree(dest, ignore_errors=True)
         fd, temp_name = tempfile.mkstemp(prefix=f".{dest.name}.", dir=dest.parent)
@@ -1448,6 +1481,13 @@ def install_from_directory(
                 f"extension. Install from a copy in a different location instead."
             )
 
+        # Validate the source ignore file BEFORE any destructive step. It can
+        # raise on a malformed/invalid-UTF-8 file, and neither the --force
+        # self.remove() below nor the later rmtree must run if it does — otherwise
+        # a bad ignore file would uninstall a working extension (and drop its
+        # registry entry) before validation fails. Reused as-is at the copytree.
+        ignore_fn = self._load_extensionignore(source_dir)
+
         # Remove existing installation AFTER all validations pass so that a
         # validation failure doesn't leave the user with a half-uninstalled
         # extension (configs stranded in .backup/).
@@ -1499,21 +1539,18 @@ def install_from_directory(
 
         # Install extension (dest_dir computed above during self-install guard).
         # Preserved configs are the ONLY surviving copy once the old extension dir
-        # is removed, so guard the whole destructive reinstall against loss at any
-        # step (a malformed source ignore file, the rmtree, copytree, or a
-        # restore):
-        #   1. Validate the source ignore file BEFORE deleting anything — it can
-        #      raise on a malformed file, and nothing is destroyed yet if it does.
+        # is removed, so guard the whole destructive reinstall against loss:
+        #   1. The source ignore file is already validated above (before any
+        #      destructive step), so a malformed one never reaches here.
         #   2. Stage a durable on-disk backup of the configs.
         #   3. Run the destructive rmtree + copytree + restore inside one block;
-        #      on ANY failure, roll the configs back into a clean dest_dir and
-        #      re-raise. The backup is removed only once the configs are provably
-        #      in place (normal success OR a completed rollback) — otherwise it is
-        #      left on disk so nothing is ever lost, even on a rollback failure.
-        # Each config write is atomic (temp file + os.replace) so it never follows
-        # a symlink that may occupy the path (see _atomic_restore_config).
-        ignore_fn = self._load_extensionignore(source_dir)
-
+        #      on ANY failure, reset dest_dir to a clean, symlink-safe directory
+        #      and roll the configs back, then re-raise. The backup is removed
+        #      only once the configs are provably in place (normal success OR a
+        #      completed rollback) — otherwise it is left on disk so nothing is
+        #      ever lost, even on a rollback failure.
+        # Each config write is atomic (temp file + os.replace) AND refuses a
+        # symlinked ancestor of the destination (see _atomic_restore_config).
         backup_dir: Path | None = None
         if preserved_configs:
             backup_dir = Path(
@@ -1534,16 +1571,29 @@ def install_from_directory(
                 self._atomic_restore_config(dest_dir / name, data, mode, atime, mtime)
             restored_ok = True
         except Exception:
-            # Roll the configs back into a clean dest_dir from the durable backup
-            # so a failed reinstall leaves them intact, then re-raise the original.
+            # Roll the configs back into a clean, symlink-safe dest_dir from the
+            # durable backup so a failed reinstall leaves them intact, then
+            # re-raise the original error. Reset dest_dir (unlink a symlink /
+            # rmtree a real dir — surfacing failures, never ignore_errors) and
+            # recreate it under a verified non-symlink ancestor chain BEFORE
+            # copying, so recovery can never merge through a symlinked or
+            # partially-removed directory into an external target.
             if backup_dir is not None:
                 try:
-                    if dest_dir.exists():
-                        shutil.rmtree(dest_dir, ignore_errors=True)
+                    from ..shared_infra import _ensure_safe_shared_directory
+                    self._reset_dir(dest_dir)
+                    _ensure_safe_shared_directory(
+                        self.project_root,
+                        dest_dir,
+                        create=True,
+                        context="extension directory",
+                    )
                     shutil.copytree(backup_dir, dest_dir, dirs_exist_ok=True)
                     restored_ok = True
-                except OSError:
-                    restored_ok = False  # keep the backup below for recovery
+                except (OSError, ValueError):
+                    # ValueError covers SymlinkedSharedPathError (a symlinked
+                    # ancestor): leave the durable backup on disk for recovery.
+                    restored_ok = False
             raise
         finally:
             # Drop the backup only when the configs are provably in dest_dir.

tests/test_extensions.py

@@ -1768,13 +1768,13 @@ def test_reinstall_failed_config_restore_is_not_silent(
         # staging (a ".cfgbak-" temp dir) succeed so rollback can recover.
         real_restore = ExtensionManager._atomic_restore_config
 
-        def flaky_restore(dest, data, mode, atime, mtime):
+        def flaky_restore(self, dest, data, mode, atime, mtime):
             if "cfgbak" not in dest.parent.name:
                 raise OSError("simulated restore failure")
-            return real_restore(dest, data, mode, atime, mtime)
+            return real_restore(self, dest, data, mode, atime, mtime)
 
         monkeypatch.setattr(
-            ExtensionManager, "_atomic_restore_config", staticmethod(flaky_restore)
+            ExtensionManager, "_atomic_restore_config", flaky_restore
         )
 
         with pytest.raises(OSError):
@@ -1786,6 +1786,90 @@ def flaky_restore(dest, data, mode, atime, mtime):
         assert config_file.is_file()
         assert "MY-CUSTOMIZED-VALUE" in config_file.read_text()
 
+    def test_reinstall_force_validates_ignore_before_removal(
+        self, extension_dir, project_dir, monkeypatch
+    ):
+        """A --force reinstall must validate the source ignore file BEFORE
+        self.remove() runs. Otherwise a malformed ignore file uninstalls a
+        working extension (dropping its files and registry entry) before the
+        validation raises. (The leftover-path test covers the non-force case.)"""
+        manager = ExtensionManager(project_dir)
+        manager.install_from_directory(extension_dir, "0.1.0", register_commands=False)
+        assert manager.registry.is_installed("test-ext")
+        ext_dir = project_dir / ".specify" / "extensions" / "test-ext"
+        manifest_file = ext_dir / "extension.yml"
+        assert manifest_file.is_file()
+
+        def bad_ignore(src):
+            raise ValueError("malformed .extensionignore")
+
+        monkeypatch.setattr(manager, "_load_extensionignore", bad_ignore)
+
+        with pytest.raises(ValueError):
+            manager.install_from_directory(
+                extension_dir, "0.1.0", register_commands=False, force=True
+            )
+
+        # The working extension was NOT removed by the failed --force reinstall.
+        assert manager.registry.is_installed("test-ext")
+        assert manifest_file.is_file()
+
+    def test_reinstall_refuses_symlinked_dest_dir(
+        self, extension_dir, project_dir, monkeypatch
+    ):
+        """If dest_dir is swapped to a symlink after copytree, the config restore
+        must NOT create bytes (potential secrets) inside the symlinked external
+        target — the repo's ancestor guard rejects it, and rollback recreates a
+        real directory. Guards the symlinked-parent gap (leaf-only guards miss
+        it). Skipped where symlinks need privilege; runs on Linux CI."""
+        probe_t = project_dir / "_pt"
+        probe_l = project_dir / "_pl"
+        try:
+            probe_t.mkdir()
+            probe_l.symlink_to(probe_t, target_is_directory=True)
+        except (OSError, NotImplementedError):
+            pytest.skip("symlinks not supported in this environment")
+        finally:
+            if probe_l.is_symlink():
+                probe_l.unlink()
+            if probe_t.exists():
+                shutil.rmtree(probe_t, ignore_errors=True)
+
+        manager = ExtensionManager(project_dir)
+        manager.install_from_directory(extension_dir, "0.1.0", register_commands=False)
+        ext_dir = project_dir / ".specify" / "extensions" / "test-ext"
+        config_file = ext_dir / "test-ext-config.yml"
+        config_file.write_text("api_key: SECRET-VALUE")
+        assert manager.remove("test-ext", keep_config=True) is True
+
+        victim = project_dir / "victim_target"
+        victim.mkdir()
+
+        real_copytree = shutil.copytree
+        calls = {"n": 0}
+
+        def copytree_symlinks_destdir_first(src, dst, *args, **kwargs):
+            calls["n"] += 1
+            if calls["n"] == 1:
+                # Simulate a racing swap: dest_dir is a symlink to an external dir.
+                Path(dst).symlink_to(victim, target_is_directory=True)
+                return dst
+            return real_copytree(src, dst, *args, **kwargs)
+
+        monkeypatch.setattr(shutil, "copytree", copytree_symlinks_destdir_first)
+
+        with pytest.raises(Exception):
+            manager.install_from_directory(
+                extension_dir, "0.1.0", register_commands=False
+            )
+
+        # Secrets never landed in the external target.
+        assert list(victim.iterdir()) == []
+        # dest_dir is a real directory again (rollback), with the config recovered.
+        assert not ext_dir.is_symlink()
+        assert config_file.is_file()
+        assert "SECRET-VALUE" in config_file.read_text()
+
 
 # ===== CommandRegistrar Tests =====