[PPSC-958] fix(supply-chain): correct PyPI prerelease misclassification#222
Open
yiftach-armis wants to merge 2 commits into
Open
[PPSC-958] fix(supply-chain): correct PyPI prerelease misclassification#222yiftach-armis wants to merge 2 commits into
yiftach-armis wants to merge 2 commits into
Conversation
… filter summary (PPSC-958) PyPI BlockedPackage.Version holds a filename (filelock-3.29.2.tar.gz), but isPrerelease() split on the first '-' and read every PyPI package as a prerelease. That tripped the onlyPrerelease branch and printed the false "withheld N prereleases; a default install was unaffected" header for stable releases the proxy actually downgraded. Classify on a normalized DisplayVersion (semver parsed from the filename) instead of the raw Version. Also flip the per-line summary to lead with the installed safe version and its age, demoting the skipped version to a trailing clause; thread the installed version's age through the proxy to support it.
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 1 |
Total: 1
View all 1 findings
🟠 HIGH (1)
CWE-918 - Server-Side Request Forgery (CWE-918
Location: internal/supplychain/proxy.go:258
Server-Side Request Forgery (SSRF)): The proxy receives an HTTP request from a client and then builds a new request to the upstream registry by simply appending the client‑provided path and query string to a fixed base URL. Because the code does not check or filter what the client sends, a user can supply any path or query they like. If the upstream registry responds with a redirect, the Go HTTP client will automatically follow that redirect, which can lead the proxy to contact a completely different server chosen by the attacker. This means an attacker can make the proxy reach internal or external services that it should not access. The proxy runs its own HTTP server, so the vulnerable code can be triggered by anyone who can send a request to that server. No sanitizing or validation steps are present, so the problem is exploitable as described.
p.handleMetadataFiltering(w, r, pkgName)
}
func (p *Proxy) handleMetadataFiltering(w http.ResponseWriter, r *http.Request, pkgName string) {
// Use RequestURI() (escaped path + raw query) rather than just Path so the
// filtered branch is symmetric with the reverse-proxy passthrough: query
// params (e.g. ?write=true) and path-escaping nuances reach the upstream.
// armis:ignore cwe:918 reason:p.upstreamURL is a startup-configured trusted host (defaults to registry.npmjs.org); r.URL.RequestURI() is the path/query from the local proxy client and cannot change the host
upstreamReq, err := http.NewRequestWithContext(r.Context(), http.MethodGet, p.upstreamURL.String()+r.URL.RequestURI(), nil) //nolint:gosec // upstream URL is configured at startup, path is from local proxy
if err != nil {
http.Error(w, fmt.Sprintf("[armis] supply-chain: failed to create request for %s", pkgName), http.StatusBadGateway)
return
}
if p.mode == ModePyPI {
// Request the PEP 691 JSON form so the response carries PEP 700 per-file
// upload-time fields; the default Simple API HTML has no timestamps.
upstreamReq.Header.Set("Accept", pypiSimpleJSONAccept)
} else {
upstreamReq.Header.Set("Accept", "application/json")
}
CWEs: CWE-918: Server-Side Request Forgery (SSRF)
Test Coverage Reporttotal: (statements) 72.2% Coverage by function |
There was a problem hiding this comment.
Pull request overview
Fixes supply-chain summary behavior so PyPI blocked artifact filenames (e.g. filelock-3.29.2.tar.gz) are no longer misclassified as prereleases, and updates summary output to lead with the resolved/installed safe version (including its age).
Changes:
- Add
BlockedPackage.DisplayVersion(normalized version for display/classification) and thread resolved version age viaInstalledPackage.Age. - Update proxy bookkeeping to store resolved safe version + age (
allowedVersion) and expose it throughProxy.Allowed(). - Revise CLI summary line layout to “installed … — skipped …” and add regression coverage for PyPI filename parsing/misclassification.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/supplychain/proxy.go | Adds DisplayVersion, threads resolved version age through allowed, and parses PyPI versions from filenames for display/classification. |
| internal/supplychain/proxy_test.go | Adjusts proxy tests to use the new allowedVersion map type. |
| internal/cmd/supply_chain_wrap.go | Updates summary formatting and switches prerelease filtering/display to use normalized blocked versions and resolved-age tokens. |
| internal/cmd/supply_chain_wrap_summary_test.go | Updates summary tests for new layout and adds PyPI filename regression test. |
| docs/CHANGELOG.md | Documents the PyPI prerelease misclassification fix and summary output changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
444
to
446
| func ageToken(age time.Duration) string { | ||
| return fmt.Sprintf("(%s old)", formatDurationShort(age)) | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Fixed in 3cafef3. The — skipped … clause now uses optionalAgeToken, which returns empty for non-positive ages, so an undatable file (Age == 0) or a clock-skew negative renders as — skipped 1.0.0 with no age rather than a false (0 minutes old). Added TestPrintBlockSummary_UndatableSkippedOmitsAge.
Comment on lines
640
to
645
| } | ||
| if bestVersion != "" { | ||
| p.allowedMu.Lock() | ||
| p.allowed[pkgName] = bestVersion | ||
| p.allowed[pkgName] = allowedVersion{version: bestVersion, age: bestAge} | ||
| p.allowedMu.Unlock() | ||
| } |
Collaborator
Author
There was a problem hiding this comment.
Good catch — fixed in 3cafef3. Consolidated the two duplicate isPrerelease helpers into one exported supplychain.IsPrerelease that now recognizes PEP 440 prereleases (1.0.0rc1, 1.0.0b2, 1.0.0.dev1, etc.) in addition to the SemVer - form, with .postN correctly staying stable. It's used both when choosing bestVersion (so an rc/dev is no longer picked as the safe fallback) and in the allResultsPrerelease framing. Added TestIsPrerelease and a proxy test that an age-passing rc is skipped in favor of the newest stable.
…ped age (PPSC-958) Addresses two PR review findings on the prerelease/summary fix: 1. isPrerelease only recognized SemVer "-" suffixes, so PyPI PEP 440 prereleases (1.0.0rc1, 1.0.0b2, 1.0.0.dev1 — no dash) were treated as stable. The proxy could then pick an rc/dev as the safe fallback and the summary could misframe the result. Consolidate the two duplicate isPrerelease copies into one exported supplychain.IsPrerelease that handles both grammars (SemVer dash tag + dash-less PEP 440 markers; post-releases stay stable). 2. The "— skipped" clause called ageToken(OldAge) directly, printing "(0 minutes old)" for undatable PyPI files (Age == 0) or under clock skew. Use the optional age token so the clause names the version and omits a false age. Adds TestIsPrerelease, a proxy test that an old PEP 440 rc is not chosen as the fallback, and a summary test that an undatable skipped version omits its age.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Related Issue
Type of Change
Problem
The supply-chain filter summary printed
withheld N prereleases; a default install was unaffectedfor PyPI packages that were actually stable releases the proxy downgraded — the opposite of what happened. Root cause: for PyPI,BlockedPackage.Versionholds a filename (filelock-3.29.2.tar.gz), butisPrerelease()split on the first-and read every PyPI package as a prerelease, tripping theonlyPrereleaseframing.Solution
Classification now runs on a normalized
DisplayVersion(semver parsed from the filename) rather than the rawVersion, so PyPI filenames are no longer misread (genuine npm prereleases like2.0.0-alpha.1still classify correctly). The per-line summary is also flipped to lead with the installed safe version and its age, demoting the skipped version to a trailing clause (● superdialog 0.2.3 installed (8 days old) — skipped 0.2.5 (6 hours old)); this required threading the resolved version's age through the proxy (InstalledPackage.Age), which was previously computed then discarded.Testing
Automated Tests
Manual Testing
Rendered all four summary tiers (resolved / mixed-unresolved / install-failed / genuine-prerelease) and confirmed correct framing. Added regression test
TestPrintBlockSummary_PyPIFilenameNotPrereleaseusing the reportedfilelock/superdialogfilenames. Full suite green (2545 passed, 71.3% coverage),golangci-lint0 issues, diff security scan clean.Reviewer Notes
BlockedPackage.Versionstays the raw filename intentionally (it's what makes a block actionable and what proxy tests assert on); only the newDisplayVersionis used for classification/display.Checklist