fix(security): harden package-fetch archive extraction (gzip bomb, ../ reject, caps)#676
Merged
Merged
Conversation
Apply four hardenings to the npx/uvx package-source fetch extractor (internal/security/scanner/package_fetch.go), for both the tar and zip paths, while keeping behavior identical for well-formed small archives: 1. Decompression bomb across skipped members: read the gzip tar stream through a cappedReader so EVERY decompressed byte is bounded — including the bodies of members the extractor skips (oversized, symlink, traversal), which tar.Reader still decompresses while advancing to the next header. 2. safeJoin REJECTS path-traversal and absolute entries with an error instead of cleaning '../' into an in-dest path (the rewrite bug). 3. Oversized and partially-written files are charged to the file-count and byte caps (a truncated-but-large file still counts). 4. Directory creation is capped (charged to the same combined entry limit) before the file-count limit can be bypassed by an all-directories archive — for both tar and zip. extractTarballGz/extractZip take an explicit maxFileBytes so the per-file cap is testable. Adds TDD fixtures: gzip bomb of skipped members, traversal reject (not rewrite), oversized-charged, dir cap. Related #670
Deploying mcpproxy-docs with
|
| Latest commit: |
984992b
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://263cafaa.mcpproxy-docs.pages.dev |
| Branch Preview URL: | https://sec-mcp2444-archive-hardenin.mcpproxy-docs.pages.dev |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![mcpproxy-gatekeeper[bot]](https://avatars.githubusercontent.com/u/216428285?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Gatekeeper approval — Claude Code review (BackendEngineer self-review + inline verification) verdict: ACCEPT.
Review findings: Four archive-extraction hardenings verified correct:
- cappedReader counts skipped-member decompressed bytes — confirmed errArchiveTooLarge propagates through tar.Reader.Next()
- safeJoin rewrite bug confirmed via Go test: old code wrote traversal entries to dest/etc/evil.txt (not rejected); new code errors out
- entryCount charged before oversize-skip — correct
- Directory MkdirAll capped before entryCount++ — correct order
All CI checks passing except 2 pre-existing Server Edition failures (unrelated, reproducible on origin/main before this commit). QA PASS posted by QATester at head 984992b.
Auto-approved per Model B (MCP-1249). No Codex quota; Claude Code review substituted per established pattern.
Dumbris
added a commit
that referenced
this pull request
Jun 15, 2026
Codex re-review (MCP-2445): runnerPackageSpec returned the first positional even when the runner's required subcommand keyword was absent, so with pnpm/yarn/bun gated as package runners: pnpm start -> "start" bun server.ts -> "server.ts" pipx install x -> "install" The wrong token was then fetched/scanned = false coverage + typosquat risk. Fix: for subcommand-style runners (pnpm/yarn `dlx`, `bun x`, `pipx run`), a package spec is resolved ONLY when the keyword is present. A bare local invocation (`pnpm start`, `bun server.ts`, `pipx install foo`) now yields "" (no package) and degrades to tool-definitions-only instead of fetching a bogus package. npx/uvx/bunx (no subcommand) are unchanged. TDD: added pnpm start / pnpm run build / bun server.ts / bun run dev / yarn build / pipx install cases (-> ""); pnpm dlx <pkg> / npx -y <pkg> / uvx --from still resolve. Rebased onto origin/main (picks up #676 archive-extraction hardening + the teams/broker -> serveredition/broker move). Related MCP-2445
Dumbris
added a commit
that referenced
this pull request
Jun 15, 2026
Codex re-review (MCP-2445): runnerPackageSpec returned the first positional even when the runner's required subcommand keyword was absent, so with pnpm/yarn/bun gated as package runners: pnpm start -> "start" bun server.ts -> "server.ts" pipx install x -> "install" The wrong token was then fetched/scanned = false coverage + typosquat risk. Fix: for subcommand-style runners (pnpm/yarn `dlx`, `bun x`, `pipx run`), a package spec is resolved ONLY when the keyword is present. A bare local invocation (`pnpm start`, `bun server.ts`, `pipx install foo`) now yields "" (no package) and degrades to tool-definitions-only instead of fetching a bogus package. npx/uvx/bunx (no subcommand) are unchanged. TDD: added pnpm start / pnpm run build / bun server.ts / bun run dev / yarn build / pipx install cases (-> ""); pnpm dlx <pkg> / npx -y <pkg> / uvx --from still resolve. Rebased onto origin/main (picks up #676 archive-extraction hardening + the teams/broker -> serveredition/broker move). Related MCP-2445
Dumbris
added a commit
that referenced
this pull request
Jun 15, 2026
…r source resolution (MCP-2445) (#677) * fix(scanner): correct spec parsing & version-pin in package-runner source resolution The scanner mis-parsed several package-runner launch commands and ignored requested version pins, so it scanned the wrong source (or none). - Add runnerPackageSpec(): a command-aware parser that understands subcommand runners (`pipx run X`, `pnpm dlx X`, `yarn dlx X`, `bun x X`), package-naming flags (`npx --package/-p X`, `uvx --from X`), and skips non-target value flags (`uvx --with <dep>`, `-p/--python <ver>`, `-c <cmd>`). Replaces firstPackageArg and the ad-hoc loops in resolveNpxCache/resolveUvxCache/npx+uvxTargetPackage. - Enable pnpm/yarn in isPackageRunnerCommand so the pnpm fetch dispatch (a dead branch before) actually runs; wire bun/yarn through the npm fetch path too. - Honor an exact version pin in the local caches: resolveNpxCache prefers the package.json-version match, findUvxArchiveDir prefers the .dist-info-version match, and the container npx locate script prefers the matching bucket — all falling back to newest/first when unpinned or unmatched. - Bound the published-source fetch (download + extract) with a 90s timeout so a hung registry degrades to tool-definitions-only instead of stalling the scan. - Pin cisco-ai-mcp-scanner==4.7.3 in the cisco scanner Dockerfile (reproducible, rug-pull guard). TDD: table tests for the parser (pipx run / pnpm dlx / uvx --with / npx -p), version-pinned cache selection (npx + uvx archive), and the container locate script (run under /bin/sh). `go test ./internal/security/... -race` green. Related MCP-2445 * fix(scanner): require subcommand keyword for subcommand-style runners Codex re-review (MCP-2445): runnerPackageSpec returned the first positional even when the runner's required subcommand keyword was absent, so with pnpm/yarn/bun gated as package runners: pnpm start -> "start" bun server.ts -> "server.ts" pipx install x -> "install" The wrong token was then fetched/scanned = false coverage + typosquat risk. Fix: for subcommand-style runners (pnpm/yarn `dlx`, `bun x`, `pipx run`), a package spec is resolved ONLY when the keyword is present. A bare local invocation (`pnpm start`, `bun server.ts`, `pipx install foo`) now yields "" (no package) and degrades to tool-definitions-only instead of fetching a bogus package. npx/uvx/bunx (no subcommand) are unchanged. TDD: added pnpm start / pnpm run build / bun server.ts / bun run dev / yarn build / pipx install cases (-> ""); pnpm dlx <pkg> / npx -y <pkg> / uvx --from still resolve. Rebased onto origin/main (picks up #676 archive-extraction hardening + the teams/broker -> serveredition/broker move). Related MCP-2445 * fix(scanner): close 3 spec-parsing holes from Codex round-2 re-review 1. Flag-bypass: runnerPackageSpec parsed package flags (-p/--package/--from) BEFORE enforcing the dlx/x/run subcommand, so `pnpm -p start` / `bun -p server.ts` still produced a spec. Now the required subcommand keyword is enforced FIRST (skipping only leading global flags); flag parsing happens only on the args that follow it. 2. Argument-aware classification: isPackageRunnerCommand classified bare pnpm/yarn/bun/pipx as runners by name alone. It now takes args and treats them as runners only when the ephemeral-run keyword is actually present (npx/uvx/bunx remain runners by name). Decided up front, not via a later parser failure. 3. Version-pin miss never substitutes: when an exact pin is requested but absent from a local cache, resolveNpxCache, findUvxArchiveDir, the uv tools dir (Strategy 2), and the container npx locate script all now resolve NOTHING instead of falling back to a mismatched cached version (false coverage). The published-source fetch then gets the pinned version. Also fixed cleanPkg to strip PEP 440 specifiers (pkg==1.0), not just @ver. TDD: pnpm -p start / bun -p server.ts -> no spec; isPackageRunnerCommand arg-aware table; npx + uvx-archive + uv-tools-dir pin-miss -> not resolved; container locate pin-miss -> empty; exact-pin hits + unpinned still resolve. go test ./internal/security/... -race green; golangci v2 0 issues. Related MCP-2445 * fix(scanner): container version match must be literal, not a regex Codex round-3 (MCP-2445): npxContainerLocateScript embedded the requested version inside a `grep -E` pattern, so regex metacharacters were active — a pin like `1.0.0-alpha.1` matched a cached `1.0.0-alpha-1` ('.' as wildcard), substituting the WRONG cached package despite the host-side pin-miss guard. Fix: extract the package.json "version" VALUE (grep matches only the key; the value is captured as "[^\"]*", never the pin), then compare it to the pin with a LITERAL shell string test (`[ "$v" = '<pin>' ]`). No regex over the version. The `if`-form keeps a no-match iteration's exit status 0 (clean empty result). TDD: cached `1.0.0-alpha-1` + pin `1.0.0-alpha.1` -> no match (literal); exact `1.0.0-alpha-1` pin still resolves; prior pin-hit/miss/unpinned cases unchanged. go test ./internal/security/... -race green; golangci v2 0 issues. Related MCP-2445
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
Hardens the untrusted-archive extractor used by the npx/uvx package-source fetch scanner path (
internal/security/scanner/package_fetch.go). Closes four issues found in the Codex scanner audit (MCP-2444). Applied to both the tar and zip paths; behavior is identical for well-formed small archives.tar.Readerstill decompresses a skipped member's body while seeking the next header, so a gzip bomb could exhaust resources. Fixed by reading the gzip stream through acappedReaderthat fails once total decompressed output exceeds the cap.safeJoinrewrote../instead of rejecting it —../../etc/passwdwasCleaned into an in-dest path and silently written there. NowsafeJoinrejects any path-traversal component or absolute path with an error (no sanitize/rewrite).extractTarballGz/extractZipnow take an explicitmaxFileBytes(production callers passfetchMaxFileBytes) so the per-file cap is testable.Tests (TDD)
TestSafeJoin_RejectsTraversal— traversal/absolute rejected, legit names allowed.TestExtractTarballGz_GzipBombSkippedMembers— bomb of skipped members aborts at the decompressed-byte cap.TestExtract{Tarball,Zip}_RejectsZipSlip— strengthened to assert the entry is not rewritten into dest.TestExtractTarballGz_OversizedFileCharged— oversized files trip the count cap.TestExtractTarballGz_DirCap/TestExtractZip_DirCap— many-tiny-dirs aborts at the entry cap.Verification
go test ./internal/security/... -race— greengo build ./...— greengolangci-lint v2.5.0(CI config.github/.golangci.yml) on the package — 0 issuesRelated #670