Skip to content

Commit d3d873a

Browse files
committed
fix(security): reject non-registry specs before package-source fetch
The scanner's published-source fetch passed the server's configured package spec to `pip download` / `uv pip download` UNVALIDATED. For a local-path, `file:`, URL, or VCS (`git+…`, ssh) spec, those commands still invoke the package's setup.py / PEP 517 build backend to resolve metadata — EVEN with `--only-binary=:all:` — executing untrusted code from the server config on the meant-to-be-static scan path. Add validatePackageSpec(ecosystem, spec): require a bare PEP 503 / npm registry name (optionally version-pinned); reject paths, `file:`, URLs, and VCS/ssh specs. resolveFromPackageFetch now gates both the npm and python fetch paths through it, falling back to tool_definitions_only (no fetch, no execution) on rejection. The already-safe registry-name path is unchanged. TDD: TestValidatePackageSpec (registry pass / non-registry reject) and TestResolveFromPackageFetch_RejectsNonRegistrySpec (setup.py execution marker stays absent for a local-path/git+ spec). Related MCP-2442
1 parent 2566a36 commit d3d873a

3 files changed

Lines changed: 158 additions & 0 deletions

File tree

docs/features/security-scanner-plugins.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -259,6 +259,7 @@ Package-runner servers (`npx`, `uvx`) are the **primary** quarantine/scan target
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

262+
- **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.
262263
- **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`).
263264
- **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.
264265

internal/security/scanner/package_fetch.go

Lines changed: 63 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"os"
1212
"os/exec"
1313
"path/filepath"
14+
"regexp"
1415
"strings"
1516

1617
"go.uber.org/zap"
@@ -104,6 +105,62 @@ func parsePackageSpec(spec string) (name, version string) {
104105
return spec, ""
105106
}
106107

108+
var (
109+
// npmNameRe matches a bare npm registry package name: an optional
110+
// "@scope/" prefix followed by the package name. It deliberately excludes
111+
// any character ('/', ':', etc. beyond the single scope separator) that
112+
// would appear in a path, URL, or VCS spec.
113+
npmNameRe = regexp.MustCompile(`^(@[a-z0-9][a-z0-9._-]*/)?[a-z0-9][a-z0-9._-]*$`)
114+
// pep503NameRe matches a PEP 503 / PEP 508 distribution name (case
115+
// insensitive). No path/URL/VCS characters are permitted.
116+
pep503NameRe = regexp.MustCompile(`^[A-Za-z0-9]([A-Za-z0-9._-]*[A-Za-z0-9])?$`)
117+
)
118+
119+
// validatePackageSpec rejects any package spec that is not a bare registry name
120+
// (optionally version-pinned). This is a SECURITY gate, not a convenience check:
121+
// for a local-path, file:, URL, or VCS (git+/hg+/ssh) spec, `pip download` /
122+
// `uv pip download` STILL invoke the package's setup.py / PEP 517 build backend
123+
// to resolve metadata — EVEN with --only-binary=:all: — which executes untrusted
124+
// code from the server config on the (meant-to-be-static) scan path (MCP-2442).
125+
// `npm pack` of a non-registry spec is similarly unsafe. Only a bare PEP 503
126+
// (python) / npm registry name is guaranteed to download-without-build. On
127+
// rejection the caller falls back to tool_definitions_only (no fetch, no exec).
128+
func validatePackageSpec(ecosystem, spec string) error {
129+
if spec == "" {
130+
return fmt.Errorf("empty package spec")
131+
}
132+
// Disqualify URLs (any scheme), file:, VCS (git+/hg+/...), ssh, drive
133+
// letters, and Windows paths up front. None of ':' or '\' ever appear in a
134+
// bare registry name or a PEP 508 version pin, so their mere presence means
135+
// the spec is not a registry name.
136+
if strings.ContainsAny(spec, ":\\") {
137+
return fmt.Errorf("package spec %q is not a bare registry name (contains path/URL/VCS markers)", spec)
138+
}
139+
// Disqualify filesystem paths (absolute, relative, home-relative) and any
140+
// path traversal. A scoped npm name legitimately starts with '@', which is
141+
// not matched here.
142+
if strings.HasPrefix(spec, ".") || strings.HasPrefix(spec, "/") || strings.HasPrefix(spec, "~") {
143+
return fmt.Errorf("package spec %q looks like a filesystem path", spec)
144+
}
145+
if strings.Contains(spec, "..") {
146+
return fmt.Errorf("package spec %q contains a path traversal", spec)
147+
}
148+
name, _ := parsePackageSpec(spec)
149+
switch ecosystem {
150+
case "npm":
151+
if !npmNameRe.MatchString(name) {
152+
return fmt.Errorf("%q is not a valid npm package name", name)
153+
}
154+
case "python":
155+
if !pep503NameRe.MatchString(name) {
156+
return fmt.Errorf("%q is not a valid PEP 503 package name", name)
157+
}
158+
default:
159+
return fmt.Errorf("unknown ecosystem %q", ecosystem)
160+
}
161+
return nil
162+
}
163+
107164
// firstPackageArg returns the package spec from a runner command's args. It
108165
// honors `--from <pkg>` (uvx) and otherwise returns the first non-flag arg.
109166
func firstPackageArg(args []string) string {
@@ -392,8 +449,14 @@ func (r *SourceResolver) resolveFromPackageFetch(ctx context.Context, info Serve
392449

393450
switch cmdBase {
394451
case "npx", "bunx", "pnpm":
452+
if err := validatePackageSpec("npm", spec); err != nil {
453+
return nil, fmt.Errorf("refusing to fetch untrusted spec for %s: %w", info.Name, err)
454+
}
395455
return r.fetchNpmPackage(ctx, info, spec)
396456
case "uvx", "pipx":
457+
if err := validatePackageSpec("python", spec); err != nil {
458+
return nil, fmt.Errorf("refusing to fetch untrusted spec for %s: %w", info.Name, err)
459+
}
397460
return r.fetchPythonPackage(ctx, info, spec)
398461
}
399462
return nil, fmt.Errorf("package fetch unsupported for command %q", cmdBase)

internal/security/scanner/package_fetch_test.go

Lines changed: 94 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,13 @@ import (
55
"archive/zip"
66
"bytes"
77
"compress/gzip"
8+
"context"
89
"os"
910
"path/filepath"
1011
"strings"
1112
"testing"
13+
14+
"go.uber.org/zap"
1215
)
1316

1417
func TestParsePackageSpec(t *testing.T) {
@@ -465,3 +468,94 @@ func TestFindDownloadedArchive(t *testing.T) {
465468
t.Errorf("wheel should be preferred: got (%q,%v)", got2, kind2)
466469
}
467470
}
471+
472+
// MCP-2442: a `pip download` / `uv pip download` of a NON-REGISTRY spec
473+
// (local path, git+, URL, file:, VCS, ssh) STILL executes the package's
474+
// setup.py / PEP 517 build backend even with --only-binary=:all: — arbitrary
475+
// code execution from untrusted server config on the (static) scan path. The
476+
// fetch must validate the spec is a bare PEP 503 / npm registry name and REFUSE
477+
// anything else, falling back to tool-definitions-only.
478+
func TestValidatePackageSpec(t *testing.T) {
479+
cases := []struct {
480+
ecosystem string
481+
spec string
482+
wantOK bool
483+
}{
484+
// Registry names (allowed) — must not regress.
485+
{"python", "flights", true},
486+
{"python", "flights==0.2.4", true},
487+
{"python", "google-flights-mcp-server==0.2.4", true},
488+
{"python", "Flask", true},
489+
{"python", "some.dotted.name", true},
490+
{"npm", "google-flights-mcp-server", true},
491+
{"npm", "google-flights-mcp-server@0.2.4", true},
492+
{"npm", "@modelcontextprotocol/server-everything", true},
493+
{"npm", "@modelcontextprotocol/server-everything@1.0.0", true},
494+
// Non-registry specs (must be REJECTED → fall back to tool-defs).
495+
{"python", "./local-pkg", false},
496+
{"python", "../evil", false},
497+
{"python", "/abs/path/pkg", false},
498+
{"python", "~/pkg", false},
499+
{"python", "file:./pkg", false},
500+
{"python", "git+https://github.com/user/repo.git", false},
501+
{"python", "git+ssh://[email protected]/user/repo.git", false},
502+
{"python", "https://example.com/pkg.tar.gz", false},
503+
{"python", "[email protected]:user/repo.git", false},
504+
{"python", "hg+https://example.com/repo", false},
505+
{"python", `C:\Users\evil\pkg`, false},
506+
{"python", "", false},
507+
{"npm", "./local-pkg", false},
508+
{"npm", "/abs/path", false},
509+
{"npm", "git+https://github.com/user/repo.git", false},
510+
{"npm", "file:../evil", false},
511+
{"npm", "https://example.com/pkg.tgz", false},
512+
{"npm", "", false},
513+
}
514+
for _, c := range cases {
515+
err := validatePackageSpec(c.ecosystem, c.spec)
516+
if c.wantOK && err != nil {
517+
t.Errorf("validatePackageSpec(%q, %q) = %v, want OK", c.ecosystem, c.spec, err)
518+
}
519+
if !c.wantOK && err == nil {
520+
t.Errorf("validatePackageSpec(%q, %q) = nil, want rejection", c.ecosystem, c.spec)
521+
}
522+
}
523+
}
524+
525+
// TestResolveFromPackageFetch_RejectsNonRegistrySpec proves the resolver refuses
526+
// a non-registry uvx spec BEFORE invoking any download command — so the untrusted
527+
// package's setup.py is never executed. We point the spec at a local directory
528+
// containing a setup.py that drops an execution marker; the resolver must return
529+
// an error (caller falls back to tool_definitions_only) and the marker must be
530+
// absent.
531+
func TestResolveFromPackageFetch_RejectsNonRegistrySpec(t *testing.T) {
532+
pkgDir := t.TempDir()
533+
marker := filepath.Join(t.TempDir(), "EXECUTED.marker")
534+
setupPy := "import pathlib\npathlib.Path(" + pyQuote(marker) + ").write_text('pwned')\n"
535+
if err := os.WriteFile(filepath.Join(pkgDir, "setup.py"), []byte(setupPy), 0o644); err != nil {
536+
t.Fatal(err)
537+
}
538+
539+
r := NewSourceResolver(zap.NewNop())
540+
cases := []ServerInfo{
541+
{Name: "evil-path", Protocol: "stdio", Command: "uvx", Args: []string{"--from", pkgDir, "evil"}},
542+
{Name: "evil-git", Protocol: "stdio", Command: "uvx", Args: []string{"--from", "git+https://example.com/repo.git", "evil"}},
543+
{Name: "evil-npm-path", Protocol: "stdio", Command: "npx", Args: []string{"-y", "./local-evil"}},
544+
}
545+
for _, info := range cases {
546+
res, err := r.resolveFromPackageFetch(context.Background(), info)
547+
if err == nil {
548+
if res != nil && res.Cleanup != nil {
549+
res.Cleanup()
550+
}
551+
t.Errorf("%s: expected error (fallback to tool-defs), got nil", info.Name)
552+
}
553+
}
554+
if _, err := os.Stat(marker); err == nil {
555+
t.Fatalf("setup.py was EXECUTED — marker present at %s", marker)
556+
}
557+
}
558+
559+
func pyQuote(s string) string {
560+
return "r'" + s + "'"
561+
}

0 commit comments

Comments
 (0)