Validate uv package names against PEP 508 before uv pip show - #51016
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
uv pip show
PR TriageCategory: bug (security hardening) · Risk: medium · Score: 60/100 (impact 25, urgency 18, quality 17) Recommendation: Validates uv package names against PEP 508 before shelling out to
|
There was a problem hiding this comment.
Pull request overview
Adds stricter uv package-name validation to prevent unsafe arguments reaching uv pip show.
Changes:
- Validates extracted uv package names before command execution.
- Adds regression coverage for malformed package names.
Show a summary per file
| File | Description |
|---|---|
pkg/workflow/pip_validation.go |
Adds uv package-name validation and security annotation. |
pkg/workflow/argument_injection_test.go |
Tests rejection of malformed uv package names. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 3
- Review effort level: Balanced
|
|
||
| // Validate the package name against PyPI naming rules (PEP 508) before | ||
| // passing it as a command argument to uv (argument injection guard). | ||
| if err := validatePipPackageName(pkgName); err != nil { |
There was a problem hiding this comment.
Fixed in 242d733: added stripUvPackageVersion which strips a trailing @version (in addition to ==version) before validation and before passing the name to uv pip show. Added TestValidateUvPackages_AcceptsVersionedUvxSpec to cover uvx ruff@0.1.0.
| if _, err := fileutil.ResolveExecutablePath("uv"); err != nil { | ||
| t.Skip("uv not installed - skipping uv argument validation test") | ||
| } |
There was a problem hiding this comment.
Fixed in 242d733: package-name syntax validation (including the - prefix and PEP 508 checks) now runs before uv/pip are resolved or invoked, so TestValidateUvPackages_RejectsInvalidPackageName no longer skips when uv is absent.
| // passing it as a command argument to uv (argument injection guard). | ||
| if err := validatePipPackageName(pkgName); err != nil { | ||
| pipValidationLog.Printf("Invalid uv package name %s: %v", pkgName, err) | ||
| errors = append(errors, fmt.Sprintf("uv package '%s' is invalid: %v", pkg, err)) |
There was a problem hiding this comment.
Fixed in 242d733: invalid-name failures are now collected separately and returned immediately as a distinct NewValidationError with a message about PEP 508 naming rules, before any network/cache validation is attempted. They no longer share the "requires network access or a local cache" message.
|
No ADR enforcement needed: PR does not have the implementation label and has ≤100 new lines of code in business logic directories (33 additions). |
|
|
|
|
|
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
The PEP 508 validation guard added to validateUvPackages is a good step. Existing review comments from a previous pass already flag the three issues worth addressing:
- Versioned
uvxspecs rejected (uvx ruff@0.1.0) — only==is stripped before the name check, so@-versioned specs (whichextractUvFromCommandsexplicitly supports) will be incorrectly rejected. - Test skips on hosts without
uv— the invalid-name branch should never needuv; the skip guard makes the test non-deterministic. Remove thefileutil.ResolveExecutablePathguard from this particular test. - Misleading aggregate error message — invalid-name failures are mixed into the same
errorsslice as network/cache failures, so the finalNewValidationErrormay suggest retrying with a cache when the real fix is to rename the package.
No additional issues found beyond those already raised.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · sonnet46 · 13.1 AIC · ⊞ 5.3K
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on three issues before merge.
📋 Key Themes & Highlights
Key Themes
- Versioned package spec regression (
@-pinned specs likeruff@0.1.0fail the new PEP 508 check because only==is stripped before validation) - Flaky / skipped test — the regression test guards on the
uvbinary check before exercising validation logic, so it is a no-op in most CI environments - Misleading error message — invalid-name errors are mixed into the same slice as network/cache failures, producing confusing user-facing messages
All three are captured in the existing inline comments. Please address them before merge.
Positive Highlights
- ✅ Root cause correctly identified:
validatePipPackageNamenow consistently applied to theuvpath - ✅
#nosec G204comment is accurate and well-scoped - ✅ PR description audit table is clear and helpful
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 16.3 AIC · ⊞ 7.1K
Comment /matt to run again
Test Quality Sentinel Report - placeholder
|
There was a problem hiding this comment.
Verdict: Request changes — the fix introduces a confirmed correctness regression
The added uv package-name validation correctly closes the argument-injection gap for malicious names, but it strips only == (pip-style) version specifiers before validating, not the @ version separator that uvx uses (e.g. ruff@0.1.0). This causes legitimate, previously-working uvx tool@version custom steps to be rejected at compile time — verified locally against validatePipPackageName.
Themes and additional concerns
- Correctness (blocking):
@-pinned uvx specs are now hard-rejected as "invalid package names" — a false positive that breaks working workflows. - UX/error clarity: invalid-name failures are folded into the same aggregate error as network/cache failures, producing a misleading "install uv/pip, check network" message for what is actually a rejected package name.
- Test coverage gap: the new regression test skips entirely when
uvisn't installed on the runner, so the injection guard may never be exercised in CI; the PR author flagged this same concern in the description without resolving it.
The security intent is sound and matches the existing pip guard pattern, but the @ handling gap must be fixed before merge, and the fallback/test-skip behavior needs a decision (hard-fail both paths, or make the test independent of uv availability).
🔎 Code quality review by PR Code Quality Reviewer · auto · 48 AIC · ⊞ 7.8K
Comment /review to run again
| pkgName = pkg[:eqIndex] | ||
| } | ||
|
|
||
| // Validate the package name against PyPI naming rules (PEP 508) before |
There was a problem hiding this comment.
This rejects valid versioned uvx specs like ruff@0.1.0 as invalid package names, since only pip-style == is stripped, not the @ version separator that extractUvFromCommands explicitly supports.
💡 Missing `@` version-specifier handling breaks legitimate uvx workflows
extractUvFromCommands (pkg/workflow/pip.go) produces package specs like ruff@0.1.0 for uvx ruff@0.1.0 (covered by the existing test uvx with version in pip_test.go). This new code only strips ==:
pkgName := pkg
if eqIndex := strings.Index(pkg, "=="); eqIndex > 0 {
pkgName = pkg[:eqIndex]
}pypiPackageNameRE (^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$) does not allow @, so validatePipPackageName("ruff@0.1.0") fails, and the workflow compile now hard-errors on a perfectly valid, previously-working uvx tool@version custom step. I verified this directly:
err=invalid pip package name: "ruff@0.1.0" — PyPI names must start and end with a letter or digit...
Fix: strip an @version suffix (in addition to ==) before validating, e.g.:
pkgName := pkg
if eqIndex := strings.Index(pkg, "=="); eqIndex > 0 {
pkgName = pkg[:eqIndex]
} else if atIndex := strings.Index(pkg, "@"); atIndex > 0 {
pkgName = pkg[:atIndex]
}This is a real behavioral regression, not just a style nit — it will break existing CI workflows that pin uvx tool versions with @.
There was a problem hiding this comment.
Fixed in 242d733: added stripUvPackageVersion which strips a trailing @version (in addition to ==version) before validation, and covered uvx tool@version with a new regression test.
| // passing it as a command argument to uv (argument injection guard). | ||
| if err := validatePipPackageName(pkgName); err != nil { | ||
| pipValidationLog.Printf("Invalid uv package name %s: %v", pkgName, err) | ||
| errors = append(errors, fmt.Sprintf("uv package '%s' is invalid: %v", pkg, err)) |
There was a problem hiding this comment.
Invalid-name failures are merged into the same errors slice as network/cache failures, so the final aggregate error message (below) always claims validation "requires network access or a local cache", which is misleading for a plain naming violation.
💡 Aggregate error message is wrong for invalid-name failures
Further down in this function, NewValidationError is built from the combined errors slice with guidance like "validation requires network access or a local cache; install uv/pip...". A user hitting uvx pkg;whoami will be told to install uv/pip or check their network — completely unrelated to the actual problem (an invalid package name). This makes the actionable security rejection confusing and harder to diagnose, undermining the value of the new guard.
Suggested fix: track invalid-name errors in a separate slice/counter and surface a distinct, accurate error message for them (e.g. "invalid package name(s): ...") instead of folding them into the generic network/cache guidance.
There was a problem hiding this comment.
Fixed in 242d733: invalid-name failures are now tracked separately and returned as a distinct error before any network/cache validation, with a message about PEP 508 naming rules instead of the generic network/cache guidance.
|
|
||
| // TestValidateUvPackages_RejectsInvalidPackageName verifies that uv package names | ||
| // which do not conform to the PyPI naming rules are rejected before being passed | ||
| // as arguments to the uv CLI. |
There was a problem hiding this comment.
This regression test silently t.Skips in any CI environment without uv installed, meaning the argument-injection guard being added in this PR may never actually run in CI.
💡 Security regression test can silently never execute
validateUvPackages has two code paths: the uv-based path (where this PR adds the fix) and a pip-fallback path (validateUvPackagesWithPip) used when uv is absent, which the PR description itself says treats invalid names as warnings, not errors. If the CI runner lacks uv, this test — the only regression coverage for the new guard — skips entirely and gives false confidence that the injection is blocked.
Also note the PR author flagged this exact gap ("Note on the test guard... skips when uv is absent... Worth a second opinion on whether that fallback should also hard-fail") but shipped without resolving it.
Fix: don't gate the test on uv being present. Either (a) call validatePipPackageName directly to assert rejection independent of which downstream tool is available, or (b) make the pip-fallback path also hard-fail on invalid names so both paths are equally protected and testable without uv.
There was a problem hiding this comment.
Fixed in 242d733: package-name validation now runs before uv/pip are resolved or invoked, so the test no longer skips when uv is absent and reliably exercises the guard in CI.
|
@copilot run pr-finisher skill |
|
@copilot Please address the remaining review feedback, refresh the branch if needed, and run the Open review items to address:
Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31185909757
|
…stic test Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
|
@copilot run pr-finisher skill |
|
@copilot Please address the remaining review feedback, failed checks, refresh the branch if GitHub can update it cleanly, and run the Open review items to address (newest first):
Failed checks:
Branch refresh was requested. Run context: https://github.com/github/gh-aw/actions/runs/31192669795
|
Current head |
|
🎉 This pull request is included in a new release. Release: |
Sighthound flagged five Critical command-injection findings around
exec.Commandcalls built from dynamic data. Four of the five are already mitigated in the current code; the exception is theuvvalidation path, which only rejected--prefixed names.Audit of reported findings
pip_validation.go—pip index versions <pkg> --prevalidatePipPackageName(PEP 508 allowlist)dependabot.go—npm install --package-lock-onlynpmPathresolved viafileutil.ResolveExecutablePathpoutine.go—docker run -v <volumeMount>buildDockerVolumeMountcanonicalizes and validates host path (absolute, no:) and container pathpip_validation.go—uv pip show <pkg> --no-cacheChanges
pkg/workflow/pip_validation.go:validateUvPackagesnow appliesvalidatePipPackageNameto each extracted package name before theexec.Commandcall, matching the pip path. Invalid names are collected into the existingerrorsslice and surfaced as a validation error rather than reachinguv. Added a#nosec G204justification consistent with the pip call site.pkg/workflow/argument_injection_test.go: regression test asserting a malformed name is rejected.Package extraction is permissive enough to yield names like
pkg;whoamifromuvx pkg;whoamiin workflow custom steps, which previously flowed straight into the argv:Note on the test guard: it skips when
uvis absent, becausevalidateUvPackagesthen falls back to the pip path where invalid names are warnings rather than errors. Worth a second opinion on whether that fallback should also hard-fail.Run context: https://github.com/github/gh-aw/actions/runs/31185909757> Generated by 👨🍳 PR Sous Chef · gpt54 · 9.44 AIC · ⊞ 8.3K · ◷