You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
ExtensionManager.remove() and PresetManager.remove() used the
registry-supplied extension_id / pack_id directly to build the removal
path (and, for extensions, the .backup/<id> config-backup path) without:
validating the id is a single, well-formed path component (the same ^[a-z0-9-]+$ slug format already enforced when the id is first written),
and
checking that the resolved target, if it exists, is a real directory
rather than a symlink.
Since the registry is a plain JSON file, a corrupted or tampered entry
(e.g. an id like ../outside) could point the removal path outside .specify/extensions / .specify/presets. Separately, a symlink planted at
the extension/preset directory — or at .backup/<extension_id> — could
redirect deletion or config backups to an attacker-chosen location; the
previous code also crashed with an unhandled OSError from shutil.rmtree on a symlinked top-level target instead of failing
explicitly, as the issue requires.
Both remove() methods now:
Reject an id that doesn't match the existing slug pattern before any
path is constructed.
Refuse to proceed if the removal target exists but is a symlink or not
a directory.
(Extensions only) Refuse to proceed if .backup/<extension_id> is a
symlink, before it is ever created/written into.
All three checks happen before any commands/skills/hooks are unregistered,
so an unsafe id leaves the extension/preset registry and on-disk state
untouched rather than partially removed.
Per the issue's scope, I also audited:
Workflow/step removal (src/specify_cli/workflows/command_remove.py)
already validates the id, stages via rename, and refuses a symlinked
target — no changes needed there.
Integration uninstall (IntegrationManifest.uninstall in src/specify_cli/integrations/manifest.py) already rejects paths with .. segments, containment-checks via os.path.normpath, and never
follows/removes symlinks unless force=True — no changes needed there
either.
Testing
Added regression tests for both managers that reproduce the issue's
reproduction steps (register an unsafe id / plant a symlink, then call remove()), and confirmed they fail without the fix:
$ git checkout adbd62a -- src/specify_cli/extensions/__init__.py src/specify_cli/presets/__init__.py
$ .venv/bin/python -m pytest tests/test_extensions.py -k "remove_rejects_unsafe or remove_refuses_symlinked" \
tests/test_presets.py -k "remove_rejects_unsafe or remove_refuses_symlinked" -q
...
FAILED tests/test_extensions.py::TestExtensionManager::test_remove_rejects_unsafe_registry_id - assert True is False
FAILED tests/test_extensions.py::TestExtensionManager::test_remove_refuses_symlinked_extension_dir - OSError: Cannot call rmtree on a symbolic link
FAILED tests/test_extensions.py::TestExtensionManager::test_remove_refuses_symlinked_backup_dir - assert True is False
FAILED tests/test_presets.py::TestPresetManager::test_remove_rejects_unsafe_registry_id - assert True is False
FAILED tests/test_presets.py::TestPresetManager::test_remove_refuses_symlinked_preset_dir - OSError: Cannot call rmtree on a symbolic link
5 failed, 1069 deselected in 0.82s
Full suite (.venv/bin/python -m pytest -q): 8536 passed, 13 skipped, 10 failed.
The 10 failures are all test_*_python_parity.py "composed" template-variant
cases (check_prerequisites, create_new_feature, resolve_template, setup_plan, setup_tasks), unrelated to extension/preset removal.
Confirmed pre-existing by reverting just this PR's two source files back to adbd62a (current upstream/main) and re-running: the same failures
reproduce with none of this PR's changes present.
Implemented with Claude Code (model: Claude Sonnet 5, autonomous). The
agent read the issue, located the two remove() implementations, audited
the workflow/step and integration-uninstall paths named in the issue for
existing equivalent guards, wrote the fix and the regression tests, and
verified them (including the fail-without-fix check above) without human
line-by-line review prior to this PR being opened.
…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/<id> 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.
Fixesgithub#4744
Assisted-by: Claude Code (model: Claude Sonnet 5, autonomous)
…-removal
Resolves conflicts from upstream's preset domain split (PR github#4747): the
ExtensionManager/PresetManager.remove() security fix from b6dc0a6 is
ported from the old monolithic presets/__init__.py into the new
PresetManager.remove() in presets/_manager.py, and its regression
tests are ported into tests/specify_cli/presets/test_manager.py.
Assisted-by: Claude Code (model: Claude Sonnet 5, autonomous)
Posted on behalf of @chelsealong by Claude Code (model: Claude Sonnet 5, autonomous); comment fully AI-drafted.
Resolved the conflicts with main in 79abc46. Upstream split presets/__init__.py into presets/_manager.py + friends (#4747) since this PR was opened, so the merge itself was mechanical, but the security fix (PresetManager.remove() id/symlink validation) had to be ported by hand into the new _manager.py, and its two regression tests into tests/specify_cli/presets/test_manager.py (the old tests/test_presets.py unit tests for PresetManager moved there too).
Verified: reverted just _manager.py's fix and confirmed both regression tests fail the same way as before (assert True is False / OSError: Cannot call rmtree on a symbolic link), restored the fix and confirmed they pass, then ran the full suite (8563 passed, 13 skipped) and ruff check on the touched files (all checks passed). No unrelated failures.
Address Copilot review on github#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.
extensions/__init__.py: registry-id check now uses fullmatch instead of match (a trailing newline was previously accepted past the $ anchor).
extensions/__init__.py: extension_dir symlink check no longer gated behind exists(), so a dangling symlink is rejected instead of silently passed through.
extensions/__init__.py: added a check that the .backup parent directory itself isn't a symlink, before it's ever created/written into.
presets/_manager.py: same dangling-symlink fix for pack_dir (its id check already used fullmatch).
Added one regression test per finding (tests/test_extensions.py, tests/specify_cli/presets/test_manager.py); confirmed all 4 fail without the corresponding fix and pass with it. Full suite: 8567 passed, 13 skipped. ruff check on touched files: all checks passed.
_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().
Posted on behalf of @chelsealong by Claude Code (model: Claude Sonnet 5, autonomous); comment fully AI-drafted.
Addressed the 2 remaining Copilot findings in 52bacea:
bundles/primitives.py: _ExtensionKindManager.remove() now raises BundlerError when ExtensionManager.remove() returns False instead of discarding the result, so a refused removal (unsafe registry id or symlinked target) fails bundle uninstall instead of letting remove_bundle() drop the bundle's ownership metadata while the extension is still on disk.
presets/_manager.py: added the missing regression test for the regular-file (non-directory) preset-target rejection; no code change was needed there, the guard already existed.
Confirmed the bundle-removal test fails without the fix (DID NOT RAISE BundlerError) and passes with it restored. Full suite: 8569 passed, 13 skipped. ruff check on touched files: all checks passed.
_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.
bundles/primitives.py: _PresetKindManager.remove() now raises BundlerError when PresetManager.remove() returns False, mirroring the extension-side fix from 52bacea (it previously discarded a refused preset removal, letting bundle uninstall drop ownership metadata for a preset still on disk).
extensions/__init__.py: the .backup symlink checks now only run if not keep_config and extension_dir.exists() — the same condition under which a backup is actually written — so keep_config=True or an already-missing extension directory no longer trips on an unrelated symlink at .backup.
Added one regression test per finding; confirmed both fail without the corresponding fix and pass with it. Full suite: 8571 passed, 13 skipped. ruff check on touched files: all checks passed.
The other two items Copilot still lists as open (Propagate extension removal refusal... / Test regular-file preset targets...) were already fixed in 52bacea — verified the code and tests are in place, no further change needed there.
Add regression test for regular-file backup path rejection
src/specify_cli/extensions/__init__.py:2973
Add regression coverage for the new regular-file rejection branch. Preset removal verifies this case in tests/specify_cli/presets/test_manager.py:358-372, but extension removal only tests unsafe IDs and symlinks, so the exists() and not is_dir() behavior can regress unnoticed despite the repository's positive/negative testing requirement.
This issue also appears on line 2977 of the same file.
Posted on behalf of @chelsealong by Claude Code (model: Claude Sonnet 5.5, autonomous); comment fully AI-drafted.
Addressed the remaining Copilot finding: ExtensionManager.remove() now also refuses (before unregistering anything) when .backup or .backup/<id> exists as a non-directory, using the same not keep_config and extension_dir.exists() gate. Added test_remove_refuses_regular_file_backup_path; it fails without the change (FileExistsError) and passes with it. tests/test_extensions.py + tests/specify_cli: 3092 passed.
Extension ID validation permits IDs that removal rejects
src/specify_cli/extensions/__init__.py:2969
This removal check is stricter than the current writer-side validation: ExtensionManifest uses re.match(r"^[a-z0-9-]+$", ...) at line 320, which accepts an ID ending in a newline because $ can match before that newline. Such an extension can be installed normally and written to the registry, but this fullmatch then makes it unremovable. Change manifest validation to the same full-match semantics and add an install-to-remove regression case.
This issue also appears on line 2979 of the same file.
Install and update callers ignore failed removal
src/specify_cli/extensions/__init__.py:2974
Existing callers do not consistently honor this new refusal. install_from_directory(..., force=True) stores the False result at line 2170 but continues to shutil.rmtree(dest_dir) at line 2540 (after already deleting the backup path), while _command_update_transaction.py:523 discards the result and starts installation/rollback. A symlinked target therefore still produces later raw filesystem errors and can trigger unrelated cleanup. Make both callers abort immediately when removal returns False, and cover the force/update paths with regression tests.
Manifest ID validation permits IDs that removal rejects
src/specify_cli/presets/_manager.py:649
PresetManifest still validates IDs with re.match(...$) at _manifest.py:120, so a trailing-newline ID can pass normal installation and be persisted. This stricter registry check then refuses to remove that normally installed preset. Align the manifest validator with this fullmatch rule and add an end-to-end regression test so installation cannot create an ID that removal rejects.
This issue also appears on line 650 of the same file.
Abort force reinstall when removal refuses symlinked target
src/specify_cli/presets/_manager.py:652
install_from_directory(force=True) still ignores this new False result at line 398 and then calls shutil.rmtree(dest_dir) on the same symlink at line 402. Thus force-reinstalling a preset with a symlinked target still raises the unhandled OSError this hardening is intended to replace. The force path must abort when remove() refuses, as the extension installer now does.
extensions/__init__.py: install_from_directory(force=True) now rejects a symlinked .backup root before the stale-backup cleanup runs.
presets/_manager.py: install_from_directory(force=True) now aborts when remove() refuses, instead of proceeding to rmtree.
_command_update_transaction.py: installation_modified is reset before raising on a refused removal, so rollback stays non-destructive.
Added a test each for the first two; both fail without the source change and pass with it. No test for the third (the existing update-refusal test passes either way). tests/test_extensions.py + tests/specify_cli: 1265 passed.
remove_bundle bypasses preset refusal for corrupt symlink targets
src/specify_cli/bundles/primitives.py:225
Preset refusal is still bypassed by the real remove_bundle() path. _PresetKindManager.is_installed() calls get_pack(), which returns None when the registered target is an empty/corrupt symlink; remove_bundle() then skips this remove() call and deletes the bundle record while the preset registry entry remains. Use the preset registry as the installed-state source (as the extension adapter does) and cover this through remove_bundle(), rather than only invoking the adapter directly.
Add extension regression test for regular-file target refusal
src/specify_cli/extensions/__init__.py:2983
The new regular-file refusal branch lacks extension-side regression coverage. The preset suite has a matching regular-file target test, but the extension tests only exercise symlink targets and a regular file at the backup path; add a case asserting that a regular file at .specify/extensions/<id> is preserved, remove() returns False, and the registry remains unchanged.
extensions/__init__.py: remove() now refuses (before unregistering) when .backup/<id>/<config> is a symlink, so copy2 can't write through it.
bundles/primitives.py: _PresetKindManager.is_installed() uses the preset registry, so remove_bundle() reaches remove() (and its refusal) for a corrupt symlinked target.
Added an extension regular-file target test (coverage only; the guard already existed).
The backup-file and preset is_installed tests fail without the source change and pass with it. Affected test dirs: 1657 passed; ruff 0.15.0 clean on touched files.
Clarify refusal message for unexpected preset removal targets
src/specify_cli/bundles/primitives.py:234
The refusal reason is incomplete: PresetManager.remove() also returns False when the target is a regular file, so this message incorrectly tells users that only an unsafe ID or symlink can be responsible. Describe this as an unsafe registry ID or unexpected on-disk target so the regular-file case is actionable.
This issue also appears on line 327 of the same file.
Test removing registered presets with missing install directories
src/specify_cli/presets/_manager.py:656
Add the required positive regression for a valid registered preset whose install directory is already missing. The new guard intentionally permits that state, but the added tests only cover an absent registry entry and rejected filesystem objects; they do not verify that remove() still returns True and clears the registry when the registered target is missing, as required by issue #4744.
Posted on behalf of @chelsealong by Claude Code (model: Claude Sonnet 5.5, autonomous); comment fully AI-drafted.
Addressed the open high-severity finding: ExtensionManager.remove() now also refuses (before unregistering) when .backup/<id>/<config> exists as a directory, not just a symlink. Added test_remove_refuses_directory_backup_config_path; it fails without the source change and passes with it. tests/test_extensions.py + tests/specify_cli: 3102 passed; ruff 0.15.0 clean.
Not changed in this push: the two low-severity items (preset refusal message wording, missing-target preset positive test).
Posted on behalf of @chelsealong by Claude Code (model: Claude Sonnet 5.5, autonomous); comment fully AI-drafted.
Addressed the two remaining low-severity items: the bundle refusal messages in bundles/primitives.py now say "unsafe registry id or unexpected on-disk target", and I added test_remove_registered_preset_with_missing_dir (remove() returns True and clears the registry when the target is already gone). Test-only plus message change. tests/specify_cli/presets/test_manager.py + tests/specify_cli/bundles: 479 passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
author-awaitingWaiting on author responseauthor-needs-rebaseBranch conflicts with main — rebase/resolve before mergetriage-can-waitVerdict: valid and in-scope but deprioritized; held behind the evidence gate
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mnriem
added
triage-can-wait
Summary
Fixes #4744.
ExtensionManager.remove()andPresetManager.remove()used theregistry-supplied
extension_id/pack_iddirectly to build the removalpath (and, for extensions, the
.backup/<id>config-backup path) without:^[a-z0-9-]+$slug format already enforced when the id is first written),and
rather than a symlink.
Since the registry is a plain JSON file, a corrupted or tampered entry
(e.g. an id like
../outside) could point the removal path outside.specify/extensions/.specify/presets. Separately, a symlink planted atthe extension/preset directory — or at
.backup/<extension_id>— couldredirect deletion or config backups to an attacker-chosen location; the
previous code also crashed with an unhandled
OSErrorfromshutil.rmtreeon a symlinked top-level target instead of failingexplicitly, as the issue requires.
Both
remove()methods now:path is constructed.
a directory.
.backup/<extension_id>is asymlink, before it is ever created/written into.
All three checks happen before any commands/skills/hooks are unregistered,
so an unsafe id leaves the extension/preset registry and on-disk state
untouched rather than partially removed.
Per the issue's scope, I also audited:
src/specify_cli/workflows/command_remove.py)already validates the id, stages via rename, and refuses a symlinked
target — no changes needed there.
IntegrationManifest.uninstallinsrc/specify_cli/integrations/manifest.py) already rejects paths with..segments, containment-checks viaos.path.normpath, and neverfollows/removes symlinks unless
force=True— no changes needed thereeither.
Testing
Added regression tests for both managers that reproduce the issue's
reproduction steps (register an unsafe id / plant a symlink, then call
remove()), and confirmed they fail without the fix:With the fix restored:
Full suite (
.venv/bin/python -m pytest -q):8536 passed, 13 skipped, 10 failed.The 10 failures are all
test_*_python_parity.py"composed" template-variantcases (
check_prerequisites,create_new_feature,resolve_template,setup_plan,setup_tasks), unrelated to extension/preset removal.Confirmed pre-existing by reverting just this PR's two source files back to
adbd62a(currentupstream/main) and re-running: the same failuresreproduce with none of this PR's changes present.
Lint:
uvx ruff@0.15.0 check src/specify_cli/extensions/__init__.py src/specify_cli/presets/__init__.py tests/test_extensions.py tests/test_presets.py→ All checks passed.AI Disclosure
Implemented with Claude Code (model: Claude Sonnet 5, autonomous). The
agent read the issue, located the two
remove()implementations, auditedthe workflow/step and integration-uninstall paths named in the issue for
existing equivalent guards, wrote the fix and the regression tests, and
verified them (including the fail-without-fix check above) without human
line-by-line review prior to this PR being opened.
🤖 Generated with Claude Code