From fb004cb7844e8823597f7a7edcf368ccd3ca0e91 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 29 Oct 2026 09:24:09 -0500 Subject: [PATCH 1/4] fix: reject symlinked preset registry paths Use a project-scoped deny-mode FileHelper for preset registry reads and writes, validating save destinations before file-change observers. Preserve registry serialization and metadata semantics and cover symlink rejection and normal persistence. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 10 ++ src/specify_cli/presets/_registry.py | 24 +-- tests/specify_cli/presets/test_registry.py | 167 +++++++++++++++++++++ 3 files changed, 190 insertions(+), 11 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index f5497ef256..1c75a86952 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -8,6 +8,16 @@ enforce symlink behavior on those operations. Shared infrastructure, registratio managed storage, catalogs, scripts and runtime operations retain their existing behavior until their dependent migration stages. +Preset registry reads and writes use a project-root-scoped deny-mode helper. +For the documented `.specify/presets` registry location, the root is two +parents above that directory, never the preset +storage directory itself. Registry metadata and its parent hierarchy must +not be links, independently of the operational `allow_symlinks` setting. Saves +validate the destination before file-change observers or directory creation +and preserve the existing registry text I/O and metadata behavior. This +migration covers registry persistence only, not the remaining preset lifecycle +filesystem operations. + Only `specify init --allow-symlinks` exposes the setting; init defaults to deny and persists the literal JSON boolean `allow_symlinks` in `.specify/init-options.json`. Re-init without the flag records `false`. diff --git a/src/specify_cli/presets/_registry.py b/src/specify_cli/presets/_registry.py index a774f724f1..3bc189c80b 100644 --- a/src/specify_cli/presets/_registry.py +++ b/src/specify_cli/presets/_registry.py @@ -7,6 +7,7 @@ from typing import Dict, List, Optional from ..extensions import normalize_priority +from ..file_helper import FileHelper class PresetRegistry: @@ -23,19 +24,16 @@ def __init__(self, packs_dir: Path): """ self.packs_dir = packs_dir self.registry_path = packs_dir / self.REGISTRY_FILE + # Scope above .specify so neither it nor presets becomes a trusted alias. + self._files = FileHelper(packs_dir.absolute().parent.parent) self.data = self._load() def _load(self) -> dict: """Load registry from disk.""" - if not self.registry_path.exists(): - return { - "schema_version": self.SCHEMA_VERSION, - "presets": {} - } - try: - with open(self.registry_path, 'r', encoding='utf-8') as f: - data = json.load(f) + data = json.loads( + self._files.read_text(self.registry_path.absolute(), encoding="utf-8") + ) # Validate loaded data is a dict (handles corrupted registry files) if not isinstance(data, dict): return { @@ -61,10 +59,14 @@ def _save(self): """Save registry to disk.""" from ..integrations._file_changes import after_file_change, before_file_change + path = self.registry_path.absolute() + try: + self._files.read_bytes(path) + except FileNotFoundError: + pass before_file_change(self.registry_path) - self.packs_dir.mkdir(parents=True, exist_ok=True) - with open(self.registry_path, 'w', encoding='utf-8') as f: - json.dump(self.data, f, indent=2) + self._files.mkdir(self.packs_dir.absolute(), parents=True, exist_ok=True) + self._files.overwrite_text(path, json.dumps(self.data, indent=2), encoding="utf-8") after_file_change(self.registry_path) def add(self, pack_id: str, metadata: dict): diff --git a/tests/specify_cli/presets/test_registry.py b/tests/specify_cli/presets/test_registry.py index 07ae8ca8fd..7ae6689ca3 100644 --- a/tests/specify_cli/presets/test_registry.py +++ b/tests/specify_cli/presets/test_registry.py @@ -1,7 +1,10 @@ """Tests for preset registry persistence and priority in specify_cli.presets._registry.""" +import json + import pytest +from specify_cli.file_helper import FileHelper, SymlinkDeniedError from specify_cli.presets import PresetRegistry @@ -282,6 +285,170 @@ def test_list_by_priority_includes_disabled_when_requested(self, temp_dir): assert pack_ids[0] == "pack-disabled" +class TestPresetRegistrySymlinks: + @pytest.mark.parametrize("relative", [False, True]) + def test_regular_registry_preserves_persistence_and_observers( + self, tmp_path, monkeypatch, relative + ): + project = tmp_path / "project" + project.mkdir() + monkeypatch.chdir(project) + packs_dir = project / ".specify" / "presets" + if relative: + packs_dir = packs_dir.relative_to(project) + registry = PresetRegistry(packs_dir) + observed = [] + monkeypatch.setattr( + "specify_cli.integrations._file_changes.before_file_change", + lambda path: observed.append(("before", path)), + ) + monkeypatch.setattr( + "specify_cli.integrations._file_changes.after_file_change", + lambda path: observed.append(("after", path)), + ) + + registry.add("example", {"label": "caf\u00e9"}) + + assert observed == [ + ("before", packs_dir / ".registry"), + ("after", packs_dir / ".registry"), + ] + assert PresetRegistry(packs_dir).get("example")["label"] == "caf\u00e9" + content = registry.registry_path.read_text(encoding="utf-8") + assert content == json.dumps(registry.data, indent=2) + + @pytest.mark.parametrize("operation", ["read_text", "overwrite_text"]) + def test_io_errors_propagate(self, tmp_path, monkeypatch, operation): + project = tmp_path / "project" + project.mkdir() + packs_dir = project / ".specify" / "presets" + registry = PresetRegistry(packs_dir) + registry.add("example", {"version": "1.0"}) + original = registry.registry_path.read_bytes() + + def fail(*args, **kwargs): + raise PermissionError("registry access denied") + + monkeypatch.setattr(FileHelper, operation, fail) + + with pytest.raises(PermissionError, match="registry access denied"): + if operation == "read_text": + PresetRegistry(packs_dir) + else: + registry.update("example", {"version": "2.0"}) + + assert registry.registry_path.read_bytes() == original + + @pytest.mark.parametrize("linked_entry", ["presets", ".registry"]) + @pytest.mark.parametrize("dangling", [False, True]) + @pytest.mark.parametrize("allow_symlinks", [False, True]) + def test_load_rejects_external_links( + self, tmp_path, linked_entry, dangling, allow_symlinks + ): + project = tmp_path / "project" + packs_dir = project / ".specify" / "presets" + packs_dir.parent.mkdir(parents=True) + (packs_dir.parent / "init-options.json").write_text( + json.dumps({"allow_symlinks": allow_symlinks}), encoding="utf-8" + ) + outside = tmp_path / "outside" + outside.mkdir() + sentinel = outside / ".registry" + sentinel.write_bytes(b"not registry JSON") + target = outside / "missing" if dangling else outside + if linked_entry == ".registry": + packs_dir.mkdir() + link = packs_dir / ".registry" + target = target / ".registry" + else: + link = packs_dir + link.symlink_to(target, target_is_directory=linked_entry == "presets") + + with pytest.raises(SymlinkDeniedError): + PresetRegistry(packs_dir) + + assert sentinel.read_bytes() == b"not registry JSON" + assert link.is_symlink() + assert not (outside / "missing").exists() + + @pytest.mark.parametrize("linked_entry", [".specify", "presets", ".registry"]) + @pytest.mark.parametrize("dangling", [False, True]) + @pytest.mark.parametrize("operation", ["add", "update", "remove", "restore"]) + def test_save_rejects_links_before_observer_or_mutation( + self, tmp_path, monkeypatch, linked_entry, dangling, operation + ): + project = tmp_path / "project" + project.mkdir() + packs_dir = project / ".specify" / "presets" + registry = PresetRegistry(packs_dir) + registry.add("example", {"version": "1.0"}) + outside = tmp_path / "outside" + outside.mkdir() + sentinel = ( + outside / "presets" / ".registry" + if linked_entry == ".specify" + else outside / ".registry" + ) + sentinel.parent.mkdir(parents=True, exist_ok=True) + sentinel.write_bytes(b"do not overwrite") + link = { + ".specify": project / ".specify", + "presets": packs_dir, + ".registry": packs_dir / ".registry", + }[linked_entry] + link.rename(project / "original") + target = outside / "missing" if dangling else outside + if linked_entry == ".registry": + target = target / ".registry" + link.symlink_to(target, target_is_directory=linked_entry != ".registry") + observed = [] + monkeypatch.setattr( + "specify_cli.integrations._file_changes.before_file_change", observed.append + ) + monkeypatch.setattr( + "specify_cli.integrations._file_changes.after_file_change", observed.append + ) + + with pytest.raises(SymlinkDeniedError): + if operation == "remove": + registry.remove("example") + else: + getattr(registry, operation)("example", {"version": "2.0"}) + + assert sentinel.read_bytes() == b"do not overwrite" + assert not (outside / "missing").exists() + assert link.is_symlink() + assert observed == [] + + @pytest.mark.parametrize("linked_entry", ["presets", ".registry"]) + def test_registry_metadata_rejects_even_opted_in_contained_links( + self, tmp_path, linked_entry + ): + project = tmp_path / "project" + project.mkdir() + packs_dir = project / ".specify" / "presets" + registry = PresetRegistry(packs_dir) + registry.add("example", {"version": "1.0"}) + link = packs_dir if linked_entry == "presets" else registry.registry_path + target = project / "contained" + link.rename(target) + link.symlink_to(target, target_is_directory=linked_entry == "presets") + + with pytest.raises(SymlinkDeniedError): + PresetRegistry(packs_dir) + + (project / ".specify" / "init-options.json").write_text( + '{"allow_symlinks": true}', encoding="utf-8" + ) + with pytest.raises(SymlinkDeniedError): + PresetRegistry(packs_dir) + + assert link.is_symlink() + registry_target = target / ".registry" if linked_entry == "presets" else target + saved = json.loads(registry_target.read_text(encoding="utf-8")) + assert saved["presets"]["example"]["version"] == "1.0" + + class TestRegistryPriority: """Test registry priority sorting.""" From 59baec19f8217c0f4d5bb23d54a956a474162b34 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 29 Oct 2026 10:50:50 -0500 Subject: [PATCH 2/4] fix: translate registry path failures in artifact operations Map FileHelperError into the existing artifact resolution errors for shared list/info operations and contribution lookup. Preserve CLI JSON and MCP error envelopes without weakening registry symlink rejection, with before-and-after regressions across both delivery adapters. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 5 +++- src/specify_cli/artifacts/_operation_info.py | 3 +- src/specify_cli/artifacts/_operation_list.py | 3 +- src/specify_cli/artifacts/catalog.py | 3 +- tests/specify_cli/artifacts/conftest.py | 28 +++++++++++++++++++ .../artifacts/test_command_info.py | 14 ++++++++++ .../artifacts/test_command_list.py | 13 +++++++++ .../artifacts/test_command_lookup.py | 15 ++++++++++ tests/specify_cli/artifacts/test_mcp_info.py | 23 +++++++++++++++ tests/specify_cli/artifacts/test_mcp_list.py | 20 +++++++++++++ .../artifacts/test_operation_info.py | 19 +++++++++++++ .../artifacts/test_operation_list.py | 16 +++++++++++ 12 files changed, 158 insertions(+), 4 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index 1c75a86952..04bffaa8cb 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -16,7 +16,10 @@ not be links, independently of the operational `allow_symlinks` setting. Saves validate the destination before file-change observers or directory creation and preserve the existing registry text I/O and metadata behavior. This migration covers registry persistence only, not the remaining preset lifecycle -filesystem operations. +filesystem operations. Shared artifact list/info operations translate registry +`FileHelperError` failures into their existing resolution errors, preserving +the CLI JSON envelope and MCP `artifact_resolution_failed` code. Contribution +lookup likewise translates registry failures into an artifact resolution error. Only `specify init --allow-symlinks` exposes the setting; init defaults to deny and persists the literal JSON boolean `allow_symlinks` in diff --git a/src/specify_cli/artifacts/_operation_info.py b/src/specify_cli/artifacts/_operation_info.py index 9993ec67b2..2e25244e0a 100644 --- a/src/specify_cli/artifacts/_operation_info.py +++ b/src/specify_cli/artifacts/_operation_info.py @@ -7,6 +7,7 @@ from typing import Any, Literal from .._file_policy import ProjectPolicyError, project_files +from ..file_helper import FileHelperError from ..presets import PresetError from . import ( AmbiguousArtifactError, @@ -536,7 +537,7 @@ def get_artifact_info(request: ArtifactInfoRequest) -> ArtifactInfoResult: raise ArtifactInfoResolutionError( project_directory, request.identifier, message=exc.message, ) from exc - except (OSError, PresetError) as exc: + except (FileHelperError, OSError, PresetError) as exc: raise ArtifactInfoResolutionError( project_directory, request.identifier, diff --git a/src/specify_cli/artifacts/_operation_list.py b/src/specify_cli/artifacts/_operation_list.py index 91afd43347..3b40c2ffa2 100644 --- a/src/specify_cli/artifacts/_operation_list.py +++ b/src/specify_cli/artifacts/_operation_list.py @@ -7,6 +7,7 @@ from typing import Any, Literal, TypedDict, cast from .._file_policy import ProjectPolicyError, project_files +from ..file_helper import FileHelperError from ..presets import PresetError from . import ( ArtifactCatalog, @@ -245,7 +246,7 @@ def list_artifacts(request: ArtifactListRequest) -> ArtifactListResult: raise ArtifactListResolutionError(project_directory, message=str(exc)) from exc except NotASpecKitProjectError as exc: raise ArtifactListProjectError(project_directory) from exc - except (ArtifactResolutionError, OSError, PresetError) as exc: + except (ArtifactResolutionError, FileHelperError, OSError, PresetError) as exc: raise ArtifactListResolutionError(project_directory) from exc all_rows = tuple(cast(ArtifactListItem, row) for row in rows) diff --git a/src/specify_cli/artifacts/catalog.py b/src/specify_cli/artifacts/catalog.py index 2780201cbf..3e57649944 100644 --- a/src/specify_cli/artifacts/catalog.py +++ b/src/specify_cli/artifacts/catalog.py @@ -19,6 +19,7 @@ import yaml +from ..file_helper import FileHelperError from ._identifiers import ( IdentifierComponentError, derive_hook_lookup_id, @@ -430,7 +431,7 @@ def get_contribution_info(self, lookup_id: str) -> dict[str, Any]: resolved = self._find_extension_contribution( resolver, source_id, kind, name ) - except (OSError, PresetError) as exc: + except (FileHelperError, OSError, PresetError) as exc: raise ArtifactResolutionError() from exc if resolved is None: raise ContributionNotFoundError(lookup_id) diff --git a/tests/specify_cli/artifacts/conftest.py b/tests/specify_cli/artifacts/conftest.py index 48a493e152..5d1b1385d7 100644 --- a/tests/specify_cli/artifacts/conftest.py +++ b/tests/specify_cli/artifacts/conftest.py @@ -1,5 +1,6 @@ from __future__ import annotations +from collections.abc import Iterator from pathlib import Path import pytest @@ -23,3 +24,30 @@ def non_project(tmp_path: Path) -> Path: root = tmp_path / "not-proj" root.mkdir() return root + + +@pytest.fixture( + params=["presets", ".registry", "dangling-presets", "dangling-registry"] +) +def symlinked_preset_registry( + spec_kit_project: Path, tmp_path: Path, request: pytest.FixtureRequest, +) -> Iterator[Path]: + outside = tmp_path / "outside-presets" + outside.mkdir() + sentinel = outside / ".registry" + sentinel.write_bytes(b"not registry JSON") + packs_dir = spec_kit_project / ".specify" / "presets" + is_directory = request.param.endswith("presets") + link = packs_dir if is_directory else packs_dir / ".registry" + target = outside if is_directory else sentinel + if request.param.startswith("dangling"): + target = outside / "missing" + if is_directory: + packs_dir.rmdir() + link.symlink_to(target, target_is_directory=is_directory) + + yield spec_kit_project + + assert link.is_symlink() + assert sentinel.read_bytes() == b"not registry JSON" + assert not (outside / "missing").exists() diff --git a/tests/specify_cli/artifacts/test_command_info.py b/tests/specify_cli/artifacts/test_command_info.py index 17e02e5a61..7150fee861 100644 --- a/tests/specify_cli/artifacts/test_command_info.py +++ b/tests/specify_cli/artifacts/test_command_info.py @@ -237,6 +237,20 @@ def test_info_corrupt_extension_registry_uses_json_error_envelope( assert result.stdout == "" assert json.loads(result.stderr) == {"error": "artifact resolution failed"} + def test_info_symlinked_registry_uses_json_error_envelope( + self, symlinked_preset_registry: Path, monkeypatch: pytest.MonkeyPatch, + ): + monkeypatch.chdir(symlinked_preset_registry) + + result = CliRunner().invoke( + app, ["artifact", "info", "speckit.plan", "--json"], + catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert result.stdout == "" + assert result.stderr == '{"error": "artifact resolution failed"}\n' + def test_stdout_empty_on_error( self, non_project: Path, monkeypatch: pytest.MonkeyPatch ): diff --git a/tests/specify_cli/artifacts/test_command_list.py b/tests/specify_cli/artifacts/test_command_list.py index bcefd1d768..cb888764fb 100644 --- a/tests/specify_cli/artifacts/test_command_list.py +++ b/tests/specify_cli/artifacts/test_command_list.py @@ -230,6 +230,19 @@ def test_list_corrupt_extension_registry_uses_json_error_envelope( assert result.stderr.count("\n") == 1 assert "\x1b[" not in result.stderr + def test_list_symlinked_registry_uses_json_error_envelope( + self, symlinked_preset_registry: Path, monkeypatch: pytest.MonkeyPatch, + ): + monkeypatch.chdir(symlinked_preset_registry) + + result = CliRunner().invoke( + app, ["artifact", "list", "--json"], catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert result.stdout == "" + assert result.stderr == '{"error": "artifact resolution failed"}\n' + def test_cli_context_filesystem_failure_uses_json_error_envelope( self, monkeypatch: pytest.MonkeyPatch ): diff --git a/tests/specify_cli/artifacts/test_command_lookup.py b/tests/specify_cli/artifacts/test_command_lookup.py index cde4b94ad5..1c2a8a8668 100644 --- a/tests/specify_cli/artifacts/test_command_lookup.py +++ b/tests/specify_cli/artifacts/test_command_lookup.py @@ -117,6 +117,21 @@ def test_lookup_policy_failure_precedes_loader_and_catalog( loader.assert_not_called() catalog.assert_not_called() + def test_lookup_symlinked_registry_uses_json_error_envelope( + self, symlinked_preset_registry: Path, monkeypatch: pytest.MonkeyPatch, + ): + monkeypatch.chdir(symlinked_preset_registry) + + result = CliRunner().invoke( + app, + ["artifact", "lookup", "preset:example:template:example", "--json"], + catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert result.stdout == "" + assert result.stderr == '{"error": "artifact resolution failed"}\n' + def test_lookup_json_rejects_unknown_contribution( self, spec_kit_project: Path, monkeypatch: pytest.MonkeyPatch ): diff --git a/tests/specify_cli/artifacts/test_mcp_info.py b/tests/specify_cli/artifacts/test_mcp_info.py index 8fad3f1944..9ef6e2a80b 100644 --- a/tests/specify_cli/artifacts/test_mcp_info.py +++ b/tests/specify_cli/artifacts/test_mcp_info.py @@ -728,6 +728,29 @@ def test_artifact_info_maps_corrupt_registry_error(spec_kit_project: Path): ) +def test_artifact_info_maps_symlinked_registry_error( + symlinked_preset_registry: Path, +): + result = _run( + create_server(launch_directory=symlinked_preset_registry).call_tool( + TOOL_NAME, {"identifier": "speckit.plan"}, + ) + ) + + assert result.is_error is True + assert result.structured_content == _expected_error( + "artifact_resolution_failed", + "artifact resolution failed", + details={ + "project_directory": str(symlinked_preset_registry), + "identifier": "speckit.plan", + }, + ) + assert result.content[0].text == ( + "artifact_resolution_failed: artifact resolution failed" + ) + + @pytest.mark.parametrize( "failure", [ diff --git a/tests/specify_cli/artifacts/test_mcp_list.py b/tests/specify_cli/artifacts/test_mcp_list.py index 7c57883838..9b2e7f74e9 100644 --- a/tests/specify_cli/artifacts/test_mcp_list.py +++ b/tests/specify_cli/artifacts/test_mcp_list.py @@ -705,6 +705,26 @@ def test_artifact_list_maps_corrupt_registry_error(spec_kit_project: Path): ) +def test_artifact_list_maps_symlinked_registry_error( + symlinked_preset_registry: Path, +): + result = _run( + create_server(launch_directory=symlinked_preset_registry).call_tool( + TOOL_NAME, {}, + ) + ) + + assert result.is_error is True + assert result.structured_content == _expected_error( + "artifact_resolution_failed", + "artifact resolution failed", + details={"project_directory": str(symlinked_preset_registry)}, + ) + assert result.content[0].text == ( + "artifact_resolution_failed: artifact resolution failed" + ) + + @pytest.mark.parametrize( "operation_failure", [ diff --git a/tests/specify_cli/artifacts/test_operation_info.py b/tests/specify_cli/artifacts/test_operation_info.py index 45a5f3b96d..54a869c7ba 100644 --- a/tests/specify_cli/artifacts/test_operation_info.py +++ b/tests/specify_cli/artifacts/test_operation_info.py @@ -32,6 +32,7 @@ get_artifact_info, ) from specify_cli.extensions import ExtensionRegistry +from specify_cli.file_helper import SymlinkDeniedError from specify_cli.presets import PresetError from tests.conftest import install_preset from tests.specify_cli.artifacts.helpers import install_extension_with_hooks @@ -504,6 +505,24 @@ def test_info_maps_corrupt_registry_to_resolution_error( assert exc_info.value.message == "artifact resolution failed" +def test_info_maps_symlinked_registry_to_resolution_error( + symlinked_preset_registry: Path, +): + with pytest.raises(ArtifactInfoResolutionError) as exc_info: + get_artifact_info( + ArtifactInfoRequest(symlinked_preset_registry, "speckit.plan") + ) + + assert exc_info.value.code == "artifact_resolution_failed" + assert exc_info.value.message == "artifact resolution failed" + assert exc_info.value.details == { + "project_directory": str(symlinked_preset_registry), + "identifier": "speckit.plan", + } + assert exc_info.value.retryable is False + assert isinstance(exc_info.value.__cause__, SymlinkDeniedError) + + @pytest.mark.parametrize( "failure", [ diff --git a/tests/specify_cli/artifacts/test_operation_list.py b/tests/specify_cli/artifacts/test_operation_list.py index 52fdb9ea20..166caf67af 100644 --- a/tests/specify_cli/artifacts/test_operation_list.py +++ b/tests/specify_cli/artifacts/test_operation_list.py @@ -22,6 +22,7 @@ list_artifacts, ) from specify_cli.extensions import ExtensionRegistry +from specify_cli.file_helper import SymlinkDeniedError from specify_cli.presets import PresetError @@ -269,6 +270,21 @@ def test_list_artifacts_maps_corrupt_registry_to_resolution_error( assert exc_info.value.retryable is False +def test_list_artifacts_maps_symlinked_registry_to_resolution_error( + symlinked_preset_registry: Path, +): + with pytest.raises(ArtifactListResolutionError) as exc_info: + list_artifacts(ArtifactListRequest(symlinked_preset_registry)) + + assert exc_info.value.code == "artifact_resolution_failed" + assert exc_info.value.message == "artifact resolution failed" + assert exc_info.value.details == { + "project_directory": str(symlinked_preset_registry), + } + assert exc_info.value.retryable is False + assert isinstance(exc_info.value.__cause__, SymlinkDeniedError) + + def test_list_artifacts_maps_preset_failures_to_resolution_error( spec_kit_project: Path, monkeypatch: pytest.MonkeyPatch, From 9d56782092d8373bfe1b077d4117815547f05173 Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 29 Oct 2026 11:23:11 -0500 Subject: [PATCH 3/4] fix: report preset registry policy failures consistently Catch filesystem policy errors across preset CLI invocation, preserving JSON error envelopes and giving registry-specific recovery guidance. Keep dangling preset directories visible to the registry safety checks and propagate composition and replacement policy failures instead of warning or suggesting symlink opt-in. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 5 + docs/reference/presets.md | 8 + src/specify_cli/presets/_commands.py | 29 +++ src/specify_cli/presets/_resolver.py | 6 +- src/specify_cli/presets/command_info.py | 5 +- src/specify_cli/presets/command_list.py | 5 +- src/specify_cli/presets/command_resolve.py | 3 + src/specify_cli/presets/command_update.py | 4 + tests/specify_cli/presets/test_commands.py | 239 +++++++++++++++++++++ tests/specify_cli/presets/test_resolver.py | 20 ++ 10 files changed, 320 insertions(+), 4 deletions(-) create mode 100644 tests/specify_cli/presets/test_commands.py diff --git a/design/file-helper.md b/design/file-helper.md index 04bffaa8cb..ddfb2ecf50 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -20,6 +20,11 @@ filesystem operations. Shared artifact list/info operations translate registry `FileHelperError` failures into their existing resolution errors, preserving the CLI JSON envelope and MCP `artifact_resolution_failed` code. Contribution lookup likewise translates registry failures into an artifact resolution error. +The preset CLI group reports filesystem policy failures across the entire +invocation, including manager/resolver construction. Registry symlink errors +in text and JSON output direct users to replace linked entries with regular +files/directories, not enable operational symlinks. JSON list/info retain a +single error envelope with no stdout output. Only `specify init --allow-symlinks` exposes the setting; init defaults to deny and persists the literal JSON boolean `allow_symlinks` in diff --git a/docs/reference/presets.md b/docs/reference/presets.md index 9975578ce6..3fe657a87d 100644 --- a/docs/reference/presets.md +++ b/docs/reference/presets.md @@ -45,6 +45,14 @@ may omit the digest. > **Note:** All preset commands require a project already initialized with `specify init`. +The preset registry at `.specify/presets/.registry` and its parent directories +must not be symlinks, including dangling links. This metadata restriction +applies even when `allow_symlinks` is enabled for project operations. Replace +linked registry entries with regular files or directories; enabling operational +symlinks does not make registry links usable. Registry access failures exit 1 +with a human-readable diagnostic, or a single JSON error on stderr for +`preset list --json` and `preset info --json`. + ### Exclude development files with `.presetignore` Place a UTF-8 `.presetignore` (with or without a byte-order mark) beside diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index b419293dbd..1a63f48f73 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -3,16 +3,45 @@ from __future__ import annotations import typer +from rich.markup import escape as _escape_markup +from typer.core import TyperGroup from .._console import console from .._download_security import read_response_limited as _read_response_limited +from ..file_helper import FileHelperError, SymlinkDeniedError read_response_limited = _read_response_limited + +def _registry_file_error(error: FileHelperError) -> ValueError: + """Translate filesystem policy failures without suggesting registry opt-in.""" + if isinstance(error, SymlinkDeniedError): + return ValueError( + "Cannot access preset registry: registry files and parent directories " + "must not be symlinks. Replace linked entries with regular files or " + "directories; allow_symlinks does not apply to preset registry metadata." + ) + return ValueError(f"Cannot access preset registry: {error}") + + +class _PresetGroup(TyperGroup): + """Report filesystem policy failures across the entire preset invocation.""" + + def invoke(self, ctx): + try: + return super().invoke(ctx) + except FileHelperError as error: + console.print( + f"[red]Error:[/red] {_escape_markup(str(_registry_file_error(error)))}" + ) + raise typer.Exit(1) from error + + preset_app = typer.Typer( name="preset", help="Manage spec-kit presets", add_completion=False, + cls=_PresetGroup, ) # Lowest priority a user may request. Lower numbers win resolution, so the diff --git a/src/specify_cli/presets/_resolver.py b/src/specify_cli/presets/_resolver.py index 5700fa8378..515ccd7f30 100644 --- a/src/specify_cli/presets/_resolver.py +++ b/src/specify_cli/presets/_resolver.py @@ -263,7 +263,9 @@ def resolve( return override # Priority 2: Installed presets (sorted by priority — lower number wins) - if not skip_presets and self.presets_dir.exists(): + if not skip_presets and ( + self.presets_dir.exists() or self.presets_dir.is_symlink() + ): for pack_id, _metadata in self._get_all_presets_by_priority(): pack_dir = self.presets_dir / pack_id # The preset manifest is authoritative: if it declares this @@ -559,7 +561,7 @@ def _find_in_subdirs(base_dir: Path) -> Optional[Path]: }) # Priority 2: Installed presets (sorted by priority — lower number = higher precedence) - if self.presets_dir.exists(): + if self.presets_dir.exists() or self.presets_dir.is_symlink(): for pack_id, metadata in self._get_all_presets_by_priority(): pack_dir = self.presets_dir / pack_id # Read strategy and manifest file path from preset manifest diff --git a/src/specify_cli/presets/command_info.py b/src/specify_cli/presets/command_info.py index b6b7b2848f..ef24c1fe60 100644 --- a/src/specify_cli/presets/command_info.py +++ b/src/specify_cli/presets/command_info.py @@ -11,7 +11,8 @@ from .._installed_info_json import preset_info_item from .._installed_list_json import InstalledListJSONCommand, emit_json, emit_json_error from .._project import resolve_specify_project_root -from ._commands import preset_app +from ..file_helper import FileHelperError +from ._commands import _registry_file_error, preset_app @preset_app.command("info", cls=InstalledListJSONCommand) @@ -36,6 +37,8 @@ def preset_info( manager = PresetManager(resolve_specify_project_root()) emit_json(preset_info_item(manager.list_installed(), manager, preset_id)) return + except FileHelperError as error: + emit_json_error(_registry_file_error(error)) except Exception as error: # noqa: BLE001 - emit the JSON error contract emit_json_error(error) diff --git a/src/specify_cli/presets/command_list.py b/src/specify_cli/presets/command_list.py index f7fd72efe3..2b0eb15832 100644 --- a/src/specify_cli/presets/command_list.py +++ b/src/specify_cli/presets/command_list.py @@ -13,7 +13,8 @@ installed_list_item, ) from .._project import resolve_specify_project_root -from ._commands import preset_app +from ..file_helper import FileHelperError +from ._commands import _registry_file_error, preset_app @preset_app.command("list", cls=InstalledListJSONCommand) @@ -39,6 +40,8 @@ def preset_list( [installed_list_item(pack, include_hooks=False) for pack in installed] ) return + except FileHelperError as error: + emit_json_error(_registry_file_error(error)) except Exception as error: # noqa: BLE001 - emit the JSON error contract emit_json_error(error) diff --git a/src/specify_cli/presets/command_resolve.py b/src/specify_cli/presets/command_resolve.py index 7256307160..ff2b46d945 100644 --- a/src/specify_cli/presets/command_resolve.py +++ b/src/specify_cli/presets/command_resolve.py @@ -8,6 +8,7 @@ from rich.markup import escape as _escape_markup from .._console import console +from ..file_helper import FileHelperError from ._commands import preset_app @@ -63,6 +64,8 @@ def preset_resolve( # Verify composition is actually possible try: composed = resolver.resolve_content(template_name, template_type) + except FileHelperError: + raise except Exception as exc: # noqa: BLE001 - render composition failures composed = None console.print( diff --git a/src/specify_cli/presets/command_update.py b/src/specify_cli/presets/command_update.py index cde9b256bb..0c4b0484dd 100644 --- a/src/specify_cli/presets/command_update.py +++ b/src/specify_cli/presets/command_update.py @@ -9,6 +9,7 @@ from rich.markup import escape as _escape_markup from .._console import console +from ..file_helper import FileHelperError from . import _commands from ._commands import preset_app @@ -90,6 +91,9 @@ def report_add_failure() -> None: dev=dev, priority=priority, ) + except FileHelperError: + report_add_failure() + raise except typer.Exit as error: report_add_failure() raise typer.Exit(error.exit_code or 1) diff --git a/tests/specify_cli/presets/test_commands.py b/tests/specify_cli/presets/test_commands.py new file mode 100644 index 0000000000..bbc36da47e --- /dev/null +++ b/tests/specify_cli/presets/test_commands.py @@ -0,0 +1,239 @@ +"""Shared error-boundary regressions for the preset CLI group.""" + +from __future__ import annotations + +import json +from collections.abc import Iterator +from pathlib import Path + +import pytest +from typer.testing import CliRunner + +from specify_cli import app +from specify_cli.file_helper import FileHelperError, SymlinkDeniedError +from specify_cli.presets import PresetManager, PresetRegistry, PresetResolver +from tests.conftest import strip_ansi + + +REGISTRY_SYMLINK_ERROR = ( + "Cannot access preset registry: registry files and parent directories must " + "not be symlinks. Replace linked entries with regular files or directories; " + "allow_symlinks does not apply to preset registry metadata." +) + + +@pytest.fixture(params=["directory", "file", "dangling-directory", "dangling-file"]) +def linked_registry(tmp_path: Path, request: pytest.FixtureRequest) -> Iterator[Path]: + project = tmp_path / "project" + presets = project / ".specify" / "presets" + presets.mkdir(parents=True) + outside = tmp_path / "outside" + outside.mkdir() + sentinel = outside / ".registry" + sentinel.write_bytes(b"not registry JSON") + is_directory = request.param.endswith("directory") + link = presets if is_directory else presets / ".registry" + target = outside if is_directory else sentinel + if request.param.startswith("dangling"): + target = outside / "missing" + if is_directory: + presets.rmdir() + try: + link.symlink_to(target, target_is_directory=is_directory) + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") + + yield project + + assert link.is_symlink() + assert sentinel.read_bytes() == b"not registry JSON" + assert not (outside / "missing").exists() + + +@pytest.mark.parametrize( + "arguments", + [ + ["list"], + ["info", "example"], + ["resolve", "spec-template"], + ["add", "example"], + ["remove", "example"], + ["update", "example"], + ["enable", "example"], + ["disable", "example"], + ["set-priority", "example", "5"], + ], +) +@pytest.mark.parametrize("allow_symlinks", [False, True]) +def test_text_commands_report_registry_specific_symlink_error( + linked_registry: Path, + monkeypatch: pytest.MonkeyPatch, + arguments: list[str], + allow_symlinks: bool, +): + (linked_registry / ".specify" / "init-options.json").write_text( + json.dumps({"allow_symlinks": allow_symlinks}), encoding="utf-8", + ) + monkeypatch.chdir(linked_registry) + + result = CliRunner().invoke( + app, ["preset", *arguments], catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert " ".join(strip_ansi(result.stdout).split()) == ( + f"Error: {REGISTRY_SYMLINK_ERROR}" + ) + assert result.stderr == "" + + +@pytest.mark.parametrize("arguments", [["list"], ["info", "example"]]) +@pytest.mark.parametrize("allow_symlinks", [False, True]) +def test_json_commands_report_registry_specific_symlink_error( + linked_registry: Path, + monkeypatch: pytest.MonkeyPatch, + arguments: list[str], + allow_symlinks: bool, +): + (linked_registry / ".specify" / "init-options.json").write_text( + json.dumps({"allow_symlinks": allow_symlinks}), encoding="utf-8", + ) + monkeypatch.chdir(linked_registry) + + result = CliRunner().invoke( + app, ["preset", *arguments, "--json"], catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert result.stdout == "" + assert json.loads(result.stderr) == {"error": REGISTRY_SYMLINK_ERROR} + assert result.stderr.count("\n") == 1 + + +@pytest.mark.parametrize( + "command", ["add", "remove", "update", "enable", "disable", "set-priority"], +) +def test_mutating_commands_handle_registry_save_errors( + project_dir: Path, pack_dir: Path, monkeypatch: pytest.MonkeyPatch, command: str, +): + if command != "add": + manager = PresetManager(project_dir) + manager.install_from_directory(pack_dir, "99.0.0") + if command == "enable": + manager.registry.update("test-pack", {"enabled": False}) + + def fail_save(self): + raise SymlinkDeniedError( + "Refusing symlink; explicitly allow symlinks on this FileHelper" + ) + + monkeypatch.setattr(PresetRegistry, "_save", fail_save) + monkeypatch.chdir(project_dir) + arguments = ["preset", command, "test-pack"] + if command == "add": + arguments = ["preset", "add", "--dev", str(pack_dir)] + elif command == "set-priority": + arguments.append("5") + + result = CliRunner().invoke(app, arguments, catch_exceptions=False) + + assert result.exit_code == 1 + output = " ".join(strip_ansi(result.stdout).split()) + assert REGISTRY_SYMLINK_ERROR in output + assert "explicitly allow symlinks" not in output + assert "Traceback" not in result.output + + +@pytest.mark.parametrize("phase", ["construction", "composition"]) +def test_resolve_handles_filesystem_errors_at_each_phase( + project_dir: Path, monkeypatch: pytest.MonkeyPatch, phase: str, +): + def fail(*args, **kwargs): + raise SymlinkDeniedError("explicitly allow symlinks") + + if phase == "construction": + monkeypatch.setattr("specify_cli.presets.PresetResolver", fail) + else: + monkeypatch.setattr( + PresetResolver, "collect_all_layers", + lambda *args: [ + {"path": Path("top.md"), "source": "preset:example", "strategy": "append"}, + ], + ) + monkeypatch.setattr(PresetResolver, "resolve_content", fail) + monkeypatch.chdir(project_dir) + + result = CliRunner().invoke( + app, ["preset", "resolve", "spec-template"], catch_exceptions=False, + ) + + assert result.exit_code == 1 + assert REGISTRY_SYMLINK_ERROR in " ".join(strip_ansi(result.stdout).split()) + assert "explicitly allow symlinks" not in result.output + assert "composition error" not in result.output + + +def test_update_replacement_policy_error_preserves_removal_context( + project_dir: Path, monkeypatch: pytest.MonkeyPatch, +): + PresetRegistry(project_dir / ".specify" / "presets").restore( + "example", {"version": "1.0", "enabled": True}, + ) + + def fail_add(**kwargs): + raise SymlinkDeniedError("explicitly allow symlinks") + + monkeypatch.setattr( + "specify_cli.presets._commands.preset_remove", lambda preset_id: None, + ) + monkeypatch.setattr("specify_cli.presets._commands.preset_add", fail_add) + monkeypatch.chdir(project_dir) + + result = CliRunner().invoke( + app, ["preset", "update", "example"], catch_exceptions=False, + ) + + assert result.exit_code == 1 + output = " ".join(strip_ansi(result.stdout).split()) + assert "previous preset was removed" in output + assert "specify preset add" in output + assert REGISTRY_SYMLINK_ERROR in output + assert "explicitly allow symlinks" not in output + + +@pytest.mark.parametrize("json_output", [False, True]) +def test_other_file_helper_errors_keep_their_diagnostics( + project_dir: Path, monkeypatch: pytest.MonkeyPatch, json_output: bool, +): + def fail(*args, **kwargs): + raise FileHelperError("unsupported registry path [red]") + + monkeypatch.setattr("specify_cli.presets.PresetManager", fail) + monkeypatch.chdir(project_dir) + arguments = ["preset", "list"] + if json_output: + arguments.append("--json") + + result = CliRunner().invoke(app, arguments, catch_exceptions=False) + + assert result.exit_code == 1 + message = "Cannot access preset registry: unsupported registry path [red]" + if json_output: + assert result.stdout == "" + assert json.loads(result.stderr) == {"error": message} + else: + assert " ".join(strip_ansi(result.stdout).split()) == f"Error: {message}" + assert result.stderr == "" + + +def test_preset_error_boundary_does_not_hide_unexpected_errors( + project_dir: Path, monkeypatch: pytest.MonkeyPatch, +): + def fail(*args, **kwargs): + raise RuntimeError("unexpected failure") + + monkeypatch.setattr("specify_cli.presets.PresetManager", fail) + monkeypatch.chdir(project_dir) + + with pytest.raises(RuntimeError, match="unexpected failure"): + CliRunner().invoke(app, ["preset", "list"], catch_exceptions=False) diff --git a/tests/specify_cli/presets/test_resolver.py b/tests/specify_cli/presets/test_resolver.py index 5c62cb9c9f..364f225eaa 100644 --- a/tests/specify_cli/presets/test_resolver.py +++ b/tests/specify_cli/presets/test_resolver.py @@ -7,6 +7,7 @@ import yaml from specify_cli.extensions import ExtensionRegistry +from specify_cli.file_helper import SymlinkDeniedError from specify_cli.presets import ( PresetManager, PresetRegistry, @@ -22,6 +23,25 @@ class TestPresetResolver: """Test PresetResolver priority stack.""" + @pytest.mark.parametrize( + "method", ["resolve", "resolve_with_source", "collect_all_layers", "resolve_content"], + ) + def test_dangling_preset_directory_is_not_treated_as_missing( + self, project_dir, method, + ): + presets = project_dir / ".specify" / "presets" + target = project_dir.parent / "missing-presets" + try: + presets.symlink_to(target, target_is_directory=True) + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") + + with pytest.raises(SymlinkDeniedError): + getattr(PresetResolver(project_dir), method)("spec-template") + + assert presets.is_symlink() + assert not target.exists() + def test_resolve_core_template(self, project_dir): """Test resolving a core template.""" resolver = PresetResolver(project_dir) From 1dd17e4f9e0dfadefc841a42d88ee6b5c2b910bf Mon Sep 17 00:00:00 2001 From: Manfred Riem <15701806+mnriem@users.noreply.github.com> Date: Thu, 29 Oct 2026 11:53:53 -0500 Subject: [PATCH 4/4] fix: fail closed on registry links during integration upgrades Reuse the deny-mode registry reader for integration layout guards while retaining strict ownership validation. Provide path-preserving registry symlink diagnostics at the persistence boundary for CLI, integration warnings, and bundles. Guard symlink test setup on unsupported hosts and cover migration rejection before mutation. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --- design/file-helper.md | 19 +++-- docs/integrations/cli.md | 8 ++ .../integrations/_command_upgrade_layout.py | 14 ++-- src/specify_cli/presets/_commands.py | 15 +--- src/specify_cli/presets/_registry.py | 37 ++++++--- src/specify_cli/presets/command_info.py | 5 +- src/specify_cli/presets/command_list.py | 5 +- tests/specify_cli/artifacts/conftest.py | 5 +- tests/specify_cli/bundles/test_primitives.py | 21 +++++ .../integrations/test_command_upgrade.py | 61 +++++++++++++++ .../test_command_upgrade_layout.py | 76 +++++++++++++++++++ .../integrations/test_command_use.py | 26 +++++++ tests/specify_cli/presets/test_commands.py | 32 ++++---- tests/specify_cli/presets/test_registry.py | 49 ++++++++++-- 14 files changed, 308 insertions(+), 65 deletions(-) diff --git a/design/file-helper.md b/design/file-helper.md index ddfb2ecf50..687cc0439a 100644 --- a/design/file-helper.md +++ b/design/file-helper.md @@ -20,11 +20,20 @@ filesystem operations. Shared artifact list/info operations translate registry `FileHelperError` failures into their existing resolution errors, preserving the CLI JSON envelope and MCP `artifact_resolution_failed` code. Contribution lookup likewise translates registry failures into an artifact resolution error. -The preset CLI group reports filesystem policy failures across the entire -invocation, including manager/resolver construction. Registry symlink errors -in text and JSON output direct users to replace linked entries with regular -files/directories, not enable operational symlinks. JSON list/info retain a -single error envelope with no stdout output. +Registry reads and writes translate `SymlinkDeniedError` at the persistence +boundary, preserving the registry path and directing users to replace linked +entries with regular files/directories, not enable operational symlinks. +The preset CLI renders that diagnostic without rewriting it, including for +manager/resolver construction failures; integration registration warnings and +bundle errors receive the same guidance. JSON list/info retain a single error +envelope with no stdout output. + +Integration upgrade layout guards reuse the same deny-mode registry text +reader, while retaining their stricter ownership/JSON validation. Only genuine +absence means no installed presets. Symlinked files or parents, including +dangling links, and other read/parse failures become +`_PresetRegistryUnreadableError`, refusing command-root or command/skills +migrations before mutation while preset ownership is unknown. Only `specify init --allow-symlinks` exposes the setting; init defaults to deny and persists the literal JSON boolean `allow_symlinks` in diff --git a/docs/integrations/cli.md b/docs/integrations/cli.md index c320a0f5e6..c0192b3625 100644 --- a/docs/integrations/cli.md +++ b/docs/integrations/cli.md @@ -275,6 +275,14 @@ persisted only after the upgrade regenerates the managed files successfully. If an upgrade would change an integration between command and skills layouts while preset artifacts are registered for it, the upgrade is rejected before changing files. Remove the affected presets, run the layout-changing upgrade, then reinstall them. +Command-root and command/skills migrations also refuse to proceed when preset +ownership cannot be determined. A symlinked `.specify/presets/.registry` or +parent directory, including a dangling link, is an unreadable registry rather +than evidence that no presets are installed. This restriction applies even +with `--force` and `allow_symlinks` enabled. Replace linked entries with regular +files or directories and restore a readable, valid registry before retrying. +A genuinely absent registry or a valid empty registry does not block migration. + For external adapters, forced upgrade or uninstall can recover damaged or unloadable packages using user-local ownership records. Without that proof, old-only generated files are preserved with a manual-cleanup warning. Recovery diff --git a/src/specify_cli/integrations/_command_upgrade_layout.py b/src/specify_cli/integrations/_command_upgrade_layout.py index caf4ba52d1..c94b234856 100644 --- a/src/specify_cli/integrations/_command_upgrade_layout.py +++ b/src/specify_cli/integrations/_command_upgrade_layout.py @@ -112,19 +112,17 @@ def _installed_presets_affecting_agent( preset-overridden files while their registry state can't be reconciled — the exact inconsistency the guard exists to prevent. """ + from ..file_helper import FileHelper from ..presets import PresetRegistry + from ..presets._registry import _read_registry_text registry_path = ( - Path(project_root) / ".specify" / "presets" / PresetRegistry.REGISTRY_FILE + Path(project_root).absolute() / ".specify" / "presets" / PresetRegistry.REGISTRY_FILE ) - # Genuinely absent registry → no presets installed → safe to proceed. - if not registry_path.exists(): - return [] - - # The registry exists: any failure to read or parse it must surface as an - # error, not be swallowed into an empty ("no presets") result. try: - data = json.loads(registry_path.read_text(encoding="utf-8")) + data = json.loads(_read_registry_text(FileHelper(project_root), registry_path)) + except FileNotFoundError: + return [] except (OSError, ValueError) as exc: raise _PresetRegistryUnreadableError(str(exc)) from exc if not isinstance(data, dict) or not isinstance(data.get("presets", {}), dict): diff --git a/src/specify_cli/presets/_commands.py b/src/specify_cli/presets/_commands.py index 1a63f48f73..854dc81cbc 100644 --- a/src/specify_cli/presets/_commands.py +++ b/src/specify_cli/presets/_commands.py @@ -8,22 +8,11 @@ from .._console import console from .._download_security import read_response_limited as _read_response_limited -from ..file_helper import FileHelperError, SymlinkDeniedError +from ..file_helper import FileHelperError read_response_limited = _read_response_limited -def _registry_file_error(error: FileHelperError) -> ValueError: - """Translate filesystem policy failures without suggesting registry opt-in.""" - if isinstance(error, SymlinkDeniedError): - return ValueError( - "Cannot access preset registry: registry files and parent directories " - "must not be symlinks. Replace linked entries with regular files or " - "directories; allow_symlinks does not apply to preset registry metadata." - ) - return ValueError(f"Cannot access preset registry: {error}") - - class _PresetGroup(TyperGroup): """Report filesystem policy failures across the entire preset invocation.""" @@ -32,7 +21,7 @@ def invoke(self, ctx): return super().invoke(ctx) except FileHelperError as error: console.print( - f"[red]Error:[/red] {_escape_markup(str(_registry_file_error(error)))}" + f"[red]Error:[/red] {_escape_markup(str(error))}", soft_wrap=True, ) raise typer.Exit(1) from error diff --git a/src/specify_cli/presets/_registry.py b/src/specify_cli/presets/_registry.py index 3bc189c80b..d0c448ebe4 100644 --- a/src/specify_cli/presets/_registry.py +++ b/src/specify_cli/presets/_registry.py @@ -7,7 +7,23 @@ from typing import Dict, List, Optional from ..extensions import normalize_priority -from ..file_helper import FileHelper +from ..file_helper import FileHelper, SymlinkDeniedError + + +def _registry_symlink_error(registry_path: Path) -> SymlinkDeniedError: + return SymlinkDeniedError( + f"Cannot access preset registry {registry_path}: registry files and parent " + "directories must not be symlinks. Replace linked entries with regular " + "files or directories; allow_symlinks does not apply to preset registry metadata." + ) + + +def _read_registry_text(files: FileHelper, registry_path: Path) -> str: + """Read registry text under the caller's root without following links.""" + try: + return files.read_text(registry_path, encoding="utf-8") + except SymlinkDeniedError as error: + raise _registry_symlink_error(registry_path) from error class PresetRegistry: @@ -32,7 +48,7 @@ def _load(self) -> dict: """Load registry from disk.""" try: data = json.loads( - self._files.read_text(self.registry_path.absolute(), encoding="utf-8") + _read_registry_text(self._files, self.registry_path.absolute()) ) # Validate loaded data is a dict (handles corrupted registry files) if not isinstance(data, dict): @@ -61,13 +77,16 @@ def _save(self): path = self.registry_path.absolute() try: - self._files.read_bytes(path) - except FileNotFoundError: - pass - before_file_change(self.registry_path) - self._files.mkdir(self.packs_dir.absolute(), parents=True, exist_ok=True) - self._files.overwrite_text(path, json.dumps(self.data, indent=2), encoding="utf-8") - after_file_change(self.registry_path) + try: + self._files.read_bytes(path) + except FileNotFoundError: + pass + before_file_change(self.registry_path) + self._files.mkdir(self.packs_dir.absolute(), parents=True, exist_ok=True) + self._files.overwrite_text(path, json.dumps(self.data, indent=2), encoding="utf-8") + after_file_change(self.registry_path) + except SymlinkDeniedError as error: + raise _registry_symlink_error(path) from error def add(self, pack_id: str, metadata: dict): """Add preset to registry. diff --git a/src/specify_cli/presets/command_info.py b/src/specify_cli/presets/command_info.py index ef24c1fe60..b6b7b2848f 100644 --- a/src/specify_cli/presets/command_info.py +++ b/src/specify_cli/presets/command_info.py @@ -11,8 +11,7 @@ from .._installed_info_json import preset_info_item from .._installed_list_json import InstalledListJSONCommand, emit_json, emit_json_error from .._project import resolve_specify_project_root -from ..file_helper import FileHelperError -from ._commands import _registry_file_error, preset_app +from ._commands import preset_app @preset_app.command("info", cls=InstalledListJSONCommand) @@ -37,8 +36,6 @@ def preset_info( manager = PresetManager(resolve_specify_project_root()) emit_json(preset_info_item(manager.list_installed(), manager, preset_id)) return - except FileHelperError as error: - emit_json_error(_registry_file_error(error)) except Exception as error: # noqa: BLE001 - emit the JSON error contract emit_json_error(error) diff --git a/src/specify_cli/presets/command_list.py b/src/specify_cli/presets/command_list.py index 2b0eb15832..f7fd72efe3 100644 --- a/src/specify_cli/presets/command_list.py +++ b/src/specify_cli/presets/command_list.py @@ -13,8 +13,7 @@ installed_list_item, ) from .._project import resolve_specify_project_root -from ..file_helper import FileHelperError -from ._commands import _registry_file_error, preset_app +from ._commands import preset_app @preset_app.command("list", cls=InstalledListJSONCommand) @@ -40,8 +39,6 @@ def preset_list( [installed_list_item(pack, include_hooks=False) for pack in installed] ) return - except FileHelperError as error: - emit_json_error(_registry_file_error(error)) except Exception as error: # noqa: BLE001 - emit the JSON error contract emit_json_error(error) diff --git a/tests/specify_cli/artifacts/conftest.py b/tests/specify_cli/artifacts/conftest.py index 5d1b1385d7..3f26df9a8e 100644 --- a/tests/specify_cli/artifacts/conftest.py +++ b/tests/specify_cli/artifacts/conftest.py @@ -44,7 +44,10 @@ def symlinked_preset_registry( target = outside / "missing" if is_directory: packs_dir.rmdir() - link.symlink_to(target, target_is_directory=is_directory) + try: + link.symlink_to(target, target_is_directory=is_directory) + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") yield spec_kit_project diff --git a/tests/specify_cli/bundles/test_primitives.py b/tests/specify_cli/bundles/test_primitives.py index dfb0788a20..8eca4b13fb 100644 --- a/tests/specify_cli/bundles/test_primitives.py +++ b/tests/specify_cli/bundles/test_primitives.py @@ -23,6 +23,7 @@ primitive_manager, ) from specify_cli.extensions import ExtensionRegistry +from specify_cli.file_helper import SymlinkDeniedError from specify_cli.presets import PresetRegistry from specify_cli.workflows.catalog import StepRegistry, WorkflowRegistry from tests.specify_cli.bundles.helpers import valid_manifest_dict @@ -85,6 +86,26 @@ def test_primitive_manager_rejects_unknown_kind(tmp_path: Path): primitive_manager("bogus", tmp_path) +def test_preset_manager_error_preserves_registry_path_and_guidance(tmp_path: Path): + presets = tmp_path / ".specify" / "presets" + presets.mkdir(parents=True) + registry = presets / ".registry" + target = tmp_path / "missing-registry" + try: + registry.symlink_to(target) + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") + + with pytest.raises(SymlinkDeniedError) as exc_info: + primitive_manager("presets", tmp_path) + + assert str(registry) in str(exc_info.value) + assert "Replace linked entries" in str(exc_info.value) + assert "explicitly allow symlinks" not in str(exc_info.value) + assert registry.is_symlink() + assert not target.exists() + + def test_offline_preset_not_bundled_refuses(tmp_path: Path): manager = primitive_manager("presets", tmp_path, allow_network=False) with pytest.raises(BundlerError, match="network access is disabled"): diff --git a/tests/specify_cli/integrations/test_command_upgrade.py b/tests/specify_cli/integrations/test_command_upgrade.py index b6c3b0690a..1e610b3f7d 100644 --- a/tests/specify_cli/integrations/test_command_upgrade.py +++ b/tests/specify_cli/integrations/test_command_upgrade.py @@ -27,6 +27,67 @@ ) class TestIntegrationUpgradeDetailed: + @pytest.mark.parametrize("agent", ["bob", "kilocode"]) + @pytest.mark.parametrize("entry", ["presets", ".registry"]) + @pytest.mark.parametrize("dangling", [False, True]) + def test_layout_migration_rejects_registry_links_before_mutation( + self, tmp_path, agent, entry, dangling, + ): + if agent == "bob": + project = _init_project( + tmp_path, agent, integration_options="--legacy-commands", + ) + old_root = project / ".bob" / "commands" + new_root = project / ".bob" / "skills" + options = ["--integration-options", "--skills"] + else: + project = _init_project(tmp_path, agent) + new_root, old_root = _move_kilocode_install_to_legacy_layout(project) + options = [] + commands = {path: path.read_bytes() for path in old_root.glob("speckit.*.md")} + assert commands + manifest = project / ".specify" / "integrations" / f"{agent}.manifest.json" + manifest_before = manifest.read_bytes() + config = project / ".specify" / "init-options.json" + options_data = json.loads(config.read_text(encoding="utf-8")) + options_data["allow_symlinks"] = True + config.write_text(json.dumps(options_data), encoding="utf-8") + config_before = config.read_bytes() + presets = project / ".specify" / "presets" + presets.mkdir(parents=True, exist_ok=True) + outside = tmp_path / "outside" + outside.mkdir() + sentinel = outside / ".registry" + sentinel.write_text('{"presets": {}}', encoding="utf-8") + link = presets if entry == "presets" else presets / ".registry" + if entry == "presets": + presets.rename(project / "original-presets") + else: + link.unlink(missing_ok=True) + target = outside / "missing" if dangling else ( + outside if entry == "presets" else sentinel + ) + try: + link.symlink_to(target, target_is_directory=entry == "presets") + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") + + result = _run_in_project( + project, ["integration", "upgrade", agent, *options, "--script", "sh", "--force"], + ) + + assert result.exit_code == 1, result.output + assert "preset registry" in result.output.lower() + assert "Replace linked entries" in result.output + assert "explicitly allow symlinks" not in result.output + assert not new_root.exists() + assert manifest.read_bytes() == manifest_before + assert config.read_bytes() == config_before + assert all(path.read_bytes() == content for path, content in commands.items()) + assert sentinel.read_text(encoding="utf-8") == '{"presets": {}}' + assert link.is_symlink() + assert not (outside / "missing").exists() + @pytest.mark.parametrize( "bad_handler", [ diff --git a/tests/specify_cli/integrations/test_command_upgrade_layout.py b/tests/specify_cli/integrations/test_command_upgrade_layout.py index 671be65ac3..982b7674da 100644 --- a/tests/specify_cli/integrations/test_command_upgrade_layout.py +++ b/tests/specify_cli/integrations/test_command_upgrade_layout.py @@ -6,6 +6,82 @@ class TestIntegrationUpgradeLayout: + @pytest.mark.parametrize("entry", [".specify", "presets", ".registry"]) + @pytest.mark.parametrize("dangling", [False, True]) + @pytest.mark.parametrize("include_skills", [False, True]) + def test_preset_registry_links_fail_closed( + self, tmp_path, entry, dangling, include_skills, + ): + from specify_cli.file_helper import SymlinkDeniedError + from specify_cli.integrations._command_upgrade_layout import ( + _PresetRegistryUnreadableError, + _installed_presets_affecting_agent, + ) + + project = tmp_path / "project" + presets = project / ".specify" / "presets" + presets.mkdir(parents=True) + (project / ".specify" / "init-options.json").write_text( + '{"allow_symlinks": true}', encoding="utf-8", + ) + outside = tmp_path / "outside" + outside.mkdir() + sentinel = ( + outside / "presets" / ".registry" + if entry == ".specify" + else outside / ".registry" + ) + sentinel.parent.mkdir(parents=True, exist_ok=True) + content = '{"presets": {}}' + sentinel.write_text(content, encoding="utf-8") + link = { + ".specify": project / ".specify", + "presets": presets, + ".registry": presets / ".registry", + }[entry] + if entry != ".registry": + link.rename(project / "original") + target = outside / "missing" if dangling else ( + sentinel if entry == ".registry" else outside + ) + try: + link.symlink_to(target, target_is_directory=entry != ".registry") + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") + + with pytest.raises(_PresetRegistryUnreadableError) as exc_info: + _installed_presets_affecting_agent( + project, "bob", include_skills=include_skills, + ) + + assert isinstance(exc_info.value.__cause__, SymlinkDeniedError) + assert str(presets / ".registry") in str(exc_info.value) + assert "Replace linked entries" in str(exc_info.value) + assert "explicitly allow symlinks" not in str(exc_info.value) + assert sentinel.read_text(encoding="utf-8") == content + assert link.is_symlink() + assert not (outside / "missing").exists() + + def test_preset_registry_permission_error_is_not_treated_as_absence( + self, tmp_path, monkeypatch, + ): + from specify_cli.file_helper import FileHelper + from specify_cli.integrations._command_upgrade_layout import ( + _PresetRegistryUnreadableError, + _installed_presets_affecting_agent, + ) + + def fail(*args, **kwargs): + raise PermissionError("registry access denied") + + monkeypatch.setattr(FileHelper, "read_text", fail) + + with pytest.raises(_PresetRegistryUnreadableError) as exc_info: + _installed_presets_affecting_agent(tmp_path, "bob") + + assert isinstance(exc_info.value.__cause__, PermissionError) + assert "registry access denied" in str(exc_info.value) + def test_installed_presets_affecting_agent_absent_vs_unreadable(self, tmp_path): """Unit (review #3415, 4744636079): fail closed only when unreadable. diff --git a/tests/specify_cli/integrations/test_command_use.py b/tests/specify_cli/integrations/test_command_use.py index ad47da7d44..aad41aea33 100644 --- a/tests/specify_cli/integrations/test_command_use.py +++ b/tests/specify_cli/integrations/test_command_use.py @@ -23,6 +23,32 @@ ) class TestIntegrationUse: + def test_preset_registration_warning_preserves_registry_path_and_guidance( + self, tmp_path, capsys, + ): + from specify_cli.integrations._helpers import _register_presets_for_agent + + project = tmp_path / "project" + presets = project / ".specify" / "presets" + presets.mkdir(parents=True) + target = tmp_path / "missing-registry" + registry = presets / ".registry" + try: + registry.symlink_to(target) + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") + + _register_presets_for_agent(project, "copilot", continuing="continuing") + + captured = capsys.readouterr() + output = strip_ansi(captured.out + captured.err) + assert str(registry) in "".join(output.split()) + plain = " ".join(output.split()) + assert "Replace linked entries" in plain + assert "explicitly allow symlinks" not in plain + assert registry.is_symlink() + assert not target.exists() + @pytest.mark.parametrize("edited_command", [None, "plan", "tasks"]) def test_use_syncs_manifest_without_blessing_user_edits( self, tmp_path, tasks_preset_src, edited_command diff --git a/tests/specify_cli/presets/test_commands.py b/tests/specify_cli/presets/test_commands.py index bbc36da47e..307b335ad6 100644 --- a/tests/specify_cli/presets/test_commands.py +++ b/tests/specify_cli/presets/test_commands.py @@ -15,11 +15,13 @@ from tests.conftest import strip_ansi -REGISTRY_SYMLINK_ERROR = ( - "Cannot access preset registry: registry files and parent directories must " - "not be symlinks. Replace linked entries with regular files or directories; " - "allow_symlinks does not apply to preset registry metadata." -) +def _registry_error(project: Path) -> str: + registry = project / ".specify" / "presets" / ".registry" + return ( + f"Cannot access preset registry {registry}: registry files and parent " + "directories must not be symlinks. Replace linked entries with regular " + "files or directories; allow_symlinks does not apply to preset registry metadata." + ) @pytest.fixture(params=["directory", "file", "dangling-directory", "dangling-file"]) @@ -82,7 +84,7 @@ def test_text_commands_report_registry_specific_symlink_error( assert result.exit_code == 1 assert " ".join(strip_ansi(result.stdout).split()) == ( - f"Error: {REGISTRY_SYMLINK_ERROR}" + f"Error: {_registry_error(linked_registry)}" ) assert result.stderr == "" @@ -106,7 +108,7 @@ def test_json_commands_report_registry_specific_symlink_error( assert result.exit_code == 1 assert result.stdout == "" - assert json.loads(result.stderr) == {"error": REGISTRY_SYMLINK_ERROR} + assert json.loads(result.stderr) == {"error": _registry_error(linked_registry)} assert result.stderr.count("\n") == 1 @@ -123,9 +125,7 @@ def test_mutating_commands_handle_registry_save_errors( manager.registry.update("test-pack", {"enabled": False}) def fail_save(self): - raise SymlinkDeniedError( - "Refusing symlink; explicitly allow symlinks on this FileHelper" - ) + raise SymlinkDeniedError(_registry_error(project_dir)) monkeypatch.setattr(PresetRegistry, "_save", fail_save) monkeypatch.chdir(project_dir) @@ -139,7 +139,7 @@ def fail_save(self): assert result.exit_code == 1 output = " ".join(strip_ansi(result.stdout).split()) - assert REGISTRY_SYMLINK_ERROR in output + assert _registry_error(project_dir) in output assert "explicitly allow symlinks" not in output assert "Traceback" not in result.output @@ -149,7 +149,7 @@ def test_resolve_handles_filesystem_errors_at_each_phase( project_dir: Path, monkeypatch: pytest.MonkeyPatch, phase: str, ): def fail(*args, **kwargs): - raise SymlinkDeniedError("explicitly allow symlinks") + raise SymlinkDeniedError(_registry_error(project_dir)) if phase == "construction": monkeypatch.setattr("specify_cli.presets.PresetResolver", fail) @@ -168,7 +168,7 @@ def fail(*args, **kwargs): ) assert result.exit_code == 1 - assert REGISTRY_SYMLINK_ERROR in " ".join(strip_ansi(result.stdout).split()) + assert _registry_error(project_dir) in " ".join(strip_ansi(result.stdout).split()) assert "explicitly allow symlinks" not in result.output assert "composition error" not in result.output @@ -181,7 +181,7 @@ def test_update_replacement_policy_error_preserves_removal_context( ) def fail_add(**kwargs): - raise SymlinkDeniedError("explicitly allow symlinks") + raise SymlinkDeniedError(_registry_error(project_dir)) monkeypatch.setattr( "specify_cli.presets._commands.preset_remove", lambda preset_id: None, @@ -197,7 +197,7 @@ def fail_add(**kwargs): output = " ".join(strip_ansi(result.stdout).split()) assert "previous preset was removed" in output assert "specify preset add" in output - assert REGISTRY_SYMLINK_ERROR in output + assert _registry_error(project_dir) in output assert "explicitly allow symlinks" not in output @@ -217,7 +217,7 @@ def fail(*args, **kwargs): result = CliRunner().invoke(app, arguments, catch_exceptions=False) assert result.exit_code == 1 - message = "Cannot access preset registry: unsupported registry path [red]" + message = "unsupported registry path [red]" if json_output: assert result.stdout == "" assert json.loads(result.stderr) == {"error": message} diff --git a/tests/specify_cli/presets/test_registry.py b/tests/specify_cli/presets/test_registry.py index 7ae6689ca3..1de416c339 100644 --- a/tests/specify_cli/presets/test_registry.py +++ b/tests/specify_cli/presets/test_registry.py @@ -286,6 +286,30 @@ def test_list_by_priority_includes_disabled_when_requested(self, temp_dir): class TestPresetRegistrySymlinks: + @pytest.mark.parametrize("operation", ["mkdir", "overwrite_text"]) + def test_save_policy_errors_preserve_registry_path_and_guidance( + self, tmp_path, monkeypatch, operation, + ): + project = tmp_path / "project" + project.mkdir() + registry = PresetRegistry(project / ".specify" / "presets") + registry.add("example", {"version": "1.0"}) + original = registry.registry_path.read_bytes() + + def fail(*args, **kwargs): + raise SymlinkDeniedError("explicitly allow symlinks") + + monkeypatch.setattr(FileHelper, operation, fail) + + with pytest.raises(SymlinkDeniedError) as exc_info: + registry.update("example", {"version": "2.0"}) + + assert str(registry.registry_path) in str(exc_info.value) + assert "Replace linked entries" in str(exc_info.value) + assert "explicitly allow symlinks" not in str(exc_info.value) + assert isinstance(exc_info.value.__cause__, SymlinkDeniedError) + assert registry.registry_path.read_bytes() == original + @pytest.mark.parametrize("relative", [False, True]) def test_regular_registry_preserves_persistence_and_observers( self, tmp_path, monkeypatch, relative @@ -362,11 +386,17 @@ def test_load_rejects_external_links( target = target / ".registry" else: link = packs_dir - link.symlink_to(target, target_is_directory=linked_entry == "presets") + try: + link.symlink_to(target, target_is_directory=linked_entry == "presets") + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") - with pytest.raises(SymlinkDeniedError): + with pytest.raises(SymlinkDeniedError) as exc_info: PresetRegistry(packs_dir) + assert str(packs_dir / ".registry") in str(exc_info.value) + assert "Replace linked entries" in str(exc_info.value) + assert "explicitly allow symlinks" not in str(exc_info.value) assert sentinel.read_bytes() == b"not registry JSON" assert link.is_symlink() assert not (outside / "missing").exists() @@ -400,7 +430,10 @@ def test_save_rejects_links_before_observer_or_mutation( target = outside / "missing" if dangling else outside if linked_entry == ".registry": target = target / ".registry" - link.symlink_to(target, target_is_directory=linked_entry != ".registry") + try: + link.symlink_to(target, target_is_directory=linked_entry != ".registry") + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") observed = [] monkeypatch.setattr( "specify_cli.integrations._file_changes.before_file_change", observed.append @@ -409,12 +442,15 @@ def test_save_rejects_links_before_observer_or_mutation( "specify_cli.integrations._file_changes.after_file_change", observed.append ) - with pytest.raises(SymlinkDeniedError): + with pytest.raises(SymlinkDeniedError) as exc_info: if operation == "remove": registry.remove("example") else: getattr(registry, operation)("example", {"version": "2.0"}) + assert str(registry.registry_path) in str(exc_info.value) + assert "Replace linked entries" in str(exc_info.value) + assert "explicitly allow symlinks" not in str(exc_info.value) assert sentinel.read_bytes() == b"do not overwrite" assert not (outside / "missing").exists() assert link.is_symlink() @@ -432,7 +468,10 @@ def test_registry_metadata_rejects_even_opted_in_contained_links( link = packs_dir if linked_entry == "presets" else registry.registry_path target = project / "contained" link.rename(target) - link.symlink_to(target, target_is_directory=linked_entry == "presets") + try: + link.symlink_to(target, target_is_directory=linked_entry == "presets") + except (OSError, NotImplementedError) as error: + pytest.skip(f"Symlinks unavailable: {error}") with pytest.raises(SymlinkDeniedError): PresetRegistry(packs_dir)