Allowlist guard: four auto-approved write/disclosure paths pass validate-settings.py

Follow-up to #198 (deep review, post-merge). Four auto-approved capabilities in the team allowlist contradict the invariants standards/validate-settings.py advertises, and the validator passes all of them with exit 0. Every claim below was verified by execution (git 2.53.0, gh 2.91.0, the validator against crafted settings files, live MCP tool schemas) and independently re-verified by a second model before filing.

None of this argues against #198's direction — the evidence-driven derivation and the fail-closed MCP allowlist are the right design. These are gaps in the guard, not a case for reverting the allowlist.

---

1. Eight allowlisted git commands can write and overwrite an arbitrary file — no shell redirect

standards/settings.json lines 78 (git log), 79 (git diff), 80 (git show), 82 (git rev-list), 88 (git shortlog), 89 (git blame), 92 (git diff-tree), 93 (git diff-index) all accept --output=<file>.

Verified, writing over a pre-existing 32-byte file each time:

git log        --output=v.txt : rc=0  before=32b after=233b  "commit b00dba6f..."
git show       --output=v.txt : rc=0  before=32b after=218b  "commit b00dba6f..."
git diff       --output=v.txt : rc=0  before=32b after=101b  "diff --git a/a.txt b/a.txt"
git diff-tree  --output=v.txt : rc=0  before=32b after=146b
git diff-index --output=v.txt : rc=0  before=32b after=105b
git shortlog   --output=v.txt : rc=0  before=32b after=18b
git rev-list   --output=v.txt : rc=0  before=32b after=0b    <- truncated
git blame      --output=v.txt : rc=0  before=32b after=0b    <- truncated

Original contents gone in all eight. Empty output truncates to zero bytes. The path is unrestricted — git diff --output=../ESCAPED.txt wrote outside the repo, so "the allowlisted git reads are repo-scoped" does not hold in the write direction.

The repo's own matcher agrees these are covered:

bash_covered("git diff --output=/c/repos/claude/README.md", prefixes)  # True
bash_covered("git log > victim.txt", prefixes)                         # False  <- redirect correctly excluded

So the redirect case #198 carefully documents is handled, and the non-redirect case that does the same damage is not. validate-settings.py:8-9 names exactly this shape as its reason to exist — "ones that write without a redirect: sort -o, uniq in out, git worktree add -B" — and py standards/validate-settings.py returns OK: 95 allow entries checked (47 Bash, 46 MCP), exit 0.

The other 12 git entries correctly do not support it (status, rev-parse, ls-files, ls-tree, cat-file, merge-base, for-each-ref, describe, name-rev fail with error: unknown option 'output=').

This is a capability, not an incident — nothing exploits it and an agent must deliberately type --output=. It is the concrete instance of the broader "WRITE_CAPABLE is incomplete" gap (item 5), and one fix closes both.

2. The validator silently skips whatever it cannot parse, including defaultMode: bypassPermissions

standards/validate-settings.py:216-227. Line 223-224 treats a None parse as "not a Bash rule" and continues.

{"permissions":{
  "defaultMode": "bypassPermissions",
  "allow": [
    "Bash(sort -o /etc/passwd",
    "Write(**)", "Read(//**)",
    "WebFetch(domain:evil.example.com)",
    "Bash(git log:*)"
]}}

OK: 6 allow entries checked (2 Bash, 0 MCP) - none match a known-dangerous pattern.
EXIT=0

Three distinct problems:

2a. The summary miscounts and reassures about entries it never checked. n_bash (line 235) uses startswith("Bash("); the check uses parse_bash_prefix's regex ^Bash\((.*?)(?::\*| \*)?\)$. It reported "2 Bash" when only one entry reached check_bash. The skipped entry contained sort -o — already in WRITE_CAPABLE — and passed anyway. A well-formed Bash(rm -rf /) is caught, so the hole is confined to unparseable Bash entries and non-Bash rule types.

2b. permissions.defaultMode: "bypassPermissions" passes. This disables all permission prompts. Traced through the jq merge at setup-env.sh:196-210: team file is jq input $a, user file is $b; line 207's else $b means the user wins only when the key exists on both sides, and else $a[$k] carries a team-only key straight through. So it propagates to any developer who has not already set defaultMode themselves. Verified by transliterating the jq program (jq is not installed on this machine) and corroborated against the Python reimplementation now on main (scripts/merge_settings.py, same documented semantics). One line in a file synced to every developer would undo every safety property #198 argues for, past a green check named "Validate allowlist has no dangerous entries".

2c. Write(**), Read(//**), WebFetch(...) are invisible to the guard. Write-anywhere, read-anywhere, arbitrary outbound fetch. Line 224's comment calls these "out of scope here" — defensible for a v1, but nothing in the docstring, the workflow step name, or standards/CLAUDE.md says the scope is that narrow, and the OK: line reads as a clean bill of health.

3. wit_get_work_item_attachment writes local files and is in the "verified read-only" MCP set

standards/settings.json:41 and validate-settings.py:125 (KNOWN_SAFE_MCP). Live schema:

savePath — "Optional local directory path where the file should be saved. Must be a relative path (e.g. 'temp' or 'downloads/attachments'); absolute paths and path traversals are not allowed. If provided, saves the attachment to this directory and returns the file path."

Tool description: "If savePath is provided, saves file locally to that directory." The filename is caller-chosen too (fileName: "Used to determine the MIME type or the saved file's name"), so savePath: "src" + fileName: "Program.cs" writes into the working tree. Content originates from an ADO attachment, which any project member can upload.

This contradicts an invariant #198 deliberately made fail-closed. validate-settings.py:115-118: "MCP tools reviewed and verified read-only. An mcp__ allow entry MUST be in this set (fail-closed). Adding a name here is a deliberate security decision: confirm the tool cannot mutate state before adding it."

The MUTATING_MCP substring backstop (validate-settings.py:109-113) cannot catch it — the name matches none of _update/_create/_add_/_delete/_remove/… — a fair illustration of why name-shaped heuristics can't substitute for reading the schema.

Scope, stated honestly: it is the only one of the 46 allowlisted MCP tools with a local-write parameter. I checked the schemas of the other write-plausible candidates (pipelines_list_artifacts, pipelines_get_build_log, pipelines_get_build_log_by_id, repo_get_file_content, repo_get_pull_request_changes, wit_query_by_wiql, execute_nrql_query) — all pure reads, no path parameters. Blast radius is bounded: the server rejects absolute paths and traversal, so writes land under the working directory.

4. Bash(gh auth status:*) pre-approves printing the live OAuth token

standards/settings.json:21. From gh auth status --help (gh 2.91.0) — flag confirmed, not run:

-t, --show-token        Display the auth token
EXAMPLES:  # Display tokens in plain text
           $ gh auth status --show-token

The prefix covers it: bash_covered("gh auth status --show-token", prefixes) and bash_covered("gh auth status -t", prefixes) both return True — -t is just an argument to an approved prefix.

Deliberately narrow framing: bare gh auth status redacts the token and no ordinary task motivates -t, so this is a pre-approved capability, not a live leak. What makes it worth changing is that the disclosure would land in context and in the plaintext transcript at ~/.claude/projects/*.jsonl with no checkpoint, and #198's new "Never move sensitive data across the read→network boundary" rule cannot help — the disclosure precedes any network step. It also sits awkwardly against the Secret Handling standard in the same standards/CLAUDE.md.

Not claiming scope creep here: gh auth status is absent from #183's candidate list, but so are roughly 70 of the 95 entries (including all 20 git and all 46 MCP entries). The list was a starting point; the token flag is the objection.

---

Also open (lower severity, no separate issue yet)

5. WRITE_CAPABLE has large holes, several one token from shipped entries. A settings file allowing Bash(git difftool:*), Bash(gh auth:*), Bash(az pipelines:*), Bash(sed:*), Bash(find:*), Bash(awk:*), Bash(git format-patch:*), Bash(git cherry-pick:*), Bash(git fetch:*), Bash(pwsh:*) passes 10/10, exit 0. git difftool --extcmd=<cmd> is arbitrary execution with no config needed; az pipelines run queues a build (and az pipelines show is already allowed); gh auth login/logout/token (and gh auth status is already allowed).
6. Non-token-aligned prefixes slip past — validate-settings.py:178 uses k.startswith(cmd + " "), so Bash(git c:*) and Bash(gh pr m:*) pass. Dropping + " " closes it; checked against all 47 current entries, none regress. Note #198's own description defers the matcher's "token-boundary behavior for short prefixes" as unchecked, so the guard's soundness currently rests on the assumption it admits it hasn't verified.
7. Legit read forms are false-rejected — git config --get-all and --get-regexp, both named as reads in standards/CLAUDE.md:366, fail with -> the write form (git config key value) mutates config. BASH_READONLY_EXCEPTIONS:104 covers only --get and --list.
8. Arbitrary reads outside the repo remain reachable. #198 acknowledges only git diff --no-index, but git config --list --file <any path> and git blame --contents <any path> -- <tracked> both read outside the working tree (verified). The stated rationale for excluding bare grep/cat/head/tail therefore doesn't hold for the entries kept.
9. No revocation path. setup-env.sh:206 merges arrays with ($a + $b) | unique — additive only. Removing an entry from standards/settings.json never removes it from a developer who already synced. Verified still true after #204 (scripts/merge_settings.py keeps the same union semantics). 93 pre-approvals with no way to withdraw one.
10. The 40 ADO MCP entries hardcode the server alias azure-devops, which is per-developer local MCP config. Nothing in the repo pins or documents it — no .mcp.json, no mcpServers key anywhere. A developer whose ADO server registers under a different alias gets no benefit from any of them, silently, and the ~1,350-call figure in #198 doesn't generalize. Fails safe (you get a prompt — the pre-PR status quo), and the same alias is already hardcoded in plugins/start-work/commands/start-work.md:5 and plugins/improve-stories/commands/improve-stories.md:4, so this is a documentation gap rather than a regression.
11. No test covers either sync path, which is why the #201 corruption reached a live machine. (#198 does ship 136 lines of tests wired into CI — the gap is specifically the sync.)
12. standards/CLAUDE.md:188 asserts the allowlist "auto-approves only read-only, non-mutating commands" — a guarantee validate-settings.py:19-21 explicitly declines to make ("not 'provably safe'"). The candid version lives only in a script no agent reads. The redirect bullet at :179 is also self-contradictory: redirection "forces a permission prompt" and "a stray cmd > path can silently overwrite a real file" cannot both be true.
13. measure-allowlist-impact.py — 142 lines of logic, zero tests, and the workflow triggers on changes to it but never runs it. --projects and --since are unvalidated, so a wrong path or --since yesterday yields a silent all-zeros report. load_rules has no error handling, unlike validate-settings.py:207-211.
14. Workflow — .github/workflows/validate-settings.yml has no permissions: block (inherits default GITHUB_TOKEN scope), pins actions by mutable tag, and its paths filter will stall PRs permanently pending if the check is ever made required.
15. python3 is unavailable on a stock Windows box (Microsoft Store stub) — standards/test-validate-settings.sh:6 defaults to it; only py works. The PYTHON override is undocumented. The repo already documents an interpreter-resolution cascade in CLAUDE.md:67-81.

Explicitly not in scope

Two findings from the review were withdrawn after verification. Both reproduce exactly — the setup-env.ps1 merge corruption was replicated verbatim, producing the single null-valued garbage key from #201 — but #202 and #204 already fixed them, merged 38 and 101 minutes after #198, and neither file is in #198's diff. standards/settings.json also had a single top-level key before #198 (#183 quotes the pre-PR file), so #198 neither introduced nor could have fixed it. Recorded here only so they don't get re-reported.

Acceptance criteria

• check_bash rejects any Bash prefix whose command family accepts --output= (covers all eight entries in item 1), or those entries are narrowed.
• validate-settings.py flags entry.startswith("Bash(") with a None parse as a violation instead of skipping it.
• Rule types are allowlisted fail-closed (like KNOWN_SAFE_MCP): an unrecognized prefix such as Write(/Read(/WebFetch( is a violation, not "out of scope".
• defaultMode, hooks, env, additionalDirectories, and permissions.deny are validated; permissions.defaultMode: bypassPermissions fails the check.
• The OK: summary counts only entries actually checked, so it can never over-report.
• wit_get_work_item_attachment is removed from settings.json and KNOWN_SAFE_MCP, or the KNOWN_SAFE_MCP procedure documents why it stays despite savePath.
• The KNOWN_SAFE_MCP procedure requires checking each tool's schema for path/file parameters, not just its name.
• Bash(gh auth status:*) is dropped or pinned to the exact form with no wildcard.
• WRITE_CAPABLE covers the item-5 commands; Bash(git difftool:*), Bash(gh auth:*), Bash(az pipelines:*), Bash(sed:*) all fail.
• validate-settings.py:178 uses k.startswith(cmd) so Bash(git c:*) fails; all 47 current entries still pass.
• git config --get-all / --get-regexp pass.
• standards/CLAUDE.md:188 states the intent plus the denylist caveat rather than an absolute guarantee; the :179 redirect bullet states one consequence, not two mutually exclusive ones.
• Test coverage for each of the above, plus validate-settings.py's file-error path (:207-211, currently written but never exercised).

Related

• #198 — the PR under review (merged)
• #183 — the driving issue. Its AC "Verified after setup-env sync that the intended commands stop prompting and the excluded ones still prompt" is still unmet; its open question "Verify the matcher semantics… Do not assume — test it" was answered by reading docs, which is what left item 6 open.
• #201, #202, #204 — the sync-path corruption and its fixes (context for the withdrawn findings)

Start with standards/validate-settings.py and standards/settings.json, then inspect setup-env.sh and scripts/merge_settings.py to reproduce the listed validator and sync-path cases. Review the existing validator tests and CI wiring; done means the reported write, disclosure, parsing, MCP, matching, and documentation inconsistencies are either safely handled or explicitly scoped, with coverage for the changed behavior.