[Bug]: Harden filesystem removal of installed components

[mnriem]

Bug Description

Removal paths should not be derived from persisted component IDs without checking that the component is installed or owned, that the ID is a valid single path component, and that the target is the expected filesystem object.

Extension and preset removal currently lack these checks at their destructive manager operations. Extension removal can also write config backups. This is conditional hardening: reaching the reported bundle path requires coordinated project-state changes and an explicit user removal command, not an automatic trigger.

Steps to Reproduce

In an isolated temporary project, add an unsafe ID to the extension registry and a matching bundle record, then exercise the removal path using disposable targets. Inspect the paths used for deletion and config backup. Review direct preset removal with an unsafe registered ID. No reproduction should target real user files. This is a source-level finding; no destructive live reproduction was performed for this issue.

Expected Behavior

• Each removal operation checks installed or owned state using that component's existing source of truth.
• IDs used as directory names are well formed before path construction.
• An existing directory target must be a real directory, not a symlink. Invalid targets fail explicitly without deletion.
• Extension config backups cannot be redirected through a symlinked backup destination.
• Valid removals, including existing missing-target and config-preservation behavior, continue to work.

Actual Behavior

ExtensionManager.remove() and PresetManager.remove() use registry IDs to construct removal paths without an independent ID and target check at the destructive operation. Bundle removal can pass a recorded extension ID to the extension manager.

Specify CLI Version

Source checkout declares 1.0.10.dev0 in pyproject.toml.

AI Agent

Not applicable.

Operating System

Not platform-specific.

Python Version

Any supported version (>=3.11).

Error Logs

N/A; source-level finding.

Additional Context

Scope the implementation to extension and preset removal, plus an audit of integration uninstall's separately recorded file paths. Verify that workflow and step removal retain their existing ID and target guards; they do not need a new common registry check. Add isolated regression tests for invalid IDs, symlink targets, backup redirection, and valid removals. Do not prohibit symlinked project or storage parents solely as part of this ID fix.

AI Disclosure

Drafted by GitHub Copilot (model: GPT-6 Sol; interactive, autonomous AI drafting under user direction; reasoning-effort setting not available to me). The issue text was fully AI-drafted and approved by the user for filing.

[github-actions]

Bug assessment — harden-removal-paths: Valid · severity medium

---

Bug Assessment: Harden filesystem removal of installed components

• Slug: harden-removal-paths
• Created: 2026-09-25T16:28:26Z
• Source: issue [Bug]: Harden filesystem removal of installed components #4744
• Verdict: valid
• Severity: medium

Report (summarized)

Issue #4744 reports that extension and preset removal construct destructive paths directly from persisted registry IDs. It requests independent ownership/installed-state checks, validation that IDs are a single safe path component, rejection of symlink targets, and protection against symlink-redirection of extension config backups. The report is explicitly a source-level finding; no destructive live reproduction was performed. It also asks for an audit of integration uninstall paths while retaining existing guards for workflow and step removal.

The issue has no additional comments. The issue body references an open remediation pull request, but this assessment is against the checked-out main source.

Symptom

ExtensionManager.remove() and PresetManager.remove() verify registry membership but then append the persisted ID to a filesystem root and perform backup, recursive deletion, or directory removal without a dedicated ID/target safety check at the destructive boundary. If an attacker or corrupted state can place an unsafe ID in the registry and a user explicitly invokes removal, deletion or backup writes may be redirected outside the intended component directory. Bundle removal delegates to these same managers.

Reproduction

1. In an isolated disposable project, add an unsafe extension ID and matching registry metadata, plus a corresponding filesystem entry or symlink target.
2. Invoke extension removal and inspect the constructed extension and .backup paths; repeat with an unsafe preset ID and direct preset removal.
3. Do not target real user files. The report does not provide a safe live reproduction, so the impact is established from source inspection rather than an executed destructive test.

[NEEDS CLARIFICATION: The report does not specify the exact malformed IDs and registry/file-state fixture used by the reporter.]

Suspected Code Paths

• src/specify_cli/extensions/__init__.py:2950 — ExtensionManager.remove() only checks registry.is_installed(extension_id) before constructing extension_dir at line 2975.
• src/specify_cli/extensions/__init__.py:3010 — config backup destination is built as .backup / extension_id; mkdir() and copy2() operate on that derived path before shutil.rmtree(extension_dir) at line 3023.
• src/specify_cli/presets/_manager.py:631 — PresetManager.remove() checks registry membership at line 640, then constructs pack_dir from pack_id at line 671 and recursively removes it at line 861.
• src/specify_cli/bundles/adapters.py:335 and src/specify_cli/bundles/primitives.py:234 — bundle removal delegates to the extension/preset managers, so recorded bundle component IDs reach the same destructive operations.
• src/specify_cli/integrations/manifest.py:325 — integration uninstall has lexical containment and symlink handling for the final tracked path at lines 353–386, but the audit should confirm that tampered recorded paths cannot traverse through symlinked ancestors and that force behavior remains intentional.
• src/specify_cli/bundles/references.py:23 — bundle resolution uses registry membership for presets and extensions, which is an ownership lookup but does not itself validate the IDs used later by removal.

Root Cause Hypothesis

The managers treat a registry key as both an ownership assertion and a trusted filesystem component. Registry membership prevents removal of an unknown component, but it does not establish that the key is a valid single directory name or that the resolved target is the expected non-symlink directory. Extension backup handling adds a second derived path whose destination is not independently checked. Confidence: high.

Proposed Remediation

Preferred: Add a shared, narrowly scoped component-ID validator for extension and preset removal that accepts only the repository’s canonical ID format and rejects empty values, path separators, ./.., and other multi-component or platform-special names. At the start of each manager’s destructive remove() operation, validate the ID, confirm the registry entry is a valid installed/owned record, and validate the derived target without following symlinks. Require an existing target to be a real directory (or preserve the documented missing-target behavior); fail explicitly on invalid or symlinked targets. Apply equivalent no-follow validation to the extension backup root and backup destination before creating directories or copying config files. Keep valid removal, missing-target, and keep_config behavior unchanged.

For integration uninstall, retain manifest hash ownership checks and final symlink refusal, but audit and, if needed, validate every ancestor of a recorded path without following symlinks before unlinking. Do not ban symlinked project or storage parents globally; scope the check to the recorded removal path.

Alternatives:

• Resolve the target and compare it to the expected component root before deletion. This is simpler for normal directories but must separately handle missing targets and can be less explicit than rejecting symlinks at each path component.
• Migrate registries to normalized/sanitized IDs on write and reject legacy unsafe entries on read. This improves future state but requires migration/error handling and does not by itself protect already-corrupted registries during removal.

Files likely to change:

• src/specify_cli/extensions/__init__.py
• src/specify_cli/presets/_manager.py
• A shared path/ID validation helper near the component managers, if existing validation utilities do not fit
• src/specify_cli/integrations/manifest.py if the ancestor audit identifies a gap
• src/specify_cli/bundles/primitives.py or bundle removal tests only if delegation needs explicit error translation
• tests/specify_cli/extensions/ and tests/specify_cli/presets/
• tests/specify_cli/bundles/ and tests/integrations/test_manifest.py for the audit coverage

Tests to add or update:

• Invalid extension and preset IDs are rejected before any deletion or registry mutation.
• Valid single-component IDs remove their own directories and registry entries.
• Missing targets preserve existing successful removal semantics.
• Symlinked extension/preset targets are rejected without deleting the symlink target.
• Extension config backup paths cannot be redirected through a symlink; valid backups still preserve config files.
• Bundle removal propagates a clear failure for an invalid or unsafe component ID.
• Integration manifest uninstall covers recorded paths with symlinked ancestors, normal files, modified files, and force behavior.

Risks & Considerations

• Tightening ID validation may reject legacy registry entries; provide an explicit error and preserve the registry for repair rather than silently deleting it.
• Symlink checks must use no-follow filesystem operations and must not accidentally remove a target outside the project.
• keep_config, config backup preservation, missing-target handling, and post-removal reconciliation are compatibility-sensitive.
• The issue requires an explicit removal command and coordinated persisted-state changes, so this is a conditional local filesystem-integrity issue rather than an automatic-trigger vulnerability; that limits blast radius and supports medium severity.

Open Questions

• [NEEDS CLARIFICATION: What exact ID grammar is canonical for all currently supported extension and preset IDs, including legacy IDs?]
• [NEEDS CLARIFICATION: Should an existing symlinked component directory be reported as an error or as a non-removable/skipped target in the public CLI output?]
• [NEEDS CLARIFICATION: Should --force ever permit removal of a symlink entry, or should symlinks remain unconditionally rejected for component managers?]

Generated by 🐛 Assess Bug from Labeled Issue for #4744 · copilot · gpt52codex · 2.66 AIC · ⌖ 7.27 AIC · ⊞ 22.4K · ◷