diff --git a/CLAUDE.md b/CLAUDE.md index 6067915..6aeab3e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -109,7 +109,7 @@ Default: auto-detects scope from git context. Override with `/config-audit full| node --test 'tests/**/*.test.mjs' ``` -918 tests across 56 test files (17 lib + 29 scanner + 1 hook + 1 agent + 3 commands + 1 knowledge + 4 top-level). Test fixtures in `tests/fixtures/`. Top-level humanizer tests: `json-backcompat.test.mjs`, `raw-backcompat.test.mjs`, `scenario-read-test.test.mjs`, `snapshot-default-output.test.mjs`. +929 tests across 56 test files (17 lib + 29 scanner + 1 hook + 1 agent + 3 commands + 1 knowledge + 4 top-level). Test fixtures in `tests/fixtures/`. Top-level humanizer tests: `json-backcompat.test.mjs`, `raw-backcompat.test.mjs`, `scenario-read-test.test.mjs`, `snapshot-default-output.test.mjs`. ### CML scanner — context-window-scaled char budget @@ -140,8 +140,15 @@ Beyond deny/allow overlap, the DIS scanner now also flags: only as a glob-free `mcp____*`. New `CA-DIS` finding, severity low. - **`Tool(*)` deny-all glob** — treated as equivalent to a bare deny (`Bash(*)` ≡ `Bash`), so a bare allow killed by it is correctly reported as dead config. +- **Forbidden-param rules** — `Tool(param:value)` whose key is the tool's own canonicalizing + field (`command` for Bash/PowerShell, `file_path` for Read/Edit/Write, `path` for + Grep/Glob, `notebook_path` for NotebookEdit, `url` for WebFetch). CC ignores these and + emits a startup warning. Severity follows intent: **deny/ask = false security (medium)** — + the block never applies; **allow = dead config (low)** — `param:value` matching is + deny/ask-only. Valid forms (`Bash(npm:*)`, `WebFetch(domain:host)`, `Agent(model:opus)`) + are never flagged. Predicate `forbiddenParamRule` in `permission-rules.mjs`. -Both predicates live in `scanners/lib/permission-rules.mjs` (shared with the CNF +These predicates live in `scanners/lib/permission-rules.mjs` (shared with the CNF conflict-detector). Behavior verified against `code.claude.com/docs/en/permissions`. ## Gotchas diff --git a/README.md b/README.md index f76f5ec..f92c795 100644 --- a/README.md +++ b/README.md @@ -12,7 +12,7 @@ ![Commands](https://img.shields.io/badge/commands-18-green) ![Agents](https://img.shields.io/badge/agents-6-orange) ![Hooks](https://img.shields.io/badge/hooks-4-red) -![Tests](https://img.shields.io/badge/tests-918+-brightgreen) +![Tests](https://img.shields.io/badge/tests-929+-brightgreen) ![License](https://img.shields.io/badge/license-MIT-lightgrey) A Claude Code plugin that checks configuration health, suggests context-aware improvements, and auto-fixes issues — `CLAUDE.md`, `settings.json`, hooks, rules, MCP servers, `@imports`, and plugins. 13 deterministic scanners across 10 quality areas, context-aware feature recommendations, auto-fix with backup/rollback, a prompt-cache-aware Token Hotspots scanner with optional API-calibrated `--accurate-tokens` mode, plus cache-prefix stability, dead-tool, and cross-plugin collision detection. Zero external dependencies. @@ -309,7 +309,7 @@ By default, `/config-audit` auto-detects scope from your git context. Override w | `feature-gap-scanner.mjs` | GAP | 25 feature checks shown as opportunities, not grades — plus a conditional `disableBundledSkills` recommendation when the active skill listing is over budget | | `token-hotspots.mjs` | TOK | Cache-breaking volatile content, redundant tool permissions, deep import chains, oversized cascades, bloated skill descriptions, MCP tool-schema budget | | `cache-prefix-scanner.mjs` | CPS | Volatile content in lines 31–150 of the CLAUDE.md cascade — beyond the cache-prefix window but still re-loaded every turn | -| `disabled-in-schema-scanner.mjs` | DIS | Dead/ineffective permission entries: (1) tools in BOTH `permissions.deny` and `permissions.allow` — deny wins (incl. the `Tool(*)` deny-all glob, equivalent to a bare deny); (2) unanchored allow wildcards (`*`, `B*`, `mcp__*`) that Claude Code silently skips — valid only as `mcp____*` | +| `disabled-in-schema-scanner.mjs` | DIS | Dead/ineffective permission entries: (1) tools in BOTH `permissions.deny` and `permissions.allow` — deny wins (incl. the `Tool(*)` deny-all glob, equivalent to a bare deny); (2) unanchored allow wildcards (`*`, `B*`, `mcp__*`) that Claude Code silently skips — valid only as `mcp____*`; (3) `Tool(param:value)` rules whose key is the tool's own canonicalizing field (`command`/`file_path`/`path`/`notebook_path`/`url`) — CC ignores these and emits a startup warning | | `collision-scanner.mjs` | COL | Cross-plugin skill name collisions; user-vs-plugin overlaps | > **Cross-scanner remediation — diagnosis meets the fix.** SKL diagnoses an over-budget @@ -330,6 +330,17 @@ By default, `/config-audit` auto-detects scope from your git context. Override w > both signals earn their place. The 200k/1M window constants live in the shared > `scanners/lib/context-window.mjs` (single source of truth with the skill-listing budget). +> **Permission rules CC silently ignores — severity follows intent.** `Tool(param:value)` +> matching is real (CC 2.1.178), but the tool's own canonicalizing fields are off-limits: +> `command` (Bash/PowerShell), `file_path` (Read/Edit/Write), `path` (Grep/Glob), +> `notebook_path` (NotebookEdit), `url` (WebFetch). CC ignores a rule keyed on its tool's +> field and emits a startup warning, because `Bash(command:rm *)` is bypassable by a compound +> command. DIS splits severity by where the rule lives: in **deny/ask** it is **false +> security** (medium — the block you intended never applies), in **allow** it is **dead +> config** (low — `param:value` matching is deny/ask-only, so the entry grants nothing). The +> predicate lives in `scanners/lib/permission-rules.mjs`; valid forms like `Bash(npm:*)`, +> `WebFetch(domain:host)`, and `Agent(model:opus)` are never flagged. + ### CLI Tools All tools work standalone — no Claude Code session needed: diff --git a/scanners/disabled-in-schema-scanner.mjs b/scanners/disabled-in-schema-scanner.mjs index d8e4ed8..7e97182 100644 --- a/scanners/disabled-in-schema-scanner.mjs +++ b/scanners/disabled-in-schema-scanner.mjs @@ -22,7 +22,7 @@ import { readTextFile } from './lib/file-discovery.mjs'; import { finding, scannerResult } from './lib/output.mjs'; import { SEVERITY } from './lib/severity.mjs'; import { parseJson } from './lib/yaml-parser.mjs'; -import { dominates, parseRule, isIneffectiveAllowGlob } from './lib/permission-rules.mjs'; +import { dominates, parseRule, isIneffectiveAllowGlob, forbiddenParamRule } from './lib/permission-rules.mjs'; const SCANNER = 'DIS'; @@ -65,6 +65,28 @@ function findIneffectiveAllowGlobs(settings) { return allowList.filter(e => isIneffectiveAllowGlob(e)); } +/** + * Find permission rules CC silently ignores because their `Tool(param:value)` + * key is the tool's own canonicalizing field (`command`, `file_path`, `path`, + * `notebook_path`, `url`). Scans allow + deny + ask so severity can split: + * deny/ask hits are false security, allow hits are dead config. Returns array + * of { list, entry, tool, key, hint }. + */ +function findForbiddenParamRules(settings) { + if (!settings || typeof settings !== 'object') return []; + const perms = settings.permissions; + if (!perms || typeof perms !== 'object') return []; + const results = []; + for (const list of ['allow', 'deny', 'ask']) { + const arr = Array.isArray(perms[list]) ? perms[list] : []; + for (const entry of arr) { + const hit = forbiddenParamRule(entry); + if (hit) results.push({ list, entry, ...hit }); + } + } + return results; +} + /** * Main scanner entry point. * @@ -127,6 +149,57 @@ export async function scan(targetPath, discovery) { category: 'permissions-hygiene', })); } + + const forbidden = findForbiddenParamRules(parsed); + const falseSecurity = forbidden.filter(x => x.list === 'deny' || x.list === 'ask'); + const deadAllow = forbidden.filter(x => x.list === 'allow'); + + if (falseSecurity.length > 0) { + const evidence = falseSecurity.slice(0, 5) + .map(x => `${x.list}: "${x.entry}" → use ${x.hint}`) + .join('; '); + findings.push(finding({ + scanner: SCANNER, + severity: SEVERITY.medium, + title: 'Permission rule silently ignored — deny/ask uses a forbidden param key', + description: + `${f.relPath || f.absPath} has ${falseSecurity.length} deny/ask ` + + `rule${falseSecurity.length === 1 ? '' : 's'} whose \`Tool(param:value)\` key is ` + + 'the tool\'s own canonicalizing field (`command`/`file_path`/`path`/`notebook_path`/' + + '`url`). Claude Code ignores these and emits a startup warning, so the guard you ' + + 'intended does NOT apply — the action you meant to block or gate is effectively ' + + 'unrestricted.', + file: f.absPath, + evidence, + recommendation: + 'Rewrite each rule with the tool\'s own specifier syntax (e.g. `Bash(rm *)`, ' + + '`Read(./path)`, `WebFetch(domain:host)`). As written these rules block nothing.', + category: 'permissions-hygiene', + })); + } + + if (deadAllow.length > 0) { + const evidence = deadAllow.slice(0, 5) + .map(x => `allow: "${x.entry}" → use ${x.hint}`) + .join('; '); + findings.push(finding({ + scanner: SCANNER, + severity: SEVERITY.low, + title: 'Permission rule silently ignored — allow uses a forbidden param key (dead config)', + description: + `${f.relPath || f.absPath} has ${deadAllow.length} permissions.allow ` + + `rule${deadAllow.length === 1 ? '' : 's'} using \`Tool(param:value)\` on the tool's ` + + 'own canonicalizing field. `param:value` matching applies only to deny/ask rules; ' + + 'allow rules use each tool\'s own specifier syntax. Claude Code ignores these and ' + + 'emits a startup warning — they grant nothing.', + file: f.absPath, + evidence, + recommendation: + 'Replace with the tool\'s specifier syntax (e.g. `Read(./path)`), or remove the ' + + 'entry. As written it auto-approves nothing.', + category: 'permissions-hygiene', + })); + } } return scannerResult(SCANNER, 'ok', findings, filesScanned, Date.now() - start); diff --git a/scanners/lib/permission-rules.mjs b/scanners/lib/permission-rules.mjs index 5fa04ba..777cd10 100644 --- a/scanners/lib/permission-rules.mjs +++ b/scanners/lib/permission-rules.mjs @@ -123,3 +123,64 @@ export function isIneffectiveAllowGlob(entry) { } return true; } + +/** + * Tools whose canonicalizing input field collides with `Tool(param:value)` + * matching. CC ignores a rule whose param key is the tool's own field and + * emits a startup warning, because the rule would be bypassable (e.g. a + * compound command defeats `Bash(command:rm *)`). + * + * CC: "Fields that a tool already matches with its own canonicalizing rules are + * not matchable this way: `command` for Bash and PowerShell, `file_path` for + * Read, Edit, and Write, `path` for Grep and Glob, `notebook_path` for + * NotebookEdit, and `url` for WebFetch." + * (code.claude.com/docs/en/permissions — "Match by input parameter") + */ +const FORBIDDEN_PARAMS = Object.freeze({ + Bash: 'command', + PowerShell: 'command', + Read: 'file_path', + Edit: 'file_path', + Write: 'file_path', + Grep: 'path', + Glob: 'path', + NotebookEdit: 'notebook_path', + WebFetch: 'url', +}); + +/** Correct specifier syntax to suggest in place of the forbidden param form. */ +const FORBIDDEN_PARAM_HINT = Object.freeze({ + Bash: 'Bash(rm *)', + PowerShell: 'PowerShell(Remove-Item *)', + Read: 'Read(./path)', + Edit: 'Edit(/src/**)', + Write: 'Write(/src/**)', + Grep: 'a Read rule (covers Grep)', + Glob: 'a Read rule (covers Glob)', + NotebookEdit: 'Edit(/notebooks/**)', + WebFetch: 'WebFetch(domain:host)', +}); + +/** + * Is this entry a `Tool(param:value)` rule whose param KEY is the tool's own + * canonicalizing field? CC silently ignores these (any list) and emits a + * startup warning. Returns `{ tool, key, hint }` or `null`. + * + * Only the `param:value` form (a colon present) is forbidden — `Bash(command)` + * is a literal command-prefix match and stays valid. The key must equal the + * tool's forbidden field, so `Bash(npm:*)`, `WebFetch(domain:x)`, and + * `Agent(model:opus)` are NOT flagged. + * @param {string} entry + * @returns {{ tool: string, key: string, hint: string }|null} + */ +export function forbiddenParamRule(entry) { + const { tool, param } = parseRule(entry); + if (!tool || param === null) return null; + const forbidden = FORBIDDEN_PARAMS[tool]; + if (!forbidden) return null; + const colon = param.indexOf(':'); + if (colon === -1) return null; // no `param:value` — literal specifier, valid + const key = param.slice(0, colon).trim(); + if (key !== forbidden) return null; + return { tool, key, hint: FORBIDDEN_PARAM_HINT[tool] }; +} diff --git a/tests/fixtures/forbidden-param-permissions/.claude/settings.json b/tests/fixtures/forbidden-param-permissions/.claude/settings.json new file mode 100644 index 0000000..601c934 --- /dev/null +++ b/tests/fixtures/forbidden-param-permissions/.claude/settings.json @@ -0,0 +1,17 @@ +{ + "permissions": { + "allow": [ + "Read(file_path:/etc/passwd)", + "Bash(npm:*)", + "WebFetch(domain:good.com)" + ], + "deny": [ + "Bash(command:rm *)", + "Grep(path:/secrets)", + "Agent(model:opus)" + ], + "ask": [ + "WebFetch(url:http://evil.com)" + ] + } +} diff --git a/tests/lib/permission-rules.test.mjs b/tests/lib/permission-rules.test.mjs index 083165f..47114dd 100644 --- a/tests/lib/permission-rules.test.mjs +++ b/tests/lib/permission-rules.test.mjs @@ -6,6 +6,7 @@ import { dominates, rulesIntersect, isIneffectiveAllowGlob, + forbiddenParamRule, } from '../../scanners/lib/permission-rules.mjs'; describe('permission-rules — parseRule', () => { @@ -109,6 +110,55 @@ describe('permission-rules — isIneffectiveAllowGlob (CC silently skips these a }); }); +describe('permission-rules — forbiddenParamRule (CC ignores Tool(param:value) for the tool\'s own canonicalizing field)', () => { + // CC: "Fields that a tool already matches with its own canonicalizing rules + // are not matchable this way: command for Bash and PowerShell, file_path for + // Read/Edit/Write, path for Grep/Glob, notebook_path for NotebookEdit, and + // url for WebFetch. ... Claude Code ignores it and emits a startup warning." + // (code.claude.com/docs/en/permissions — "Match by input parameter") + + it('flags command: for Bash and PowerShell', () => { + assert.equal(forbiddenParamRule('Bash(command:rm *)').key, 'command'); + assert.equal(forbiddenParamRule('PowerShell(command:Remove-Item *)').key, 'command'); + }); + + it('flags file_path: for Read/Edit/Write', () => { + assert.equal(forbiddenParamRule('Read(file_path:/etc/passwd)').key, 'file_path'); + assert.equal(forbiddenParamRule('Edit(file_path:/src/x)').tool, 'Edit'); + assert.equal(forbiddenParamRule('Write(file_path:/src/x)').tool, 'Write'); + }); + + it('flags path: for Grep/Glob, notebook_path: for NotebookEdit, url: for WebFetch', () => { + assert.equal(forbiddenParamRule('Grep(path:/secrets)').key, 'path'); + assert.equal(forbiddenParamRule('Glob(path:/secrets)').key, 'path'); + assert.equal(forbiddenParamRule('NotebookEdit(notebook_path:/nb.ipynb)').key, 'notebook_path'); + assert.equal(forbiddenParamRule('WebFetch(url:http://evil.com)').key, 'url'); + }); + + it('returns a tool-specific correct-syntax hint', () => { + assert.match(forbiddenParamRule('Bash(command:rm *)').hint, /Bash\(/); + assert.match(forbiddenParamRule('WebFetch(url:http://x)').hint, /domain:/); + }); + + it('does NOT flag valid specifier/param syntax for the same tools', () => { + assert.equal(forbiddenParamRule('Bash(npm:*)'), null); // npm: is a trailing-wildcard prefix, not command: + assert.equal(forbiddenParamRule('WebFetch(domain:good.com)'), null); // domain: is the valid WebFetch syntax + assert.equal(forbiddenParamRule('Read(./path)'), null); // gitignore-style path, no param:value + assert.equal(forbiddenParamRule('Bash(command)'), null); // literal command-prefix, no colon + }); + + it('does NOT flag param:value on tools that have no forbidden field', () => { + assert.equal(forbiddenParamRule('Agent(model:opus)'), null); + assert.equal(forbiddenParamRule('Bash(run_in_background:true)'), null); + }); + + it('bare tools and non-strings → null', () => { + assert.equal(forbiddenParamRule('Bash'), null); + assert.equal(forbiddenParamRule('Bash(*)'), null); + assert.equal(forbiddenParamRule(null), null); + }); +}); + describe('permission-rules — rulesIntersect (cross-scope conflict)', () => { it('exact same rule intersects', () => { assert.equal(rulesIntersect('Bash(npm run *)', 'Bash(npm run *)'), true); diff --git a/tests/scanners/disabled-in-schema.test.mjs b/tests/scanners/disabled-in-schema.test.mjs index e045d5a..6dba3b0 100644 --- a/tests/scanners/disabled-in-schema.test.mjs +++ b/tests/scanners/disabled-in-schema.test.mjs @@ -126,6 +126,50 @@ describe('DIS scanner — ineffective allow wildcards (CC skips unanchored tool- }); }); +describe('DIS scanner — forbidden-param rules CC silently ignores (Tool(param:value) on a canonicalizing field)', () => { + // fixture forbidden-param-permissions/.claude/settings.json: + // allow: Read(file_path:/etc/passwd), Bash(npm:*), WebFetch(domain:good.com) + // deny: Bash(command:rm *), Grep(path:/secrets), Agent(model:opus) + // ask: WebFetch(url:http://evil.com) + // CC ignores command:/file_path:/path:/url: and emits a startup warning. + // deny/ask hits = false security (medium); allow hits = dead config (low). + // Bash(npm:*), WebFetch(domain:good.com), Agent(model:opus) are VALID — never flagged. + + it('flags deny/ask forbidden-param rules with MEDIUM severity (false security)', async () => { + const result = await runScanner('forbidden-param-permissions'); + const f = result.findings.find(x => /silently ignored.*deny\/ask|deny\/ask.*forbidden param/i.test(x.title || '')); + assert.ok(f, `expected a medium forbidden-param finding; got: ${result.findings.map(x => x.title).join(' | ')}`); + assert.equal(f.severity, 'medium', `expected medium, got ${f.severity}`); + assert.match(f.id, /^CA-DIS-\d{3}$/); + }); + + it('medium evidence cites the deny + ask forbidden entries, not the valid ones', async () => { + const result = await runScanner('forbidden-param-permissions'); + const f = result.findings.find(x => /silently ignored.*deny\/ask|deny\/ask.*forbidden param/i.test(x.title || '')); + assert.ok(f); + assert.match(String(f.evidence || ''), /command:/); // Bash(command:rm *) + assert.match(String(f.evidence || ''), /path:/); // Grep(path:/secrets) + assert.match(String(f.evidence || ''), /url:/); // WebFetch(url:...) from ask + assert.doesNotMatch(String(f.evidence || ''), /Agent/); // model:opus is valid + }); + + it('flags allow forbidden-param rules with LOW severity (dead config)', async () => { + const result = await runScanner('forbidden-param-permissions'); + const f = result.findings.find(x => /silently ignored.*allow|allow.*forbidden param/i.test(x.title || '')); + assert.ok(f, `expected a low forbidden-param finding; got: ${result.findings.map(x => x.title).join(' | ')}`); + assert.equal(f.severity, 'low', `expected low, got ${f.severity}`); + assert.match(String(f.evidence || ''), /file_path:/); // Read(file_path:/etc/passwd) + assert.doesNotMatch(String(f.evidence || ''), /npm:/); // Bash(npm:*) is valid + assert.doesNotMatch(String(f.evidence || ''), /domain:/); // WebFetch(domain:...) is valid + }); + + it('valid param/specifier syntax produces no forbidden-param finding', async () => { + const result = await runScanner('param-qualified-permissions'); + const f = result.findings.find(x => /forbidden param|silently ignored/i.test(x.title || '')); + assert.equal(f, undefined, `expected no forbidden-param finding for valid syntax; got: ${f?.title}`); + }); +}); + describe('DIS scanner — deny-all glob Tool(*) kills a bare allow (end-to-end)', () => { // settings.json: allow: ["Bash"], deny: ["Bash(*)"] // Bash(*) deny ≡ bare Bash deny → the bare Bash allow is dead config.