fix: harden ExtensionManager.remove and PresetManager.remove against unsafe registry IDs

src/specify_cli/bundles/primitives.py

@@ -152,7 +152,7 @@ def __init__(self, project_root: Path, allow_network: bool) -> None:
 
     def is_installed(self, component: ComponentRef) -> bool:
         try:
-            return self._manager.get_pack(component.id) is not None
+            return self._manager.registry.is_installed(component.id)
         except Exception:  # noqa: BLE001
             return False
 
@@ -222,11 +222,16 @@ def _do_install(self, component: ComponentRef, *, force: bool) -> None:
 
     def remove(self, component: ComponentRef) -> None:
         try:
-            self._manager.remove(component.id)
+            removed = self._manager.remove(component.id)
         except Exception as exc:  # noqa: BLE001
             raise BundlerError(
                 f"Failed to remove preset '{component.id}': {exc}"
             ) from exc
+        if not removed:
+            raise BundlerError(
+                f"Failed to remove preset '{component.id}': removal was "
+                "refused (unsafe registry id or unexpected on-disk target)."
+            )
 
 
 class _ExtensionKindManager:
@@ -313,11 +318,16 @@ def _do_install(self, component: ComponentRef, *, force: bool) -> None:
 
     def remove(self, component: ComponentRef) -> None:
         try:
-            self._manager.remove(component.id)
+            removed = self._manager.remove(component.id)
         except Exception as exc:  # noqa: BLE001
             raise BundlerError(
                 f"Failed to remove extension '{component.id}': {exc}"
             ) from exc
+        if not removed:
+            raise BundlerError(
+                f"Failed to remove extension '{component.id}': removal was "
+                "refused (unsafe registry id or unexpected on-disk target)."
+            )
 
 
 class _WorkflowKindManager:

src/specify_cli/extensions/__init__.py

@@ -317,7 +317,7 @@ def _validate(self):
                 )
 
         # Validate extension ID format
-        if not re.match(r"^[a-z0-9-]+$", ext["id"]):
+        if not re.fullmatch(r"[a-z0-9-]+", ext["id"]):
             raise ValidationError(
                 f"Invalid extension ID '{ext['id']}': "
                 "must be lowercase alphanumeric with hyphens only"
@@ -2159,6 +2159,11 @@ def install_from_directory(
             # Clear any stale backup from a previous remove so that only the
             # backup produced by the current remove() call is restored later.
             backup_config_dir = self.extensions_dir / ".backup" / manifest.id
+            if backup_config_dir.parent.is_symlink():
+                raise ExtensionError(
+                    f"Refusing to reinstall '{manifest.id}': "
+                    f"'.backup' is a symlink"
+                )
             # Check is_symlink first: is_dir() follows symlinks so a
             # symlink-to-directory would pass, but rmtree() raises on them.
             if backup_config_dir.is_symlink():
@@ -2168,6 +2173,11 @@ def install_from_directory(
             elif backup_config_dir.exists():
                 backup_config_dir.unlink()
             did_remove = self.remove(manifest.id)
+            if not did_remove:
+                raise ExtensionError(
+                    f"Refusing to reinstall '{manifest.id}': existing "
+                    f"installation could not be safely removed"
+                )
 
         # Load and validate .extensionignore BEFORE reading/creating the rescue
         # staging directory (and thus before deleting dest_dir). The loader can
@@ -2960,6 +2970,35 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool:
         if not self.registry.is_installed(extension_id):
             return False
 
+        # A registered extension_id is trusted only as far as the registry
+        # file itself is trustworthy; validate it as a single, well-formed
+        # path component before it is used to construct removal/backup
+        # targets, and refuse a target that is not a real directory (e.g. a
+        # symlink planted to redirect the deletion elsewhere).
+        if not VALID_EXTENSION_ARTIFACT_NAME_PATTERN.fullmatch(extension_id):
+            return False
+        extension_dir = self.extensions_dir / extension_id
+        if extension_dir.is_symlink() or (
+            extension_dir.exists() and not extension_dir.is_dir()
+        ):
+            return False
+        backup_root = self.extensions_dir / ".backup"
+        backup_dir = backup_root / extension_id
+        if not keep_config and extension_dir.exists():
+            if (
+                backup_root.is_symlink()
+                or backup_dir.is_symlink()
+                or (backup_root.exists() and not backup_root.is_dir())
+                or (backup_dir.exists() and not backup_dir.is_dir())
+                or any(
+                    (backup_dir / f.name).is_symlink()
+                    or (backup_dir / f.name).is_dir()
+                    for f in list(extension_dir.glob("*-config.yml"))
+                    + list(extension_dir.glob("*-config.local.yml"))
+                )
+            ):
+                return False
+
         # Get registered commands and skills before removal
         metadata = self.registry.get(extension_id)
         registered_commands = (
@@ -2972,8 +3011,6 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool:
         else:
             registered_skills = []
 
-        extension_dir = self.extensions_dir / extension_id
-
         # Unregister commands from all AI agents
         if registered_commands:
             registrar = CommandRegistrar()
@@ -3007,7 +3044,6 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool:
             if extension_dir.exists():
                 # Use subdirectory per extension to avoid name accumulation
                 # (e.g., jira-jira-config.yml on repeated remove/install cycles)
-                backup_dir = self.extensions_dir / ".backup" / extension_id
                 backup_dir.mkdir(parents=True, exist_ok=True)
 
                 # Backup both primary and local override config files

src/specify_cli/extensions/_command_update_transaction.py

@@ -520,7 +520,11 @@ def backup_extension_skills(skill_names, *, skills_dir=None):
 
                     # 7. Remove old extension (handles command file cleanup and registry removal)
                     installation_modified = True
-                    manager.remove(extension_id, keep_config=True)
+                    if manager.remove(extension_id, keep_config=True) is False:
+                        installation_modified = False
+                        raise RuntimeError(
+                            f"Could not safely remove existing '{extension_id}'"
+                        )
 
                     # 8. Install new version
                     _ = manager.install_from_zip(

src/specify_cli/presets/_manager.py

@@ -395,7 +395,11 @@ def install_from_directory(
                     f"Preset '{manifest.id}' is already installed. "
                     f"Use 'specify preset remove {manifest.id}' first."
                 )
-            self.remove(manifest.id)
+            if not self.remove(manifest.id):
+                raise PresetError(
+                    f"Refusing to reinstall '{manifest.id}': existing "
+                    f"installation could not be safely removed"
+                )
 
         dest_dir = self.presets_dir / manifest.id
         if dest_dir.exists():
@@ -640,6 +644,17 @@ def remove(self, pack_id: str) -> bool:
         if not self.registry.is_installed(pack_id):
             return False
 
+        # A registered pack_id is trusted only as far as the registry file
+        # itself is trustworthy; validate it as a single, well-formed path
+        # component before it is used to construct the removal target, and
+        # refuse a target that is not a real directory (e.g. a symlink
+        # planted to redirect the deletion elsewhere).
+        if not PresetResolver._is_safe_registry_id(pack_id):
+            return False
+        pack_dir = self.presets_dir / pack_id
+        if pack_dir.is_symlink() or (pack_dir.exists() and not pack_dir.is_dir()):
+            return False
+
         metadata = self.registry.get(pack_id)
         # Restore original skills when preset is removed
         registered_skills = metadata.get("registered_skills", []) if metadata else []
@@ -668,7 +683,6 @@ def remove(self, pack_id: str) -> bool:
                 fallback_agent=fallback_agent,
             )
         registered_commands = metadata.get("registered_commands", {}) if metadata else {}
-        pack_dir = self.presets_dir / pack_id
 
         # Record which historical agents this preset's registered_commands
         # actually targeted, *before* any filtering below, so post-removal

src/specify_cli/presets/_manifest.py

@@ -117,7 +117,7 @@ def _validate(self):
                 )
 
         # Validate pack ID format
-        if not re.match(r'^[a-z0-9-]+$', pack["id"]):
+        if not re.fullmatch(r'[a-z0-9-]+', pack["id"]):
             raise PresetValidationError(
                 f"Invalid preset ID '{pack['id']}': "
                 "must be lowercase alphanumeric with hyphens only"

tests/specify_cli/bundles/test_primitives.py

@@ -70,6 +70,67 @@ def test_default_installer_threads_allow_network(tmp_path: Path):
         installer.install(tmp_path, _component("workflows"))
 
 
+def test_extension_remove_propagates_refusal_as_bundler_error(tmp_path: Path):
+    """A tampered registry/symlinked target must fail the bundle removal, not
+    silently report success while leaving the extension installed on disk.
+
+    ``ExtensionManager.remove()`` returns ``False`` when it refuses to act on
+    an unsafe target; the bundle adapter must surface that refusal instead of
+    discarding it, or ``remove_bundle()`` would record the component as
+    uninstalled while it is still on disk.
+    """
+    manager = primitive_manager("extensions", tmp_path)
+    manager._manager.registry.add("test-ext", {"version": "1.0.0"})
+
+    ext_dir = tmp_path / ".specify" / "extensions" / "test-ext"
+    ext_dir.parent.mkdir(parents=True, exist_ok=True)
+    real_target = tmp_path / "real-target"
+    real_target.mkdir()
+    ext_dir.symlink_to(real_target, target_is_directory=True)
+
+    with pytest.raises(BundlerError, match="removal was refused"):
+        manager.remove(_component("extensions", "test-ext"))
+
+    assert ext_dir.is_symlink()
+    assert manager._manager.registry.is_installed("test-ext")
+
+
+def test_preset_remove_propagates_refusal_as_bundler_error(tmp_path: Path):
+    """Mirrors ``test_extension_remove_propagates_refusal_as_bundler_error``:
+    ``PresetManager.remove()`` also returns ``False`` on an unsafe target, and
+    the bundle adapter must surface that refusal rather than discarding it.
+    """
+    manager = primitive_manager("presets", tmp_path)
+    manager._manager.registry.add("test-preset", {"version": "1.0.0"})
+
+    pack_dir = tmp_path / ".specify" / "presets" / "test-preset"
+    pack_dir.parent.mkdir(parents=True, exist_ok=True)
+    real_target = tmp_path / "real-target"
+    real_target.mkdir()
+    pack_dir.symlink_to(real_target, target_is_directory=True)
+
+    with pytest.raises(BundlerError, match="removal was refused"):
+        manager.remove(_component("presets", "test-preset"))
+
+    assert pack_dir.is_symlink()
+    assert manager._manager.registry.is_installed("test-preset")
+
+
+def test_preset_is_installed_uses_registry_for_corrupt_symlink_target(tmp_path: Path):
+    """``remove_bundle()`` only calls ``remove()`` when ``is_installed()`` is
+    true, so a registered preset behind a symlink must still count as installed.
+    """
+    manager = primitive_manager("presets", tmp_path)
+    manager._manager.registry.add("test-preset", {"version": "1.0.0"})
+    pack_dir = tmp_path / ".specify" / "presets" / "test-preset"
+    pack_dir.parent.mkdir(parents=True, exist_ok=True)
+    real_target = tmp_path / "real-target"
+    real_target.mkdir()
+    pack_dir.symlink_to(real_target, target_is_directory=True)
+
+    assert manager.is_installed(_component("presets", "test-preset"))
+
+
 @pytest.mark.parametrize("kind", ["presets", "extensions", "workflows", "steps"])
 def test_offline_refresh_explains_component_needs_network(tmp_path: Path, kind: str):
     installer = DefaultPrimitiveInstaller(allow_network=False)

tests/specify_cli/extensions/test_command_update_transaction.py

@@ -2228,3 +2228,25 @@ def fail_partial_skill_backup(src, dst, *args, **kwargs):
     assert first_support.read_text(encoding="utf-8") == "FIRST SUPPORT"
     assert second_support.read_text(encoding="utf-8") == "SECOND SUPPORT"
     assert _update_backup_dirs(project_dir) == []
+
+
+
+def test_extension_update_aborts_when_removal_refused(project_dir, monkeypatch):
+    """A refused remove() (returns False) must abort the update before install."""
+    monkeypatch.chdir(project_dir)
+    monkeypatch.setattr(ExtensionManager, "list_installed", lambda self: [{"id": "test-ext", "name": "Test Ext", "version": "1.0.0"}])
+    monkeypatch.setattr(ExtensionRegistry, "get", lambda self, ext_id: {"version": "1.0.0", "enabled": True})
+    mock_zip = project_dir / "mock.zip"
+    _write_update_zip(mock_zip)
+    monkeypatch.setattr(ExtensionCatalog, "download_extension", lambda self, ext_id: mock_zip)
+    monkeypatch.setattr(ExtensionCatalog, "get_extension_info", lambda self, ext_id: {"id": "test-ext", "name": "Test Ext", "version": "1.1.0", "download_url": "https://example.com/ext.zip"})
+    monkeypatch.setattr(ExtensionManager, "remove", lambda self, ext_id, keep_config=False: False)
+    installs = []
+    monkeypatch.setattr(ExtensionManager, "install_from_zip", lambda *a, **k: installs.append(1))
+    monkeypatch.setattr("typer.confirm", lambda _: True)
+
+    result = runner.invoke(app, ["extension", "update", "test-ext"], obj={"project_root": project_dir})
+
+    assert result.exit_code == 1
+    assert "Could not safely remove" in result.output
+    assert installs == []