From d8c697db2d945730bb35448232951cd4cd25f314 Mon Sep 17 00:00:00 2001 From: Conal <33135619+Conalh@users.noreply.github.com> Date: Fri, 22 May 2026 16:28:11 -0700 Subject: [PATCH] Fix bare-verb severity + line locator for modern MCP runners MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two follow-up bugs from the previous bare-verb / bunx fixes: 1. `severityForAllow` matched `bash(`, `write(`, `edit(` literally, requiring the opening paren. Bare `"Bash"` — which the previous security fix correctly added to the broad-allow finding — silently dropped to `medium` severity despite granting unlimited shell execution. Switch to a `\b(bash|write|edit)\b` word-boundary regex so bare verbs and parenthesized scopes are both `high`. `read` deliberately stays out of the high tier: a bare `"Read"` token is still flagged as a finding, just at `medium`, because read access is less destructive than execute/modify. That asymmetry is intentional. 2. `lineForUnpinnedCommand` only mapped npx/uvx/pipx to the args-scan locator. `bunx` (added to `isUnpinnedCommand` in fb56768) and the wrapper runners npm/yarn/pnpm (added in 3f44e1e) were missing — their findings fell back to the server declaration line instead of pointing at the unpinned package. `bunx` is a direct runner like npx (package in args[0..]), so it joins that branch directly. `npm`/`yarn`/`pnpm` are wrappers: args[0] is the executor subcommand (`exec`, `dlx`, `x`). The inspector's proposed fix put them in the same branch as npx, which is broken — `looksLikePackageName('exec')` and `looksLikePackageName('dlx')` both return `true`, so a naive scan mis-locates to the subcommand line. Add a separate branch that calls `slice(1)` on args before scanning, gated on the subcommand being a recognized executor (`npm exec/x`, `yarn dlx`, `pnpm dlx/exec/x`). Non-executor `npm foo bar` configs don't trigger this path. Regression tests: - Bare Bash/Write/Edit assert `high` severity; bare Read asserts `medium` to lock the design asymmetry. - Hand-written .mcp.json with known line numbers asserts that bunx / npm exec / yarn dlx line locators all point at the package line, not the subcommand line or the server header. --- dist/detectors/claude-settings.js | 12 ++++- dist/detectors/mcp.js | 27 ++++++++++- src/detectors/claude-settings.ts | 12 ++++- src/detectors/mcp.ts | 25 +++++++++- test/heuristics-and-snapshot.test.mjs | 22 +++++++++ test/mcp-drift.test.mjs | 67 +++++++++++++++++++++++++++ 6 files changed, 161 insertions(+), 4 deletions(-) diff --git a/dist/detectors/claude-settings.js b/dist/detectors/claude-settings.js index 4e3a872..0353853 100644 --- a/dist/detectors/claude-settings.js +++ b/dist/detectors/claude-settings.js @@ -205,7 +205,17 @@ function isBroadMcpGrant(normalized) { } function severityForAllow(permission) { const normalized = permission.toLowerCase(); - if (normalized.includes('bash(') || normalized.includes('write(') || normalized.includes('edit(')) { + // Match bare verbs (`Bash`, `Write`, `Edit`) and parenthesized + // scoped grants (`Bash(npm *)`, `Write(./foo)`, `Edit(...)`) + // uniformly. The previous `includes('bash(')` check required the + // opening paren, so bare `"Bash"` — which `isBroadAllow` now + // correctly flags as broad — silently fell to `medium` severity + // despite granting unlimited shell execution. + // + // `read` stays medium even when bare/wildcard — read access is + // less destructive than execute/modify. Bare `Read` still surfaces + // as a finding via `isBroadAllow`, just at medium severity. + if (/\b(bash|write|edit)\b/.test(normalized)) { return 'high'; } return 'medium'; diff --git a/dist/detectors/mcp.js b/dist/detectors/mcp.js index 9929d01..ae3f365 100644 --- a/dist/detectors/mcp.js +++ b/dist/detectors/mcp.js @@ -290,9 +290,24 @@ function lineForUnpinnedCommand(server) { if (/\b(curl|iwr|invoke-webrequest)\b/.test(normalized)) { return firstLineForValues(server, [server.command, ...(server.args ?? []), server.url, server.serverUrl]); } - if (['npx', 'uvx', 'pipx'].includes((server.command ?? '').toLowerCase())) { + const cmd = (server.command ?? '').toLowerCase(); + // Direct runners: the package name lives in args[0..]. `bunx` was + // added to `isUnpinnedCommand` in fb56768 but not to the line + // locator — bunx findings fell back to the server declaration's + // line instead of pointing at the package. + if (['npx', 'uvx', 'pipx', 'bunx'].includes(cmd)) { return firstLineForValues(server, server.args ?? [], (arg) => looksLikePackageName(arg) && !hasExactVersion(arg)); } + // Wrapper runners: args[0] is the executor subcommand (`exec`, + // `dlx`, `x`). Skip it before locating — `exec` and `dlx` both + // pass `looksLikePackageName`, so a naive scan would mis-locate + // to the subcommand line instead of the package. + if (['npm', 'yarn', 'pnpm'].includes(cmd)) { + const args = server.args ?? []; + if (args.length > 1 && isWrapperSubcommand(cmd, args[0])) { + return firstLineForValues(server, args.slice(1), (arg) => looksLikePackageName(arg) && !hasExactVersion(arg)); + } + } if (/https:\/\/github\.com\/[^ ]+/.test(normalized)) { return firstLineForValues(server, [server.url, server.serverUrl, ...(server.args ?? [])], (value) => value.toLowerCase().includes('https://github.com/')); } @@ -301,6 +316,16 @@ function lineForUnpinnedCommand(server) { function lineForRemoteEndpoint(server) { return firstLineForValues(server, [server.url, server.serverUrl], isRemoteEndpoint); } +function isWrapperSubcommand(cmd, arg) { + const sub = arg.toLowerCase(); + if (cmd === 'npm') + return sub === 'exec' || sub === 'x'; + if (cmd === 'yarn') + return sub === 'dlx'; + if (cmd === 'pnpm') + return sub === 'dlx' || sub === 'exec' || sub === 'x'; + return false; +} function firstLineForValues(server, values, predicate = () => true) { const source = getSourceText(server); for (const value of values) { diff --git a/src/detectors/claude-settings.ts b/src/detectors/claude-settings.ts index 4bd11fa..4cead71 100644 --- a/src/detectors/claude-settings.ts +++ b/src/detectors/claude-settings.ts @@ -240,7 +240,17 @@ function isBroadMcpGrant(normalized: string): boolean { function severityForAllow(permission: string): Severity { const normalized = permission.toLowerCase(); - if (normalized.includes('bash(') || normalized.includes('write(') || normalized.includes('edit(')) { + // Match bare verbs (`Bash`, `Write`, `Edit`) and parenthesized + // scoped grants (`Bash(npm *)`, `Write(./foo)`, `Edit(...)`) + // uniformly. The previous `includes('bash(')` check required the + // opening paren, so bare `"Bash"` — which `isBroadAllow` now + // correctly flags as broad — silently fell to `medium` severity + // despite granting unlimited shell execution. + // + // `read` stays medium even when bare/wildcard — read access is + // less destructive than execute/modify. Bare `Read` still surfaces + // as a finding via `isBroadAllow`, just at medium severity. + if (/\b(bash|write|edit)\b/.test(normalized)) { return 'high'; } diff --git a/src/detectors/mcp.ts b/src/detectors/mcp.ts index ee023fa..87c7e61 100644 --- a/src/detectors/mcp.ts +++ b/src/detectors/mcp.ts @@ -353,9 +353,24 @@ function lineForUnpinnedCommand(server: McpServerModel): number | undefined { return firstLineForValues(server, [server.command, ...(server.args ?? []), server.url, server.serverUrl]); } - if (['npx', 'uvx', 'pipx'].includes((server.command ?? '').toLowerCase())) { + const cmd = (server.command ?? '').toLowerCase(); + // Direct runners: the package name lives in args[0..]. `bunx` was + // added to `isUnpinnedCommand` in fb56768 but not to the line + // locator — bunx findings fell back to the server declaration's + // line instead of pointing at the package. + if (['npx', 'uvx', 'pipx', 'bunx'].includes(cmd)) { return firstLineForValues(server, server.args ?? [], (arg) => looksLikePackageName(arg) && !hasExactVersion(arg)); } + // Wrapper runners: args[0] is the executor subcommand (`exec`, + // `dlx`, `x`). Skip it before locating — `exec` and `dlx` both + // pass `looksLikePackageName`, so a naive scan would mis-locate + // to the subcommand line instead of the package. + if (['npm', 'yarn', 'pnpm'].includes(cmd)) { + const args = server.args ?? []; + if (args.length > 1 && isWrapperSubcommand(cmd, args[0])) { + return firstLineForValues(server, args.slice(1), (arg) => looksLikePackageName(arg) && !hasExactVersion(arg)); + } + } if (/https:\/\/github\.com\/[^ ]+/.test(normalized)) { return firstLineForValues(server, [server.url, server.serverUrl, ...(server.args ?? [])], (value) => @@ -370,6 +385,14 @@ function lineForRemoteEndpoint(server: McpServerModel): number | undefined { return firstLineForValues(server, [server.url, server.serverUrl], isRemoteEndpoint); } +function isWrapperSubcommand(cmd: string, arg: string): boolean { + const sub = arg.toLowerCase(); + if (cmd === 'npm') return sub === 'exec' || sub === 'x'; + if (cmd === 'yarn') return sub === 'dlx'; + if (cmd === 'pnpm') return sub === 'dlx' || sub === 'exec' || sub === 'x'; + return false; +} + function firstLineForValues( server: McpServerModel, diff --git a/test/heuristics-and-snapshot.test.mjs b/test/heuristics-and-snapshot.test.mjs index c802123..2ad8872 100644 --- a/test/heuristics-and-snapshot.test.mjs +++ b/test/heuristics-and-snapshot.test.mjs @@ -68,6 +68,28 @@ test('isBroadAllow: bare Bash/Read/Write/Edit ARE broad (regression for security assert.equal(isBroadAllow('Edit'), true); }); +test('Claude detector: bare Bash/Write/Edit get high severity (not medium)', async () => { + // Pre-fix gap: `severityForAllow` required the opening paren + // (`bash(`, `write(`, `edit(`) to assign `high`, so bare `"Bash"` + // — which grants unlimited shell execution — silently dropped to + // `medium`. Bare `Read` deliberately stays medium because read + // access is less destructive than execute/modify. + const dir = await makeClaudeFixture( + { permissions: { allow: [], deny: [] }, hooks: {} }, + { permissions: { allow: ['Bash', 'Write', 'Edit', 'Read'], deny: [] }, hooks: {} } + ); + try { + const findings = await detectClaudeSettingsDrift(dir.oldRoot, dir.newRoot); + const bySubject = Object.fromEntries(findings.map((f) => [f.subject, f])); + assert.equal(bySubject['Bash'].severity, 'high', 'bare Bash should be high'); + assert.equal(bySubject['Write'].severity, 'high', 'bare Write should be high'); + assert.equal(bySubject['Edit'].severity, 'high', 'bare Edit should be high'); + assert.equal(bySubject['Read'].severity, 'medium', 'bare Read stays medium by design'); + } finally { + await rm(dir.root, { recursive: true, force: true }); + } +}); + test('isBroadAllow: narrowly-scoped Bash/Read/Write/Edit stay narrow', () => { // The bare-verb fix must not over-fire on legitimate narrow scopes. assert.equal(isBroadAllow('Bash(npm test)'), false); diff --git a/test/mcp-drift.test.mjs b/test/mcp-drift.test.mjs index dc12de2..044277e 100644 --- a/test/mcp-drift.test.mjs +++ b/test/mcp-drift.test.mjs @@ -46,6 +46,73 @@ test('mcp_sample_remote_endpoint: http:// fires high severity, https:// stays me } }); +test('lineForUnpinnedCommand pinpoints package line for bunx + npm exec / yarn dlx / pnpm dlx', async () => { + // Pre-fix gap: `lineForUnpinnedCommand` only mapped npx/uvx/pipx, + // so bunx findings (and the wrapper-runners npm/yarn/pnpm with + // exec/dlx subcommands) fell back to the server-declaration line + // instead of pointing at the package the reviewer needs to see. + // + // For wrappers we also have to skip args[0] (the subcommand) — + // `exec` and `dlx` both pass `looksLikePackageName`, so a naive + // scan would mis-locate to the subcommand line. + const { mkdtempSync, writeFileSync, mkdirSync, rmSync } = await import('node:fs'); + const { tmpdir } = await import('node:os'); + + const root = mkdtempSync(join(tmpdir(), 'scopetrail-line-')); + try { + const oldDir = join(root, 'old'); + const newDir = join(root, 'new'); + mkdirSync(oldDir, { recursive: true }); + mkdirSync(newDir, { recursive: true }); + writeFileSync(join(oldDir, '.mcp.json'), JSON.stringify({ mcpServers: {} })); + // Hand-written JSON so we know exact line numbers — package + // string for each server is on its own line. + const content = [ + '{', + ' "mcpServers": {', + ' "bun-server": {', + ' "command": "bunx",', + ' "args": [', + ' "@vendor/bun-pkg"', + ' ]', + ' },', + ' "npm-server": {', + ' "command": "npm",', + ' "args": [', + ' "exec",', + ' "@vendor/npm-pkg"', + ' ]', + ' },', + ' "yarn-server": {', + ' "command": "yarn",', + ' "args": [', + ' "dlx",', + ' "@vendor/yarn-pkg"', + ' ]', + ' }', + ' }', + '}', + '' + ].join('\n'); + writeFileSync(join(newDir, '.mcp.json'), content); + + const findings = await detectMcpDrift(oldDir, newDir); + const unpinned = findings.filter((f) => f.kind === 'scope_trail.unpinned_mcp_command'); + const bySubject = Object.fromEntries(unpinned.map((f) => [f.subject, f])); + + assert.ok(bySubject['bun-server'], 'expected unpinned finding for bunx'); + assert.equal(bySubject['bun-server'].line, 6, 'bunx package is on line 6'); + + assert.ok(bySubject['npm-server'], 'expected unpinned finding for npm exec'); + assert.equal(bySubject['npm-server'].line, 13, 'npm exec package is on line 13 (skips exec subcommand on line 12)'); + + assert.ok(bySubject['yarn-server'], 'expected unpinned finding for yarn dlx'); + assert.equal(bySubject['yarn-server'].line, 20, 'yarn dlx package is on line 20 (skips dlx subcommand on line 19)'); + } finally { + rmSync(root, { recursive: true, force: true }); + } +}); + test('isUnpinnedCommand flags bunx packages without exact versions', async () => { // bunx is the Bun equivalent of npx and ships as a standalone // binary, so MCP configs use `"command": "bunx"` directly. Prior