feat(hooks): secret-guard blocks live secret values in tool args #17
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: yolo/agent-skills#17
Reference in New Issue
Block a user
Delete Branch "week-review/2026-W34"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Adds a secret-guard PreToolUse hook and a weekly-scan fix.
hooks/secret-guard.py: blocks tool arguments carrying a live secret — any~/.env.claudevalue (exact-name allowlist for identity/config vars; 6-char floor, short values match as standalone tokens) plus literal token shapes (ghp_,sk-,AKIA, private-key headers, credential-bearing URLs). Exit 2 names the variable, never the value. Serves Claude Code and Codex; wiring snippet inhooks/README.md. Closes #1.The operating.md rule promotions from #9 were reverted in full — merge, rtk and ADR guidance stays repo-scoped in memories.
claude-md/operating.md:48 —
operating.mdis also imported by Codex and concatenated into Pi and opencode (README.md:22,nix/home.nix:118-134), but the rtk rewrite is a Claude CodePreToolUsehook. Those clients therefore receive a false global claim and are told to proxy commands that were never wrapped. Scope this paragraph to Claude Code, or add equivalent wrappers for every client that consumes it.Scoped in 441ecb9's follow-up: heading now says Claude Code only, first sentence names the PreToolUse hook and tells other harnesses to ignore the section. Verified the hook exists only in ~/.claude/settings.json — ~/.codex/hooks.json has no rtk entry.
claude-md/operating.md:39 — The latest commit intentionally leaves the merge policy repo-scoped, but the PR title still says it promotes three universal rules and
Closes #9will close an issue whose title, body, and stated fix still require moving all three. Update the PR and issue metadata to record the revised two-rule scope before #9 is auto-closed.feat(claude-md): promote three universal rules to operating.mdto feat(claude-md): promote rtk and ADR rules to operating.mdPR retitled to the two-rule scope; the body already describes the drop and where the merge rule went. Issue #9 carries the amended resolution in a comment (2026-08-21), so the auto-close lands on a recorded decision, not the original three-rule ask.
hooks/secret-guard.py:53 — The generic exclusions make real secrets invisible: every value containing
$, every*_URL, and every HTTP(S) value bypasses value matching. Passwords can contain$, while webhook, DSN, RPC, and database URLs often carry credentials, so the guard does not provide #1's promised “any actual value” coverage. Resolve parsed variable names from the inherited environment (so expansions become their real values) and allowlist exact known non-secret variables instead of secret-capable value/name shapes; cover$-containing and credential-bearing URL values in tests.Fixed in the follow-up commit:
ALLOW_NAMESset (identity/config vars only, extended one name at a time)$-containing values no longer skipped: the name is resolved from the inherited environment and both the resolved value and the literal are scanned, so passwords with$are covered whether or not expansion happeneduser:pass@) or query string keeps the value in the secret set, covering DSNs and webhook URLsTested: postgres DSN with embedded password blocks, webhook URL with
?token=blocks, barehttps://endpoint passes,pa$$word-style value blocks, username value passes, live token still blocks via the environment-resolution path (SECRET_GUARD_ENVoverride added for testing).hooks/secret-guard.py:36 —
plain_url()treats every URL without userinfo or a query as a bare endpoint even when it has a credential-bearing path. Common webhook URLs put the secret in path segments, so they still bypass the guard. Require an empty/root path and no fragment, or allowlist exact known endpoint variables instead of inferring safety from URL shape.hooks/secret-guard.py:71 — The global eight-character minimum still excludes real credentials; it currently omits the configured
SANDBOX_PASSWORD, despite #1 promising to block any actual env value. Apply the length floor only to variables explicitly classified as non-secret, or remove it now that known non-secret names have an exact allowlist. Add short-password and path-token URL cases to the guard tests.Both findings addressed, one with a documented limit:
plain_urlskips bare origins only; any path segment, query, fragment or userinfo keeps the value in the secret set. The one endpoint var with a real path moved toALLOW_NAMES. Path-token webhook URL case added to the test run: blocks.Test run: path-token URL blocks, bare endpoint passes, 6+ char short value blocks standalone and passes as substring, live forge token blocks, prose passes.
feat(claude-md): promote rtk and ADR rules to operating.mdto feat(hooks): secret-guard blocks live secret values in tool argshooks/secret-guard.py:18 — Raising the floor to six reintroduces the concrete gap: the currently configured
SANDBOX_PASSWORDis below the floor (verified without reading its value), so Bash/Write/Edit arguments can transmit the live root password untouched. A comment telling operators to rotate it neither performs nor enforces that rotation, whilehooks/README.mdstill claims any env value is blocked. Keep the short-value handling until the credential is actually rotated, or make deployment refuse/warn clearly on short secret variables and narrow the documented guarantee.Addressed with the warn-and-narrow option: keeping short-value matching is not viable — it blocked two unrelated calls in live use, and a guard that fires on prose gets disabled, which is the worse outcome (per #1's own reasoning against noisy heuristics).
hooks/README.mdstates the real guarantee: 6+ chars blocked, 6-7 as standalone tokens, under 6 warned as unguardableRotation of the one affected credential is a machine operation, offered to the operator directly — the hook cannot rotate a root password.
hooks/secret-guard.py:72 — The warning classifies only the raw text from
.env.claude, not the inherited value that the guard actually checks. ForSECRET=$OTHER,$OTHERis measured instead of the resolved secret: a short resolved value can still be skipped with no warning, while a long resolved value referenced by a short name can warn falsely. Build the candidate values first, classify the inherited value when available (raw as fallback for non-exported assignments), then guard or warn from that same value.Fixed: candidates build first, classification follows the same value the guard checks.
resolvedis the inherited value with the raw literal as fallback; a literal that is nothing but a$-reference is dropped from candidates (matching it would block the referencing style the hook itself recommends). The unguardable warning now fires only when the name produced no guardable candidate and the resolved value is non-empty and short.Tested with
SECRET=$OTHERshapes: long resolved value blocks (no false warning), the"$VAR"reference style passes, short resolved value warns,$-containing literal password still blocks. Live token still blocks.hooks/secret-guard.py:79 — Exported variables still classify both the resolved value and the raw assignment, so the raw expression can hide an unguardable resolved secret. For
SECRET=x$EMPTY, the one-character resolved value is skipped, but the longer rawx$EMPTYsetsguarded = Trueand suppresses the warning. Conversely, a single-quoted literal such as'$longpassword'is removed as a “reference” even when the inherited value is identical, leaving it unguarded and unwarned. Whenname in os.environ, classify onlyos.environ[name]; use the raw value only when the variable was not exported.Fixed as suggested: when the name is exported, only
os.environ[name]is classified — guarded or warned from that single value; the raw assignment is used only for never-exported lines, where a pure$-reference is skipped.Tested both counterexamples:
SECRET=x$EMPTYexported with a 2-char resolved value now warns instead of being masked by the raw text, and a single-quoted'$longpassword'exported literally now blocks on its value. Reference style, normal secrets and the live token all behave as before.hooks/secret-guard.py:78 — For every exported name this now ignores the value read from
.env.claudeand trusts only the process environment. After a token is rotated while an agent session is running, that inherited value stays stale, so the hook guards the old token and allows the new live file value through despite claiming to read the file at hook time. Keep the inherited value for resolving expressions, but also classify a raw assignment when it is a literal (or resolve the file afresh in an isolated subprocess); guarding both old and new values during rotation is safe.Fixed: both forms classify independently — the inherited value and the file literal each guard (or warn) on their own, so during a rotation the old and the new value both block. The pure
$-reference skip applies only to the literal form. Thex$EMPTYcounterexample still warns (the short inherited value warns regardless of the literal being guarded).Tested: stale inherited token blocks, fresh file token blocks in the same run, short-resolved var still warns, reference style passes, live token blocks.