fix(scanner): spec-parsing + version-pin correctness in package-runner source resolution (MCP-2445)

docker/scanners/cisco/Dockerfile

@@ -14,11 +14,12 @@ LABEL org.opencontainers.image.source="https://github.com/smart-mcp-proxy/mcppro
 LABEL org.opencontainers.image.description="Cisco MCP Scanner (YARA + readiness) packaged for MCPProxy"
 LABEL org.opencontainers.image.licenses="Apache-2.0"
 
-# Install the vendor package. We pin the top-level tool name so the image
-# fails at build time if the upstream package name ever changes. The CLI
-# does not accept --version, so we run `-h` as a sanity check that the
-# binary is on PATH.
-RUN pip install --no-cache-dir cisco-ai-mcp-scanner && mcp-scanner -h >/dev/null
+# Install the vendor package. We pin BOTH the top-level tool name and an exact
+# version so the image is reproducible and cannot silently pull a new upstream
+# release (supply-chain / rug-pull guard). Bump this pin deliberately. The CLI
+# does not accept --version, so we run `-h` as a sanity check that the binary is
+# on PATH.
+RUN pip install --no-cache-dir cisco-ai-mcp-scanner==4.7.3 && mcp-scanner -h >/dev/null
 
 WORKDIR /scan
 ENTRYPOINT ["mcp-scanner"]

docs/features/security-scanner-plugins.md

@@ -255,15 +255,15 @@ The resolved method and path are recorded on the scan job and visible via both t
 
 #### Published package fetch (npx / uvx)
 
-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.
+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.
 
 **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:
 
 - **Registry name required.** Only a server whose configured spec is a **bare registry name** (PEP 503 for PyPI, npm package name — optionally version-pinned) is fetched. Local-path, `file:`, URL, and VCS (`git+…`, `[email protected]:…`) specs are **rejected** and fall back to tool definitions only. This is mandatory: for a non-registry spec, `pip download` / `uv pip download` still invokes the package's `setup.py` / PEP 517 build backend to resolve metadata **even with `--only-binary=:all:`** — executing untrusted code on the (static) scan path. `--only-binary=:all:` alone protects only bare registry names. Validation covers the **whole spec including the `@`-tail**: a PEP 508 / npm direct reference such as `pkg@./local`, `pkg@/abs/path`, or `pkg@git+https://…` is rejected (the `name@<ref>` tail must be a bare version specifier, not a path/URL/VCS); for PyPI, any `@` is treated as a direct reference and refused.
 - **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`).
 - **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.
 
-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.
+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.
 
 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.
 

internal/security/scanner/package_fetch.go

@@ -13,6 +13,7 @@ import (
 	"path/filepath"
 	"regexp"
 	"strings"
+	"time"
 
 	"go.uber.org/zap"
 )
@@ -45,6 +46,10 @@ const (
 	fetchMaxTotalBytes int64 = 256 << 20 // 256 MiB
 	// fetchMaxFileBytes caps any single extracted file.
 	fetchMaxFileBytes int64 = 64 << 20 // 64 MiB
+	// packageFetchTimeout bounds a single published-source fetch (download +
+	// extract). A hung or throttled registry must not stall the scan; on timeout
+	// the caller degrades to a tool-definitions-only scan.
+	packageFetchTimeout = 90 * time.Second
 )
 
 type archiveKind int
@@ -182,16 +187,124 @@ func validatePackageSpec(ecosystem, spec string) error {
 	return nil
 }
 
-// firstPackageArg returns the package spec from a runner command's args. It
-// honors `--from <pkg>` (uvx) and otherwise returns the first non-flag arg.
-func firstPackageArg(args []string) string {
-	for i, arg := range args {
-		if arg == "--from" && i+1 < len(args) {
-			return args[i+1]
+// runnerSubcommand returns the keyword that must precede the package token for a
+// subcommand-style runner (e.g. `pipx run <pkg>`, `pnpm dlx <pkg>`). Runners that
+// take the package as their first positional (npx, uvx, bunx) return "".
+func runnerSubcommand(commandBase string) string {
+	switch commandBase {
+	case "pipx":
+		return "run"
+	case "pnpm", "yarn":
+		return "dlx"
+	case "bun":
+		return "x"
+	}
+	return ""
+}
+
+// isNpmRunner reports whether a runner belongs to the npm/Node family (as opposed
+// to the uv/Python family). The two families differ in how flags name packages:
+// npx uses `--package`/`-p`, uv uses `--from`, and `-p` means `--python` for uv.
+func isNpmRunner(commandBase string) bool {
+	switch commandBase {
+	case "npx", "pnpm", "yarn", "bunx", "bun":
+		return true
+	}
+	return false
+}
+
+// runnerFlagTakesValue reports whether a flag consumes the FOLLOWING token as a
+// value that is NOT the target package (a python version, an extra dependency, a
+// command string, an index URL, ...). Its value must be skipped so it is never
+// mistaken for the package. Attached forms ("--python=3.11") carry their value
+// inline and consume no extra token.
+func runnerFlagTakesValue(npm bool, flag string) bool {
+	if strings.Contains(flag, "=") {
+		return false
+	}
+	if npm {
+		// npx -c/--call <cmd> carries a command string; --package/-p are handled
+		// earlier as package-naming flags, so they never reach here.
+		switch flag {
+		case "-c", "--call":
+			return true
 		}
-		if !strings.HasPrefix(arg, "-") {
-			return arg
+		return false
+	}
+	// uv / uvx / pipx value flags whose argument is not the target package.
+	switch flag {
+	case "-p", "--python",
+		"--with", "--with-editable", "--with-requirements",
+		"-i", "--index", "--index-url", "--index-strategy",
+		"-c", "--constraint", "--reinstall-package", "--refresh-package":
+		return true
+	}
+	return false
+}
+
+// runnerPackageSpec extracts the TARGET package spec (name plus any version pin,
+// returned verbatim) that a package-runner command will execute, from its command
+// base and argument list. It understands:
+//   - a leading runner subcommand keyword (`pipx run`, `pnpm dlx`, `yarn dlx`,
+//     `bun x`) that precedes the package token — and which is REQUIRED for those
+//     runners: a bare `pnpm start` / `bun server.ts` is a local invocation, not a
+//     remote fetch, and yields "" (no package);
+//   - flags that NAME the target package — uv `--from <pkg>`, npx `--package`/
+//     `-p <pkg>` — whose value is returned directly;
+//   - value-bearing flags whose argument is NOT the target (`--with <dep>`,
+//     uv `-p`/`--python <ver>`, `-c <cmd>`, index flags), whose value is skipped.
+//
+// The first remaining positional token is the package. Returns "" when no package
+// can be determined.
+func runnerPackageSpec(commandBase string, args []string) string {
+	npm := isNpmRunner(commandBase)
+
+	// Subcommand-style runners (pnpm/yarn `dlx`, `bun x`, `pipx run`) REQUIRE
+	// their keyword before anything is treated as a package. The keyword check
+	// runs BEFORE any flag parsing, so a package flag cannot bypass it: a bare
+	// `pnpm start`, `bun server.ts`, `pipx install foo`, or `pnpm -p start` has
+	// no keyword and resolves nothing (a local invocation, not a remote fetch —
+	// fetching the wrong token would mean false coverage + typosquat risk;
+	// MCP-2445). Leading global flags before the keyword (e.g. `pnpm --silent
+	// dlx`) are skipped. npx/uvx/bunx have no subcommand and parse from the start.
+	if sub := runnerSubcommand(commandBase); sub != "" {
+		i := 0
+		for i < len(args) {
+			arg := args[i]
+			if strings.HasPrefix(arg, "-") {
+				if i+1 < len(args) && runnerFlagTakesValue(npm, arg) {
+					i++
+				}
+				i++
+				continue
+			}
+			break // first positional token
+		}
+		if i >= len(args) || args[i] != sub {
+			return "" // first positional is not the required keyword → no package
 		}
+		args = args[i+1:] // parse only what follows the keyword
+	}
+
+	for i := 0; i < len(args); i++ {
+		arg := args[i]
+		hasNext := i+1 < len(args)
+		// Flags that name the target package directly: return their value.
+		if hasNext {
+			if !npm && arg == "--from" {
+				return args[i+1]
+			}
+			if npm && (arg == "--package" || arg == "-p") {
+				return args[i+1]
+			}
+		}
+		if strings.HasPrefix(arg, "-") {
+			if hasNext && runnerFlagTakesValue(npm, arg) {
+				i++ // skip the flag's value too
+			}
+			continue
+		}
+		return arg
 	}
 	return ""
 }
@@ -463,13 +576,20 @@ func writeArchiveFile(target string, src io.Reader, limit int64) (int64, error)
 // guaranteeing no regression versus today's behavior.
 func (r *SourceResolver) resolveFromPackageFetch(ctx context.Context, info ServerInfo) (*ResolvedSource, error) {
 	cmdBase := strings.ToLower(filepath.Base(info.Command))
-	spec := firstPackageArg(info.Args)
+	spec := runnerPackageSpec(cmdBase, info.Args)
 	if spec == "" {
 		return nil, fmt.Errorf("no package spec in args for %s", info.Name)
 	}
 
+	// Bound the whole fetch (download + extract): a slow or hung registry must
+	// not stall the scan indefinitely. exec.CommandContext kills the download
+	// subprocess when this deadline fires, and the caller falls back to a
+	// tool-definitions-only scan.
+	ctx, cancel := context.WithTimeout(ctx, packageFetchTimeout)
+	defer cancel()
+
 	switch cmdBase {
-	case "npx", "bunx", "pnpm":
+	case "npx", "bunx", "bun", "pnpm", "yarn":
 		if err := validatePackageSpec("npm", spec); err != nil {
 			return nil, fmt.Errorf("refusing to fetch untrusted spec for %s: %w", info.Name, err)
 		}

internal/security/scanner/package_fetch_test.go

@@ -37,21 +37,58 @@ func TestParsePackageSpec(t *testing.T) {
 	}
 }
 
-func TestFirstPackageArg(t *testing.T) {
+func TestRunnerPackageSpec(t *testing.T) {
 	cases := []struct {
-		args []string
-		want string
+		name    string
+		command string
+		args    []string
+		want    string
 	}{
-		{[]string{"-y", "google-flights-mcp-server@0.2.4"}, "google-flights-mcp-server@0.2.4"},
-		{[]string{"--from", "pkg-a", "cmd"}, "pkg-a"},
-		{[]string{"pkg-only"}, "pkg-only"},
-		{[]string{"-y"}, ""},
-		{nil, ""},
+		// npx: -y is a boolean flag, the package follows.
+		{"npx -y", "npx", []string{"-y", "google-flights-mcp-server@0.2.4"}, "google-flights-mcp-server@0.2.4"},
+		{"npx bare", "npx", []string{"server-everything"}, "server-everything"},
+		// npx --package / -p NAMES the target; -c carries a command string.
+		{"npx -p", "npx", []string{"-p", "@scope/srv@1.2.3", "-c", "srv"}, "@scope/srv@1.2.3"},
+		{"npx --package", "npx", []string{"--package", "srv", "-c", "run-srv"}, "srv"},
+		// Subcommand runners: the subcommand keyword is NOT the package.
+		{"pipx run", "pipx", []string{"run", "some-pkg"}, "some-pkg"},
+		{"pnpm dlx", "pnpm", []string{"dlx", "some-pkg"}, "some-pkg"},
+		{"yarn dlx", "yarn", []string{"dlx", "some-pkg@2.0.0"}, "some-pkg@2.0.0"},
+		{"bun x", "bun", []string{"x", "some-pkg"}, "some-pkg"},
+		// uvx: --from names the distribution; a positional after it is the command.
+		{"uvx --from", "uvx", []string{"--from", "pkg-a", "cmd"}, "pkg-a"},
+		{"uvx bare", "uvx", []string{"pkg-only"}, "pkg-only"},
+		// uvx --with adds an EXTRA dep; the target is the first real positional.
+		{"uvx --with then target", "uvx", []string{"--with", "extra-dep", "main-pkg"}, "main-pkg"},
+		{"uvx --with then --from", "uvx", []string{"--with", "extra-dep", "--from", "main-pkg", "cmd"}, "main-pkg"},
+		// uvx -p/--python carries a version, NOT the package.
+		{"uvx -p python", "uvx", []string{"-p", "3.11", "main-pkg"}, "main-pkg"},
+		{"uvx --python", "uvx", []string{"--python", "3.12", "--from", "main-pkg", "cmd"}, "main-pkg"},
+		// Subcommand runners WITHOUT the keyword must NOT resolve a package spec —
+		// these are local invocations, not remote package fetches (MCP-2445 re-review).
+		// Fetching the first positional here = false coverage + typosquat risk.
+		{"pnpm start (local script)", "pnpm", []string{"start"}, ""},
+		{"pnpm run build (local script)", "pnpm", []string{"run", "build"}, ""},
+		{"bun server.ts (local file)", "bun", []string{"server.ts"}, ""},
+		{"bun run dev (local script)", "bun", []string{"run", "dev"}, ""},
+		{"yarn build (local script)", "yarn", []string{"build"}, ""},
+		{"pipx install foo (not ephemeral run)", "pipx", []string{"install", "foo"}, ""},
+		// The subcommand keyword is enforced BEFORE flag parsing, so a package
+		// flag cannot bypass it (MCP-2445 round-2 hole #1).
+		{"pnpm -p start (flag bypass)", "pnpm", []string{"-p", "start"}, ""},
+		{"bun -p server.ts (flag bypass)", "bun", []string{"-p", "server.ts"}, ""},
+		{"pnpm --package x (no dlx)", "pnpm", []string{"--package", "x", "start"}, ""},
+		// Leading global flags before the keyword are fine; keyword still required.
+		{"pnpm --silent dlx pkg", "pnpm", []string{"--silent", "dlx", "pkg"}, "pkg"},
+		// Degenerate inputs.
+		{"npx flag only", "npx", []string{"-y"}, ""},
+		{"nil", "npx", nil, ""},
+		{"pipx run only", "pipx", []string{"run"}, ""},
 	}
 	for _, c := range cases {
-		got := firstPackageArg(c.args)
+		got := runnerPackageSpec(filepath.Base(c.command), c.args)
 		if got != c.want {
-			t.Errorf("firstPackageArg(%v) = %q, want %q", c.args, got, c.want)
+			t.Errorf("%s: runnerPackageSpec(%q, %v) = %q, want %q", c.name, c.command, c.args, got, c.want)
 		}
 	}
 }