313 lines
16 KiB
Markdown
313 lines
16 KiB
Markdown
---
|
|
title: Classification bugs in claude-permissions-optimizer extract-commands script
|
|
category: logic-errors
|
|
date: 2026-03-18
|
|
severity: high
|
|
tags: [security, classification, normalization, permissions, command-extraction, destructive-commands, dcg]
|
|
component: claude-permissions-optimizer
|
|
symptoms:
|
|
- Dangerous commands (find -delete, git push -f) recommended as safe to auto-allow
|
|
- Safe/common commands (git blame, gh CLI) invisible or misclassified in output
|
|
- 632 commands reported as below-threshold noise due to filtering before normalization
|
|
- git restore -S (safe unstage) incorrectly classified as red (destructive)
|
|
---
|
|
|
|
# Classification Bugs in claude-permissions-optimizer
|
|
|
|
## Problem
|
|
|
|
The `extract-commands.mjs` script in the claude-permissions-optimizer skill had three categories of bugs that affected both security and UX of permission recommendations.
|
|
|
|
**Symptoms observed:** Running the skill across 200 sessions reported 632 commands as "below threshold noise" -- suspiciously high. Cross-referencing against the Destructive Command Guard (DCG) project confirmed classification gaps on both spectrums.
|
|
|
|
## Root Cause
|
|
|
|
### 1. Threshold before normalization (architectural ordering)
|
|
|
|
The min-count filter was applied to each raw command **before** normalization and grouping. Hundreds of variants of the same logical command (e.g., `git log --oneline src/foo.ts`, `git log --oneline src/bar.ts`) were each discarded individually for falling below the threshold of 5, even though their normalized form (`git log *`) had 200+ total uses.
|
|
|
|
### 2. Normalization broadens classification
|
|
|
|
Safety classification happened on the **raw** command, but the result was carried forward to the **normalized** pattern. `node --version` (green via `--version$` regex) would normalize to the dangerously broad `node *`, inheriting the green classification despite `node` being a yellow-tier base command.
|
|
|
|
### 3. Compound command classification leak
|
|
|
|
Classify ran on the full raw command string, but normalize only used the first command in a compound chain. So `cd /dir && git branch -D feature` was classified as RED (from the `git branch -D` part) but normalized to `cd *`. The red classification from the second command leaked into the first command's pattern, causing `cd *` to appear in the blocked list.
|
|
|
|
### 4. Global risk flags causing false fragmentation
|
|
|
|
Risk flags (`-f`, `-v`) were preserved globally during normalization to keep dangerous variants separate. But `-f` means "force" in `git push -f` and "pattern file" in `grep -f`, while `-v` means "remove volumes" in `docker-compose down -v` and "verbose/invert" everywhere else. Global preservation fragmented green patterns unnecessarily (`grep -v *` separate from `grep *`) and contaminated benign patterns with wrong risk reasons.
|
|
|
|
### 5. Allowlist glob broader than classification intent
|
|
|
|
Commands with mode-switching flags (`sed -i`, `find -delete`, `ast-grep --rewrite`) were classified green without the flag but normalized to a broad pattern like `sed *`. The resulting allowlist rule `Bash(sed *)` would auto-allow the destructive form too, since Claude Code's glob matching treats `*` as matching everything. The classification was correct for the individual command but the recommended pattern was unsafe.
|
|
|
|
### 6. Classification gaps (found via DCG cross-reference)
|
|
|
|
**Security bugs (dangerous classified as green):**
|
|
- `find` unconditionally in `GREEN_BASES` -- `find -delete` and `find -exec rm` passed as safe
|
|
- `git push -f` regex required `-f` after other args, missed `-f` immediately after `push`
|
|
- `git restore -S` falsely red (lookahead only checked `--staged`, not the `-S` alias)
|
|
- `git clean -fd` regex required `f` at end of flag group, missed `-fd` (f then d)
|
|
- `git checkout HEAD -- file` pattern didn't allow a ref between `checkout` and `--`
|
|
- `git branch --force` not caught alongside `-D`
|
|
- Missing RED patterns: `npm unpublish`, `cargo yank`, `dd of=`, `mkfs`, `pip uninstall`, `apt remove/purge`, `brew uninstall`, `git reset --merge`
|
|
|
|
**UX bugs (safe commands misclassified):**
|
|
- `git blame`, `git shortlog` -> unknown (missing from GREEN_COMPOUND)
|
|
- `git tag -l`, `git stash list/show` -> yellow instead of green
|
|
- `git clone` -> unknown (not in any YELLOW pattern)
|
|
- All `gh` CLI commands -> unknown (no patterns at all)
|
|
- `git restore --staged/-S` -> red instead of yellow
|
|
|
|
## Solution
|
|
|
|
### Fix 1: Reorder the pipeline
|
|
|
|
Normalize and group commands first, then apply the min-count threshold to the grouped totals:
|
|
|
|
```javascript
|
|
// Group ALL non-allowed commands by normalized pattern first
|
|
for (const [command, data] of commands) {
|
|
if (isAllowed(command)) { alreadyCovered++; continue; }
|
|
const pattern = "Bash(" + normalize(command) + ")";
|
|
// ... group by pattern, merge sessions, escalate tiers
|
|
}
|
|
|
|
// THEN filter by min-count on GROUPED totals
|
|
for (const [pattern, data] of patternGroups) {
|
|
if (data.totalCount < minCount) {
|
|
belowThreshold += data.rawCommands.length;
|
|
patternGroups.delete(pattern);
|
|
}
|
|
}
|
|
```
|
|
|
|
### Fix 2: Post-grouping safety reclassification
|
|
|
|
After grouping, re-classify the normalized pattern itself. If the broader form maps to a more restrictive tier, escalate:
|
|
|
|
```javascript
|
|
for (const [pattern, data] of patternGroups) {
|
|
if (data.tier !== "green") continue;
|
|
if (!pattern.includes("*")) continue;
|
|
const cmd = pattern.replace(/^Bash\(|\)$/g, "");
|
|
const { tier, reason } = classify(cmd);
|
|
if (tier === "red") { data.tier = "red"; data.reason = reason; }
|
|
else if (tier === "yellow") { data.tier = "yellow"; }
|
|
else if (tier === "unknown") { data.tier = "unknown"; }
|
|
}
|
|
```
|
|
|
|
### Fix 3: Classify must match normalize's scope
|
|
|
|
Classify now extracts the first command from compound chains (`&&`, `||`, `;`) and pipe chains before checking patterns, matching what normalize does. Pipe-to-shell (`| bash`) is excluded from stripping since the pipe itself is the danger.
|
|
|
|
```javascript
|
|
function classify(command) {
|
|
const compoundMatch = command.match(/^(.+?)\s*(&&|\|\||;)\s*(.+)$/);
|
|
if (compoundMatch) return classify(compoundMatch[1].trim());
|
|
const pipeMatch = command.match(/^(.+?)\s*\|\s*(.+)$/);
|
|
if (pipeMatch && !/\|\s*(sh|bash|zsh)\b/.test(command)) {
|
|
return classify(pipeMatch[1].trim());
|
|
}
|
|
// ... RED/GREEN/YELLOW checks on the first command only
|
|
}
|
|
```
|
|
|
|
### Fix 4: Context-specific risk flags
|
|
|
|
Replaced global `-f`/`-v` risk flags with a contextual system. Flags are only preserved during normalization when they're risky for the specific base command:
|
|
|
|
```javascript
|
|
const CONTEXTUAL_RISK_FLAGS = {
|
|
"-f": new Set(["git", "docker", "rm"]),
|
|
"-v": new Set(["docker", "docker-compose"]),
|
|
};
|
|
|
|
function isRiskFlag(token, base) {
|
|
if (GLOBAL_RISK_FLAGS.has(token)) return true;
|
|
const contexts = CONTEXTUAL_RISK_FLAGS[token];
|
|
if (contexts && base && contexts.has(base)) return true;
|
|
// ...
|
|
}
|
|
```
|
|
|
|
Risk flags are a **presentation improvement**, not a safety mechanism. Classification + tier escalation handles safety regardless. The contextual approach prevents fragmentation of green patterns (`grep -v *` merges with `grep *`) while keeping dangerous variants visible in the blocked table (`git push -f *` stays separate from `git push *`).
|
|
|
|
Commands with mode-switching flags (`sed -i`, `ast-grep --rewrite`) are handled via dedicated normalization rules rather than risk flags, since their safe and dangerous forms need entirely different classification.
|
|
|
|
### Fix 5: Mode-preserving normalization
|
|
|
|
Commands with mode-switching flags get dedicated normalization rules that preserve the safe/dangerous mode flag, producing narrow patterns safe to recommend:
|
|
|
|
```javascript
|
|
// sed: preserve the mode flag
|
|
if (/^sed\s/.test(command)) {
|
|
if (/\s-i\b/.test(command)) return "sed -i *";
|
|
const sedFlag = command.match(/^sed\s+(-[a-zA-Z])\s/);
|
|
return sedFlag ? "sed " + sedFlag[1] + " *" : "sed *";
|
|
}
|
|
|
|
// find: preserve the predicate/action flag
|
|
if (/^find\s/.test(command)) {
|
|
if (/\s-delete\b/.test(command)) return "find -delete *";
|
|
if (/\s-exec\s/.test(command)) return "find -exec *";
|
|
const findFlag = command.match(/\s(-(?:name|type|path|iname))\s/);
|
|
return findFlag ? "find " + findFlag[1] + " *" : "find *";
|
|
}
|
|
```
|
|
|
|
GREEN_COMPOUND then matches the narrow normalized forms:
|
|
|
|
```javascript
|
|
/^sed\s+-(?!i\b)[a-zA-Z]\s/ // sed -n *, sed -e * (not sed -i *)
|
|
/^find\s+-(?:name|type|path|iname)\s/ // find -name *, find -type *
|
|
/^(ast-grep|sg)\b(?!.*--rewrite)/ // ast-grep * (not ast-grep --rewrite *)
|
|
```
|
|
|
|
Bare forms without a mode flag (`sed *`, `find *`) fall to yellow/unknown since `Bash(sed *)` would match the destructive variant.
|
|
|
|
### Fix 6: Patch classification gaps
|
|
|
|
Key regex fixes:
|
|
|
|
```javascript
|
|
// find: removed from GREEN_BASES; destructive forms caught by RED
|
|
{ test: /\bfind\b.*\s-delete\b/, reason: "find -delete permanently removes files" },
|
|
{ test: /\bfind\b.*\s-exec\s+rm\b/, reason: "find -exec rm permanently removes files" },
|
|
// Safe find via GREEN_COMPOUND:
|
|
/^find\b(?!.*(-delete|-exec))/
|
|
|
|
// git push -f: catch -f in any position
|
|
{ test: /git\s+(?:\S+\s+)*push\s+.*-f\b/ },
|
|
{ test: /git\s+(?:\S+\s+)*push\s+-f\b/ },
|
|
|
|
// git restore: exclude both --staged and -S from red
|
|
{ test: /git\s+restore\s+(?!.*(-S\b|--staged\b))/ },
|
|
// And add yellow pattern for the safe form:
|
|
/^git\s+restore\s+.*(-S\b|--staged\b)/
|
|
|
|
// git clean: match f anywhere in combined flags
|
|
{ test: /git\s+clean\s+.*(-[a-z]*f[a-z]*\b|--force\b)/ },
|
|
|
|
// git branch: catch both -D and --force
|
|
{ test: /git\s+branch\s+.*(-D\b|--force\b)/ },
|
|
```
|
|
|
|
New GREEN_COMPOUND patterns for safe commands:
|
|
|
|
```javascript
|
|
/^git\s+(status|log|diff|show|blame|shortlog|...)\b/ // added blame, shortlog
|
|
/^git\s+tag\s+(-l\b|--list\b)/ // tag listing
|
|
/^git\s+stash\s+(list|show)\b/ // stash read-only
|
|
/^gh\s+(pr|issue|run)\s+(view|list|status|diff|checks)\b/ // gh read-only
|
|
/^gh\s+repo\s+(view|list|clone)\b/
|
|
/^gh\s+api\b/
|
|
```
|
|
|
|
New YELLOW_COMPOUND patterns:
|
|
|
|
```javascript
|
|
/^git\s+(...|clone)\b/ // added clone
|
|
/^gh\s+(pr|issue)\s+(create|edit|comment|close|reopen|merge)\b/ // gh write ops
|
|
```
|
|
|
|
## Verification
|
|
|
|
- Built a test suite of 70+ commands across both spectrums (dangerous and safe)
|
|
- Cross-referenced against DCG rule packs: core/git, core/filesystem, package_managers
|
|
- Final result: 0 dangerous commands classified as green, 0 safe commands misclassified
|
|
- Repo test suite: 344 tests pass
|
|
|
|
## Prevention Strategies
|
|
|
|
### Pipeline ordering is an architectural invariant
|
|
|
|
The correct pipeline order is:
|
|
|
|
```
|
|
filter(allowlist) -> normalize -> group -> threshold -> re-classify(normalized) -> output
|
|
```
|
|
|
|
The post-grouping safety check that re-classifies normalized patterns containing wildcards is load-bearing. It must never be removed or moved before the grouping step.
|
|
|
|
### The allowlist pattern is the product, not the classification
|
|
|
|
The skill's output is an allowlist glob like `Bash(sed *)`, not a safety tier. Classification determines whether to recommend a pattern, but the pattern itself must be safe to auto-allow. This creates a critical constraint: **commands with mode-switching flags that change safety profile need normalization that preserves the safe mode flag**, so the resulting glob can't match the destructive form.
|
|
|
|
Example: `sed -n 's/foo/bar/' file` is read-only and safe. But normalizing it to `sed *` produces `Bash(sed *)` which also matches `sed -i 's/foo/bar/' file` (destructive in-place edit). The fix is mode-preserving normalization: `sed -n *` produces `Bash(sed -n *)` which is narrow enough to be safe.
|
|
|
|
This applies to any command where a flag changes the safety profile:
|
|
- `sed -n *` (green) vs `sed -i *` (red) -- `-n` is read-only, `-i` edits in place
|
|
- `find -name *` (green) vs `find -delete *` (red) -- `-name` is a predicate, `-delete` removes files
|
|
- `ast-grep *` (green) vs `ast-grep --rewrite *` (red) -- default is search, `--rewrite` modifies files
|
|
|
|
Commands like these should NOT go in `GREEN_BASES` (which produces the blanket `X *` pattern). They need dedicated normalization rules that preserve the mode flag, and `GREEN_COMPOUND` patterns that match the narrower normalized form.
|
|
|
|
### GREEN_BASES requires proof of no destructive subcommands
|
|
|
|
Before adding any command to `GREEN_BASES`, verify it has NO destructive flags or modes. If in doubt, use `GREEN_COMPOUND` with explicit negative lookaheads. Commands that should never be in `GREEN_BASES`: `find`, `xargs`, `sed`, `awk`, `curl`, `wget`.
|
|
|
|
### Regex negative lookaheads must enumerate ALL flag aliases
|
|
|
|
Every flag exclusion must cover both long and short forms. For git, consult `git <subcmd> --help` for every alias. Example: `(?!.*(-S\b|--staged\b))` not just `(?!.*--staged\b)`.
|
|
|
|
### Classify and normalize must operate on the same scope
|
|
|
|
If normalize extracts the first command from compound chains, classify must do the same. Otherwise a dangerous second command (`git branch -D`) contaminates the first command's pattern (`cd *`). Any future change to normalize's scoping logic must be mirrored in classify.
|
|
|
|
### Risk flags are contextual, not global
|
|
|
|
Short flags like `-f` and `-v` mean different things for different commands. Adding a short flag to `GLOBAL_RISK_FLAGS` will fragment every green command that uses it innocently. Use `CONTEXTUAL_RISK_FLAGS` with explicit base-command sets instead. For commands where a flag completely changes the safety profile (`sed -i`, `ast-grep --rewrite`), use a dedicated normalization rule rather than a risk flag.
|
|
|
|
### GREEN_BASES must exclude commands useless as allowlist rules
|
|
|
|
Commands like `cd` and `cal` are technically safe but useless as standalone allowlist rules in agent contexts (shell state doesn't persist, novelty commands never used). Including them creates noise in recommendations. Before adding to GREEN_BASES, ask: would a user actually benefit from `Bash(X *)` in their allowlist?
|
|
|
|
### RISK_FLAGS must stay synchronized with RED_PATTERNS
|
|
|
|
Every flag in a `RED_PATTERNS` regex must have a corresponding entry in `GLOBAL_RISK_FLAGS` or `CONTEXTUAL_RISK_FLAGS` so normalization preserves it.
|
|
|
|
## External References
|
|
|
|
### Destructive Command Guard (DCG)
|
|
|
|
**Repository:** https://github.com/Dicklesworthstone/destructive_command_guard
|
|
|
|
DCG is a Rust-based security hook with 49+ modular security packs that classify destructive commands. Its pack-based architecture maps well to the classifier's rule sections:
|
|
|
|
| DCG Pack | Classifier Section |
|
|
|---|---|
|
|
| `core/filesystem` | RED_PATTERNS (rm, find -delete, chmod, chown) |
|
|
| `core/git` | RED_PATTERNS (force push, reset --hard, clean -f, filter-branch) |
|
|
| `strict_git` | Additional git patterns (rebase, amend, worktree remove) |
|
|
| `package_managers` | RED_PATTERNS (publish, unpublish, uninstall) |
|
|
| `system` | RED_PATTERNS (sudo, reboot, kill -9, dd, mkfs) |
|
|
| `containers` | RED_PATTERNS (--privileged, system prune, volume rm) |
|
|
|
|
DCG's rule packs are a goldmine for validating classifier completeness. When adding new command categories or modifying rules, cross-reference the corresponding DCG pack. Key packs not yet fully cross-referenced: `database`, `kubernetes`, `cloud`, `infrastructure`, `secrets`.
|
|
|
|
DCG also demonstrates smart detection patterns worth studying:
|
|
- Scans heredocs and inline scripts (`python -c`, `bash -c`)
|
|
- Context-aware (won't block `grep "rm -rf"` in string literals)
|
|
- Explicit safe-listing of temp directory operations (`rm -rf /tmp/*`)
|
|
|
|
## Related Documentation
|
|
|
|
- [Script-first skill architecture](./script-first-skill-architecture.md) -- documents the architectural pattern used by this skill; the classification bugs highlight edge cases in the script-first approach
|
|
- [Compound refresh skill improvements](./compound-refresh-skill-improvements.md) -- related skill maintenance patterns
|
|
|
|
## Testing Recommendations
|
|
|
|
Future work should add a dedicated classification test suite covering:
|
|
|
|
1. **Red boundary tests:** Every RED_PATTERNS entry with positive match AND safe variant
|
|
2. **Green boundary tests:** Every GREEN_BASES/COMPOUND with destructive flag variants
|
|
3. **Normalization safety tests:** Verify that `classify(normalize(cmd))` never returns a lower tier than `classify(cmd)`
|
|
4. **DCG cross-reference tests:** Data-driven test with one entry per DCG pack rule, asserting never-green
|
|
5. **Broadening audit:** For each green rule, generate variants with destructive flags and assert they are NOT green
|
|
6. **Compound command tests:** Verify that `cd /dir && git branch -D feat` classifies as green (cd), not red
|
|
7. **Contextual flag tests:** Verify `grep -v pattern` normalizes to `grep *` (not `grep -v *`), while `docker-compose down -v` preserves `-v`
|
|
8. **Allowlist safety tests:** For every green pattern containing `*`, verify that the glob cannot match a known destructive variant (e.g., `Bash(sed -n *)` must not match `sed -i`)
|