Skip to content

Validate uv package names against PEP 508 before uv pip show #51016

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 7 commits into from
Aug 7, 2026
+74 −10
Merged

Validate uv package names against PEP 508 before uv pip show #51016

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
a6b2fde
Initial plan
Copilot Aug 7, 2026
34fd315
Validate uv package names against PEP 508 before uv pip show
Copilot Aug 7, 2026
c716df6
Merge branch 'main' into copilot/sighthound-security-findings-again
github-actions[bot] Aug 7, 2026
242d733
Fix uv @version stripping and separate invalid-name errors; determini…
Copilot Aug 7, 2026
dbefd32
Merge branch 'main' into copilot/sighthound-security-findings-again
Copilot Aug 7, 2026
304ca65
Merge branch 'main' into copilot/sighthound-security-findings-again
github-actions[bot] Aug 7, 2026
d6b4bb5
Merge branch 'main' into copilot/sighthound-security-findings-again
pelikhan Aug 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions pkg/workflow/argument_injection_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"fmt"
"strings"
"testing"

"github.com/github/gh-aw/pkg/fileutil"
)

// TestRejectHyphenPrefixPackages tests the shared helper that guards against
Expand Down Expand Up @@ -395,3 +397,23 @@ func TestValidatePipPackageName(t *testing.T) {
})
}
}

// 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.

@github-actions github-actions Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

func TestValidateUvPackages_RejectsInvalidPackageName(t *testing.T) {
if _, err := fileutil.ResolveExecutablePath("uv"); err != nil {
t.Skip("uv not installed - skipping uv argument validation test")
}

Copilot AI Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


compiler := NewCompiler()
err := compiler.validateUvPackages(&WorkflowData{
CustomSteps: "uvx pkg;whoami",
})
if err == nil {
t.Fatal("expected error for invalid uv package name but got none")
}
if !strings.Contains(err.Error(), "invalid pip package name") {
t.Errorf("expected error to mention invalid package name, got: %v", err)
}
}
11 changes: 11 additions & 0 deletions pkg/workflow/pip_validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,18 @@ func (c *Compiler) validateUvPackages(workflowData *WorkflowData) error {
pkgName = pkg[:eqIndex]
}

// Validate the package name against PyPI naming rules (PEP 508) before

@github-actions github-actions Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 @.

Copilot AI Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 {

Copilot AI Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

pipValidationLog.Printf("Invalid uv package name %s: %v", pkgName, err)
errors = append(errors, fmt.Sprintf("uv package '%s' is invalid: %v", pkg, err))

Copilot AI Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@github-actions github-actions Bot Aug 7, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copilot AI Aug 7, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

continue
}

// Use uv pip show to check if package exists on PyPI
// #nosec G204 -- uvPath is resolved from the hardcoded executable name "uv" via
// fileutil.ResolveExecutablePath; pkgName is validated above by validatePipPackageName
// against the strict PyPI PEP 508 allowlist.
cmd := exec.Command(uvPath, "pip", "show", pkgName, "--no-cache")
_, err := cmd.CombinedOutput()

Expand Down
Loading