Fix bare-verb severity tier + line locator for bunx / npm exec / yarn dlx#46
Merged
Merged
Conversation
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two follow-up bugs from the previous bare-verb and bunx work. Both surfaced in a reinspection; one of the inspector's proposed fixes was structurally broken and is corrected here.
Bug 1 — Bare
Bash/Write/Editgot medium severity instead of highisBroadAllowwas fixed in the prior PR to flag bare"Bash"/"Read"/"Write"/"Edit"as broad, butseverityForAllowstill matchedbash(,write(,edit(with the opening paren. So bare"Bash"— unlimited shell execution — gotmediumseverity instead ofhigh. Switched to a\b(bash|write|edit)\bword-boundary regex.readdeliberately stays atmedium(bare and scoped). Read access is less destructive than execute/modify; that asymmetry is intentional, not a bug.Bug 2 — Line locator never updated for modern runners
lineForUnpinnedCommandstill only mappednpx/uvx/pipx.bunxand the wrapper runners (npm exec,yarn dlx,pnpm dlx) were added toisUnpinnedCommandin prior PRs but the locator was left behind — those findings fell back to the server-declaration line instead of pointing at the unpinned package.Inspector's proposed fix put
npm/yarn/pnpmin the same branch asnpx. That's broken —looksLikePackageName('exec')andlooksLikePackageName('dlx')both returntrue, so a naive scan mis-locates to the subcommand line. Verified empirically:Correct fix:
bunxjoins the direct-runner branch (package inargs[0..]).npm/yarn/pnpmget their own branch that callsslice(1)to skip the subcommand, gated on the subcommand being a recognized executor (npm exec/x,yarn dlx,pnpm dlx/exec/x).Test plan
npm test— 54/54 passing locally (up from 52).mcp.jsonwith known line numbers asserts bunx + npm exec + yarn dlx all locate to the package line, not the subcommand lineNot in this PR
.aider.conf.yml,.cursorrules,.windsurfrulesdetection — same as the previous inspection, already filed as #42, #43, #44.🤖 Generated with Claude Code