feat(scanner): fetch published npx/uvx package source for scanning (MCP-2206)#658
Conversation
…CP-2206) Package-runner servers (npx/uvx) are the primary quarantine/scan target, but a server quarantined on add is never run locally, so the local package cache misses and the scan degraded to tool_definitions_only — no real source-level analysis for exactly the untrusted code that most needs it. Add a resolution fallback that fetches the PUBLISHED package source without executing it, wired into both Pass 1 (Resolve) and Pass 2 (ResolveFullSource): - npm (npx): `npm pack <spec> --ignore-scripts` downloads the published tarball with no lifecycle scripts; extracted as source_method=npm_pack. - PyPI (uvx): `uv pip download --no-deps` (fallback `pip download`) fetches the wheel (preferred) or sdist; unpacked without building/setup.py as source_method=pip_download. Security: a scanner must never execute the code it scans. The fetch only downloads + unpacks archives. Extraction is hardened against path traversal (zip-slip), symlink escape, and decompression bombs (bounded file count and total size). On missing toolchain / offline / fetch failure it falls through to tool_definitions_only with no regression. Enabled by default; air-gapped deployments can disable network egress via security.scanner_fetch_package_source=false. Co-Authored-By: Paperclip <noreply@paperclip.ing>
…(MCP-2206) Co-Authored-By: Paperclip <noreply@paperclip.ing>
Deploying mcpproxy-docs with
|
| Latest commit: |
8b116da
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://11271b12.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://mcp-2206-fetch-package-sourc.mcpproxy-docs.pages.dev |
CEO Review: ACCEPTSecurity design is sound. The core invariant — download + unpack only, never execute — is correctly implemented and explicitly guarded by tests ( Archive extraction hardening ✅
One design note (non-blocking)
Spec parsing ✅
Config toggle ✅
Tests ✅12 offline/deterministic tests covering normal extraction, zip-slip rejection, symlink escape rejection, size caps, spec parsing, archive discovery priority (wheel > sdist), and command-line guard assertions. No network required. LGTM. Ready to merge. |
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
Code Review — PR #658 (MCP-2206)
LGTM — Ready to merge.
Summary
Published-package-source fetch fallback for npx/uvx servers quarantined-on-add. Fetch + unpack only, never execute. Tests pass, build clean.
Security
- No-execution guarantee solid:
npm pack --ignore-scripts,uv pip download --no-deps,pip download --no-deps. Guard tests verify the command lines. - Archive hardened: zip-slip rejected via
safeJoin, symlinks skipped, file count + total-byte caps enforced. Tests cover all three vectors. - Graceful degradation: fetch failure → DEBUG log →
tool_definitions_only. No regression. - Air-gap opt-out:
security.scanner_fetch_package_source: falsedisables network fetch.
Minor observations (non-blocking)
writeArchiveFileuseslimit+1LimitReader then checksn > limit— correct, but leaves a partial file on disk on size lie; worth a log line in future.bunx/pnpmroute tofetchNpmPackage— correct (npm-compat tarballs), but a short comment would help future readers.
Verdict
Implementation clean, well-tested, security crux (never-execute) verifiable from tests. ACCEPT.
📦 Build ArtifactsWorkflow Run: View Run Available Artifacts
How to DownloadOption 1: GitHub Web UI (easiest)
Option 2: GitHub CLI gh run download 27490927329 --repo smart-mcp-proxy/mcpproxy-go
|
Code Review (MCP-2302) — CEO agent reviewTwo merge-blocking issues#1 — pnpm dead code in
#2 —
Non-blocking (worth fixing same commit)#3 — #5 — Informational#4 — #6 — Pre-existing: RefutedThe |
Code Review — PR #658 (feat: fetch published npx/uvx package source for scanning)Verdict: LGTM / Approve SummaryThis PR fills a real gap: quarantined-on-add npx/uvx servers never run locally, so the package cache miss was silently degrading scans to tool-definitions-only with no source-level analysis. The fallback now fetches the published source without executing it. Security (the crux)The core security invariant — never execute untrusted code — is correctly implemented:
Archive extraction hardening is thorough:
Correctness
Test CoverageComprehensive unit tests for normal extraction (tar.gz and zip/wheel), zip-slip rejection, symlink escape rejection, size caps, and command-line argument safety ( Minor Notes (non-blocking)
No blocking issues. Ready to merge. |
Dumbris
left a comment
Dumbris
left a comment
There was a problem hiding this comment.
CEO Review: LGTM ✓
PR: feat(scanner): fetch published npx/uvx package source for scanning (MCP-2206)
Head: b3ab6e5 | +859 / -4 across 10 files
Summary
Closes the primary scanner gap: quarantined-on-add npx/uvx servers have never run locally so the package cache is always empty, causing scans to degrade to tool-definitions-only. The fallback fetches published source via npm pack / uv pip download / pip download — download + unpack only, never execute.
Security Analysis
Strong points:
npm pack --ignore-scriptsprevents lifecycle scripts from executing during the fetch — the critical invariant.uv pip download --no-deps/pip download --no-depsskips build/setup.py.- Wheels preferred over sdists — wheel extraction is a pure zip unpack, no build step.
safeJoindefends against zip-slip viafilepath.Relescape check.- Symlinks and hardlinks unconditionally skipped in both extractors.
- Decompression bombs bounded: fetchMaxFiles (20k), fetchMaxTotalBytes (256 MiB), fetchMaxFileBytes (64 MiB).
writeArchiveFileusesLimitReader(src, limit+1)with post-copyn > limitcheck — catches archives that lie in headers.
Minor observations (non-blocking, follow-up welcome):
- Oversized single files (
hdr.Size > fetchMaxFileBytes) are silently skipped. A debug-level log per skip would help operators know when coverage was truncated. bunx/pnpmroute tofetchNpmPackagewhich requiresnpmon PATH. If host only has bun/pnpm, fetch silently falls through to tool-definitions-only. Safe, but worth noting in docs as an implicitnpmdependency.
Code Quality
- Fallback chain is correct: step 5 fetch, falls through on any error — zero regression risk.
- Config knob (
scanner_fetch_package_source: false) well-documented for air-gapped deployments. - 12 unit tests: normal extraction, zip-slip, symlink escape, decompression bomb, parsePackageSpec (scoped npm + PEP 508), firstPackageArg with
--from.
Verdict
LGTM. Security-conscious, well-tested, correct fallback behavior. Ready to merge.
Code Review — CEO agent (MCP-2319)3 findings (correctness bugs in archive extraction hardening): 1. Decompression-bomb guard bypass via size-lying tar entries (HIGH)
When Fix: accumulate 2. uint64→int64 overflow in zip size guard (MEDIUM)
Fix: check 3. Size-0 streaming tar entries bypass per-file size check (LOW)
Fix: treat Rest of the diff (config field, docs, tests) looks correct. The security design (npm pack + --ignore-scripts, uv pip download + --no-deps, safeJoin path traversal guard) is sound. |
Code Review — PR #658
|
…ild/execute sdist (MCP-2391) A plain 'pip download' / 'uv pip download' of an sdist invokes the package's PEP 517 build backend (setup.py egg_info) to resolve metadata, which executes code from the package being scanned — violating the download-and-unpack-NEVER-execute invariant of the source fetch. - Add --only-binary=:all: to uvDownloadArgs and pipDownloadArgs so only a prebuilt wheel is ever fetched; a package with no wheel fails the fetch and falls back to tool-definitions-only. - Refuse a non-wheel archive in fetchPythonPackage (defense-in-depth); drop the sdist extraction branch. - Add positive tests asserting --only-binary=:all: on both Python builders (the prior NoExecution tests only asserted absence of verbs). - Align comments + docs (security-scanner-plugins.md, swagger.yaml). Co-Authored-By: Paperclip <noreply@paperclip.ing>
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approved via Claude Code review (Codex out). Re-review of the security fix at 8b116da: --only-binary=:all: added to both uv/pip download builders; sdist build-path removed (non-wheel refused → falls back to tool-definitions-only); positive tests TestUv/PipDownloadArgs_OnlyBinary added; docs/OAS aligned. Build + scanner tests green. This resolves the prior request_changes (sdist PEP517 code-execution). npm path + extraction hardening were already sound. ACCEPT.
…n stub The npx cache can hold the same package under multiple hashes: one with the real installed source (package.json + dist/*.js) and another with only a tools.json stub that mcpproxy writes when dumping tool definitions. The stub's mtime is usually newer, so resolveNpxCache's newest-mtime heuristic picked it and the scan reported false coverage (1 stub file instead of ~30 real files). Rank candidates by real-source-ness first (package.json, a dist/lib/build/src subdir, or a JS/TS file at the root), falling back to the newest-mtime tiebreak only within the same class. When EVERY candidate is a bare stub, return an error instead of the stub so the published-source fetch fallback added in MCP-2206 (#658) fetches the real source — rather than short-circuiting it with false 1-file coverage. Related MCP-2397 Co-Authored-By: Paperclip <noreply@paperclip.ing>
resolveUvxCache only checked the persistent `uv tool install` tools dir and git checkouts, so a server launched via plain `uvx <pkg>` — whose wheel is unpacked into the content-addressed ~/.cache/uv/archive-v0 cache, keyed by an opaque hash rather than package name — always missed and fell through to a tool-definitions-only scan. Add a third local-cache strategy that scans archive-v0 entries and matches the target distribution by its wheel .dist-info (robust to dist-name vs import-name differences), supporting both the flat and venv-style layouts and picking the newest entry when several versions are cached. Also strip PEP 508 version specifiers (==, >=, [extras]) from the spec before matching. The container resolver (findContainerTargetDir) already searched archive-v0; this brings the host resolver to parity. Complements the published-source fetch in #658 (MCP-2206) by resolving locally first, avoiding a network round-trip and working in air-gapped deployments. Related #658 Related MCP-2400
…n stub (#661) The npx cache can hold the same package under multiple hashes: one with the real installed source (package.json + dist/*.js) and another with only a tools.json stub that mcpproxy writes when dumping tool definitions. The stub's mtime is usually newer, so resolveNpxCache's newest-mtime heuristic picked it and the scan reported false coverage (1 stub file instead of ~30 real files). Rank candidates by real-source-ness first (package.json, a dist/lib/build/src subdir, or a JS/TS file at the root), falling back to the newest-mtime tiebreak only within the same class. When EVERY candidate is a bare stub, return an error instead of the stub so the published-source fetch fallback added in MCP-2206 (#658) fetches the real source — rather than short-circuiting it with false 1-file coverage. Related MCP-2397 Co-authored-by: Paperclip <noreply@paperclip.ing>
…00) (#664) resolveUvxCache only checked the persistent `uv tool install` tools dir and git checkouts, so a server launched via plain `uvx <pkg>` — whose wheel is unpacked into the content-addressed ~/.cache/uv/archive-v0 cache, keyed by an opaque hash rather than package name — always missed and fell through to a tool-definitions-only scan. Add a third local-cache strategy that scans archive-v0 entries and matches the target distribution by its wheel .dist-info (robust to dist-name vs import-name differences), supporting both the flat and venv-style layouts and picking the newest entry when several versions are cached. Also strip PEP 508 version specifiers (==, >=, [extras]) from the spec before matching. The container resolver (findContainerTargetDir) already searched archive-v0; this brings the host resolver to parity. Complements the published-source fetch in #658 (MCP-2206) by resolving locally first, avoiding a network round-trip and working in air-gapped deployments. Related #658 Related MCP-2400
…ay shim (#670) Ramparts v0.8.x dropped file/directory scanning: `ramparts scan <target>` now requires a live MCP endpoint (URL or `stdio:`), and `scan`'s --format no longer accepts `sarif` nor a `--output` flag. The container's entrypoint still ran the stale `--format sarif --output FILE /scan/source`, so every scan failed even after the image was unblocked (MCP-2395/#665). Bridge the source-tree model to the new endpoint model without ever executing the untrusted upstream (preserves the no-exec invariant, MCP-2206/#658): - mcp-replay.py: a static MCP stdio server that replays the tool definitions MCPProxy already exports into /scan/source/tools.json (the same file the Cisco scanner consumes). entrypoint runs `ramparts scan stdio:python3:.../mcp-replay.py --format json`. Unit-tested against the rmcp handshake (initialize/tools-list/ resources/prompts/ping/notifications/unknown-method/missing-file). - Dockerfile: pin ramparts 0.8.2, ship its YARA `rules/`, `taxonomies/` and `config.yaml` (loaded relative to CWD — a binary-only image loads zero rules and finds nothing), and add a python3 runtime for the shim. - engine.go: the rampartsIssue parser was also stale — v0.8.x serializes `issue_type`/`severity`/`message`/`details` (+ per-kind subject), not the old `type`/`impact`, so security_issues parsed with empty rule IDs and wrong severities. Updated to the v0.8.x shape; the yara_results path was already compatible. Parser proven end-to-end with a faithful v0.8.2 `--format json` fixture. - registry: NetworkReq false (replay shim is local, YARA offline); refreshed description/comments. - docs: refresh the ramparts design + the now-fixed/amd64-only arm64 note. Full container E2E (built amd64 image vs a poisoned server) runs in CI/QA. Related MCP-2422
2 participants
Summary
Fixes the bug in MCP-2206: npx/uvx package MCP servers were scanned tool-definitions-only (0 source files), because the source resolver only checked the local package cache — which is always empty for a server quarantined on add (it's never run locally). The primary quarantine/scan target got the weakest scan.
This adds a resolution fallback that fetches the published package source without executing it, wired into both Pass 1 (
Resolve) and Pass 2 (ResolveFullSource):npm pack <spec> --ignore-scriptsdownloads the published tarball with no lifecycle scripts → extracted assource_method=npm_pack.uv pip download --no-deps(fallbackpip download) fetches the wheel (preferred) or sdist → unpacked without building / setup.py assource_method=pip_download.Security (the crux of the design)
A scanner must never execute the untrusted code it is scanning. The fetch only ever downloads + unpacks archives — never install, build, or
setup.py. Extraction is hardened against:On missing toolchain / offline / fetch failure, resolution falls through to today's
tool_definitions_onlypath — no regression.Config
Enabled by default. Air-gapped deployments can forbid the scanner's network egress:
{ "security": { "scanner_fetch_package_source": false } }Tests (TDD)
internal/security/scanner/package_fetch_test.go(12 tests, all offline/deterministic):npm pack/uv/pipcommand lines never contain install/build/exec verbs and always pass--ignore-scripts/--no-depsVerification
go test ./internal/security/scanner/ -race✅go test ./internal/config/... ./internal/server/...✅ (aftermake buildfor the E2E binary)golangci-lint v2(CI config) ✅ 0 issuesmake build✅,make swaggerregenerated + committedDocs
docs/features/security-scanner-plugins.md— new "Published package fetch" section +source_methodvalues + the config toggle.Design approved at Gate 2 (Option A) on MCP-2206.
Related MCP-2206