Skip to content

Commit f730e70

Browse files
committed
fix(scanner): npx resolver prefers real package source over tools.json stub
The npx cache can hold the same package under multiple hashes: one with the real installed source (package.json + dist/*.js) and another with only a tools.json stub that mcpproxy writes when dumping tool definitions. The stub's mtime is usually newer, so resolveNpxCache's newest-mtime heuristic picked it and the scan reported false coverage (1 stub file instead of ~30 real files). Rank candidates by real-source-ness first (package.json, a dist/lib/build/src subdir, or a JS/TS file at the root), falling back to the newest-mtime tiebreak only within the same class. Warn when every candidate is a stub. Related MCP-2397
1 parent 9f4930c commit f730e70

2 files changed

Lines changed: 123 additions & 7 deletions

File tree

internal/security/scanner/source_resolver.go

Lines changed: 72 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1138,8 +1138,18 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro
11381138
}
11391139

11401140
// Search for the package in npx cache: ~/.npm/_npx/<hash>/node_modules/<package>/
1141+
//
1142+
// The SAME package name can appear under multiple npx cache hashes. One may
1143+
// hold the real installed source (package.json + dist/lib/*.js) while another
1144+
// holds only a tools.json stub that mcpproxy itself wrote into the cache when
1145+
// it dumped tool definitions. The stub's mtime is frequently NEWER than the
1146+
// real source (it was just written), so a naive newest-mtime pick selects the
1147+
// stub and the scan reports a false "1 file" coverage (MCP-2397). We therefore
1148+
// prefer candidates that look like real package source, and only fall back to
1149+
// the newest-mtime tiebreak among candidates of the same class.
11411150
var bestMatch string
11421151
var bestModTime int64
1152+
var bestReal bool
11431153

11441154
entries, err := os.ReadDir(npxCacheDir)
11451155
if err != nil {
@@ -1152,22 +1162,45 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro
11521162
}
11531163
candidatePath := filepath.Join(npxCacheDir, entry.Name(), "node_modules", pkgName)
11541164
stat, err := os.Stat(candidatePath)
1155-
if err != nil {
1165+
if err != nil || !stat.IsDir() {
11561166
continue
11571167
}
1158-
if stat.IsDir() {
1159-
modTime := stat.ModTime().Unix()
1160-
if modTime > bestModTime {
1161-
bestModTime = modTime
1162-
bestMatch = candidatePath
1163-
}
1168+
real := npxCacheLooksReal(candidatePath)
1169+
modTime := stat.ModTime().Unix()
1170+
1171+
// Selection order:
1172+
// 1. any real-source candidate beats any stub;
1173+
// 2. within the same class, the newest mtime wins.
1174+
better := false
1175+
switch {
1176+
case bestMatch == "":
1177+
better = true
1178+
case real && !bestReal:
1179+
better = true
1180+
case real == bestReal && modTime > bestModTime:
1181+
better = true
1182+
}
1183+
if better {
1184+
bestMatch = candidatePath
1185+
bestModTime = modTime
1186+
bestReal = real
11641187
}
11651188
}
11661189

11671190
if bestMatch == "" {
11681191
return nil, fmt.Errorf("package %q not found in npx cache (%s)", pkgName, npxCacheDir)
11691192
}
11701193

1194+
if !bestReal {
1195+
// Every candidate looked like a stub. Scan it anyway (preserving prior
1196+
// behavior) but warn so the false-coverage case is visible in logs.
1197+
r.logger.Warn("npx cache resolution found only stub directories (no package.json); scan coverage may be incomplete",
1198+
zap.String("server", info.Name),
1199+
zap.String("package", pkgName),
1200+
zap.String("path", bestMatch),
1201+
)
1202+
}
1203+
11711204
r.logger.Info("Resolved source from npx cache",
11721205
zap.String("server", info.Name),
11731206
zap.String("package", pkgName),
@@ -1181,6 +1214,38 @@ func (r *SourceResolver) resolveNpxCache(info ServerInfo) (*ResolvedSource, erro
11811214
}, nil
11821215
}
11831216

1217+
// npxCacheLooksReal reports whether an npx cache package directory holds the
1218+
// real installed package rather than a bare stub. Every npm package ships a
1219+
// package.json, so its presence is the canonical marker of real source. A stub
1220+
// directory mcpproxy creates just to hold a dumped tools.json has none. As a
1221+
// secondary signal we also accept a directory that contains a conventional
1222+
// build/source subdirectory (dist/lib/build/src) or any JavaScript/TypeScript
1223+
// source file, in case an unusual package omits package.json at the cache root.
1224+
func npxCacheLooksReal(dir string) bool {
1225+
if _, err := os.Stat(filepath.Join(dir, "package.json")); err == nil {
1226+
return true
1227+
}
1228+
for _, sub := range []string{"dist", "lib", "build", "src"} {
1229+
if stat, err := os.Stat(filepath.Join(dir, sub)); err == nil && stat.IsDir() {
1230+
return true
1231+
}
1232+
}
1233+
entries, err := os.ReadDir(dir)
1234+
if err != nil {
1235+
return false
1236+
}
1237+
for _, e := range entries {
1238+
if e.IsDir() {
1239+
continue
1240+
}
1241+
switch strings.ToLower(filepath.Ext(e.Name())) {
1242+
case ".js", ".mjs", ".cjs", ".ts":
1243+
return true
1244+
}
1245+
}
1246+
return false
1247+
}
1248+
11841249
// resolveUvxCache finds a uvx package's source in ~/.cache/uv/ or ~/.local/share/uv/tools/
11851250
func (r *SourceResolver) resolveUvxCache(info ServerInfo) (*ResolvedSource, error) {
11861251
// Extract package name from args

internal/security/scanner/source_resolver_test.go

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import (
66
"path/filepath"
77
"strings"
88
"testing"
9+
"time"
910

1011
"go.uber.org/zap"
1112
)
@@ -434,6 +435,56 @@ func TestResolveNpxCache(t *testing.T) {
434435
}
435436
}
436437

438+
// TestResolveNpxCachePrefersRealSourceOverStub reproduces MCP-2397: when the
439+
// same package name exists under multiple npx cache hashes, one holding the
440+
// real installed source (package.json + dist) and another holding only a
441+
// tools.json stub that mcpproxy itself wrote, the resolver must scan the real
442+
// source even when the stub directory has a NEWER mtime. The old newest-mtime
443+
// heuristic would pick the stub and report false "1 file scanned" coverage.
444+
func TestResolveNpxCachePrefersRealSourceOverStub(t *testing.T) {
445+
homeDir := t.TempDir()
446+
t.Setenv("HOME", homeDir)
447+
t.Setenv("USERPROFILE", homeDir) // Windows uses USERPROFILE
448+
449+
pkg := filepath.Join("node_modules", "@modelcontextprotocol", "server-everything")
450+
451+
// Real source under one hash: package.json + dist/*.js (older mtime).
452+
realDir := filepath.Join(homeDir, ".npm", "_npx", "realhash", pkg)
453+
os.MkdirAll(filepath.Join(realDir, "dist"), 0755)
454+
os.WriteFile(filepath.Join(realDir, "package.json"), []byte(`{"name":"@modelcontextprotocol/server-everything"}`), 0644)
455+
os.WriteFile(filepath.Join(realDir, "dist", "index.js"), []byte("// real server source"), 0644)
456+
457+
// Stub under a different hash: only a tools.json (newer mtime → would win
458+
// under the old heuristic).
459+
stubDir := filepath.Join(homeDir, ".npm", "_npx", "stubhash", pkg)
460+
os.MkdirAll(stubDir, 0755)
461+
os.WriteFile(filepath.Join(stubDir, "tools.json"), []byte(`{"tools":[]}`), 0644)
462+
463+
// Force the stub directory to be strictly newer than the real one so the
464+
// legacy newest-mtime selection would choose the stub.
465+
older := time.Now().Add(-2 * time.Hour)
466+
newer := time.Now()
467+
if err := os.Chtimes(realDir, older, older); err != nil {
468+
t.Fatalf("chtimes real: %v", err)
469+
}
470+
if err := os.Chtimes(stubDir, newer, newer); err != nil {
471+
t.Fatalf("chtimes stub: %v", err)
472+
}
473+
474+
r := NewSourceResolver(zap.NewNop())
475+
result, err := r.resolveNpxCache(ServerInfo{
476+
Name: "everything-server",
477+
Command: "npx",
478+
Args: []string{"-y", "@modelcontextprotocol/server-everything"},
479+
})
480+
if err != nil {
481+
t.Fatalf("resolveNpxCache: %v", err)
482+
}
483+
if result.SourceDir != realDir {
484+
t.Errorf("expected real source %q, got stub %q", realDir, result.SourceDir)
485+
}
486+
}
487+
437488
func TestResolveNpxCacheNotFound(t *testing.T) {
438489
homeDir := t.TempDir()
439490
t.Setenv("HOME", homeDir)

0 commit comments

Comments
 (0)