From 7a54176f9401f72c7c1af2a9623fad0dbad3832f Mon Sep 17 00:00:00 2001 From: Conal <33135619+Conalh@users.noreply.github.com> Date: Thu, 21 May 2026 17:29:16 -0700 Subject: [PATCH] Detect sample MCP config drift --- src/detectors/mcp.ts | 170 +++++++++++++++++- src/git-snapshot.ts | 23 ++- .../new/examples/.mcp.json.sample | 15 ++ test/fixtures/mcp-sample-drift/old/.gitkeep | 1 + test/git-diff.test.mjs | 50 ++++++ test/heuristics-and-snapshot.test.mjs | 5 +- test/mcp-drift.test.mjs | 20 +++ 7 files changed, 278 insertions(+), 6 deletions(-) create mode 100644 test/fixtures/mcp-sample-drift/new/examples/.mcp.json.sample create mode 100644 test/fixtures/mcp-sample-drift/old/.gitkeep diff --git a/src/detectors/mcp.ts b/src/detectors/mcp.ts index 99548ea..f45e843 100644 --- a/src/detectors/mcp.ts +++ b/src/detectors/mcp.ts @@ -1,5 +1,6 @@ +import { readdir } from 'node:fs/promises'; import { configPath, isRecord, lineOfJsonKey, lineOfJsonStringValue, readJsonObjectWithSource } from '../discovery.js'; -import type { Finding, McpServerConfig } from '../types.js'; +import type { Finding, McpServerConfig, Severity } from '../types.js'; const MCP_CONFIGS = [ { path: '.mcp.json', serverKeys: ['mcpServers'] }, @@ -8,6 +9,27 @@ const MCP_CONFIGS = [ { path: '.codeium/windsurf/mcp_config.json', serverKeys: ['mcpServers'] } ] as const; +const MCP_SAMPLE_CONFIG_FILENAMES = new Set([ + '.mcp.json.sample', + '.mcp.json.disabled', + '.mcp.json.template', + '.mcp.json.example', + 'mcp_config.json.sample', + 'mcp_config.json.disabled', + 'mcp_config.json.template', + 'mcp_config.json.example' +]); + +const IGNORED_SAMPLE_SCAN_DIRS = new Set([ + '.git', + 'node_modules', + 'dist', + 'build', + 'coverage', + '.next', + '.turbo' +]); + // Exported so git-snapshot can materialize every surface this detector // reads. Keeping the source of truth in the detector prevents the // snapshot list and the detector list from drifting (they did, before). @@ -66,12 +88,70 @@ export async function detectMcpDrift(oldRoot: string, newRoot: string): Promise< } } + for (const path of await listMcpSampleConfigPaths(oldRoot, newRoot)) { + const config = { path, serverKeys: ['mcpServers', 'servers'] }; + const oldServers = await readMcpServers(oldRoot, config); + const newServers = await readMcpServers(newRoot, config); + + for (const [name, newServer] of Object.entries(newServers)) { + const oldServer = oldServers[name]; + const changed = oldServer && serverCommand(newServer) !== serverCommand(oldServer); + + if (!oldServer) { + findings.push({ + kind: 'mcp_sample_server_added', + severity: 'low', + file: path, + line: newServer.line, + subject: name, + message: `Sample/disabled MCP server "${name}" was added.`, + recommendation: 'Confirm this sample config is intentionally shipped and safe for users to copy before merging.' + }); + } else if (changed) { + findings.push({ + kind: 'mcp_sample_server_command_changed', + severity: 'low', + file: path, + line: lineForServerCommand(newServer) ?? newServer.line, + subject: name, + message: `Sample/disabled MCP server "${name}" changed its launch command.`, + recommendation: 'Confirm this sample config change is intentional and safe for users to copy before merging.' + }); + } + + if ((!oldServer || changed) && isUnpinnedCommand(newServer)) { + findings.push({ + kind: 'mcp_sample_unpinned_command', + severity: severityForSampleCommandRisk(newServer), + file: path, + line: lineForUnpinnedCommand(newServer) ?? newServer.line, + subject: name, + message: `Sample/disabled MCP server "${name}" uses an unpinned command: ${serverCommand(newServer)}.`, + recommendation: 'Pin sample MCP packages to an exact version so users do not copy a drifting install command.' + }); + } + + const endpoint = remoteEndpoint(newServer); + if ((!oldServer || changed) && endpoint) { + findings.push({ + kind: 'mcp_sample_remote_endpoint', + severity: 'medium', + file: path, + line: lineForRemoteEndpoint(newServer) ?? newServer.line, + subject: name, + message: `Sample/disabled MCP server "${name}" points at remote endpoint: ${endpoint}.`, + recommendation: 'Confirm the endpoint is intended for copied sample configs and does not expose unexpected data or tools.' + }); + } + } + } + return findings; } async function readMcpServers( root: string, - config: { path: McpConfigPath; serverKeys: readonly string[] } + config: { path: McpConfigPath | string; serverKeys: readonly string[] } ): Promise> { const source = await readJsonObjectWithSource(configPath(root, config.path)); const json = source.json; @@ -99,6 +179,53 @@ async function readMcpServers( return servers; } +export function isMcpSampleConfigPath(relativePath: string): boolean { + const normalized = normalizePath(relativePath); + const segments = normalized.split('/'); + if (segments.some((segment) => IGNORED_SAMPLE_SCAN_DIRS.has(segment))) { + return false; + } + + const fileName = segments.at(-1); + return fileName ? MCP_SAMPLE_CONFIG_FILENAMES.has(fileName) : false; +} + +async function listMcpSampleConfigPaths(...roots: string[]): Promise { + const paths = new Set(); + for (const root of roots) { + await collectMcpSampleConfigPaths(root, '', paths); + } + + return [...paths].sort(); +} + +async function collectMcpSampleConfigPaths(root: string, relativeDir: string, paths: Set): Promise { + let entries; + try { + entries = await readdir(configPath(root, relativeDir), { withFileTypes: true }); + } catch (error) { + if (isNodeError(error) && error.code === 'ENOENT') { + return; + } + + throw error; + } + + for (const entry of entries) { + const relativePath = relativeDir ? `${relativeDir}/${entry.name}` : entry.name; + if (entry.isDirectory()) { + if (!IGNORED_SAMPLE_SCAN_DIRS.has(entry.name)) { + await collectMcpSampleConfigPaths(root, relativePath, paths); + } + continue; + } + + if (entry.isFile() && isMcpSampleConfigPath(relativePath)) { + paths.add(relativePath); + } + } +} + function readServerMap(json: Record, serverKeys: readonly string[]): unknown { for (const key of serverKeys) { if (isRecord(json[key])) { @@ -138,6 +265,16 @@ function isUnpinnedCommand(server: McpServerModel): boolean { && packageLikeArgs.some((arg) => looksLikePackageName(arg) && !hasExactVersion(arg)); } +function severityForSampleCommandRisk(server: McpServerModel): Severity { + return isPipeToShellCommand(server) ? 'high' : 'medium'; +} + +function isPipeToShellCommand(server: McpServerModel): boolean { + const normalized = serverCommand(server).toLowerCase(); + return /\bcurl\b.+\|\s*(bash|sh)\b/.test(normalized) + || /\b(iwr|invoke-webrequest)\b.+\|\s*(iex|invoke-expression)\b/.test(normalized); +} + function looksLikePackageName(value: string): boolean { return /^[a-z0-9@][a-z0-9._/@-]+$/i.test(value) && !value.startsWith('-'); } @@ -182,6 +319,27 @@ function lineForUnpinnedCommand(server: McpServerModel): number | undefined { return server.line; } +function lineForRemoteEndpoint(server: McpServerModel): number | undefined { + return firstLineForValues(server, [server.url, server.serverUrl], isRemoteEndpoint); +} + +function remoteEndpoint(server: McpServerModel): string | undefined { + return [server.url, server.serverUrl].find((value): value is string => Boolean(value && isRemoteEndpoint(value))); +} + +function isRemoteEndpoint(value: string): boolean { + try { + const url = new URL(value); + if (url.protocol !== 'http:' && url.protocol !== 'https:') { + return false; + } + + return !['localhost', '127.0.0.1', '::1'].includes(url.hostname); + } catch { + return false; + } +} + function firstLineForValues( server: McpServerModel, values: Array, @@ -203,3 +361,11 @@ function firstLineForValues( function getSourceText(server: McpServerModel): string { return server.sourceText ?? ''; } + +function normalizePath(path: string): string { + return path.replaceAll('\\', '/'); +} + +function isNodeError(error: unknown): error is NodeJS.ErrnoException { + return error instanceof Error && 'code' in error; +} diff --git a/src/git-snapshot.ts b/src/git-snapshot.ts index f508304..aaf3ab9 100644 --- a/src/git-snapshot.ts +++ b/src/git-snapshot.ts @@ -5,7 +5,7 @@ import { dirname, join } from 'node:path'; import { promisify } from 'node:util'; import { CLAUDE_TARGET_PATHS } from './detectors/claude-settings.js'; import { CODEX_TARGET_PATHS } from './detectors/codex-config.js'; -import { MCP_TARGET_PATHS } from './detectors/mcp.js'; +import { MCP_TARGET_PATHS, isMcpSampleConfigPath } from './detectors/mcp.js'; const execFileAsync = promisify(execFile); @@ -31,7 +31,7 @@ export async function materializeGitSnapshot(repo: string, ref: string): Promise const root = await mkdtemp(join(tmpdir(), 'scopetrail-snapshot-')); let completed = false; try { - for (const relativePath of SNAPSHOT_PATHS) { + for (const relativePath of await snapshotPathsForRef(repo, ref)) { const content = await readPathAtRef(repo, ref, relativePath); if (content === null) { continue; @@ -56,10 +56,29 @@ export async function materializeGitSnapshot(repo: string, ref: string): Promise } } +async function snapshotPathsForRef(repo: string, ref: string): Promise { + const paths = new Set(SNAPSHOT_PATHS); + for (const relativePath of await listPathsAtRef(repo, ref)) { + if (isMcpSampleConfigPath(relativePath)) { + paths.add(relativePath); + } + } + + return [...paths].sort(); +} + async function verifyGitRef(repo: string, ref: string): Promise { await execFileAsync('git', ['-C', repo, 'rev-parse', '--verify', `${ref}^{commit}`]); } +async function listPathsAtRef(repo: string, ref: string): Promise { + const { stdout } = await execFileAsync('git', ['-C', repo, 'ls-tree', '-r', '--name-only', ref], { + encoding: 'utf8', + maxBuffer: 10 * 1024 * 1024 + }); + return stdout.split(/\r?\n/).filter(Boolean); +} + async function readPathAtRef(repo: string, ref: string, relativePath: string): Promise { try { const { stdout } = await execFileAsync('git', ['-C', repo, 'show', `${ref}:${relativePath}`], { diff --git a/test/fixtures/mcp-sample-drift/new/examples/.mcp.json.sample b/test/fixtures/mcp-sample-drift/new/examples/.mcp.json.sample new file mode 100644 index 0000000..d081165 --- /dev/null +++ b/test/fixtures/mcp-sample-drift/new/examples/.mcp.json.sample @@ -0,0 +1,15 @@ +{ + "mcpServers": { + "docs-search": { + "command": "npx", + "args": ["-y", "@acme/docs-mcp@1.2.3"] + }, + "copy-risk": { + "command": "npx", + "args": ["-y", "@acme/copy-risk@latest"] + }, + "remote-admin": { + "serverUrl": "https://mcp.example.invalid/admin" + } + } +} diff --git a/test/fixtures/mcp-sample-drift/old/.gitkeep b/test/fixtures/mcp-sample-drift/old/.gitkeep new file mode 100644 index 0000000..8b13789 --- /dev/null +++ b/test/fixtures/mcp-sample-drift/old/.gitkeep @@ -0,0 +1 @@ + diff --git a/test/git-diff.test.mjs b/test/git-diff.test.mjs index c357ba7..680cc0c 100644 --- a/test/git-diff.test.mjs +++ b/test/git-diff.test.mjs @@ -90,6 +90,56 @@ test('CLI diffs permission drift between git refs', async () => { } }); +test('CLI git diff snapshots sample MCP config paths', async () => { + const repo = await mkdtemp(join(tmpdir(), 'scopetrail-git-sample-')); + try { + await execGit(repo, 'init', '-b', 'main'); + await execGit(repo, 'config', 'user.name', 'ScopeTrail Test'); + await execGit(repo, 'config', 'user.email', 'scopetrail@example.invalid'); + + await writeFile(join(repo, 'README.md'), 'base\n'); + await execGit(repo, 'add', '.'); + await execGit(repo, 'commit', '-m', 'base'); + const base = await gitStdout(repo, 'rev-parse', 'HEAD'); + + await mkdir(join(repo, 'examples'), { recursive: true }); + await writeFile( + join(repo, 'examples', '.mcp.json.sample'), + `${JSON.stringify( + { + mcpServers: { + 'copy-risk': { + command: 'npx', + args: ['-y', '@acme/copy-risk@latest'] + } + } + }, + null, + 2 + )}\n` + ); + await execGit(repo, 'add', '.'); + await execGit(repo, 'commit', '-m', 'add sample mcp config'); + const head = await gitStdout(repo, 'rev-parse', 'HEAD'); + + const { stdout } = await execFileAsync( + process.execPath, + ['dist/index.js', 'diff', '--repo', repo, '--base', base, '--head', head, '--format', 'json'], + { cwd: packageRoot } + ); + const report = JSON.parse(stdout); + + assert.deepEqual( + report.findings.map((finding) => finding.kind), + ['mcp_sample_server_added', 'mcp_sample_unpinned_command'] + ); + assert.equal(report.findings[0].file, 'examples/.mcp.json.sample'); + assert.equal(report.findings.some((finding) => finding.kind === 'mcp_server_added'), false); + } finally { + await rm(repo, { recursive: true, force: true }); + } +}); + async function writeConfig(repo, { mcp, claude }) { await mkdir(join(repo, '.claude'), { recursive: true }); await writeFile(join(repo, '.mcp.json'), `${JSON.stringify(mcp, null, 2)}\n`); diff --git a/test/heuristics-and-snapshot.test.mjs b/test/heuristics-and-snapshot.test.mjs index 3758020..3d492a8 100644 --- a/test/heuristics-and-snapshot.test.mjs +++ b/test/heuristics-and-snapshot.test.mjs @@ -20,12 +20,13 @@ const codexDetector = await import(pathToFileURL(join(distRoot, 'detectors', 'co const { isBroadAllow, detectClaudeSettingsDrift } = claudeSettings; const { SNAPSHOT_PATHS } = gitSnapshot; -test('SNAPSHOT_PATHS covers every path each detector reads (regression)', () => { +test('SNAPSHOT_PATHS covers every fixed path each detector reads (regression)', () => { // Real bug observed before this PR: git-snapshot only listed // .mcp.json and .claude/settings.json, so the Action silently missed // .cursor/mcp.json, .vscode/mcp.json, .codeium/windsurf/mcp_config.json, // and .codex/config.toml. This test fails loudly if a detector adds a - // target path and the snapshot list isn't updated to match. + // fixed target path and the snapshot list isn't updated to match. + // Dynamic sample MCP paths are covered by the git-diff fixture test. const required = new Set([ ...mcpDetector.MCP_TARGET_PATHS, ...claudeSettings.CLAUDE_TARGET_PATHS, diff --git a/test/mcp-drift.test.mjs b/test/mcp-drift.test.mjs index 07ab5e3..b75c6e7 100644 --- a/test/mcp-drift.test.mjs +++ b/test/mcp-drift.test.mjs @@ -55,3 +55,23 @@ test('detects MCP drift in Windsurf config files', async () => { ] ); }); + +test('detects sample MCP config drift without treating it as active server drift', async () => { + const oldDir = join(testDir, 'fixtures', 'mcp-sample-drift', 'old'); + const newDir = join(testDir, 'fixtures', 'mcp-sample-drift', 'new'); + + const findings = await detectMcpDrift(oldDir, newDir); + + assert.equal(findings.some((finding) => finding.kind === 'mcp_server_added'), false); + assert.equal(findings.some((finding) => finding.kind === 'unpinned_mcp_command'), false); + assert.deepEqual( + findings.map((finding) => [finding.file, finding.kind, finding.subject, finding.severity, finding.line]), + [ + ['examples/.mcp.json.sample', 'mcp_sample_server_added', 'docs-search', 'low', 3], + ['examples/.mcp.json.sample', 'mcp_sample_server_added', 'copy-risk', 'low', 7], + ['examples/.mcp.json.sample', 'mcp_sample_unpinned_command', 'copy-risk', 'medium', 9], + ['examples/.mcp.json.sample', 'mcp_sample_server_added', 'remote-admin', 'low', 11], + ['examples/.mcp.json.sample', 'mcp_sample_remote_endpoint', 'remote-admin', 'medium', 12] + ] + ); +});