fix: reject symlinked preset registry paths by mnriem · Pull Request #5180 · github/spec-kit

mnriem · 2026-10-29T14:24:34Z

Description

Reject symlinked preset registry files and parent directories when reading or saving registry metadata. Use the existing project-root-scoped FileHelper in deny mode and validate save destinations before file-change observers or directory creation.

Preserve registry JSON formatting, UTF-8 encoding, existing file metadata semantics, and observer notifications for valid paths. Registry metadata remains symlink-denied independently of the operational allow_symlinks setting. Add regression coverage and document this boundary.

Testing

Tested locally with uv run specify --help
Ran existing tests with uv sync && uv run pytest
Tested with a sample project (if applicable)

Commands and results:

uv sync --extra test --quiet — passed; restored this worktree's test environment.
uv run specify --help — passed.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests -q -n 4 — passed: 13,699 passed, 130 skipped, 63 warnings.
.venv/bin/python -m pytest tests/specify_cli/presets/test_registry.py -q --tb=short — passed: 62 tests, including temporary-project persistence and symlink rejection cases.

The full suite used the worktree virtualenv directly, as recommended in CONTRIBUTING.md, and four pytest workers. Sample-project coverage is automated through temporary projects in the registry tests; no separate interactive sample-project run was performed.

AI Disclosure

I did not use AI assistance for this contribution
I did use AI assistance (fill in the disclosure below)

AI disclosure: GitHub Copilot (GPT-6.1 Sol, default reasoning settings) acted autonomously on behalf of @mnriem to implement the user-selected fix, generate the code, tests, documentation, commit message, and PR description, and run automated validation. A Copilot security-review subagent assisted the initial assessment using runtime-default model/settings. The contributor authorized the scope and publication; no human line-by-line review or manual testing is claimed.

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>

Copilot

🟡 Changes recommended

Unhandled SymlinkDeniedError exceptions for symlinked preset paths bypass CLI JSON error handling and produce the wrong MCP error code, violating the error-envelope contract.

1 open issue

Translate SymlinkDeniedError in shared artifact operations · New

What changed in this PR

Hardens Spec Kit’s preset registry persistence by rejecting symlinked registry files and parent directories, regardless of the operational allow_symlinks setting.

Changes:

Routes registry reads and writes through a project-root-scoped, deny-mode FileHelper.
Validates save destinations before observers or directory creation, preserving existing persistence behavior for valid paths.
Adds regression coverage and documents the registry-specific boundary.

Validation was limited to source inspection; before-and-after tests were not independently run.

File	Description
`tests/specify_cli/presets/test_registry.py`	Tests persistence, observer notifications, I/O errors, and symlink rejection.
`src/specify_cli/presets/_registry.py`	Uses scoped filesystem operations and preflights save destinations.
`design/file-helper.md`	Documents unconditional symlink denial for preset registry metadata.

_{Review effort: Max (auto)}

-            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")


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>

Copilot

🔵 Needs a closer look

Symlinked preset registry paths trigger uncaught CLI errors, while JSON commands misleadingly advise allowing symlinks even though registry links are always forbidden.

1 open issue

Translate SymlinkDeniedError in shared artifact operations

1 previously missed issue

In code that hasn't changed since last review

Handle symlink errors in preset CLI commands

src/specify_cli/presets/_registry.py:35

With a symlinked .specify/presets or .registry, specify preset list now raises an uncaught SymlinkDeniedError during manager construction (src/specify_cli/presets/command_list.py:45–47). JSON preset commands expose the helper's advice to allow symlinks, although registry links are always forbidden. Catch FileHelperError at the preset CLI boundaries, including manager/resolver construction, report registry-specific guidance, and add text-mode regression coverage.

_{Review effort: Max (auto)}

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>

Copilot

🔵 Needs a closer look

Integration upgrades still follow registry symlinks and treat dangling links as no installed presets, allowing migrations to bypass preset-protection guards while ownership is unknown.

1 open issue

Translate SymlinkDeniedError in shared artifact operations

3 previously missed issues

In code that hasn't changed since last review

Fail closed when preset registry links are unreadable

design/file-helper.md:11

_installed_presets_affecting_agent() in src/specify_cli/integrations/_command_upgrade_layout.py:121–127 still follows registry links and returns no presets for dangling links, so this guarantee does not hold for integration upgrades. During command-root or command/skills migrations, that false-empty result lets the preset-protection guard pass while ownership is unknown. Use a project-root-scoped deny-mode helper there and map policy failures to _PresetRegistryUnreadableError so migration fails closed.

Translate registry symlink failures into actionable diagnostics

src/specify_cli/presets/_registry.py:36

Outside the preset CLI group, registry symlink failures still tell users to "explicitly allow symlinks", although that setting cannot make registry metadata usable. Integration registration warnings and bundle errors expose this advice; meanwhile, the CLI rewrite in _commands.py:18–23 drops the denied path. Translate symlink failures in _load and _save into a path-preserving "replace linked entries" diagnostic, and have the CLI render it.

Skip symlink tests when link creation is unsupported

tests/specify_cli/presets/test_registry.py:365

On hosts without symlink privileges, these tests fail during setup rather than exercising registry behavior. Skip when link creation raises OSError or NotImplementedError, matching tests/specify_cli/presets/test_commands.py:41–44. Apply the same guard at lines 403 and 435 of this file and in tests/specify_cli/artifacts/conftest.py:47.

_{Review effort: Max (auto)}

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>

Copilot

🔵 Needs a closer look

The cross-cutting symlink security policy rejects existing symlinked preset setups even with allow_symlinks=true and still needs maintainer sign-off, before/after validation, and Windows testing.

1 open issue

Translate SymlinkDeniedError in shared artifact operations

_{Review effort: Max (auto)}

Copilot AI auto review requested due to automatic review settings October 29, 2026 14:24

Copilot started reviewing on behalf of mnriem October 29, 2026 14:25

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

Comment thread src/specify_cli/presets/_registry.py Outdated

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")

mnriem added the triage-must-have Verdict: high-value, important work for Spec Kit — do first label Oct 29, 2026

Copilot AI auto review requested due to automatic review settings October 29, 2026 15:50

Copilot started reviewing on behalf of mnriem October 29, 2026 15:51

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

Copilot AI auto review requested due to automatic review settings October 29, 2026 16:23

Copilot started reviewing on behalf of mnriem October 29, 2026 16:23

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

Copilot AI auto review requested due to automatic review settings October 29, 2026 16:53

Copilot started reviewing on behalf of mnriem October 29, 2026 16:54

View session

Copilot AI reviewed Oct 29, 2026

View reviewed changes

Navigation Menu

Repository navigation

fix: reject symlinked preset registry paths - #5180

fix: reject symlinked preset registry paths#5180
mnriem wants to merge 4 commits into
github:mainfrom
mnriem:mnriem-preset-registry-symlinks

mnriem commented Oct 29, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

mnriem commented Oct 29, 2026

Description

Testing

AI Disclosure

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

This branch has not been deployed

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants