fix(security): validate package spec before scanner source fetch (MCP-2442 P0)#672
Conversation
Deploying mcpproxy-docs with
|
| Latest commit: |
f6135c9
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://756c9a06.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://sec-mcp-2442-validate-packag.mcpproxy-docs.pages.dev |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27536590163 --repo smart-mcp-proxy/mcpproxy-go
|
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
61b5896 to
d2f5a1a
Compare
d2f5a1a to
d3d873a
Compare
|
codex review (gpt-5.5) → REQUEST_CHANGES (independently re-verified against the diff) Blocking bug: Fix: validate the whole spec including the |
The scanner's published-source fetch passed the server's configured package spec to `pip download` / `uv pip download` UNVALIDATED. For a local-path, `file:`, URL, or VCS (`git+…`, ssh) spec, those commands still invoke the package's setup.py / PEP 517 build backend to resolve metadata — EVEN with `--only-binary=:all:` — executing untrusted code from the server config on the meant-to-be-static scan path. Add validatePackageSpec(ecosystem, spec): require a bare PEP 503 / npm registry name (optionally version-pinned); reject paths, `file:`, URLs, and VCS/ssh specs. resolveFromPackageFetch now gates both the npm and python fetch paths through it, falling back to tool_definitions_only (no fetch, no execution) on rejection. The already-safe registry-name path is unchanged. TDD: TestValidatePackageSpec (registry pass / non-registry reject) and TestResolveFromPackageFetch_RejectsNonRegistrySpec (setup.py execution marker stays absent for a local-path/git+ spec). Related MCP-2442
…ypass Codex re-review of #672 found validatePackageSpec only validated the parsed NAME, not the version/@-tail. A PEP 508 / npm direct reference ("pkg@./local", "pkg@/abs/path", "pkg@git+https://…") therefore passed — the name "pkg" is a valid registry name — while the full untrusted spec still reached `pip download` / `uv pip download` / `npm pack` and executed setup.py. The P0 was not actually closed. Validate the WHOLE spec: - New bareVersionRe: the "name@<tail>" / "name==<tail>" tail must be a bare version / range / npm dist-tag — must start alphanumeric and contain no '/' or ':' — so a path/URL/VCS tail is rejected. - Python: any '@' is a PEP 508 direct reference (pins use '=='), so it is refused outright. - Bare version pins / dist-tags (pkg@1.2.3, pkg@latest, @scope/pkg@2.0.0) stay valid — no regression. TDD: TestValidatePackageSpec gains npm+python "pkg@<path/url/vcs>" reject cases and bare-version positive cases; TestResolveFromPackageFetch_ RejectsNonRegistrySpec gains uvx/npx direct-reference cases asserting the setup.py execution marker stays absent. Related MCP-2442 Co-Authored-By: Paperclip <noreply@paperclip.ing>
d3d873a to
f6135c9
Compare
Re-review fix pushed — direct-reference
|
There was a problem hiding this comment.
codex re-review ACCEPT (gpt-5.5): validatePackageSpec now checks the FULL original spec incl. the @-tail before any download; all direct-reference bypass forms (pkg@./local, pkg@/abs, pkg@git+..., pkg@file:, URLs, VCS, python @ direct refs) rejected; legitimate pins/dist-tags (pkg@1.2.3, pkg@latest, @scope/pkg@2.0.0, pkg==1.0) still pass; setup.py-exec path unreachable for bypass forms. Closes the P0 hole from round 1.
[SEC/P0] Scanner fetch executes untrusted setup.py for non-registry specs
Source: Codex security audit of the scanner program (PRs #653–#670).
internal/security/scanner/package_fetch.go.Bug
resolveFromPackageFetchpassed the server's configured packagespectopip download/uv pip download/npm packUNVALIDATED. For a uvx/pipx server configured with a local-path,file:, URL, or VCS (git+…,[email protected]:…) spec,pip download <spec> --only-binary=:all:STILL executessetup.py(PEP 517 build backend,egg_infometadata resolution) during what is meant to be a static scan — arbitrary code execution from untrusted server config.--only-binary=:all:alone protects only bare registry-name specs; that was the gap.Fix
New
validatePackageSpec(ecosystem, spec): require a bare PEP 503 (Python) / npm registry name (optionally version-pinned). Reject paths,file:, URLs, and VCS/ssh specs (:/\markers, path prefixes,..traversal).resolveFromPackageFetchgates both the npm and python fetch paths through it and falls back totool_definitions_only(no fetch, no execution) on rejection.npm pack --ignore-scripts+ wheel-only--only-binary=:all:).Tests (TDD — written failing first)
TestValidatePackageSpec— registry names pass; local-path /git+/ ssh / URL /file:/ traversal / drive-letter specs rejected (npm + python).TestResolveFromPackageFetch_RejectsNonRegistrySpec— a local-path /git+uvx spec and a./localnpx spec return an error (fallback) and asetup.pyexecution marker stays absent.Verification
go build ./...✅go test ./internal/security/... -race✅golangci-lint(CI config, v2) — 0 issues ✅Docs:
docs/features/security-scanner-plugins.mdupdated to document the registry-name requirement.Related MCP-2442
🚫 Do not merge — pre-merge gate is human-owned (Gate 3).