BREAKING CHANGE: the {NNN} in CA-{SCANNER}-{NNN} identifies the check that
produced the finding. It used to be the finding's position in that scanner's
output for that run, which made it unstable across CONFIGURATIONS, not just
across releases as STATE framed it. Measured on two fixtures: "No custom
subagents" was CA-GAP-007 on minimal-project and CA-GAP-004 on healthy-project.
A user who fixed an unrelated earlier gap silently renumbered every later one,
so a .config-audit-ignore pin retargeted to a neighbouring finding with no
version change at all.
Second measured arm: README already documented the opposite scheme. It and the
scanner headers describe ~20 numbers as check codes (CA-SKL-003 = oversized
body, CA-PLH-015 = folder shadowing, CA-TOK-006 = schema deferral), and the
counter could only produce those in the all-fire case -- source-order positions
are 4, 3 and 8. The documentation described the scheme; the implementation was
what was wrong. Every published number is preserved by construction and pinned
exhaustively in tests/lib/finding-codes.test.mjs.
scanners/lib/finding-codes.mjs is the single authority. Every finding() call
passes a `code`; an undeclared or missing one THROWS. No counter fallback --
that would reproduce D1's findGapId -> 'unknown' silent degradation and let a
half-converted scanner ship IDs that look valid. findingCounter/resetCounter
are deleted outright, not left as no-ops. Retirement is now a mechanism:
RETIRED_CODES tombstones a withdrawn key so its number is never reissued,
seeded with GAP t3_8 -- the D1 removal that opened this chunk.
IDs are consequently NOT unique per finding: one check failing in three files
emits three findings sharing an ID. That inverts which consumer is correct, so
every f.id/findingId site was classified before the change. diff-engine and
most of fix-engine already keyed on scanner+title+file (drift was never lying);
fix-engine's verification did not, and keyed on the ID alone -- fixing one of
two sibling instances marked both fixed, and the untouched one, still present
in the re-scan, was reported as a REGRESSION. Red test first, then keyed on
(findingId, file), which both planFixes and applyFixes already carry.
plugin-health's crossIds Set was measured and is a clean negative: cross
findings are allFindings.slice(crossPluginStart) and codes 18/19 are emitted
only in that tail, so the partition holds by construction.
unknownSuppressions() reports a pin that names no declared check, in the
--output-file payload (ux-rules rule 2 -- a stderr-only warning is invisible to
the commands) and only when one exists, so a clean config is byte-identical.
That is what makes the break safe: a stale pin goes loud instead of dying quiet.
Frozen tests/snapshots/v5.0.0/ untouched on disk. IDs are masked out of that
comparison (mask-finding-ids.mjs) rather than re-derived -- re-deriving
positional IDs would assert the retired scheme against itself, and #58's
isGapEntry off-by-one is the measured example of that misfiring. The dead
re-derivation is removed from strip-retired-gap.mjs. default-output snapshots
re-approved after confirming the diff is IDs and nothing else.
Guards, each seen red against its own defect: a missing code (scanner errors
out mid-sweep), an orphan declaration, a resurrected retired key, and a
documented ID naming no check. The sweep asserts the union across all 16
scanners, never per scanner -- a per-scanner assertion goes green on a partial
conversion.
Fasit written before implementation: docs/mbug28-id-semantics-fasit.local.md,
including one correction made before running (CML has 12 checks over 13 call
sites -- the anchored and calibrated char-budget arms are one check, which a
repeated-title sweep found and my call-site count had missed).
Suite 1535 -> 1573, 0 failing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01MyqCQKK2ornJ1jFWwqx17E
210 lines
8.6 KiB
JavaScript
210 lines
8.6 KiB
JavaScript
/**
|
|
* DIS Scanner — Disabled-Tools-Still-In-Schema Detector (v5 N4)
|
|
*
|
|
* Detects tools that appear in BOTH `permissions.deny` and `permissions.allow`
|
|
* within the same settings.json file. The deny list wins, so the allow entry
|
|
* is dead config — but it still loads on every turn and signals confused
|
|
* intent. Often arises from copy-paste edits where one list was updated and
|
|
* the other was forgotten.
|
|
*
|
|
* Compares rule identity param-aware (CC 2.1.178 `Tool(param:value)`,
|
|
* 2.1.172 `domain:` rules). An allow entry is dead only when some deny entry
|
|
* fully COVERS it: a bare `Bash` deny blocks all `Bash(...)` allows, but
|
|
* `Agent(model:opus)` deny does NOT kill an `Agent(model:sonnet)` allow.
|
|
* Coverage logic lives in `lib/permission-rules.mjs` (shared with CNF).
|
|
*
|
|
* Finding ID: CA-DIS-NNN. Severity: low.
|
|
*
|
|
* Zero external dependencies.
|
|
*/
|
|
|
|
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, forbiddenParamRule } from './lib/permission-rules.mjs';
|
|
|
|
const SCANNER = 'DIS';
|
|
|
|
/**
|
|
* Find allow entries that are dead config because some deny entry fully covers
|
|
* them. Returns array of { tool, allowEntry, denyEntry }.
|
|
*/
|
|
function findDenyAllowOverlaps(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 : [];
|
|
const denyList = Array.isArray(perms.deny) ? perms.deny : [];
|
|
if (allowList.length === 0 || denyList.length === 0) return [];
|
|
|
|
const overlaps = [];
|
|
const seen = new Set();
|
|
for (const a of allowList) {
|
|
if (typeof a !== 'string' || seen.has(a)) continue;
|
|
const dominator = denyList.find(d => dominates(d, a));
|
|
if (dominator) {
|
|
overlaps.push({ tool: parseRule(a).tool, allowEntry: a, denyEntry: dominator });
|
|
seen.add(a);
|
|
}
|
|
}
|
|
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));
|
|
}
|
|
|
|
/**
|
|
* 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.
|
|
*
|
|
* @param {string} targetPath
|
|
* @param {{files: Array<{absPath:string, relPath:string, type:string}>}} discovery
|
|
*/
|
|
export async function scan(targetPath, discovery) {
|
|
const start = Date.now();
|
|
const findings = [];
|
|
let filesScanned = 0;
|
|
|
|
for (const f of discovery.files) {
|
|
if (f.type !== 'settings-json') continue;
|
|
filesScanned++;
|
|
const content = await readTextFile(f.absPath);
|
|
if (!content) continue;
|
|
const parsed = parseJson(content);
|
|
if (!parsed) continue;
|
|
|
|
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,
|
|
code: 'deny-and-allow',
|
|
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,
|
|
code: 'ineffective-allow-wildcard',
|
|
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__<server>__` prefix.',
|
|
file: f.absPath,
|
|
evidence,
|
|
recommendation:
|
|
'Replace `*`/`mcp__*` with explicit tool names, or anchor MCP wildcards to ' +
|
|
'a server (`mcp__<server>__*`). As written these entries grant nothing.',
|
|
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,
|
|
code: 'forbidden-param-deny',
|
|
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,
|
|
code: 'forbidden-param-allow',
|
|
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);
|
|
}
|