Skip to content

Commit 9a7f874

Browse files
committed
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
1 parent 2566a36 commit 9a7f874

6 files changed

Lines changed: 434 additions & 119 deletions

File tree

docker/scanners/cisco/Dockerfile

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,11 +14,12 @@ LABEL org.opencontainers.image.source="https://github.com/smart-mcp-proxy/mcppro
1414
LABEL org.opencontainers.image.description="Cisco MCP Scanner (YARA + readiness) packaged for MCPProxy"
1515
LABEL org.opencontainers.image.licenses="Apache-2.0"
1616

17-
# Install the vendor package. We pin the top-level tool name so the image
18-
# fails at build time if the upstream package name ever changes. The CLI
19-
# does not accept --version, so we run `-h` as a sanity check that the
20-
# binary is on PATH.
21-
RUN pip install --no-cache-dir cisco-ai-mcp-scanner && mcp-scanner -h >/dev/null
17+
# Install the vendor package. We pin BOTH the top-level tool name and an exact
18+
# version so the image is reproducible and cannot silently pull a new upstream
19+
# release (supply-chain / rug-pull guard). Bump this pin deliberately. The CLI
20+
# does not accept --version, so we run `-h` as a sanity check that the binary is
21+
# on PATH.
22+
RUN pip install --no-cache-dir cisco-ai-mcp-scanner==4.7.3 && mcp-scanner -h >/dev/null
2223

2324
WORKDIR /scan
2425
ENTRYPOINT ["mcp-scanner"]

docs/features/security-scanner-plugins.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -255,14 +255,14 @@ The resolved method and path are recorded on the scan job and visible via both t
255255

256256
#### Published package fetch (npx / uvx)
257257

258-
Package-runner servers (`npx`, `uvx`) are the **primary** quarantine/scan target, but a server quarantined on add has never run locally — so the local package cache (step 2) misses and, before this fallback existed, the scan degraded to **tool definitions only** (no real source-level analysis). Step 5 closes that gap by downloading the *published* package source so the AI and supply-chain scanners run against real code.
258+
Package-runner servers (`npx`, `uvx`, plus `pnpm dlx` / `yarn dlx`, `pipx run`, and `bunx`) are the **primary** quarantine/scan target, but a server quarantined on add has never run locally — so the local package cache (step 2) misses and, before this fallback existed, the scan degraded to **tool definitions only** (no real source-level analysis). Step 5 closes that gap by downloading the *published* package source so the AI and supply-chain scanners run against real code. The target package is parsed from the launch command — subcommand runners (`pipx run X`, `pnpm dlx X`), package-naming flags (`npx --package X`, `uvx --from X`), and extra-dependency flags (`uvx --with <dep> X`, which names `X`, not the dep) are all handled. When the spec carries an exact version pin (`pkg@1.2.3`, `pkg==1.2.3`), the local-cache lookups (steps 2–4) prefer the cache entry matching that version rather than the newest one.
259259

260260
**The source is fetched but never executed.** A scanner must not run the untrusted code it is scanning. The fetch only ever downloads and unpacks archives:
261261

262262
- **npm (`npx`)**`npm pack <pkg>@<version> --ignore-scripts` downloads the published tarball without running any lifecycle scripts (`install`/`postinstall`), then it is extracted (`source_method=npm_pack`).
263263
- **PyPI (`uvx`)**`uv pip download <pkg>==<version> --no-deps --only-binary=:all:` (falling back to `pip download`) fetches **only a prebuilt wheel**, which is unpacked without building or running `setup.py` (`source_method=pip_download`). `--only-binary=:all:` is mandatory: downloading an sdist would invoke the package's PEP 517 build backend (`setup.py egg_info`) to resolve metadata, executing the untrusted code. A package that ships **no wheel** therefore fails the fetch and falls back to tool definitions only — sdists are never built or extracted.
264264

265-
Extraction is hardened against path traversal (zip-slip), symlink escape, and decompression bombs (bounded file count and total size). If the toolchain is missing, the host is offline, or the fetch fails, resolution falls through to **tool definitions only** with no regression.
265+
Extraction is hardened against path traversal (zip-slip), symlink escape, and decompression bombs (bounded file count and total size). The whole fetch (download + extract) is bounded by a timeout, so a hung or throttled registry cannot stall the scan. If the toolchain is missing, the host is offline, or the fetch fails or times out, resolution falls through to **tool definitions only** with no regression.
266266

267267
This fallback is **enabled by default**. Air-gapped deployments that must forbid the scanner's network egress can disable it with `security.scanner_fetch_package_source: false` — package-runner servers without local source then scan tool definitions only.
268268

internal/security/scanner/package_fetch.go

Lines changed: 107 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import (
1212
"os/exec"
1313
"path/filepath"
1414
"strings"
15+
"time"
1516

1617
"go.uber.org/zap"
1718
)
@@ -44,6 +45,10 @@ const (
4445
fetchMaxTotalBytes int64 = 256 << 20 // 256 MiB
4546
// fetchMaxFileBytes caps any single extracted file.
4647
fetchMaxFileBytes int64 = 64 << 20 // 64 MiB
48+
// packageFetchTimeout bounds a single published-source fetch (download +
49+
// extract). A hung or throttled registry must not stall the scan; on timeout
50+
// the caller degrades to a tool-definitions-only scan.
51+
packageFetchTimeout = 90 * time.Second
4752
)
4853

4954
type archiveKind int
@@ -104,16 +109,101 @@ func parsePackageSpec(spec string) (name, version string) {
104109
return spec, ""
105110
}
106111

107-
// firstPackageArg returns the package spec from a runner command's args. It
108-
// honors `--from <pkg>` (uvx) and otherwise returns the first non-flag arg.
109-
func firstPackageArg(args []string) string {
110-
for i, arg := range args {
111-
if arg == "--from" && i+1 < len(args) {
112-
return args[i+1]
112+
// runnerSubcommand returns the keyword that must precede the package token for a
113+
// subcommand-style runner (e.g. `pipx run <pkg>`, `pnpm dlx <pkg>`). Runners that
114+
// take the package as their first positional (npx, uvx, bunx) return "".
115+
func runnerSubcommand(commandBase string) string {
116+
switch commandBase {
117+
case "pipx":
118+
return "run"
119+
case "pnpm", "yarn":
120+
return "dlx"
121+
case "bun":
122+
return "x"
123+
}
124+
return ""
125+
}
126+
127+
// isNpmRunner reports whether a runner belongs to the npm/Node family (as opposed
128+
// to the uv/Python family). The two families differ in how flags name packages:
129+
// npx uses `--package`/`-p`, uv uses `--from`, and `-p` means `--python` for uv.
130+
func isNpmRunner(commandBase string) bool {
131+
switch commandBase {
132+
case "npx", "pnpm", "yarn", "bunx", "bun":
133+
return true
134+
}
135+
return false
136+
}
137+
138+
// runnerFlagTakesValue reports whether a flag consumes the FOLLOWING token as a
139+
// value that is NOT the target package (a python version, an extra dependency, a
140+
// command string, an index URL, ...). Its value must be skipped so it is never
141+
// mistaken for the package. Attached forms ("--python=3.11") carry their value
142+
// inline and consume no extra token.
143+
func runnerFlagTakesValue(npm bool, flag string) bool {
144+
if strings.Contains(flag, "=") {
145+
return false
146+
}
147+
if npm {
148+
// npx -c/--call <cmd> carries a command string; --package/-p are handled
149+
// earlier as package-naming flags, so they never reach here.
150+
switch flag {
151+
case "-c", "--call":
152+
return true
153+
}
154+
return false
155+
}
156+
// uv / uvx / pipx value flags whose argument is not the target package.
157+
switch flag {
158+
case "-p", "--python",
159+
"--with", "--with-editable", "--with-requirements",
160+
"-i", "--index", "--index-url", "--index-strategy",
161+
"-c", "--constraint", "--reinstall-package", "--refresh-package":
162+
return true
163+
}
164+
return false
165+
}
166+
167+
// runnerPackageSpec extracts the TARGET package spec (name plus any version pin,
168+
// returned verbatim) that a package-runner command will execute, from its command
169+
// base and argument list. It understands:
170+
// - a leading runner subcommand keyword (`pipx run`, `pnpm dlx`, `yarn dlx`,
171+
// `bun x`) that precedes the package token;
172+
// - flags that NAME the target package — uv `--from <pkg>`, npx `--package`/
173+
// `-p <pkg>` — whose value is returned directly;
174+
// - value-bearing flags whose argument is NOT the target (`--with <dep>`,
175+
// uv `-p`/`--python <ver>`, `-c <cmd>`, index flags), whose value is skipped.
176+
//
177+
// The first remaining positional token is the package. Returns "" when no package
178+
// can be determined.
179+
func runnerPackageSpec(commandBase string, args []string) string {
180+
sub := runnerSubcommand(commandBase)
181+
subSkipped := sub == ""
182+
npm := isNpmRunner(commandBase)
183+
for i := 0; i < len(args); i++ {
184+
arg := args[i]
185+
hasNext := i+1 < len(args)
186+
// Flags that name the target package directly: return their value.
187+
if hasNext {
188+
if !npm && arg == "--from" {
189+
return args[i+1]
190+
}
191+
if npm && (arg == "--package" || arg == "-p") {
192+
return args[i+1]
193+
}
113194
}
114-
if !strings.HasPrefix(arg, "-") {
115-
return arg
195+
if strings.HasPrefix(arg, "-") {
196+
if hasNext && runnerFlagTakesValue(npm, arg) {
197+
i++ // skip the flag's value too
198+
}
199+
continue
116200
}
201+
// First positional token. Skip a leading runner subcommand keyword once.
202+
if !subSkipped && arg == sub {
203+
subSkipped = true
204+
continue
205+
}
206+
return arg
117207
}
118208
return ""
119209
}
@@ -385,13 +475,20 @@ func writeArchiveFile(target string, src io.Reader, limit int64) (int64, error)
385475
// guaranteeing no regression versus today's behavior.
386476
func (r *SourceResolver) resolveFromPackageFetch(ctx context.Context, info ServerInfo) (*ResolvedSource, error) {
387477
cmdBase := strings.ToLower(filepath.Base(info.Command))
388-
spec := firstPackageArg(info.Args)
478+
spec := runnerPackageSpec(cmdBase, info.Args)
389479
if spec == "" {
390480
return nil, fmt.Errorf("no package spec in args for %s", info.Name)
391481
}
392482

483+
// Bound the whole fetch (download + extract): a slow or hung registry must
484+
// not stall the scan indefinitely. exec.CommandContext kills the download
485+
// subprocess when this deadline fires, and the caller falls back to a
486+
// tool-definitions-only scan.
487+
ctx, cancel := context.WithTimeout(ctx, packageFetchTimeout)
488+
defer cancel()
489+
393490
switch cmdBase {
394-
case "npx", "bunx", "pnpm":
491+
case "npx", "bunx", "bun", "pnpm", "yarn":
395492
return r.fetchNpmPackage(ctx, info, spec)
396493
case "uvx", "pipx":
397494
return r.fetchPythonPackage(ctx, info, spec)

internal/security/scanner/package_fetch_test.go

Lines changed: 31 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -34,21 +34,42 @@ func TestParsePackageSpec(t *testing.T) {
3434
}
3535
}
3636

37-
func TestFirstPackageArg(t *testing.T) {
37+
func TestRunnerPackageSpec(t *testing.T) {
3838
cases := []struct {
39-
args []string
40-
want string
39+
name string
40+
command string
41+
args []string
42+
want string
4143
}{
42-
{[]string{"-y", "google-flights-mcp-server@0.2.4"}, "google-flights-mcp-server@0.2.4"},
43-
{[]string{"--from", "pkg-a", "cmd"}, "pkg-a"},
44-
{[]string{"pkg-only"}, "pkg-only"},
45-
{[]string{"-y"}, ""},
46-
{nil, ""},
44+
// npx: -y is a boolean flag, the package follows.
45+
{"npx -y", "npx", []string{"-y", "google-flights-mcp-server@0.2.4"}, "google-flights-mcp-server@0.2.4"},
46+
{"npx bare", "npx", []string{"server-everything"}, "server-everything"},
47+
// npx --package / -p NAMES the target; -c carries a command string.
48+
{"npx -p", "npx", []string{"-p", "@scope/srv@1.2.3", "-c", "srv"}, "@scope/srv@1.2.3"},
49+
{"npx --package", "npx", []string{"--package", "srv", "-c", "run-srv"}, "srv"},
50+
// Subcommand runners: the subcommand keyword is NOT the package.
51+
{"pipx run", "pipx", []string{"run", "some-pkg"}, "some-pkg"},
52+
{"pnpm dlx", "pnpm", []string{"dlx", "some-pkg"}, "some-pkg"},
53+
{"yarn dlx", "yarn", []string{"dlx", "some-pkg@2.0.0"}, "some-pkg@2.0.0"},
54+
{"bun x", "bun", []string{"x", "some-pkg"}, "some-pkg"},
55+
// uvx: --from names the distribution; a positional after it is the command.
56+
{"uvx --from", "uvx", []string{"--from", "pkg-a", "cmd"}, "pkg-a"},
57+
{"uvx bare", "uvx", []string{"pkg-only"}, "pkg-only"},
58+
// uvx --with adds an EXTRA dep; the target is the first real positional.
59+
{"uvx --with then target", "uvx", []string{"--with", "extra-dep", "main-pkg"}, "main-pkg"},
60+
{"uvx --with then --from", "uvx", []string{"--with", "extra-dep", "--from", "main-pkg", "cmd"}, "main-pkg"},
61+
// uvx -p/--python carries a version, NOT the package.
62+
{"uvx -p python", "uvx", []string{"-p", "3.11", "main-pkg"}, "main-pkg"},
63+
{"uvx --python", "uvx", []string{"--python", "3.12", "--from", "main-pkg", "cmd"}, "main-pkg"},
64+
// Degenerate inputs.
65+
{"npx flag only", "npx", []string{"-y"}, ""},
66+
{"nil", "npx", nil, ""},
67+
{"pipx run only", "pipx", []string{"run"}, ""},
4768
}
4869
for _, c := range cases {
49-
got := firstPackageArg(c.args)
70+
got := runnerPackageSpec(filepath.Base(c.command), c.args)
5071
if got != c.want {
51-
t.Errorf("firstPackageArg(%v) = %q, want %q", c.args, got, c.want)
72+
t.Errorf("%s: runnerPackageSpec(%q, %v) = %q, want %q", c.name, c.command, c.args, got, c.want)
5273
}
5374
}
5475
}

0 commit comments

Comments
 (0)