Audit nine more agents, plus what an agent's grants add up to - #23
Open
p4gs wants to merge 11 commits into
Open
Conversation
Agent configs arrive as TOML (Codex) and YAML (Hermes), but tomllib only exists on Python 3.11+ (the project floor is 3.10) and the stdlib has no YAML parser at all. Both readers cover the narrow subset machine-written agent configs use and degrade safely: anything out of subset is skipped, never guessed, so an audit cannot report a value a file does not contain. Audited TOML/YAML sources are read-only throughout.
New grantguard/core/agents/ package with per-agent PermissionDocument sources for OpenAI Codex, Cursor, OpenCode, Google Antigravity, Pi, and Hermes Agent, discovered alongside the Claude Code sources in the default user-level audit (and at project scope for explicit targets). Grants are flattened into rule strings and classified by the existing detectors. Editability follows one rule: rewrite only what provably round-trips. Strict-JSON grant arrays/maps (Cursor allowlists, Pi trust/settings, Antigravity grant arrays, OpenCode .json) rebuild the JSON preserving every unrelated key, exactly like the Claude settings document. TOML, YAML, JSONC, secret-bearing, and app-state sources are surfaced read-only, mirroring the ~/.claude.json precedent. Protective entries (deny/ask rules) are never surfaced or removed. Hermes .env is read for a fixed set of policy key NAMES only; allowed-user IDs surface as counts and secret values never enter rule text. Adds a new AUTONOMY risk category (approvals/review disabled — e.g. approval_policy = never, approvals.mode: off, approvalMode: unrestricted, blanket permission = allow, defaultProjectTrust = always) flagged for removal under both tolerances, with matching detectors for agent policy grants and network/exfiltration overbreadth.
Three more per-agent permission sources in grantguard/core/agents/: - GitHub Copilot: the CLI's saved tool-approval store (~/.copilot/permissions-config.json) is strict JSON and editable — per-directory command/read/write/MCP approvals and allowed directories. A commands approval covering several identifiers now removes only the flagged identifier, deleting the whole approval object only once it is empty, so removing one grant never silently drops its siblings. Allowed URLs, trusted folders, MCP servers, and VS Code agent-mode chat.tools.* auto-approve keys are surfaced read-only. - Windsurf: Cascade autoExecutionPolicy (turbo) and command allowlist plus MCP servers, read-only (VS Code-fork JSONC settings). - OpenClaw: exec/elevated/filesystem/channel-admission/gateway/plugin grants from the JSON5 openclaw.json (read-only), and the strict-JSON exec-approvals.json allowlist (editable), honoring OPENCLAW_CONFIG_PATH / OPENCLAW_STATE_DIR and the legacy ~/.clawdbot directory. AUTONOMY and overbroad detectors extended for each agent's approval-bypass and network/exfiltration shapes (chat.tools.global.autoApprove, Windsurf turbo, OpenClaw exec.security=full / open dmPolicy / gateway.auth.mode=none, blanket Copilot write/read and command-family wildcards, blanket trusted folders and allowed directories).
GrantGuard audits arbitrary user-authored configs, so a key expected to be a list but holding a stray string or non-iterable must never crash the audit or emit per-character junk rules. Add a shared _base.str_list() guard and route every agent module's list iteration through it, and broaden the document read-path exception guard to include TypeError. Found by the per-module test authors while covering the six existing agents; the fix is a class sweep across all of them, with a regression test.
Add an `agent` field to PermissionDocumentInfo (default "Claude Code") and stamp per-agent sources with their AGENT_NAME in the discovery aggregator, surfacing it in the /api/audit JSON. This lets the web UI group sources by the distinct agent they belong to.
The sidebar Sources list and the All-rules list can now group by the distinct AI agent each config belongs to, as a collapsible tree: Agent -> Folder path -> File name -> Permission category (-> rules, in All rules). A per-session toggle (persisted to localStorage) switches between the nested tree and the original flat list. In the All-rules tree, group headers carry a select checkbox that selects/deselects every editable rule beneath them, showing indeterminate on a partial selection. Agents/folders/files holding a flagged grant auto-expand on load. Also de-Claude-Code the now-inaccurate copy (the audit covers nine-plus agents): scan-scope subtitle, overview/empty-state text, share caption, and the removal-confirm prompt.
Every check until now judged a single rule string, which answers "should this
entry exist?" but not "what can this agent do?". Three blind spots followed
from that, each closed here.
Combined exposure (new core/capability.py). Grants are folded into one profile
per agent and reported when it can both reach private data and send data off
the machine — the pairing that turns text somebody else wrote into credentials
leaving the box. Each capability tracks the individual grants providing it, so
a denial removes an enabler rather than a capability: denying curl does not
mean data cannot leave while wget, git push, or a remote server are still
granted, and a capability clears only when every enabler present is denied. A
detector one narrow denial could switch off would report clean on a machine
that is not, so there is a regression test pinning exactly that. deny lists are
now read for this subtraction and are still never classified or removed. Agents
whose scope layering is established are merged; others are reported per file
rather than guessing at a merge.
MCP servers (new core/mcp.py). A server is a capability held on every run, and
its credentials sit in plain text in files that are often committed. They are
now discovered from a project .mcp.json and from ~/.claude.json, top level and
per project, including the standing enable/disable trust lists, and each
definition's env, args, headers and url are scanned — query strings and
userinfo included, plus JWTs. Detection keys on the shape of the value and
never on the name of the key holding it, so ${VAR} indirection and omitted
values stay quiet; that false-positive case has its own tests.
Settings beside the allow list (new core/claudepolicy.py). defaultMode, hooks,
apiKeyHelper, env and additionalDirectories grant standing power no allow rule
mentions and were previously unread. They flow through the existing detectors,
so a hook that reads a credential store or an env entry holding a literal token
is caught by patterns that already recognize those shapes. Because these name a
setting rather than a list entry, they are reported in their own review-by-hand
section and excluded from bulk removal: deleting a hook changes what the agent
does, unlike pruning an allow rule, which only makes it ask again.
Also: a new INTERCEPT category for settings that reroute the agent's traffic or
move data off-box unprompted, and audit now exits non-zero when a source could
not be read — an unexamined source is not a clean one, and telling a pipeline
otherwise is the failure this whole change is about.
444 tests pass, up from 365; the three new modules are at 100% statement
coverage. No new runtime dependency.
A review of the previous commit found the headline guarantee did not actually hold, plus a denial-of-service reachable from a cloned repository. All four are confirmed by test and fixed here. A denial now has to be as broad as the grant before it subtracts anything. Previously any deny string containing the command name cleared the whole enabler, so `deny: Bash(curl http://169.254.169.254/*)` — blocking curl against one address — reported that data could not leave at all, and `deny: WebFetch(domain:curl.se)` did the same by merely containing the word. Both produced a clean result on a machine that was not clean, which is exactly what the module docstring promises cannot happen. Denials are now matched in the position they actually govern, and only subtract when what remains after the command is a wildcard rather than a specific target. Broad denials still close a capability, so this does not over-correct into ignoring real ones. A grant of the whole shell now provides capability. `Bash(*)` previously contributed nothing to the analysis, so the single most dangerous grant possible could never raise a combined finding. Scanning is bounded. enablers_in ran uncapped while the detector registry caps at MAX_SCAN_LEN, and the loopback exemption used a negative lookahead containing `.*`, which re-scans from every start position. Together that was quadratic: a large value in a project .mcp.json, a file that arrives with a checkout, took seconds to minutes and stalled the audit. The lookaheads are now literal substring exclusions and the scan is capped; 64 KB went from unusable to under a millisecond. Three credential shapes were reported without masking: a token in a named request header, one in an inline -H argument (hooks are free-form shell and a likely place for one), and URL parameters beyond the original keyword list — sig, signature, session, auth, credential, pat. All now redact. Also corrected an overclaim in the mcp module docstring. It said detection keys on the shape of the value and never on the name, which read as a stronger guarantee than the code gives: a high-entropy value under a name carrying no cue is not detected. The docstring now states that gap plainly instead of implying it away. 456 tests pass. Every defect above has a regression test.
CONTRIBUTING asks for a test in tests/test_detectors.py when a detector is added; the coverage for these existed but sat in the per-feature test modules. This puts it where the guide says to look for it: bucket assertions for the new category, the loopback exemption, each new credential shape, and the placeholder and variable-indirection cases that must stay quiet.
Author
|
Heads up: CI and CodeQL are sitting at While that waits, I ran CI's own steps locally against both ends of its matrix:
Only thing I can't cover locally is the Linux and Windows legs. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two things here, and they stack: audit coverage for nine more AI coding agents, then a set of checks that look at what an agent's grants add up to rather than judging each rule on its own.
Coverage beyond Claude Code. Codex, Cursor, OpenCode, Antigravity, Pi, Hermes, Copilot, Windsurf, and OpenClaw all accumulate standing permissions in their own config files. Each gets a reader under
grantguard/core/agents/, and grants flatten into the same rule strings the existing detectors already classify. Sources are editable only where a rebuild-write is provably safe (strict JSON); TOML, YAML, JSONC, and anything holding secrets is surfaced read-only, since a stdlib rewrite would eat the user's comments. Two small readers (tomlread.py,yamlread.py) cover the formats stdlib does not, so the zero-dependency rule holds.Claude settings that sit outside
permissions.allow.defaultMode,hooks,apiKeyHelper,env, andpermissions.additionalDirectoriesgrant standing power that no allow rule mentions, and none of them were being read.bypassPermissionsskips the prompt entirely, which means the allow list stops being the boundary at all. A hook is a shell command that fires on its own. These run through the detectors that already exist, so a hook reading a credential store or anenventry holding a literal token gets caught by the patterns already in the file. They report in their own "review by hand" section and--fixnever touches them: pruning an allow rule just makes the agent ask again, but deleting a hook changes what the agent does.MCP servers. A server is a capability the agent holds on every run, and its credentials sit in plaintext in files that often get committed. Claude's own MCP config was not read at all. Now discovered from a project
.mcp.jsonand from~/.claude.json(top level and per project, plus the enable/disable trust lists), withenv,args,headers, andurlscanned, including query strings and userinfo. Detection keys on what the value looks like, never on the name of the key holding it, so"API_KEY": "${MY_KEY}"stays quiet. That false-positive case has its own tests, because getting it wrong makes the whole feature unusable.Combined exposure. The part I think matters most. Reading a credential store is unremarkable. Running
curlis unremarkable. An agent holding both, that also reads a repo full of text somebody else wrote, is a different thing, and no per-rule check can see it. Grants now fold into one profile per agent and report when an agent can reach private data and send data off the machine.The rule that keeps that honest: a denial removes an enabler, not a capability.
deny: Bash(curl:*)does not mean data cannot leave whilewget,git push, and a remote MCP server are still granted, and a denial scoped to one target (deny: Bash(curl http://169.254.169.254/*)) does not close off curl generally. A capability clears only when every enabler present is denied. A check that one narrow deny could switch off would report clean on a machine that isn't, which is worse than not shipping it, so there are regression tests pinning exactly that.permissions.denyis read for this subtraction only, and is still never classified, flagged, or removed. Agents whose scope layering I could verify get merged into one profile; the rest report per file rather than guessing at a merge.Two smaller things worth calling out. There's a new
INTERCEPTcategory for settings that reroute the agent's traffic or move data off-box unprompted (ANTHROPIC_BASE_URL, proxy andNODE_OPTIONSoverrides, hooks that reach a non-loopback host). Andauditnow exits non-zero when a source could not be read: an unexamined source is not a clean one, and telling CI otherwise defeats the point of running it there.Related issue
Partially addresses #11. The combined-exposure layer catches
Bash(*)as a high-severity finding, since an unrestricted shell provides both sides of the pairing. But the per-rule detector still classifiesBash(*)as SAFE, and colon-syntax rules (Bash(nc:*)) are still missed, so the normalization that issue asks for is not done here and is worth its own change.Type of change
Checklist
uv run python -m compileall -q grantguard grantguard.pypassesredact()Notes on the checklist, since a few deserve detail:
uv run python -m unittest discover -s testspasses 462 tests, up from 88 onmain. Every behavior added here has tests, and the ones covering the deny-subtraction rule are the ones I'd read first if you're reviewing this.redact()function in the tree anymore; the equivalent isRedactingPatternDetector, and all 12 secret detectors use it, so every secret shape masks on display. Verified in the UI that a plaintext MCP credential renders as<REDACTED>with the raw value absent from the DOM.INTERCEPTis registered everywhere CONTRIBUTING lists:RiskCategory,RISK_CATEGORY_INFO,RISK_CATEGORY_ORDER, both tolerance tables,PATTERN_DETECTORS, andREASONS/REASON_SVG/REASON_ORDERinweb/app.js.--fixremoved only allow entries, leaving hooks,env,defaultMode, the deny list, and.mcp.jsonbyte-identical. Opened the UI in Chrome and confirmed the new category renders with its label and icon,.mcp.jsonshows up as a source, and the review tier populates.pnpm checkreports the same 33 type errors before and after my changes, so nothing new.pnpm fmt:checkfails onapp.js, but it also fails onmainbefore any of my commits; my three additions come out ofoxfmtunchanged, so I left the pre-existing drift alone rather than burying this diff in a whole-file reformat. Happy to do that as a separate cleanup if you want it.One process note: the combined-exposure code got an adversarial review after I first pushed it, which found four real defects, including the deny-subtraction scope bug described above and a quadratic-time regex reachable from a
.mcp.jsonin a cloned repo. Both are fixed inb36f751with regression tests. Flagging it because the review was worth more than the original implementation, and the fixes are the parts most worth scrutinizing.