fix: reject unsafe self-upgrade installer paths by mnriem · Pull Request #5172 · github/spec-kit

mnriem · 2026-10-29T12:15:18Z

Description

Require absolute installer paths that do not resolve to executables in the current working directory when selecting uv or pipx for install-method probes and self-upgrades. Reject unsafe lookup results without falling back to bare commands.

Add regression coverage for relative paths, working-directory executables, symlinks, valid installed tools, and CLI failure behavior.

Testing

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

The full suite was run using the documented worktree-local interpreter rather than the generic command above:

uv sync --extra test — passed.
LC_ALL=en_US.UTF-8 .venv/bin/python -m pytest tests -q — passed: 13,088 passed, 130 skipped.
uv run specify --help — passed.

Run on macOS; Windows-only tests were skipped locally. Sample-project testing is not applicable because this change does not affect scaffolding or slash commands.

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 using GPT-6.1 Sol in autonomous mode generated the code and tests, ran validation, and drafted this pull request.

Reject relative and working-directory installer lookup results before registry probes and self-upgrades, including symlinks to working-directory executables. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Copilot

🟡 Changes recommended

Installer validation still allows working-directory PATH entries to execute project-controlled code during probes or upgrades, leaving a critical security issue unresolved.

2 open issues

What changed in this PR

Introduces installer-path validation for Specify CLI’s detection probes and self-upgrades to prevent execution of working-directory tools.

Changes:

Centralizes uv and pipx lookup validation, rejecting unsafe results without falling back to bare commands.
Adds path-safety and CLI-failure regression tests, and updates existing mocks to use absolute executable paths.

Validation used isolated standard-library checks; the full suite and macOS/Windows-specific cases were not rerun.

AI review for the requester: GitHub Copilot (model unknown; autonomous; code inspection and isolated checks).

File	Description
`tests/specify_cli/test_version_verification.py`	Uses absolute installer mocks in upgrade-verification tests.
`tests/specify_cli/test_version_execution.py`	Updates installer mocks in execution and failure tests.
`tests/specify_cli/test_version_detection.py`	Adds installer-path safety cases and updates lookup expectations.
`tests/specify_cli/selfs/test_command_upgrade.py`	Tests unsafe lookup rejection and updates command mocks.
`src/specify_cli/_version.py`	Adds shared installer-path validation for probes and upgrade commands.

_{Review effort: Max (auto)}

Compare both lookup and resolved target directories by filesystem identity, rejecting path-validation errors. Preserve rejection details for registry probes, upgrades, and dry runs without masking unrelated missing installers. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

mnriem · 2026-10-29T12:43:02Z

Addressed both findings in 8970a703: installer validation now uses filesystem identity for the lookup and resolved target directories and rejects path-validation errors. Rejected installers retain actionable PATH/working-directory guidance in normal and dry-run output, including failed ownership probes. Added regressions for both uv and pipx; focused automated coverage passed (256 passed, 3 skipped). The full suite was not rerun for this follow-up. Posted on behalf of @mnriem by GitHub Copilot (GPT-6.1 Sol, autonomous mode); implementation, test generation/execution, and this summary were AI-generated.

Copilot

🟡 Changes recommended

Windows upgrade tests fail before the CLI runs because decoy files collide with fixture directories, and installer-rejection guidance omits the workaround for implicit current-directory matches.

3 open issues

1 resolved since last review

Reject cwd-controlled executables and handle path comparison errors

_{Review effort: Max (auto)}

Keep decoy executables in a separate working directory from installer fixtures and exercise the Windows directory layout on all platforms. Explain how to avoid implicit current-directory matches when installer lookup returns a relative path. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

mnriem · 2026-10-29T13:33:59Z

Addressed this round in f5c30633: decoy files now live in a separate project working directory, and the test creates the Windows-shaped installer directories on every platform to guard against collisions. Relative-path rejection guidance now covers implicit Windows current-directory matches in both normal and dry-run output, including ownership probes. The focused command/detection suite passed (122 passed, 3 skipped) on macOS; native Windows validation and the full suite were not rerun for this follow-up. Posted on behalf of @mnriem by GitHub Copilot (GPT-6.1 Sol, autonomous mode); implementation, test generation/execution, and this summary were AI-generated.

Copilot

🔵 Needs a closer look

When both installers own specify-cli, a rejected Tier-3 probe can select the other installer instead of returning UNSUPPORTED, risking an upgrade of the wrong installation.

No open issues 🎉

1 previously missed issue

In code that hasn't changed since last review

Reject ambiguous installer selection when a probe is rejected

src/specify_cli/_version.py:659

When both installers own specify-cli, rejecting one Tier-3 lookup leaves its match false, so this path selects the other installer instead of returning UNSUPPORTED. This can upgrade the wrong installation and occurs symmetrically for uv and pipx. Require no rejected probes before accepting exactly_one_match, and update test_rejected_probe_does_not_hide_a_different_missing_installer accordingly.

3 resolved since last review

_{Review effort: Max (auto)}

Merge upstream main without rewriting PR history. Use the existing uv-tool entrypoint fixture in authentication-failure command tests instead of mocking the detector's old enum-only return shape. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

Treat rejected Tier-3 probes as unresolved ownership rather than selecting the other installer. Cover both installer directions and normal/dry-run CLI behavior without invoking an upgrade. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

mnriem · 2026-10-29T14:01:53Z

Addressed the previously missed ownership ambiguity in 8b85bf7e: Tier-3 detection now remains unsupported when either installer probe is rejected, even if the other registry reports ownership. Symmetric regressions cover both installers and normal/dry-run commands, asserting that no upgrade runs. Authentication/version/self command coverage passed (595 passed, 3 skipped), and Ruff passed. This finding was in the review body only, with no inline conversation; all four existing conversations were already resolved. Posted on behalf of @mnriem by GitHub Copilot (GPT-6.1 Sol, autonomous mode); implementation, test generation/execution, and this summary were AI-generated.

Copilot

🟡 Changes recommended

A critical installer-detection flaw can select the wrong package manager after a probe is rejected, causing self-upgrade to modify the wrong installation.

1 open issue

Prevent tier-3 detection from selecting manager after probe rejection · New

_{Review effort: Max (auto)}

Copilot

🔵 Needs a closer look

This security-sensitive fix depends on Windows executable-search and filesystem-identity behavior, but the supplied macOS-only validation skipped Windows regressions, leaving native Windows confirmation necessary.

No open issues 🎉

1 resolved since last review

Prevent tier-3 detection from selecting manager after probe rejection

_{Review effort: Max (auto)}

* fix(cli): don't self-upgrade uv tool installs on Windows `uv tool install --force` deletes the tool environment before reinstalling it. When `specify self upgrade` runs it from that environment on Windows, uv removes Lib\site-packages and then fails on Scripts\ because the running python.exe is locked, leaving `specify` without its package. When the running interpreter is a uv tool environment on Windows, print the uv command to run once specify exits and make no changes. pipx installs and other platforms are unchanged. Fixes #5141 Assisted-by: Claude Code (model: Claude Opus 5.5, supervised) * fix(cli): preview the Windows uv tool outcome in self upgrade --dry-run On Windows, a real `specify self upgrade` from a uv tool environment prints the uv command instead of running it, but `--dry-run` still listed that command as the one that would be executed. Preview the same outcome, printing the command to run once specify exits exactly as the real run does, and add the Windows exception to the `--help` text. Refs #5141 Assisted-by: Claude Code (model: Claude Opus 5.5, supervised) * fix(cli): tell Windows uv tool users to stop other specify processes Exiting `specify self upgrade` does not release the Windows lock on the tool environment's python.exe if another process from it is still running, such as an editor's `specify mcp` server. Running the printed uv command then fails partway through the same way. Say to stop those processes first, in both the real run and --dry-run output and in docs/upgrade.md. Refs #5141 Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) * test(cli): pin the Windows uv tool no-op when already current On a Windows uv tool install that is already on the latest or requested release, self upgrade reports that and exits before the manual uv command guidance. Cover both cases so the guard can't move ahead of the version check unnoticed. * test(cli): use an absolute installer path in the Windows uv tool tests Since #5172, self upgrade rejects an installer lookup that returns a relative path, so mocking shutil.which as "uv" or "pipx" now stops the upgrade with "installer rejected". Return sys.executable instead, as the other self upgrade tests on main do, and render the expected non-Windows dry-run command from that path.

* fix: reject unsafe self-upgrade installer paths Reject relative and working-directory installer lookup results before registry probes and self-upgrades, including symlinks to working-directory executables. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix: close installer path bypasses and preserve rejection guidance Compare both lookup and resolved target directories by filesystem identity, rejecting path-validation errors. Preserve rejection details for registry probes, upgrades, and dry runs without masking unrelated missing installers. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix: isolate installer decoys and clarify Windows lookup guidance Keep decoy executables in a separate working directory from installer fixtures and exercise the Windows directory layout on all platforms. Explain how to avoid implicit current-directory matches when installer lookup returns a relative path. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * fix: reject installer ownership when a registry probe is unsafe Treat rejected Tier-3 probes as unresolved ownership rather than selecting the other installer. Cover both installer directions and normal/dry-run CLI behavior without invoking an upgrade. Assisted-by: GitHub Copilot (model: GPT-6.1 Sol, autonomous) Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

* fix(cli): don't self-upgrade uv tool installs on Windows `uv tool install --force` deletes the tool environment before reinstalling it. When `specify self upgrade` runs it from that environment on Windows, uv removes Lib\site-packages and then fails on Scripts\ because the running python.exe is locked, leaving `specify` without its package. When the running interpreter is a uv tool environment on Windows, print the uv command to run once specify exits and make no changes. pipx installs and other platforms are unchanged. Fixes github#5141 Assisted-by: Claude Code (model: Claude Opus 5.5, supervised) * fix(cli): preview the Windows uv tool outcome in self upgrade --dry-run On Windows, a real `specify self upgrade` from a uv tool environment prints the uv command instead of running it, but `--dry-run` still listed that command as the one that would be executed. Preview the same outcome, printing the command to run once specify exits exactly as the real run does, and add the Windows exception to the `--help` text. Refs github#5141 Assisted-by: Claude Code (model: Claude Opus 5.5, supervised) * fix(cli): tell Windows uv tool users to stop other specify processes Exiting `specify self upgrade` does not release the Windows lock on the tool environment's python.exe if another process from it is still running, such as an editor's `specify mcp` server. Running the printed uv command then fails partway through the same way. Say to stop those processes first, in both the real run and --dry-run output and in docs/upgrade.md. Refs github#5141 Assisted-by: Claude Code (model: Claude Opus 5.5, autonomous) * test(cli): pin the Windows uv tool no-op when already current On a Windows uv tool install that is already on the latest or requested release, self upgrade reports that and exits before the manual uv command guidance. Cover both cases so the guard can't move ahead of the version check unnoticed. * test(cli): use an absolute installer path in the Windows uv tool tests Since github#5172, self upgrade rejects an installer lookup that returns a relative path, so mocking shutil.which as "uv" or "pipx" now stops the upgrade with "installer rejected". Return sys.executable instead, as the other self upgrade tests on main do, and render the expected non-Windows dry-run command from that path.

Copilot AI dynamic review requested due to automatic review settings October 29, 2026 12:15

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

View session