fix(scanner): reject non-registry package specs before fetch (MCP-2442 P0 ACE)#673
Closed
Dumbris wants to merge 1 commit into
Closed
fix(scanner): reject non-registry package specs before fetch (MCP-2442 P0 ACE)#673Dumbris wants to merge 1 commit into
Dumbris wants to merge 1 commit into
Conversation
Member
Author
|
Tracking / coordination note
This is a P0 ACE fix — please prioritize review/merge. |
Deploying mcpproxy-docs with
|
| Latest commit: |
acbfbcc
|
| Status: | ⚡️ Build in progress... |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27522692998 --repo smart-mcp-proxy/mcpproxy-go
|
Codex audit (MCP-2442): resolveFromPackageFetch passed the server-configured package spec to npm pack / uv pip download / pip download UNVALIDATED. For a uvx/pipx server with a local-path, git+/VCS, URL, or file: spec, the download STILL executes the package's build backend (setup.py / PEP 517 egg_info) even with --only-binary=:all: / --ignore-scripts — arbitrary code execution during a supposedly static scan. Add isBareRegistrySpec(spec, ecosystem) and gate both ecosystems in resolveFromPackageFetch: only a bare PEP 503 (python) / npm registry name (+ optional version pin) is fetched. Paths, git+/hg+/svn+/bzr+, URLs (://), file:, PEP 508 direct references (name @ url), and shell metacharacters are refused → caller falls back to tool_definitions_only (no fetch, no execution). The registry-name path (already wheel-only / --ignore-scripts) is unchanged. TDD: isBareRegistrySpec accept/reject table; resolveFromPackageFetch rejection for git+/local/URL specs; and an execution-marker test proving a local-path uvx spec never runs its setup.py (validation short-circuits before any download, independent of whether uv/pip is installed). Related #MCP-2442 Co-Authored-By: Paperclip <noreply@paperclip.ing>
Dumbris
force-pushed
the
fix/mcp-2442-scanner-fetch-spec-validation
branch
from
88f28a0 to
acbfbcc
Compare
Member
Author
|
Duplicate of #672 (canonical MCP-2442 fix, includes docs); closing to avoid conflicting package_fetch.go edits. |
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
2 participants
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.
Summary — CRITICAL/P0 arbitrary code execution on the scan path
From the Codex security audit (MCP-2442).
resolveFromPackageFetchhanded the server-configured package spec tonpm pack/uv pip download/pip downloadunvalidated. For auvx/pipxserver configured with a local-path,git+/VCS, URL, orfile:spec, the download still executes the package's build backend (setup.py/ PEP 517egg_info) even with--only-binary=:all:/--ignore-scripts— arbitrary code execution during a supposedly static scan.--only-binary=:all:only protects bare registry-name specs (a wheel-less registry package falls back cleanly). It does not protect a non-registry spec, which is the whole bug.Fix
New
isBareRegistrySpec(spec, ecosystem)gates both ecosystems inresolveFromPackageFetchbefore any download:@scope/name) and version-pinned (==,>=,@1.2.3)../,../,/abs,~,\,..),git+/hg+/svn+/bzr+, URLs (://),file:, PEP 508 direct references (name @ url), and shell metacharacters → caller falls back totool_definitions_only(no fetch, no execution).npm pack --ignore-scripts) is unchanged — no regression.Tests (TDD)
TestIsBareRegistrySpec— accept/reject table for both ecosystems (incl.@scope/../escape,git+ssh, direct refs).TestResolveFromPackageFetch_RejectsNonRegistrySpec— git+/local/URL specs short-circuit with a rejection before any download command runs.TestResolveFromPackageFetch_LocalPathSpecDoesNotExecute— execution-marker proof: a local-path uvx spec whosesetup.pywould write a marker never executes (marker absent), independent of whether uv/pip is installed.Verification
go build ./...✅go test -race ./internal/security/scanner/✅ (new + existing fetch tests, registry path unchanged)./scripts/run-linter.sh→0 issues✅ ·gofmtclean ✅Branched from
origin/mainper the issue note (local tree was divergent).Related #MCP-2442