From 03949c6c987ffa0b06f60ff7e8702b05be69f529 Mon Sep 17 00:00:00 2001 From: Kjell Tore Guttormsen Date: Fri, 19 Jun 2026 06:31:18 +0200 Subject: [PATCH] feat(dis): flag ineffective allow wildcards; treat Tool(*) as deny-all MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extends the DIS scanner and its shared permission-rules lib with two documented Claude Code permission footguns. Verified verbatim against code.claude.com/docs/en/permissions (fetched 2026-06-19). - lib/permission-rules.mjs: new isIneffectiveAllowGlob(entry) — unanchored tool-name globs in permissions.allow (`*`, `B*`, `mcp__*`) that CC silently skips ("does not auto-approve anything"); valid only as a glob-free `mcp____*`. Shared with CNF. - lib/permission-rules.mjs: dominates() now treats the `Tool(*)` deny-all glob as equivalent to a bare deny (covers a bare allow) — CC: "Bash(*) is equivalent to Bash ... both forms remove the tool from Claude's context". - DIS: new finding "Ineffective allow wildcard — Claude Code ignores this rule" (low, permissions-hygiene, CA-DIS-NNN); the existing dead-allow finding now also catches a bare allow killed by a Tool(*) deny. - 9 new tests (5 lib, 4 DIS) + 2 fixtures (force-added past .gitignore .claude/). Suite 903 -> 912. Snapshot unchanged, contamination grep clean. README/CLAUDE/ scanner-internals document the broadened DIS mandate; test badge synced. self-audit: PASS, configGrade A 96, pluginGrade A 100, readme gate passed. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Ter3E2JSi1Khgmuf2kady8 --- CLAUDE.md | 15 +++- README.md | 4 +- docs/scanner-internals.md | 2 +- scanners/disabled-in-schema-scanner.mjs | 79 +++++++++++++------ scanners/lib/permission-rules.mjs | 42 +++++++++- .../deny-all-glob/.claude/settings.json | 6 ++ .../.claude/settings.json | 5 ++ tests/lib/permission-rules.test.mjs | 34 ++++++++ tests/scanners/disabled-in-schema.test.mjs | 41 ++++++++++ 9 files changed, 199 insertions(+), 29 deletions(-) create mode 100644 tests/fixtures/deny-all-glob/.claude/settings.json create mode 100644 tests/fixtures/ineffective-allow-globs/.claude/settings.json diff --git a/CLAUDE.md b/CLAUDE.md index 344708f..1559bb9 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -109,7 +109,20 @@ Default: auto-detects scope from git context. Override with `/config-audit full| node --test 'tests/**/*.test.mjs' ``` -903 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`. +912 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`. + +### DIS scanner — permission-rule hygiene + +Beyond deny/allow overlap, the DIS scanner now also flags: + +- **Ineffective allow wildcards** — unanchored tool-name globs in `permissions.allow` + (`*`, `B*`, `mcp__*`) that Claude Code silently skips (auto-approve nothing). Valid + 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. + +Both 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 0d9aca5..21fc0f6 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-903+-brightgreen) +![Tests](https://img.shields.io/badge/tests-912+-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 | Tools listed in BOTH `permissions.deny` and `permissions.allow` — deny wins, allow entries are dead config | +| `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____*` | | `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 diff --git a/docs/scanner-internals.md b/docs/scanner-internals.md index fe19728..8c20575 100644 --- a/docs/scanner-internals.md +++ b/docs/scanner-internals.md @@ -20,7 +20,7 @@ Scanner CLI: `node scanners/scan-orchestrator.mjs [--global] [--full-mach | `feature-gap-scanner.mjs` | GAP | 25 feature checks across 4 tiers — shown as opportunities, not grades | | `token-hotspots.mjs` | TOK | Cache-breaking volatile content, redundant tool permissions, deep import chains, oversized cascade, bloated SKILL.md descriptions, MCP tool-schema budget (prompt-cache patterns) | | `cache-prefix-scanner.mjs` | CPS | Volatile content in lines 31–150 of CLAUDE.md cascade (beyond Pattern A's top-30 window) | -| `disabled-in-schema-scanner.mjs` | DIS | Tools listed in BOTH `permissions.deny` AND `permissions.allow` — deny wins, allow entries are dead config | +| `disabled-in-schema-scanner.mjs` | DIS | Dead/ineffective permission entries (low). (1) Tools in BOTH `permissions.deny` AND `permissions.allow` — deny wins; dominance is param-aware and treats the `Tool(*)` deny-all glob as equivalent to a bare deny (covers a bare allow). (2) Unanchored allow wildcards (`*`, `B*`, `mcp__*`) that Claude Code silently skips — CC accepts allow globs only after a literal glob-free `mcp____` prefix. Predicates shared with CNF live in `lib/permission-rules.mjs` | | `collision-scanner.mjs` | COL | Cross-plugin skill name collisions (low); user-vs-plugin overlaps (medium); `details.namespaces` payload | | `skill-listing-scanner.mjs` | SKL | (1) `CA-SKL-001` (medium): active skill descriptions over the verified 1,536-char listing cap (CC 2.1.105) → silently truncated in the model's skill listing. (2) `CA-SKL-002` (low): sum of active descriptions (each counted up to the cap) over the listing budget (~2% of context, CC 2.1.32), anchored on a conservative 200k window with a calibration note that the budget scales 5× on 1M-context models — leads with the measured sum, an estimate not telemetry. HOME-scoped (all user + plugin skills). Remediation surfaces `disableBundledSkills` / `skillOverrides` / trim. Distinct lens from TOK pattern F (project-local 500-char bloat heuristic) | diff --git a/scanners/disabled-in-schema-scanner.mjs b/scanners/disabled-in-schema-scanner.mjs index 0968493..d8e4ed8 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 } from './lib/permission-rules.mjs'; +import { dominates, parseRule, isIneffectiveAllowGlob } from './lib/permission-rules.mjs'; const SCANNER = 'DIS'; @@ -52,6 +52,19 @@ function findDenyAllowOverlaps(settings) { return overlaps; } +/** + * Find `permissions.allow` entries that are unanchored tool-name globs Claude + * Code silently skips (e.g. `mcp__*`, `B*`, `*`). They auto-approve nothing but + * the author usually believes they grant access. Returns array of entry strings. + */ +function findIneffectiveAllowGlobs(settings) { + if (!settings || typeof settings !== 'object') return []; + const perms = settings.permissions; + if (!perms || typeof perms !== 'object') return []; + const allowList = Array.isArray(perms.allow) ? perms.allow : []; + return allowList.filter(e => isIneffectiveAllowGlob(e)); +} + /** * Main scanner entry point. * @@ -70,28 +83,50 @@ export async function scan(targetPath, discovery) { if (!content) continue; const parsed = parseJson(content); if (!parsed) continue; - const overlaps = findDenyAllowOverlaps(parsed); - if (overlaps.length === 0) continue; - const evidence = overlaps.slice(0, 5) - .map(o => `${o.tool}: allow="${o.allowEntry}" + deny="${o.denyEntry}"`) - .join('; '); - findings.push(finding({ - scanner: SCANNER, - severity: SEVERITY.low, - title: 'Tool listed in both permissions.deny and permissions.allow', - description: - `${f.relPath || f.absPath} contains ${overlaps.length} tool` + - `${overlaps.length === 1 ? '' : 's'} present in both deny and allow lists. ` + - 'The deny list wins — the allow entries are dead config but still load on ' + - 'every turn and may confuse future readers about intent.', - file: f.absPath, - evidence, - recommendation: - 'Remove the redundant allow entries. If you actually want this tool enabled, ' + - 'remove it from the deny list instead. Settings should express intent clearly.', - category: 'permissions-hygiene', - })); + const overlaps = findDenyAllowOverlaps(parsed); + if (overlaps.length > 0) { + const evidence = overlaps.slice(0, 5) + .map(o => `${o.tool}: allow="${o.allowEntry}" + deny="${o.denyEntry}"`) + .join('; '); + findings.push(finding({ + scanner: SCANNER, + severity: SEVERITY.low, + title: 'Tool listed in both permissions.deny and permissions.allow', + description: + `${f.relPath || f.absPath} contains ${overlaps.length} tool` + + `${overlaps.length === 1 ? '' : 's'} present in both deny and allow lists. ` + + 'The deny list wins — the allow entries are dead config but still load on ' + + 'every turn and may confuse future readers about intent.', + file: f.absPath, + evidence, + recommendation: + 'Remove the redundant allow entries. If you actually want this tool enabled, ' + + 'remove it from the deny list instead. Settings should express intent clearly.', + category: 'permissions-hygiene', + })); + } + + const ineffective = findIneffectiveAllowGlobs(parsed); + if (ineffective.length > 0) { + const evidence = `allow: ${ineffective.slice(0, 5).map(e => `"${e}"`).join(', ')}`; + findings.push(finding({ + scanner: SCANNER, + severity: SEVERITY.low, + title: 'Ineffective allow wildcard — Claude Code ignores this rule', + description: + `${f.relPath || f.absPath} has ${ineffective.length} permissions.allow ` + + `entr${ineffective.length === 1 ? 'y' : 'ies'} that Claude Code skips: an ` + + 'unanchored tool-name wildcard auto-approves nothing. CC accepts allow ' + + 'wildcards only after a literal `mcp____` prefix.', + file: f.absPath, + evidence, + recommendation: + 'Replace `*`/`mcp__*` with explicit tool names, or anchor MCP wildcards to ' + + 'a server (`mcp____*`). As written these entries grant 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 773af69..5fa04ba 100644 --- a/scanners/lib/permission-rules.mjs +++ b/scanners/lib/permission-rules.mjs @@ -55,8 +55,12 @@ export function paramMatches(pattern, value) { /** * Does the deny entry fully cover the allow entry, making the allow dead config? - * Used by DIS. Bare deny covers everything; a specific deny only covers the - * matching (or wildcard-subsumed) param — and does NOT cover a bare allow. + * Used by DIS. Bare deny — and the equivalent `Tool(*)` deny-all glob — covers + * everything (including a bare allow); a specific deny only covers the matching + * (or wildcard-subsumed) param and does NOT kill a bare allow. + * + * CC: "`Bash(*)` is equivalent to `Bash` ... As a deny rule, both forms remove + * the tool from Claude's context." (code.claude.com/docs/en/permissions) * @param {string} denyEntry * @param {string} allowEntry * @returns {boolean} @@ -65,7 +69,7 @@ export function dominates(denyEntry, allowEntry) { const d = parseRule(denyEntry); const a = parseRule(allowEntry); if (!d.tool || !a.tool || d.tool !== a.tool) return false; - if (d.param === null) return true; // bare deny covers all params + if (d.param === null || d.param === '*') return true; // bare / Tool(*) deny covers all params if (a.param === null) return false; // specific deny does not kill a bare allow if (d.param === a.param) return true; return paramMatches(d.param, a.param); // wildcard deny covers a matching literal allow @@ -87,3 +91,35 @@ export function rulesIntersect(ruleA, ruleB) { if (a.param === b.param) return true; return paramMatches(a.param, b.param) || paramMatches(b.param, a.param); } + +/** + * Is this `permissions.allow` entry an UNANCHORED tool-name glob that Claude + * Code silently skips? CC accepts tool-name globs in an ALLOW rule only after a + * literal `mcp____` prefix (the server segment must be glob-free). + * Unanchored globs like `*`, `B*`, or `mcp__*` are skipped with a warning and + * auto-approve nothing — dead config the author believes is granting access. + * + * Tool-name globs apply to the bare-name form only (no `(...)` specifier); a + * glob INSIDE a specifier such as `Bash(npm run *)` is normal and valid. + * + * CC: "An unanchored allow glob such as `"*"`, `"B*"`, or `"mcp__*"` is skipped + * with a warning and does not auto-approve anything." + * (code.claude.com/docs/en/permissions — "Tool name wildcards") + * + * NOTE: deny/ask rules DO accept tool-name globs, so this predicate is for the + * allow list only. + * @param {string} entry + * @returns {boolean} + */ +export function isIneffectiveAllowGlob(entry) { + if (typeof entry !== 'string') return false; + if (entry.includes('(')) return false; // specifier form — glob lives inside the param + if (!entry.includes('*')) return false; // no glob — a normal match-all-tool allow + if (entry.startsWith('mcp__')) { + const rest = entry.slice(5); // after 'mcp__' + const sep = rest.indexOf('__'); + // mcp____ with a glob-free server segment is anchored & valid + if (sep > 0 && !rest.slice(0, sep).includes('*')) return false; + } + return true; +} diff --git a/tests/fixtures/deny-all-glob/.claude/settings.json b/tests/fixtures/deny-all-glob/.claude/settings.json new file mode 100644 index 0000000..caaaeda --- /dev/null +++ b/tests/fixtures/deny-all-glob/.claude/settings.json @@ -0,0 +1,6 @@ +{ + "permissions": { + "allow": ["Bash"], + "deny": ["Bash(*)"] + } +} diff --git a/tests/fixtures/ineffective-allow-globs/.claude/settings.json b/tests/fixtures/ineffective-allow-globs/.claude/settings.json new file mode 100644 index 0000000..c9d6738 --- /dev/null +++ b/tests/fixtures/ineffective-allow-globs/.claude/settings.json @@ -0,0 +1,5 @@ +{ + "permissions": { + "allow": ["mcp__*", "B*", "mcp__puppeteer__*", "Bash(npm run *)", "Read"] + } +} diff --git a/tests/lib/permission-rules.test.mjs b/tests/lib/permission-rules.test.mjs index aa139b8..083165f 100644 --- a/tests/lib/permission-rules.test.mjs +++ b/tests/lib/permission-rules.test.mjs @@ -5,6 +5,7 @@ import { paramMatches, dominates, rulesIntersect, + isIneffectiveAllowGlob, } from '../../scanners/lib/permission-rules.mjs'; describe('permission-rules — parseRule', () => { @@ -70,11 +71,44 @@ describe('permission-rules — dominates (deny fully covers allow → dead allow assert.equal(dominates('Agent(model:opus)', 'Agent'), false); }); + it('deny-all glob Tool(*) covers a bare allow (Bash(*) ≡ bare Bash deny)', () => { + // CC: "Bash(*) is equivalent to Bash ... As a deny rule, both forms remove + // the tool from Claude's context." So Bash(*) deny kills a bare Bash allow. + assert.equal(dominates('Bash(*)', 'Bash'), true); + assert.equal(dominates('Bash(*)', 'Bash(npm:*)'), true); + }); + it('different tools never dominate', () => { assert.equal(dominates('Bash', 'Read'), false); }); }); +describe('permission-rules — isIneffectiveAllowGlob (CC silently skips these allow entries)', () => { + it('unanchored tool-name globs are ineffective', () => { + assert.equal(isIneffectiveAllowGlob('*'), true); + assert.equal(isIneffectiveAllowGlob('B*'), true); + assert.equal(isIneffectiveAllowGlob('mcp__*'), true); + assert.equal(isIneffectiveAllowGlob('mcp__*__foo'), true); // glob in server segment + }); + + it('MCP globs anchored to a literal server are valid (not flagged)', () => { + assert.equal(isIneffectiveAllowGlob('mcp__puppeteer__*'), false); + assert.equal(isIneffectiveAllowGlob('mcp__github__get_*'), false); + }); + + it('non-glob and specifier forms are valid (not flagged)', () => { + assert.equal(isIneffectiveAllowGlob('Bash'), false); // legit match-all-tool allow + assert.equal(isIneffectiveAllowGlob('mcp__puppeteer'), false); // legit match-all-server + assert.equal(isIneffectiveAllowGlob('Bash(npm run *)'), false);// glob inside specifier is fine + assert.equal(isIneffectiveAllowGlob('Read(src/**)'), false); + }); + + it('non-string → false', () => { + assert.equal(isIneffectiveAllowGlob(null), false); + assert.equal(isIneffectiveAllowGlob(undefined), false); + }); +}); + 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 807e38a..e045d5a 100644 --- a/tests/scanners/disabled-in-schema.test.mjs +++ b/tests/scanners/disabled-in-schema.test.mjs @@ -95,3 +95,44 @@ describe('DIS scanner — param-qualified permissions are param-aware', () => { assert.doesNotMatch(String(f?.evidence || ''), /WebFetch/); }); }); + +describe('DIS scanner — ineffective allow wildcards (CC skips unanchored tool-name globs)', () => { + // settings.json allow: ["mcp__*", "B*", "mcp__puppeteer__*", "Bash(npm run *)", "Read"] + // CC skips `mcp__*` and `B*` (unanchored tool-name globs) — dead config. + // `mcp__puppeteer__*` (anchored), `Bash(npm run *)` (specifier glob) and `Read` are valid. + + it('flags the ineffective allow globs with low severity', async () => { + const result = await runScanner('ineffective-allow-globs'); + const f = result.findings.find(x => /ineffective allow wildcard/i.test(x.title || '')); + assert.ok(f, `expected ineffective-allow-glob finding; got: ${result.findings.map(x => x.title).join(' | ')}`); + assert.equal(f.severity, 'low', `expected low, got ${f.severity}`); + assert.match(f.id, /^CA-DIS-\d{3}$/); + }); + + it('evidence cites only the unanchored globs, not the valid entries', async () => { + const result = await runScanner('ineffective-allow-globs'); + const f = result.findings.find(x => /ineffective allow wildcard/i.test(x.title || '')); + assert.ok(f); + assert.match(String(f.evidence || ''), /mcp__\*/); + assert.match(String(f.evidence || ''), /B\*/); + assert.doesNotMatch(String(f.evidence || ''), /puppeteer/); // anchored MCP glob is valid + assert.doesNotMatch(String(f.evidence || ''), /npm run/); // specifier glob is valid + }); + + it('clean settings (no unanchored globs) → no ineffective-allow finding', async () => { + const result = await runScanner('healthy-project'); + const f = result.findings.find(x => /ineffective allow wildcard/i.test(x.title || '')); + assert.equal(f, undefined, `expected no ineffective-allow finding; 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. + it('flags the bare Bash allow as dead under a Bash(*) deny', async () => { + const result = await runScanner('deny-all-glob'); + const f = result.findings.find(x => /both permissions\.deny and permissions\.allow/i.test(x.title || '')); + assert.ok(f, `expected the bare Bash allow to be flagged dead; got: ${result.findings.map(x => x.title).join(' | ')}`); + assert.match(String(f.evidence || ''), /Bash/); + }); +});