fix(security): land the credential perimeter ahead of the v5 rebuild - #163
Conversation
Claude Code honours only Edit(path) for file permission checks. Write(path) deny rules are silently skipped — CC prints a warning to stderr on every invocation that nothing surfaces. All 9 credential Write() rules in .claude/settings.json have therefore been no-ops since f4cd05a: Read(**/id_rsa*) -> blocked reading the SSH key Write(**/id_rsa*) -> did nothing; an agent could overwrite it Same for .env, *.pem, .git-credentials, secrets/**, *credential*, *secret*key*, *token*.json, id_ed25519*. The perimeter blocked exfiltration and permitted tampering — the wrong half. Rewrites all 9 to Edit(...), which covers every file-editing tool, so the Read(x) + Edit(x) pairs actually close. No dual-form shim for older CC — clean break, per v5.0.0 G2. Also drops "mcp__*" from permissions.allow: wildcard tool names are rejected in allow rules, so it was a no-op emitting a startup warning. It was the visible instance of the same class as the nine above. Verified: - `claude config list` emits zero parse warnings against this file - enforcement proven behaviourally, not by counting: a Write to a path matching Edit(**/id_rsa*) was blocked, while a control write to a non-matching name in the same directory succeeded Does not fix users. Marketplace installs receive no security block at all (install.sh:377 — "can't be in plugin"), and no shipped preset carries credential denies, so `pds sync` doesn't deliver them either. Those are tracked in .claude/swarm/findings.md as P0s for #159. Refs #159 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLCbseL5UX9RddrddHGYGD
…to a preset The credential perimeter has lived only in .claude/settings.json, which is applied exclusively by install.sh. No shipped preset carried any of it — pds-default has 3 denies (all rm -rf-class), dev-tools has 0. So `pds sync`, the documented "portable preferences" path, delivered velocity and no perimeter. This preset closes that half of the split. 58 entries, ported at parity with .claude/settings.json as of 912ec66. Deliberately not a redesign — composition changes belong to #159, not to the port. - scope: both on every entry - Edit() forms throughout, never Write(). CC honours only Edit(path) for file permission checks; Write() deny rules are silently skipped. The forms here were pulled from the corrected file, not retyped, precisely so this artifact does not reintroduce the bug it exists to ship the fix for. - per-entry `reason`, per the preset format's own contract - two groups flagged OPINIONATED inline (production tripwires, outbound remote access). They false-positive for real users — a consultant on client infrastructure wants them gone. Included for parity with what install.sh users already receive. A reviewer should decide whether they belong in a baseline or a separate prod-guardrails preset. UNVERIFIED — and deliberately shipped saying so. `pds sync` resolves presets from the installed plugin cache, not the repo: Error: expanding preset 'security-baseline' (known presets live in ~/.claude/plugins/cache/pds-marketplace/pds/<v>/config-presets) so it cannot be exercised until published, or until copied into the cache (mutates the user's installation). It parses. Parsing is not enforcement — that distinction is what let 9 dead rules ship since f4cd05a. Verify behaviourally before marking the #159 criterion done. Necessary, not sufficient. Correction to an earlier claim: the marketplace DOES ship settings, presets, install.sh and the CLI source — the whole repo tree lands in the plugin cache. Nothing applies any of it. The gap is activation, not distribution. Even with this preset published, a user must build the CLI from cached source, author a config.yaml, list the preset, and run `pds sync` — four steps, zero prompts. Claude Code auto-activates only skills, agents, hooks and MCP servers, so a SessionStart perimeter check is the only vehicle that reaches someone who does not already know they are exposed. Not written here: session-start.sh already ships and cannot be fire-tested from inside a session. Refs #159 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLCbseL5UX9RddrddHGYGD
/pds:verify caught this: dd1c6f1 shipped a preset that nothing told anyone existed. docs/config.md:10 instructs users to copy examples/config.yaml, and that file listed `presets: [pds-default, dev-tools]`. Follow every documented step correctly and you got no perimeter — exactly the state the preset was written to fix. Unverified AND undiscoverable is not shipped. - examples/config.yaml: security-baseline ON by default. Shipping the perimeter off-by-default is the mechanism by which it went missing everywhere. Nothing in it trades against velocity. - docs/config.md: shipped-presets table + a security-baseline section covering what it does, which parts are opinionated, and how to drop it correctly (copy the entries you want into your own deny list; read the per-entry `reason` fields first). - CHANGELOG: [Unreleased] entry for the preset, the nine dead deny rules, and the mcp__* no-op. Also fixes a guard that was never armed: examples/config.yaml shipped `protected_branches: []`. /pds:finish reads that list and prompts before pushing to a match — an empty list means nothing is protected and the prompt never fires, for every user who copied the example as documented. Now [main]. Three things recorded in docs/config.md that previously had no home: - `pds sync` cannot manage the sandbox. There is no `sandbox` key in the schema; the block reaches settings.json only via install.sh. - Presets resolve from the installed plugin cache, not a repo checkout. - `pds doctor` reports "config parses: ok" for YAML that Claude Code then rejects rule-by-rule. It validates syntax, not semantic acceptance by the consumer. Ask `claude config list` instead. - Bash() deny patterns substring-match the entire command string, so a command that merely mentions a guarded path is denied — including one documenting the perimeter itself. security-baseline remains UNVERIFIED — presets resolve from the plugin cache, so it cannot be exercised until published. Parsing is not enforcement. Refs #159 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QLCbseL5UX9RddrddHGYGD
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code review is billed via overage credits. To resume reviews, an organization admin can raise the monthly limit at claude.ai/admin-settings/claude-code.
Once credits are available, push a new commit or reopen this pull request to trigger a review.
skills/grill/SKILL.md was rewritten in 4.22.0 to require AskUserQuestion (every step) and EnterPlanMode (its Mode section), but the orchestrator's tools: frontmatter never granted either, and pds:grill wasn't in its skills: list. Subagents only get tools explicitly listed in frontmatter, so orchestrator-driven Phase 1 grill (mandatory before every swarm) had no way to ask a structured question. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
| # security-baseline preset — the credential perimeter. | ||
| # | ||
| # WHY THIS EXISTS | ||
| # PDS advertises a credential perimeter. Until now it lived ONLY in the repo's | ||
| # .claude/settings.json, which reaches users exclusively via install.sh. The | ||
| # marketplace plugin cannot ship settings (install.sh:377 — "can't be in | ||
| # plugin"), so every marketplace-install user has had `deny: []` — no perimeter | ||
| # at all. And no shipped preset carried these rules, so `pds sync` did not | ||
| # deliver them either: pds-default has 3 denies (all rm -rf-class), dev-tools | ||
| # has 0. Neither distribution path delivered the product. This preset closes | ||
| # that gap for the `pds sync` path. | ||
| # | ||
| # Ported at parity with .claude/settings.json as of 912ec66. Deliberately NOT a | ||
| # redesign — composition changes belong to #159, not to the port. | ||
| # | ||
| # EDIT(), NEVER WRITE(). Claude Code honours only Edit(path) for file permission | ||
| # checks; Write(path) deny rules are silently skipped, with a warning printed to | ||
| # stderr that nothing surfaces. All 9 file-tampering rules below were dead from | ||
| # f4cd05a until 912ec66 — the perimeter blocked reading a credential and | ||
| # permitted overwriting one. Enforcement is verified behaviourally (attempt a |
There was a problem hiding this comment.
🟡 The new preset's header comment (lines 8-9) says "pds-default has 3 denies (all rm -rf-class), dev-tools has 0" but pds-default.yaml has exactly 1 deny entry — the other 3 are ask-gated git ops, not denies. Separately, CLAUDE.md:66's Project Structure listing still reads "config-presets/ — Shipped permission presets (pds-default, dev-tools)", which this PR's new security-baseline.yaml preset makes stale.
Extended reasoning...
This is two small, PR-introduced documentation inaccuracies bundled into one finding since they live in the same new file's surrounding context.
1. Miscounted deny rules (config-presets/security-baseline.yaml:8-9)
The header comment reads: "pds-default has 3 denies (all rm -rf-class), dev-tools has 0. Neither distribution path delivered the product." I checked config-presets/pds-default.yaml directly:
$ grep -c "verb: deny" config-presets/pds-default.yaml
1
$ grep -B3 "verb: deny" config-presets/pds-default.yaml
# --- Hard stops that would lose work irrecoverably ---
- pattern: "Bash(rm -rf /)"
verb: deny
$ grep -c "verb: ask" config-presets/pds-default.yaml
3
So pds-default has exactly one deny entry (Bash(rm -rf /)), not three. The "3" almost certainly comes from conflating the deny count with the 3 ask-gated entries nearby (git push --force, git reset --hard, git clean -f) — which are ask-gated history-destroying git operations, not deny-class rules at all, and not "rm -rf-class" either. The dev-tools has 0 half of the claim is correct. Notably, this PR's own docs/config.md shipped-presets table gets it right: "Denies rm -rf /. Puts the three history-destroying git operations behind ask." — so the new preset's own header comment contradicts the PR's own correct documentation elsewhere.
Impact is limited to a comment: it doesn't affect any rule, pattern, or runtime behavior. But it does overstate the existing security coverage the file's rationale leans on ("neither distribution path delivered the product" — the "3 denies" framing inflates how much pds-default already covered before this PR).
2. Stale CLAUDE.md preset enumeration (CLAUDE.md:66)
CLAUDE.md's Project Structure section reads: "config-presets/ — Shipped permission presets (pds-default, dev-tools) — referenced from pds.config.yaml". This PR adds config-presets/security-baseline.yaml as a third shipped preset, wires it default-on into examples/config.yaml (presets: [pds-default, dev-tools, security-baseline]), and documents it correctly in docs/config.md's "Shipped presets" table — but CLAUDE.md itself is not in the PR's changed-files set, so its two-preset enumeration is now incomplete. CLAUDE.md's own Workflow rule requires updating docs that code changes invalidate, and this parenthetical is a concrete, easily-checked list the PR invalidates.
Fix: change "3 denies" to "1 deny" (and drop or reword "all rm -rf-class" since it no longer needs to cover the ask entries) in security-baseline.yaml:9; add security-baseline to the parenthetical at CLAUDE.md:66.
Both are comment/doc-only inaccuracies with zero effect on rule enforcement or preset behavior — worth a one-line fix each but not merge-blocking.
| "WebFetch", | ||
| "WebSearch", | ||
| "Task", | ||
| "mcp__*", | ||
| "Bash(git add:*)", |
There was a problem hiding this comment.
🟡 This PR removes "mcp__" from permissions.allow in .claude/settings.json (a rejected no-op wildcard), but docs/teams.md:270,319 and docs/sandbox.md:97-110,199 still describe "mcp__ wildcard" as the live default auto-allow mechanism for MCP tools, including security advice to replace it with explicit allowlists. These docs now reference a permission entry and mitigation that no longer apply.
Extended reasoning...
The bug: 912ec66 deletes the "mcp__*" line from permissions.allow in .claude/settings.json (diff line 37) because wildcard tool names are rejected by Claude Code and the entry was a no-op that only emitted a startup warning. Two docs were not updated to match:
docs/teams.md:270— lists "All MCP tools (viamcp__*wildcard — see note below)" under What's Auto-Allowed.docs/teams.md:319— "mcp__*wildcard risk: The defaultmcp__*permission auto-approves all MCP tools from any configured server. For security-sensitive environments, replace with explicit allowlists..."docs/sandbox.md:97-110— "For teams that want tighter control, replace themcp__*wildcard in permissions with explicit MCP tool names" and "The defaultmcp__*permission auto-approves all MCP tools."docs/sandbox.md:199— security-model table row:MCP tools | mcp__* allow list.
Why this happened: the PR's own CHANGELOG entry states the removal plainly ("mcp__* removed from permissions.allow... it was a no-op that emitted a startup warning"), and the PR heavily updated docs/config.md and examples/config.yaml for the sibling changes in this same commit (the Write→Edit fix, protected_branches), but the sweep never touched docs/teams.md or docs/sandbox.md, which describe the exact permission line that was deleted.
Why nothing catches this: there's no test or lint that cross-checks doc prose against .claude/settings.json contents — this is a documentation-only drift that only a reader auditing MCP tool exposure via these docs would notice.
Impact: someone reading docs/sandbox.md or docs/teams.md to understand MCP tool exposure will believe there is a live mcp__* auto-allow they need to consider narrowing for security-sensitive environments — a mitigation for a mechanism that no longer exists in shipped settings. Concretely: a reader following docs/sandbox.md:97's "replace the mcp__* wildcard" instructions today would look for a line that is no longer there and could reasonably conclude the doc (or their install) is broken, when in fact the wildcard was already inert even before this PR removed it — worth noting since it slightly softens the practical risk (the wildcard never actually granted anything, per the PR's own description).
Step-by-step proof:
- Before this PR:
.claude/settings.jsonpermissions.allow contains"mcp__*"(a no-op line, per PR description). docs/teams.md:270anddocs/sandbox.md:97-110,199describe this line as the live MCP auto-allow mechanism and give advice on narrowing it.- This PR deletes the
"mcp__*"line from.claude/settings.json(diff line 37). docs/teams.mdanddocs/sandbox.mdare not touched in the diff — grep confirms the four references still exist verbatim post-PR.- Reader consequence: docs describe a settings entry that doesn't exist in shipped config, and give security mitigation advice for something that's already gone.
Fix: in docs/teams.md, drop the "All MCP tools (via mcp__* wildcard)" bullet from What's Auto-Allowed and remove or rewrite the "mcp__* wildcard risk" note; in docs/sandbox.md, remove the "Maximum lockdown" replace-the-wildcard section, the "mcp__* wildcard risk" section, and update the table row to reflect that MCP tools now require explicit per-tool/per-server permission entries (no wildcard shorthand exists).
Severity: nit. This is documentation-only drift — nothing breaks at runtime, and per the PR's own description mcp__* was already inert before removal, so the real MCP security posture is unchanged either way. It's a legitimate cleanup the author should make (and the PR's own CHANGELOG/doc-sync habits elsewhere in this same commit show the precedent for doing so), but it doesn't block merge.
1 participant
Splits the security work off PR #160 so a live credential exposure lands now, independent of the parked v5.0.0 rebuild (#159 / #161). These three commits sit contiguously on
mainwith no swarm/agent retrofit — this branch ismain+ security only.What's here (3 commits)
✅ VERIFIED — the live-exposure fix (
912ec66)Write()deny rules →Edit(). Claude Code honours onlyEdit(path)for file permission checks;Write(path)rules are silently skipped. Every credential file-tampering rule had been a no-op sincef4cd05a— reading a private key was blocked, overwriting one was not. Wrong half of the perimeter.mcp__*removed frompermissions.allow— wildcard tool names are rejected; it was a no-op emitting a startup warning.Edit(**/id_rsa*)was blocked; a control write to a non-matching name in the same dir succeeded. Not proven by counting entries.⚠ UNVERIFIED — the marketplace-reach vehicle (
dd1c6f1,5fb86d6)config-presets/security-baseline.yaml— 58-rule credential perimeter as a shipped preset,Edit()forms throughout, per-entryreason. Default-on inexamples/config.yaml; documented indocs/config.md.pds syncresolves presets from the installed plugin cache, not this repo, so the preset cannot be exercised until published. It parses; parsing is not enforcement — that exact distinction is what let the 9 dead rules ship for months. Verify behaviourally before trusting it.protected_branches: [main]inexamples/config.yaml(shipped empty →/pds:finishguard never fired).Scope boundary
.claude/settings.json(the verified fix), the preset, its docs, CHANGELOG[Unreleased].Reviewer note
912ec66is safe to merge on its own merits. The preset commits are honest-but-unverified — if you'd rather not ship a default-on preset that hasn't been exercised, dropdd1c6f1+5fb86d6and land only912ec66. Say the word.Refs #159