[PPSC-879] feat(supply-chain): DX polish — wrap summary, error UX, CI docs#212
Open
yiftach-armis wants to merge 1 commit into
Open
[PPSC-879] feat(supply-chain): DX polish — wrap summary, error UX, CI docs#212yiftach-armis wants to merge 1 commit into
yiftach-armis wants to merge 1 commit into
Conversation
|
| Severity | Count |
|---|---|
| 🟠 HIGH | 1 |
Total: 1
View all 1 findings
🟠 HIGH (1)
CWE-20 - Injection (CWE-20
Location: internal/supplychain/proxy.go:227
Improper Input Validation): 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 the trusted base URL. No checks are done to make sure the client’s path is safe or well‑formed. Because the proxy forwards whatever the client sends, an attacker who can talk to the proxy can cause it to request any path on the upstream server, potentially reaching endpoints that should not be exposed through the proxy. This lack of validation makes the code vulnerable to improper input validation (CWE‑20). The risky operation (building the upstream URL) is directly reachable from the client’s request, and there is no code that sanitizes or restricts the input before it is used.
p.checkedMu.Unlock()
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-20: Improper Input Validation
Test Coverage Reporttotal: (statements) 72.1% Coverage by function |
There was a problem hiding this comment.
Pull request overview
This PR polishes and hardens the supply-chain feature set by improving local “wrap” enforcement feedback, adding proper PyPI proxy filtering for pip/uv, tightening CLI error/exit behavior for CI safety, and expanding CI/documentation guidance.
Changes:
- Add PyPI-aware proxy mode (PEP 691/700 JSON) and plumbing so pip/uv installs are actually filtered by release age, alongside improved proxy passthrough reliability.
- Improve CLI UX and CI correctness: non-zero exit on unknown
supply-chainsubcommands,--fail-oncase normalization, bounded git base-lockfile detection, and cleaner human output for local audits. - Add/expand documentation and examples for Supply Chain Protection, including a GitHub Actions workflow sample and changelog entries.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| README.md | Adds a new “Supply Chain Protection” section with usage examples and configuration guidance. |
| internal/supplychain/supplychain.go | Improves ParseDuration error messaging to avoid duplicated/unclear errors. |
| internal/supplychain/supplychain_test.go | Extends severity-boundary tests (e.g., exactly-at-threshold behavior). |
| internal/supplychain/shell_test.go | Adds tests for shell detection ordering and empty detection behavior. |
| internal/supplychain/registry/pypi_test.go | Adds batch publish-date resolution tests and context-cancellation behavior. |
| internal/supplychain/proxy.go | Introduces ProxyMode (npm vs PyPI), PyPI Simple JSON filtering, and Host header rewrite for passthrough reliability. |
| internal/supplychain/proxy_test.go | Adds coverage for Host rewrite behavior, Addr(), and end-to-end PyPI proxy filtering. |
| internal/supplychain/config.go | Adds ecosystems config scoping, validation helpers, and alias handling (pipenv vs pipfile). |
| internal/supplychain/config_test.go | Adds tests for new config parsing/scoping semantics and unknown-ecosystem detection. |
| internal/supplychain/check/check.go | Adds an injectable registry seam for testability and exports ecosystem detection helper. |
| internal/supplychain/check/check_test.go | Adds unit and end-to-end tests for the RunCheck pipeline and ecosystem routing. |
| internal/output/human.go | Omits “Scan ID:” line when empty (e.g., local supply-chain audits). |
| internal/output/human_format_test.go | Adds regression test ensuring empty Scan ID is not rendered. |
| internal/cmd/supply_chain.go | Adds warning for unknown ecosystem names and non-zero exit behavior for unknown subcommands (with suggestions). |
| internal/cmd/supply_chain_wrap.go | Adds exec indirection for tests, ecosystem scoping for wrap, PyPI-vs-npm proxy selection, and expanded wrap summaries/warnings. |
| internal/cmd/supply_chain_wrap_test.go | Adds wrap path tests (bypass/off/scope/proxy env injection) and Gradle staleness warning path coverage. |
| internal/cmd/supply_chain_wrap_summary_test.go | Adds comprehensive tests for the new wrap summary rendering and rationale marker behavior. |
| internal/cmd/supply_chain_wrap_pm_test.go | Updates PM-to-ecosystem mapping tests (now includes proxied PMs + Java PMs). |
| internal/cmd/supply_chain_test.go | Adds regression tests for unknown subcommand errors and case-insensitive --fail-on. |
| internal/cmd/supply_chain_init.go | Makes init honor ecosystem scoping when choosing which PMs to wrap; expands config template docs. |
| internal/cmd/supply_chain_init_test.go | Adds test ensuring init respects ecosystem scoping. |
| internal/cmd/supply_chain_check.go | Adds ecosystem-scope skip gate, bounds git subprocesses via context timeout, normalizes --fail-on handling, and improves summary wording. |
| internal/cmd/supply_chain_check_test.go | Adds tests for base-lockfile detection behaviors and ecosystem-scope skip behavior. |
| docs/ci-examples/github-actions-supply-chain.yml | Adds a GitHub Actions example workflow for supply-chain check. |
| docs/CHANGELOG.md | Documents new supply-chain capabilities and multiple fixes/security behavior changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+452
to
+457
| var glyph, outcome string | ||
| switch { | ||
| case r.NewVersion == "": | ||
| glyph = s.WarningText.Render("⚠") | ||
| outcome = s.WarningText.Render("no older safe version (install may fail)") | ||
| case mixed: |
Comment on lines
+676
to
+690
| func wrapEcosystemEnforced(canonicalPMName string) bool { | ||
| eco := pmToEcosystem(canonicalPMName) | ||
| if eco == "" { | ||
| return true | ||
| } | ||
| dir := supplychain.FindConfigDir(".") | ||
| if dir == "" { | ||
| return true | ||
| } | ||
| cfg, err := supplychain.LoadConfig(dir) | ||
| if err != nil || cfg == nil { | ||
| return true | ||
| } | ||
| return cfg.EnforcesEcosystem(eco) | ||
| } |
shb7628
approved these changes
Jun 4, 2026
Collaborator
shb7628
left a comment
shb7628
left a comment
There was a problem hiding this comment.
is /internal/cmd good or should we have subdirectories for each feature.
i guess it doesn't matter if it's a coding agent or compiler reading this, but it hurts my head a little.
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-chaincommand shipped with minimal user feedback: the proxy wrap gave no summary of what it filtered and why,--fail-onsilently ignored lowercase severities, unknown subcommands exited 0, and there were no CI integration docs.Solution
supply-chain wrapnow prints a per-package summary of every filtered release — showing the too-new version, its age, and the safe older version installed in its place (e.g.axios 1.17.0 (1 day old) → 1.16.1 installed). When the PM can't complete (a pin that only the filtered version satisfies), the summary says "available" instead of "installed" and explains how to relax the constraint. A one-time explanation of the age-enforcement policy is shown on the first filtered interactive install.runProxyWrapnow selectsModePyPIfor pip/uv (PEP 691/700 JSON filtering) vsModeNPMfor npm/pnpm/bun/yarn, fixing filtering for Python package managers routed through the proxy.supply-chain wrapnow skips enforcement when the configuredecosystemslist excludes the current PM's ecosystem, consistent withcheckandinit.--fail-oncase normalization (check): lowercase severities (e.g.--fail-on medium) are now accepted; previously they were silently ignored and the CI gate never fired.supply-chain chekcnow exits non-zero with a "Did you mean" suggestion instead of printing help and exiting 0.docs/ci-examples/github-actions-supply-chain.yml) and expanded READMESupply Chain Protectionsection.execPMFuncindirection: routes all PM exec calls through a replaceable var sorunProxyWrap/runPreInstallBlockare fully unit-testable without spawning real processes.Testing
Automated Tests
Manual Testing
Verified wrap summary output for filtered npm and PyPI packages, ecosystem-scoped bypass, and the install-incomplete warning path.
Reviewer Notes
The
execPMFuncvar replaces directexecPMcalls only in the wrap path — it's the minimal seam needed to make the new summary logic unit-testable (supply_chain_wrap_summary_test.go,supply_chain_wrap_test.go).Security suppressions added for two false-positive CWE-476 findings on
cfg.EnforcesEcosystem— the method has an explicit nil-receiver guard (if c == nil { return true }) that makes nil-cfg calls safe by design.Checklist