Skip to content

Commit 61c8539

Browse files
DumbrisDumbris
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 9f3e1ab commit 61c8539

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
@@ -11,6 +11,7 @@ import (
1111
"os/exec"
1212
"path/filepath"
1313
"strings"
14+
"time"
1415

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

4853
type archiveKind int
@@ -79,16 +84,101 @@ func parsePackageSpec(spec string) (name, version string) {
7984
return spec, ""
8085
}
8186

82-
// firstPackageArg returns the package spec from a runner command's args. It
83-
// honors `--from <pkg>` (uvx) and otherwise returns the first non-flag arg.
84-
func firstPackageArg(args []string) string {
85-
for i, arg := range args {
86-
if arg == "--from" && i+1 < len(args) {
87-
return args[i+1]
87+
// runnerSubcommand returns the keyword that must precede the package token for a
88+
// subcommand-style runner (e.g. `pipx run <pkg>`, `pnpm dlx <pkg>`). Runners that
89+
// take the package as their first positional (npx, uvx, bunx) return "".
90+
func runnerSubcommand(commandBase string) string {
91+
switch commandBase {
92+
case "pipx":
93+
return "run"
94+
case "pnpm", "yarn":
95+
return "dlx"
96+
case "bun":
97+
return "x"
98+
}
99+
return ""
100+
}
101+
102+
// isNpmRunner reports whether a runner belongs to the npm/Node family (as opposed
103+
// to the uv/Python family). The two families differ in how flags name packages:
104+
// npx uses `--package`/`-p`, uv uses `--from`, and `-p` means `--python` for uv.
105+
func isNpmRunner(commandBase string) bool {
106+
switch commandBase {
107+
case "npx", "pnpm", "yarn", "bunx", "bun":
108+
return true
109+
}
110+
return false
111+
}
112+
113+
// runnerFlagTakesValue reports whether a flag consumes the FOLLOWING token as a
114+
// value that is NOT the target package (a python version, an extra dependency, a
115+
// command string, an index URL, ...). Its value must be skipped so it is never
116+
// mistaken for the package. Attached forms ("--python=3.11") carry their value
117+
// inline and consume no extra token.
118+
func runnerFlagTakesValue(npm bool, flag string) bool {
119+
if strings.Contains(flag, "=") {
120+
return false
121+
}
122+
if npm {
123+
// npx -c/--call <cmd> carries a command string; --package/-p are handled
124+
// earlier as package-naming flags, so they never reach here.
125+
switch flag {
126+
case "-c", "--call":
127+
return true
88128
}
89-
if !strings.HasPrefix(arg, "-") {
90-
return arg
129+
return false
130+
}
131+
// uv / uvx / pipx value flags whose argument is not the target package.
132+
switch flag {
133+
case "-p", "--python",
134+
"--with", "--with-editable", "--with-requirements",
135+
"-i", "--index", "--index-url", "--index-strategy",
136+
"-c", "--constraint", "--reinstall-package", "--refresh-package":
137+
return true
138+
}
139+
return false
140+
}
141+
142+
// runnerPackageSpec extracts the TARGET package spec (name plus any version pin,
143+
// returned verbatim) that a package-runner command will execute, from its command
144+
// base and argument list. It understands:
145+
// - a leading runner subcommand keyword (`pipx run`, `pnpm dlx`, `yarn dlx`,
146+
// `bun x`) that precedes the package token;
147+
// - flags that NAME the target package — uv `--from <pkg>`, npx `--package`/
148+
// `-p <pkg>` — whose value is returned directly;
149+
// - value-bearing flags whose argument is NOT the target (`--with <dep>`,
150+
// uv `-p`/`--python <ver>`, `-c <cmd>`, index flags), whose value is skipped.
151+
//
152+
// The first remaining positional token is the package. Returns "" when no package
153+
// can be determined.
154+
func runnerPackageSpec(commandBase string, args []string) string {
155+
sub := runnerSubcommand(commandBase)
156+
subSkipped := sub == ""
157+
npm := isNpmRunner(commandBase)
158+
for i := 0; i < len(args); i++ {
159+
arg := args[i]
160+
hasNext := i+1 < len(args)
161+
// Flags that name the target package directly: return their value.
162+
if hasNext {
163+
if !npm && arg == "--from" {
164+
return args[i+1]
165+
}
166+
if npm && (arg == "--package" || arg == "-p") {
167+
return args[i+1]
168+
}
91169
}
170+
if strings.HasPrefix(arg, "-") {
171+
if hasNext && runnerFlagTakesValue(npm, arg) {
172+
i++ // skip the flag's value too
173+
}
174+
continue
175+
}
176+
// First positional token. Skip a leading runner subcommand keyword once.
177+
if !subSkipped && arg == sub {
178+
subSkipped = true
179+
continue
180+
}
181+
return arg
92182
}
93183
return ""
94184
}
@@ -314,13 +404,20 @@ func writeArchiveFile(target string, src io.Reader, limit int64) (int64, error)
314404
// guaranteeing no regression versus today's behavior.
315405
func (r *SourceResolver) resolveFromPackageFetch(ctx context.Context, info ServerInfo) (*ResolvedSource, error) {
316406
cmdBase := strings.ToLower(filepath.Base(info.Command))
317-
spec := firstPackageArg(info.Args)
407+
spec := runnerPackageSpec(cmdBase, info.Args)
318408
if spec == "" {
319409
return nil, fmt.Errorf("no package spec in args for %s", info.Name)
320410
}
321411

412+
// Bound the whole fetch (download + extract): a slow or hung registry must
413+
// not stall the scan indefinitely. exec.CommandContext kills the download
414+
// subprocess when this deadline fires, and the caller falls back to a
415+
// tool-definitions-only scan.
416+
ctx, cancel := context.WithTimeout(ctx, packageFetchTimeout)
417+
defer cancel()
418+
322419
switch cmdBase {
323-
case "npx", "bunx", "pnpm":
420+
case "npx", "bunx", "bun", "pnpm", "yarn":
324421
return r.fetchNpmPackage(ctx, info, spec)
325422
case "uvx", "pipx":
326423
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)