From b6dc0a6d03c54e4c9d7db95330281070d19c2e66 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Fri, 25 Sep 2026 12:56:14 +0000 Subject: [PATCH 01/10] fix: harden ExtensionManager.remove and PresetManager.remove against unsafe registry IDs A registered extension/preset id from the on-disk registry JSON was used directly to build the removal (and, for extensions, config-backup) path without validating it is a single well-formed path component, and without checking that the resolved target is a real directory rather than a symlink. A crafted or corrupted registry entry (e.g. an id containing "../") could therefore point removal outside .specify/extensions or .specify/presets, and a symlinked target dir or symlinked .backup/ could redirect deletion or config backups elsewhere; the previous code also crashed with an unhandled OSError from shutil.rmtree on a symlinked target rather than failing explicitly. Both remove() methods now validate the id against the same [a-z0-9-]+ pattern already enforced at install time, and refuse to proceed if the removal target (or, for extensions, the backup destination) exists but is not a plain directory. Workflow/step removal already carries equivalent guards and integration uninstall (IntegrationManifest.uninstall) already rejects traversal and symlinked paths, so neither needed changes; this was verified as part of the same audit requested by the issue. Fixes #4744 Assisted-by: Claude Code (model: Claude Sonnet 5, autonomous) --- src/specify_cli/extensions/__init__.py | 19 ++++++-- src/specify_cli/presets/__init__.py | 12 ++++- tests/test_extensions.py | 67 ++++++++++++++++++++++++++ tests/test_presets.py | 44 +++++++++++++++++ 4 files changed, 138 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index e4b9e7de9d..0858516335 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2960,6 +2960,22 @@ 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.match(extension_id): + return False + extension_dir = self.extensions_dir / extension_id + if extension_dir.exists() and ( + extension_dir.is_symlink() or not extension_dir.is_dir() + ): + return False + backup_dir = self.extensions_dir / ".backup" / extension_id + if backup_dir.is_symlink(): + return False + # Get registered commands and skills before removal metadata = self.registry.get(extension_id) registered_commands = ( @@ -2972,8 +2988,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 +3021,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 diff --git a/src/specify_cli/presets/__init__.py b/src/specify_cli/presets/__init__.py index eff1e68159..ad5da322df 100644 --- a/src/specify_cli/presets/__init__.py +++ b/src/specify_cli/presets/__init__.py @@ -4111,6 +4111,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.exists() and (pack_dir.is_symlink() or 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 [] @@ -4139,7 +4150,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 diff --git a/tests/test_extensions.py b/tests/test_extensions.py index b512742389..5407336636 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3277,6 +3277,73 @@ def test_config_backup_on_remove(self, extension_dir, project_dir): assert backup_file.exists() assert backup_file.read_text() == "test: config" + def test_remove_rejects_unsafe_registry_id(self, project_dir): + """A tampered registry entry must not reach path construction. + + The registry file is user-editable JSON; an id like ``../outside`` + would otherwise let ``remove()`` build a deletion target outside + ``.specify/extensions``. + """ + manager = ExtensionManager(project_dir) + unsafe_id = "../outside-target" + manager.registry.add(unsafe_id, {"version": "1.0.0"}) + assert manager.registry.is_installed(unsafe_id) + + outside_target = project_dir / ".specify" / "outside-target" + outside_target.mkdir() + (outside_target / "keep.txt").write_text("do not delete") + + result = manager.remove(unsafe_id) + + assert result is False + assert outside_target.exists() + assert (outside_target / "keep.txt").exists() + # Refused before the registry entry was mutated. + assert manager.registry.is_installed(unsafe_id) + + def test_remove_refuses_symlinked_extension_dir(self, project_dir): + """A symlinked extension directory must fail explicitly, not be deleted.""" + manager = ExtensionManager(project_dir) + manager.registry.add("test-ext", {"version": "1.0.0"}) + + real_target = project_dir.parent / "real-target" + real_target.mkdir() + (real_target / "important.txt").write_text("do not delete") + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + ext_dir.symlink_to(real_target, target_is_directory=True) + + result = manager.remove("test-ext") + + assert result is False + assert real_target.exists() + assert (real_target / "important.txt").exists() + assert ext_dir.is_symlink() + assert manager.registry.is_installed("test-ext") + + def test_remove_refuses_symlinked_backup_dir(self, extension_dir, project_dir): + """Config backups must not be redirected through a symlinked destination.""" + 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("test: config") + + backup_root = project_dir / ".specify" / "extensions" / ".backup" + backup_root.mkdir(parents=True, exist_ok=True) + outside_target = project_dir.parent / "outside-backup" + outside_target.mkdir() + (backup_root / "test-ext").symlink_to(outside_target, target_is_directory=True) + + result = manager.remove("test-ext", keep_config=False) + + assert result is False + assert ext_dir.exists() + assert config_file.exists() + assert not (outside_target / "test-ext-config.yml").exists() + assert manager.registry.is_installed("test-ext") + # ===== CommandRegistrar Tests ===== diff --git a/tests/test_presets.py b/tests/test_presets.py index a7e590b071..5ad3580814 100644 --- a/tests/test_presets.py +++ b/tests/test_presets.py @@ -1012,6 +1012,50 @@ def test_remove_nonexistent(self, project_dir): result = manager.remove("nonexistent") assert result is False + def test_remove_rejects_unsafe_registry_id(self, project_dir): + """A tampered registry entry must not reach path construction. + + The registry file is user-editable JSON; an id like ``../outside`` + would otherwise let ``remove()`` build a deletion target outside + ``.specify/presets``. + """ + manager = PresetManager(project_dir) + unsafe_id = "../outside-target" + manager.registry.add(unsafe_id, {"version": "1.0.0"}) + assert manager.registry.is_installed(unsafe_id) + + outside_target = project_dir / ".specify" / "outside-target" + outside_target.mkdir() + (outside_target / "keep.txt").write_text("do not delete") + + result = manager.remove(unsafe_id) + + assert result is False + assert outside_target.exists() + assert (outside_target / "keep.txt").exists() + # Refused before the registry entry was mutated. + assert manager.registry.is_installed(unsafe_id) + + def test_remove_refuses_symlinked_preset_dir(self, project_dir): + """A symlinked preset directory must fail explicitly, not be deleted.""" + manager = PresetManager(project_dir) + manager.registry.add("test-pack", {"version": "1.0.0"}) + + real_target = project_dir.parent / "real-target" + real_target.mkdir() + (real_target / "important.txt").write_text("do not delete") + + pack_dir = project_dir / ".specify" / "presets" / "test-pack" + pack_dir.symlink_to(real_target, target_is_directory=True) + + result = manager.remove("test-pack") + + assert result is False + assert real_target.exists() + assert (real_target / "important.txt").exists() + assert pack_dir.is_symlink() + assert manager.registry.is_installed("test-pack") + def test_list_installed(self, project_dir, pack_dir): """Test listing installed packs.""" manager = PresetManager(project_dir) From fd86df8c0b47a062bbf697ce2a730ca3950499f9 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Mon, 28 Sep 2026 13:07:08 +0000 Subject: [PATCH 02/10] fix: close remaining extension/preset removal symlink gaps Address Copilot review on #4745: use fullmatch (not match) for the extension-id slug check so a trailing newline can't slip past the `$` anchor, check is_symlink() independently of exists() so a dangling symlink at the extension/preset directory is rejected instead of silently passed through, and reject a symlinked `.backup` parent directory before it is ever mkdir'd/written into. --- src/specify_cli/extensions/__init__.py | 11 +++-- src/specify_cli/presets/_manager.py | 2 +- tests/specify_cli/presets/test_manager.py | 20 +++++++++ tests/test_extensions.py | 54 +++++++++++++++++++++++ 4 files changed, 82 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 0858516335..348b4bfe4c 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2965,14 +2965,17 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: # 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.match(extension_id): + if not VALID_EXTENSION_ARTIFACT_NAME_PATTERN.fullmatch(extension_id): return False extension_dir = self.extensions_dir / extension_id - if extension_dir.exists() and ( - extension_dir.is_symlink() or not extension_dir.is_dir() + if extension_dir.is_symlink() or ( + extension_dir.exists() and not extension_dir.is_dir() ): return False - backup_dir = self.extensions_dir / ".backup" / extension_id + backup_root = self.extensions_dir / ".backup" + if backup_root.is_symlink(): + return False + backup_dir = backup_root / extension_id if backup_dir.is_symlink(): return False diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index bdd80933ad..88a8e4042a 100644 --- a/src/specify_cli/presets/_manager.py +++ b/src/specify_cli/presets/_manager.py @@ -648,7 +648,7 @@ def remove(self, pack_id: str) -> bool: if not PresetResolver._is_safe_registry_id(pack_id): return False pack_dir = self.presets_dir / pack_id - if pack_dir.exists() and (pack_dir.is_symlink() or not pack_dir.is_dir()): + if pack_dir.is_symlink() or (pack_dir.exists() and not pack_dir.is_dir()): return False metadata = self.registry.get(pack_id) diff --git a/tests/specify_cli/presets/test_manager.py b/tests/specify_cli/presets/test_manager.py index cf8d00c29d..0b65cdd1c0 100644 --- a/tests/specify_cli/presets/test_manager.py +++ b/tests/specify_cli/presets/test_manager.py @@ -335,6 +335,26 @@ def test_remove_refuses_symlinked_preset_dir(self, project_dir): assert pack_dir.is_symlink() assert manager.registry.is_installed("test-pack") + def test_remove_refuses_dangling_symlinked_preset_dir(self, project_dir): + """A dangling symlink must fail explicitly rather than be silently skipped. + + ``Path.exists()`` follows the link and returns False for a broken + symlink, so a guard gated on ``exists()`` would let this through. + """ + manager = PresetManager(project_dir) + manager.registry.add("test-pack", {"version": "1.0.0"}) + + pack_dir = project_dir / ".specify" / "presets" / "test-pack" + pack_dir.parent.mkdir(parents=True, exist_ok=True) + missing_target = project_dir.parent / "does-not-exist" + pack_dir.symlink_to(missing_target, target_is_directory=True) + + result = manager.remove("test-pack") + + assert result is False + assert pack_dir.is_symlink() + assert manager.registry.is_installed("test-pack") + def test_list_installed(self, project_dir, pack_dir): """Test listing installed packs.""" manager = PresetManager(project_dir) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 5407336636..7e4d4cb95d 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3344,6 +3344,60 @@ def test_remove_refuses_symlinked_backup_dir(self, extension_dir, project_dir): assert not (outside_target / "test-ext-config.yml").exists() assert manager.registry.is_installed("test-ext") + def test_remove_refuses_dangling_symlinked_extension_dir(self, project_dir): + """A dangling symlink must fail explicitly rather than be silently skipped. + + ``Path.exists()`` follows the link and returns False for a broken + symlink, so a guard gated on ``exists()`` would let this through. + """ + manager = ExtensionManager(project_dir) + manager.registry.add("test-ext", {"version": "1.0.0"}) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + ext_dir.parent.mkdir(parents=True, exist_ok=True) + missing_target = project_dir.parent / "does-not-exist" + ext_dir.symlink_to(missing_target, target_is_directory=True) + + result = manager.remove("test-ext") + + assert result is False + assert ext_dir.is_symlink() + assert manager.registry.is_installed("test-ext") + + def test_remove_refuses_symlinked_backup_root(self, extension_dir, project_dir): + """A symlinked ``.backup`` parent must not redirect config backups.""" + 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("test: config") + + outside_target = project_dir.parent / "outside-backup-root" + outside_target.mkdir() + backup_root = project_dir / ".specify" / "extensions" / ".backup" + backup_root.symlink_to(outside_target, target_is_directory=True) + + result = manager.remove("test-ext", keep_config=False) + + assert result is False + assert ext_dir.exists() + assert config_file.exists() + assert not (outside_target / "test-ext" / "test-ext-config.yml").exists() + assert manager.registry.is_installed("test-ext") + + def test_remove_rejects_id_with_trailing_newline(self, project_dir): + """``fullmatch`` must be used so a trailing newline cannot slip past ``$``.""" + manager = ExtensionManager(project_dir) + unsafe_id = "test-ext\n" + manager.registry.add(unsafe_id, {"version": "1.0.0"}) + assert manager.registry.is_installed(unsafe_id) + + result = manager.remove(unsafe_id) + + assert result is False + assert manager.registry.is_installed(unsafe_id) + # ===== CommandRegistrar Tests ===== From 52bacea2e8ad89ecc9658673327bf3ed4adfca82 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Mon, 28 Sep 2026 16:14:00 +0000 Subject: [PATCH 03/10] fix: propagate extension removal refusal to bundle uninstall _ExtensionKindManager.remove() discarded ExtensionManager.remove()'s bool result, so a tampered registry id or symlinked target that made the security guard refuse to act still let remove_bundle() record the component as uninstalled and drop the bundle's ownership metadata, leaving the extension on disk. Raise BundlerError when remove() returns False so the bundle removal fails instead of silently succeeding. Also add a regression test covering the previously-untested regular-file preset target rejection in PresetManager.remove(). --- src/specify_cli/bundles/primitives.py | 7 +++++- tests/specify_cli/bundles/test_primitives.py | 25 ++++++++++++++++++++ tests/specify_cli/presets/test_manager.py | 16 +++++++++++++ 3 files changed, 47 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/bundles/primitives.py b/src/specify_cli/bundles/primitives.py index b32342e68d..a9dd73e2d6 100644 --- a/src/specify_cli/bundles/primitives.py +++ b/src/specify_cli/bundles/primitives.py @@ -313,11 +313,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 symlinked target)." + ) class _WorkflowKindManager: diff --git a/tests/specify_cli/bundles/test_primitives.py b/tests/specify_cli/bundles/test_primitives.py index a3b4d83f45..ec660bf952 100644 --- a/tests/specify_cli/bundles/test_primitives.py +++ b/tests/specify_cli/bundles/test_primitives.py @@ -70,6 +70,31 @@ 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") + + @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) diff --git a/tests/specify_cli/presets/test_manager.py b/tests/specify_cli/presets/test_manager.py index 0b65cdd1c0..58317c598b 100644 --- a/tests/specify_cli/presets/test_manager.py +++ b/tests/specify_cli/presets/test_manager.py @@ -355,6 +355,22 @@ def test_remove_refuses_dangling_symlinked_preset_dir(self, project_dir): assert pack_dir.is_symlink() assert manager.registry.is_installed("test-pack") + def test_remove_refuses_regular_file_preset_dir(self, project_dir): + """A regular file at the preset path must fail explicitly, not crash + ``shutil.rmtree`` or leave the registry mutated.""" + manager = PresetManager(project_dir) + manager.registry.add("test-pack", {"version": "1.0.0"}) + + pack_dir = project_dir / ".specify" / "presets" / "test-pack" + pack_dir.parent.mkdir(parents=True, exist_ok=True) + pack_dir.write_text("not a directory") + + result = manager.remove("test-pack") + + assert result is False + assert pack_dir.is_file() + assert manager.registry.is_installed("test-pack") + def test_list_installed(self, project_dir, pack_dir): """Test listing installed packs.""" manager = PresetManager(project_dir) From 6c86c61aaebc5dae3bd5bccdec8bb010327a3517 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Mon, 28 Sep 2026 17:24:06 +0000 Subject: [PATCH 04/10] fix: propagate preset removal refusal and gate backup symlink checks _PresetKindManager.remove() discarded PresetManager.remove()'s False return the same way the extension adapter did before 52bacea, letting bundle uninstall drop ownership metadata for a preset that was refused removal. Mirror the extension fix by raising BundlerError. Also stop failing extension removal when keep_config=True or the extension directory is already missing: the .backup symlink checks ran unconditionally even though no backup is written in either case, regressing valid removals. --- src/specify_cli/bundles/primitives.py | 7 ++++++- src/specify_cli/extensions/__init__.py | 7 +++---- tests/specify_cli/bundles/test_primitives.py | 21 ++++++++++++++++++++ tests/test_extensions.py | 19 ++++++++++++++++++ 4 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/specify_cli/bundles/primitives.py b/src/specify_cli/bundles/primitives.py index a9dd73e2d6..37a9aed328 100644 --- a/src/specify_cli/bundles/primitives.py +++ b/src/specify_cli/bundles/primitives.py @@ -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 symlinked target)." + ) class _ExtensionKindManager: diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 348b4bfe4c..7cd2d1a950 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2973,11 +2973,10 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: ): return False backup_root = self.extensions_dir / ".backup" - if backup_root.is_symlink(): - return False backup_dir = backup_root / extension_id - if backup_dir.is_symlink(): - return False + if not keep_config and extension_dir.exists(): + if backup_root.is_symlink() or backup_dir.is_symlink(): + return False # Get registered commands and skills before removal metadata = self.registry.get(extension_id) diff --git a/tests/specify_cli/bundles/test_primitives.py b/tests/specify_cli/bundles/test_primitives.py index ec660bf952..e633fcc466 100644 --- a/tests/specify_cli/bundles/test_primitives.py +++ b/tests/specify_cli/bundles/test_primitives.py @@ -95,6 +95,27 @@ def test_extension_remove_propagates_refusal_as_bundler_error(tmp_path: Path): 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") + + @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) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 7e4d4cb95d..27e8c7e6d7 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3386,6 +3386,25 @@ def test_remove_refuses_symlinked_backup_root(self, extension_dir, project_dir): assert not (outside_target / "test-ext" / "test-ext-config.yml").exists() assert manager.registry.is_installed("test-ext") + def test_remove_keep_config_ignores_symlinked_backup_root( + self, extension_dir, project_dir + ): + """``keep_config=True`` never writes a backup, so an unrelated symlink + at ``.backup`` must not block removal. + """ + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + outside_target = project_dir.parent / "outside-backup-root-keep-config" + outside_target.mkdir() + backup_root = project_dir / ".specify" / "extensions" / ".backup" + backup_root.symlink_to(outside_target, target_is_directory=True) + + result = manager.remove("test-ext", keep_config=True) + + assert result is True + assert not manager.registry.is_installed("test-ext") + def test_remove_rejects_id_with_trailing_newline(self, project_dir): """``fullmatch`` must be used so a trailing newline cannot slip past ``$``.""" manager = ExtensionManager(project_dir) From 2fa831a9831dae8f546237322178b15a470085d6 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Tue, 29 Sep 2026 05:08:58 +0000 Subject: [PATCH 05/10] fix: reject non-directory backup paths before extension removal --- src/specify_cli/extensions/__init__.py | 7 ++++++- tests/test_extensions.py | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 7cd2d1a950..7c81fd0785 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2975,7 +2975,12 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: 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(): + 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()) + ): return False # Get registered commands and skills before removal diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 27e8c7e6d7..1f963493ad 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3344,6 +3344,20 @@ def test_remove_refuses_symlinked_backup_dir(self, extension_dir, project_dir): assert not (outside_target / "test-ext-config.yml").exists() assert manager.registry.is_installed("test-ext") + def test_remove_refuses_regular_file_backup_path(self, extension_dir, project_dir): + """A non-directory at the backup path must not cause partial removal.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + backup_root = project_dir / ".specify" / "extensions" / ".backup" + backup_root.mkdir(parents=True, exist_ok=True) + (backup_root / "test-ext").write_text("not a directory") + + assert manager.remove("test-ext", keep_config=False) is False + assert ext_dir.exists() + assert manager.registry.is_installed("test-ext") + def test_remove_refuses_dangling_symlinked_extension_dir(self, project_dir): """A dangling symlink must fail explicitly rather than be silently skipped. From 5ed47cc09323a48db7996db99cadf154c78b75da Mon Sep 17 00:00:00 2001 From: chelsealong Date: Tue, 29 Sep 2026 11:31:00 +0000 Subject: [PATCH 06/10] fix: align manifest id validation and abort install/update on refused removal --- src/specify_cli/extensions/__init__.py | 7 +++++- .../extensions/_command_update_transaction.py | 5 ++++- src/specify_cli/presets/_manifest.py | 2 +- .../test_command_update_transaction.py | 22 +++++++++++++++++++ tests/specify_cli/presets/test_manifest.py | 7 +++--- tests/test_extensions.py | 22 ++++++++++++++++--- 6 files changed, 56 insertions(+), 9 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 7c81fd0785..98a2d6aa74 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/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" @@ -2168,6 +2168,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 diff --git a/src/specify_cli/extensions/_command_update_transaction.py b/src/specify_cli/extensions/_command_update_transaction.py index b5dd995c1c..efa18c9d3f 100644 --- a/src/specify_cli/extensions/_command_update_transaction.py +++ b/src/specify_cli/extensions/_command_update_transaction.py @@ -520,7 +520,10 @@ 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: + raise RuntimeError( + f"Could not safely remove existing '{extension_id}'" + ) # 8. Install new version _ = manager.install_from_zip( diff --git a/src/specify_cli/presets/_manifest.py b/src/specify_cli/presets/_manifest.py index b5a7ff03c3..e75ecb7966 100644 --- a/src/specify_cli/presets/_manifest.py +++ b/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" diff --git a/tests/specify_cli/extensions/test_command_update_transaction.py b/tests/specify_cli/extensions/test_command_update_transaction.py index 28efb46833..c67188302b 100644 --- a/tests/specify_cli/extensions/test_command_update_transaction.py +++ b/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 == [] diff --git a/tests/specify_cli/presets/test_manifest.py b/tests/specify_cli/presets/test_manifest.py index 27ac723daa..e2fb3bfd89 100644 --- a/tests/specify_cli/presets/test_manifest.py +++ b/tests/specify_cli/presets/test_manifest.py @@ -199,9 +199,10 @@ def test_missing_pack_id(self, temp_dir, valid_pack_data): with pytest.raises(PresetValidationError, match="Missing preset.id"): PresetManifest(manifest_path) - def test_invalid_pack_id_format(self, temp_dir, valid_pack_data): - """Test invalid pack ID format.""" - valid_pack_data["preset"]["id"] = "Invalid_ID" + @pytest.mark.parametrize("bad_id", ["Invalid_ID", "test-pack\n"]) + def test_invalid_pack_id_format(self, temp_dir, valid_pack_data, bad_id): + """Test invalid pack ID format (incl. trailing newline).""" + valid_pack_data["preset"]["id"] = bad_id manifest_path = temp_dir / "preset.yml" with open(manifest_path, 'w') as f: yaml.dump(valid_pack_data, f) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 1f963493ad..91846c8473 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -384,11 +384,12 @@ def test_invalid_utf8_bytes_raises_validation_error(self, temp_dir): with pytest.raises(ValidationError, match="not valid UTF-8"): ExtensionManifest(manifest_path) - def test_invalid_extension_id(self, temp_dir, valid_manifest_data): - """Test manifest with invalid extension ID format.""" + @pytest.mark.parametrize("bad_id", ["Invalid_ID", "test-ext\n"]) + def test_invalid_extension_id(self, temp_dir, valid_manifest_data, bad_id): + """Test manifest with invalid extension ID format (incl. trailing newline).""" import yaml - valid_manifest_data["extension"]["id"] = "Invalid_ID" # Uppercase not allowed + valid_manifest_data["extension"]["id"] = bad_id manifest_path = temp_dir / "extension.yml" with open(manifest_path, 'w') as f: @@ -3321,6 +3322,21 @@ def test_remove_refuses_symlinked_extension_dir(self, project_dir): assert ext_dir.is_symlink() assert manager.registry.is_installed("test-ext") + def test_force_reinstall_aborts_when_removal_refused(self, extension_dir, project_dir): + """install(force=True) must abort if remove() refuses a symlinked target.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + real_target = project_dir.parent / "real-target" + shutil.move(str(ext_dir), str(real_target)) + ext_dir.symlink_to(real_target, target_is_directory=True) + + with pytest.raises(ExtensionError, match="could not be safely removed"): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False, force=True + ) + assert (real_target / "extension.yml").exists() + def test_remove_refuses_symlinked_backup_dir(self, extension_dir, project_dir): """Config backups must not be redirected through a symlinked destination.""" manager = ExtensionManager(project_dir) From 95d0787b2feac5311f847d3816a1ae0140cf44df Mon Sep 17 00:00:00 2001 From: chelsealong Date: Tue, 29 Sep 2026 11:55:20 +0000 Subject: [PATCH 07/10] fix: validate .backup root on force reinstall, abort preset force reinstall on refused removal, keep update refusal non-destructive --- src/specify_cli/extensions/__init__.py | 5 +++++ .../extensions/_command_update_transaction.py | 1 + src/specify_cli/presets/_manager.py | 6 +++++- tests/specify_cli/presets/test_manager.py | 14 ++++++++++++++ tests/test_extensions.py | 17 +++++++++++++++++ 5 files changed, 42 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 98a2d6aa74..b8509c9103 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -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(): diff --git a/src/specify_cli/extensions/_command_update_transaction.py b/src/specify_cli/extensions/_command_update_transaction.py index efa18c9d3f..5e259d9716 100644 --- a/src/specify_cli/extensions/_command_update_transaction.py +++ b/src/specify_cli/extensions/_command_update_transaction.py @@ -521,6 +521,7 @@ def backup_extension_skills(skill_names, *, skills_dir=None): # 7. Remove old extension (handles command file cleanup and registry removal) installation_modified = True if manager.remove(extension_id, keep_config=True) is False: + installation_modified = False raise RuntimeError( f"Could not safely remove existing '{extension_id}'" ) diff --git a/src/specify_cli/presets/_manager.py b/src/specify_cli/presets/_manager.py index 88a8e4042a..149a6d3f1e 100644 --- a/src/specify_cli/presets/_manager.py +++ b/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(): diff --git a/tests/specify_cli/presets/test_manager.py b/tests/specify_cli/presets/test_manager.py index 58317c598b..2657ff2301 100644 --- a/tests/specify_cli/presets/test_manager.py +++ b/tests/specify_cli/presets/test_manager.py @@ -1,6 +1,7 @@ """Tests for preset installation and removal in specify_cli.presets._manager.""" import json +import shutil import tarfile import zipfile from pathlib import Path @@ -335,6 +336,19 @@ def test_remove_refuses_symlinked_preset_dir(self, project_dir): assert pack_dir.is_symlink() assert manager.registry.is_installed("test-pack") + def test_force_reinstall_aborts_when_removal_refused(self, project_dir, pack_dir): + """install_from_directory(force=True) must abort if remove() refuses.""" + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "0.1.5") + installed = project_dir / ".specify" / "presets" / "test-pack" + real_target = project_dir.parent / "real-target" + shutil.move(str(installed), str(real_target)) + installed.symlink_to(real_target, target_is_directory=True) + + with pytest.raises(PresetError, match="could not be safely removed"): + manager.install_from_directory(pack_dir, "0.1.5", force=True) + assert (real_target / "preset.yml").exists() + def test_remove_refuses_dangling_symlinked_preset_dir(self, project_dir): """A dangling symlink must fail explicitly rather than be silently skipped. diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 91846c8473..bd70ea7bbc 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3337,6 +3337,23 @@ def test_force_reinstall_aborts_when_removal_refused(self, extension_dir, projec ) assert (real_target / "extension.yml").exists() + def test_force_reinstall_refuses_symlinked_backup_root(self, extension_dir, project_dir): + """Stale-backup cleanup must not delete through a symlinked .backup root.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + outside = project_dir.parent / "outside-backup" + (outside / "test-ext").mkdir(parents=True) + (outside / "test-ext" / "keep.txt").write_text("keep") + (project_dir / ".specify" / "extensions" / ".backup").symlink_to( + outside, target_is_directory=True + ) + + with pytest.raises(ExtensionError, match="symlink"): + manager.install_from_directory( + extension_dir, "0.1.0", register_commands=False, force=True + ) + assert (outside / "test-ext" / "keep.txt").exists() + def test_remove_refuses_symlinked_backup_dir(self, extension_dir, project_dir): """Config backups must not be redirected through a symlinked destination.""" manager = ExtensionManager(project_dir) From b2b2f4863f425bbc9c194cc8191e9fed666abea7 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Wed, 30 Sep 2026 00:11:05 +0000 Subject: [PATCH 08/10] fix: reject symlinked backup config files and use preset registry for bundle removal --- src/specify_cli/bundles/primitives.py | 2 +- src/specify_cli/extensions/__init__.py | 5 +++ tests/specify_cli/bundles/test_primitives.py | 15 +++++++++ tests/test_extensions.py | 32 ++++++++++++++++++++ 4 files changed, 53 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/bundles/primitives.py b/src/specify_cli/bundles/primitives.py index 37a9aed328..42340f73cf 100644 --- a/src/specify_cli/bundles/primitives.py +++ b/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 diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index b8509c9103..f79802772d 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2990,6 +2990,11 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: 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() + for f in list(extension_dir.glob("*-config.yml")) + + list(extension_dir.glob("*-config.local.yml")) + ) ): return False diff --git a/tests/specify_cli/bundles/test_primitives.py b/tests/specify_cli/bundles/test_primitives.py index e633fcc466..472face0d3 100644 --- a/tests/specify_cli/bundles/test_primitives.py +++ b/tests/specify_cli/bundles/test_primitives.py @@ -116,6 +116,21 @@ def test_preset_remove_propagates_refusal_as_bundler_error(tmp_path: Path): 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) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index bd70ea7bbc..3f58e388b6 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3391,6 +3391,38 @@ def test_remove_refuses_regular_file_backup_path(self, extension_dir, project_di assert ext_dir.exists() assert manager.registry.is_installed("test-ext") + def test_remove_refuses_symlinked_backup_config_file( + self, extension_dir, project_dir + ): + """A symlink at ``.backup//`` must not be written through.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + (ext_dir / "test-ext-config.yml").write_text("a: 1\n") + backup_dir = ext_dir.parent / ".backup" / "test-ext" + backup_dir.mkdir(parents=True) + outside = project_dir.parent / "outside-target.yml" + outside.write_text("original") + (backup_dir / "test-ext-config.yml").symlink_to(outside) + + assert manager.remove("test-ext", keep_config=False) is False + assert outside.read_text() == "original" + assert ext_dir.exists() + assert manager.registry.is_installed("test-ext") + + def test_remove_refuses_regular_file_extension_dir(self, project_dir): + """A regular file at the extension path is preserved and not unregistered.""" + manager = ExtensionManager(project_dir) + manager.registry.add("test-ext", {"version": "1.0.0"}) + ext_path = project_dir / ".specify" / "extensions" / "test-ext" + ext_path.parent.mkdir(parents=True, exist_ok=True) + ext_path.write_text("not a directory") + + assert manager.remove("test-ext") is False + assert ext_path.read_text() == "not a directory" + assert manager.registry.is_installed("test-ext") + def test_remove_refuses_dangling_symlinked_extension_dir(self, project_dir): """A dangling symlink must fail explicitly rather than be silently skipped. From 2cdedf93c3ded9430bcec2964ab7c6f1d2e5a373 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Wed, 30 Sep 2026 13:32:02 +0000 Subject: [PATCH 09/10] fix: reject directory at backup config destination before extension removal --- src/specify_cli/extensions/__init__.py | 1 + tests/test_extensions.py | 17 +++++++++++++++++ 2 files changed, 18 insertions(+) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index f79802772d..909a00bfcc 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2992,6 +2992,7 @@ def remove(self, extension_id: str, keep_config: bool = False) -> bool: 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")) ) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 3f58e388b6..8a2a35a999 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -3411,6 +3411,23 @@ def test_remove_refuses_symlinked_backup_config_file( assert ext_dir.exists() assert manager.registry.is_installed("test-ext") + def test_remove_refuses_directory_backup_config_path( + self, extension_dir, project_dir + ): + """A directory at ``.backup//`` must not be copied into.""" + manager = ExtensionManager(project_dir) + manager.install_from_directory(extension_dir, "0.1.0", register_commands=False) + + ext_dir = project_dir / ".specify" / "extensions" / "test-ext" + (ext_dir / "test-ext-config.yml").write_text("a: 1\n") + backup_cfg = ext_dir.parent / ".backup" / "test-ext" / "test-ext-config.yml" + backup_cfg.mkdir(parents=True) + + assert manager.remove("test-ext", keep_config=False) is False + assert list(backup_cfg.iterdir()) == [] + assert ext_dir.exists() + assert manager.registry.is_installed("test-ext") + def test_remove_refuses_regular_file_extension_dir(self, project_dir): """A regular file at the extension path is preserved and not unregistered.""" manager = ExtensionManager(project_dir) From d4212256f60364a4d4396c45918455acb268f1f2 Mon Sep 17 00:00:00 2001 From: chelsealong Date: Wed, 30 Sep 2026 14:13:11 +0000 Subject: [PATCH 10/10] fix: clarify bundle removal refusal message, test removing preset with missing dir --- src/specify_cli/bundles/primitives.py | 4 ++-- tests/specify_cli/presets/test_manager.py | 8 ++++++++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/bundles/primitives.py b/src/specify_cli/bundles/primitives.py index 42340f73cf..2da9db5a95 100644 --- a/src/specify_cli/bundles/primitives.py +++ b/src/specify_cli/bundles/primitives.py @@ -230,7 +230,7 @@ def remove(self, component: ComponentRef) -> None: if not removed: raise BundlerError( f"Failed to remove preset '{component.id}': removal was " - "refused (unsafe registry id or symlinked target)." + "refused (unsafe registry id or unexpected on-disk target)." ) @@ -326,7 +326,7 @@ def remove(self, component: ComponentRef) -> None: if not removed: raise BundlerError( f"Failed to remove extension '{component.id}': removal was " - "refused (unsafe registry id or symlinked target)." + "refused (unsafe registry id or unexpected on-disk target)." ) diff --git a/tests/specify_cli/presets/test_manager.py b/tests/specify_cli/presets/test_manager.py index 2657ff2301..3528578485 100644 --- a/tests/specify_cli/presets/test_manager.py +++ b/tests/specify_cli/presets/test_manager.py @@ -349,6 +349,14 @@ def test_force_reinstall_aborts_when_removal_refused(self, project_dir, pack_dir manager.install_from_directory(pack_dir, "0.1.5", force=True) assert (real_target / "preset.yml").exists() + def test_remove_registered_preset_with_missing_dir(self, project_dir): + """A valid registered preset whose directory is already gone is removed.""" + manager = PresetManager(project_dir) + manager.registry.add("test-pack", {"version": "1.0.0"}) + + assert manager.remove("test-pack") is True + assert not manager.registry.is_installed("test-pack") + def test_remove_refuses_dangling_symlinked_preset_dir(self, project_dir): """A dangling symlink must fail explicitly rather than be silently skipped.