diff --git a/internal/security/scanner/source_resolver.go b/internal/security/scanner/source_resolver.go index 19e38b694..b8bff74d8 100644 --- a/internal/security/scanner/source_resolver.go +++ b/internal/security/scanner/source_resolver.go @@ -1177,8 +1177,18 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro } // Search for the package in npx cache: ~/.npm/_npx//node_modules// + // + // The SAME package name can appear under multiple npx cache hashes. One may + // hold the real installed source (package.json + dist/lib/*.js) while another + // holds only a tools.json stub that mcpproxy itself wrote into the cache when + // it dumped tool definitions. The stub's mtime is frequently NEWER than the + // real source (it was just written), so a naive newest-mtime pick selects the + // stub and the scan reports a false "1 file" coverage (MCP-2397). We therefore + // prefer candidates that look like real package source, and only fall back to + // the newest-mtime tiebreak among candidates of the same class. var bestMatch string var bestModTime int64 + var bestReal bool entries, err := os.ReadDir(npxCacheDir) if err != nil { @@ -1191,15 +1201,28 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro } candidatePath := filepath.Join(npxCacheDir, entry.Name(), "node_modules", pkgName) stat, err := os.Stat(candidatePath) - if err != nil { + if err != nil || !stat.IsDir() { continue } - if stat.IsDir() { - modTime := stat.ModTime().Unix() - if modTime > bestModTime { - bestModTime = modTime - bestMatch = candidatePath - } + real := npxCacheLooksReal(candidatePath) + modTime := stat.ModTime().Unix() + + // Selection order: + // 1. any real-source candidate beats any stub; + // 2. within the same class, the newest mtime wins. + better := false + switch { + case bestMatch == "": + better = true + case real && !bestReal: + better = true + case real == bestReal && modTime > bestModTime: + better = true + } + if better { + bestMatch = candidatePath + bestModTime = modTime + bestReal = real } } @@ -1207,6 +1230,22 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro return nil, fmt.Errorf("package %q not found in npx cache (%s)", pkgName, npxCacheDir) } + if !bestReal { + // Every candidate was a bare stub (e.g. a lone tools.json mcpproxy wrote + // into the cache) — no real installed source exists locally. Returning the + // stub here would report false "1 file" coverage AND, post-MCP-2206, would + // short-circuit the caller's published-source fetch fallback in Resolve(), + // which can fetch the REAL package source without executing it. So we treat + // stub-only as "not found" and let that fallback (or a tool_definitions_only + // degrade when fetch is disabled) take over instead. + r.logger.Warn("npx cache held only stub directories (no real package source); deferring to published-source fetch fallback", + zap.String("server", info.Name), + zap.String("package", pkgName), + zap.String("stub_path", bestMatch), + ) + return nil, fmt.Errorf("npx cache for %q contained only stub directories (no real source) in %s", pkgName, npxCacheDir) + } + r.logger.Info("Resolved source from npx cache", zap.String("server", info.Name), zap.String("package", pkgName), @@ -1220,6 +1259,38 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro }, nil } +// npxCacheLooksReal reports whether an npx cache package directory holds the +// real installed package rather than a bare stub. Every npm package ships a +// package.json, so its presence is the canonical marker of real source. A stub +// directory mcpproxy creates just to hold a dumped tools.json has none. As a +// secondary signal we also accept a directory that contains a conventional +// build/source subdirectory (dist/lib/build/src) or any JavaScript/TypeScript +// source file, in case an unusual package omits package.json at the cache root. +func npxCacheLooksReal(dir string) bool { + if _, err := os.Stat(filepath.Join(dir, "package.json")); err == nil { + return true + } + for _, sub := range []string{"dist", "lib", "build", "src"} { + if stat, err := os.Stat(filepath.Join(dir, sub)); err == nil && stat.IsDir() { + return true + } + } + entries, err := os.ReadDir(dir) + if err != nil { + return false + } + for _, e := range entries { + if e.IsDir() { + continue + } + switch strings.ToLower(filepath.Ext(e.Name())) { + case ".js", ".mjs", ".cjs", ".ts": + return true + } + } + return false +} + // resolveUvxCache finds a uvx package's source in ~/.cache/uv/ or ~/.local/share/uv/tools/ func (r *SourceResolver) resolveUvxCache(info ServerInfo) (*ResolvedSource, error) { // Extract package name from args diff --git a/internal/security/scanner/source_resolver_test.go b/internal/security/scanner/source_resolver_test.go index bd2684598..e40e72700 100644 --- a/internal/security/scanner/source_resolver_test.go +++ b/internal/security/scanner/source_resolver_test.go @@ -6,6 +6,7 @@ import ( "path/filepath" "strings" "testing" + "time" "go.uber.org/zap" ) @@ -434,6 +435,81 @@ func TestResolveNpxCache(t *testing.T) { } } +// TestResolveNpxCachePrefersRealSourceOverStub reproduces MCP-2397: when the +// same package name exists under multiple npx cache hashes, one holding the +// real installed source (package.json + dist) and another holding only a +// tools.json stub that mcpproxy itself wrote, the resolver must scan the real +// source even when the stub directory has a NEWER mtime. The old newest-mtime +// heuristic would pick the stub and report false "1 file scanned" coverage. +func TestResolveNpxCachePrefersRealSourceOverStub(t *testing.T) { + homeDir := t.TempDir() + t.Setenv("HOME", homeDir) + t.Setenv("USERPROFILE", homeDir) // Windows uses USERPROFILE + + pkg := filepath.Join("node_modules", "@modelcontextprotocol", "server-everything") + + // Real source under one hash: package.json + dist/*.js (older mtime). + realDir := filepath.Join(homeDir, ".npm", "_npx", "realhash", pkg) + os.MkdirAll(filepath.Join(realDir, "dist"), 0755) + os.WriteFile(filepath.Join(realDir, "package.json"), []byte(`{"name":"@modelcontextprotocol/server-everything"}`), 0644) + os.WriteFile(filepath.Join(realDir, "dist", "index.js"), []byte("// real server source"), 0644) + + // Stub under a different hash: only a tools.json (newer mtime → would win + // under the old heuristic). + stubDir := filepath.Join(homeDir, ".npm", "_npx", "stubhash", pkg) + os.MkdirAll(stubDir, 0755) + os.WriteFile(filepath.Join(stubDir, "tools.json"), []byte(`{"tools":[]}`), 0644) + + // Force the stub directory to be strictly newer than the real one so the + // legacy newest-mtime selection would choose the stub. + older := time.Now().Add(-2 * time.Hour) + newer := time.Now() + if err := os.Chtimes(realDir, older, older); err != nil { + t.Fatalf("chtimes real: %v", err) + } + if err := os.Chtimes(stubDir, newer, newer); err != nil { + t.Fatalf("chtimes stub: %v", err) + } + + r := NewSourceResolver(zap.NewNop()) + result, err := r.resolveNpxCache(ServerInfo{ + Name: "everything-server", + Command: "npx", + Args: []string{"-y", "@modelcontextprotocol/server-everything"}, + }) + if err != nil { + t.Fatalf("resolveNpxCache: %v", err) + } + if result.SourceDir != realDir { + t.Errorf("expected real source %q, got stub %q", realDir, result.SourceDir) + } +} + +// TestResolveNpxCacheStubOnlyReturnsError verifies the post-MCP-2206 +// interaction: when the npx cache holds ONLY a tools.json stub (no real +// package source anywhere locally), resolveNpxCache returns an error rather +// than the stub. This lets Resolve()'s published-source fetch fallback take +// over and fetch the real source, instead of reporting false "1 file" coverage. +func TestResolveNpxCacheStubOnlyReturnsError(t *testing.T) { + homeDir := t.TempDir() + t.Setenv("HOME", homeDir) + t.Setenv("USERPROFILE", homeDir) // Windows uses USERPROFILE + + stubDir := filepath.Join(homeDir, ".npm", "_npx", "stubhash", "node_modules", "@modelcontextprotocol", "server-everything") + os.MkdirAll(stubDir, 0755) + os.WriteFile(filepath.Join(stubDir, "tools.json"), []byte(`{"tools":[]}`), 0644) + + r := NewSourceResolver(zap.NewNop()) + _, err := r.resolveNpxCache(ServerInfo{ + Name: "everything-server", + Command: "npx", + Args: []string{"-y", "@modelcontextprotocol/server-everything"}, + }) + if err == nil { + t.Fatal("expected error when npx cache contains only a stub, got nil (would short-circuit the published-fetch fallback)") + } +} + func TestResolveNpxCacheNotFound(t *testing.T) { homeDir := t.TempDir() t.Setenv("HOME", homeDir)