fix: close five P1 enforcement-bypass issues (#159–#163)#164
Merged
Conversation
…ass (#159, #160, #162) Three P1 gaps let an agent neutralize the enforcement layer through the very tools/paths it governs: - #159 CONTEXT.md was an unprotected master switch — writing `arbiter: disabled` made every hook dormant. pre-write/pre-edit (H-18) now block a CONTEXT.md write/edit whose RESULTING content drops `arbiter: enabled` or corrupts the frontmatter (reusing frontmatter_enabled_text); pre-bash adds the shell flank. - #160 the .codearbiter/.markers/ gate tokens were writable repo files — a hand-written marker forged a security/migration/ADR gate pass. pre-write/ pre-edit (H-19) block tool writes to any marker; pre-bash blocks shell redirects/verbs naming security-gate-passed / migration-gate-passed (the sanctioned python producers and the /adr `touch` still work). NOTE: blocking the producer scripts is unworkable (identical command to the legit gate run), so this closes every forge-by-write vector; the run-the-real-producer residual is inherent to a single-agent session and documented in-code. - #162 pre-write/pre-edit classified the RAW file_path, so a symlink alias whose visible path lacked .codearbiter/ bypassed H-05/H-11. All protected-path checks now route through classify_protected(), which resolves realpath (the same helper post-write-edit/pre-read already used) and checks raw + resolved. Also wires NotebookEdit into the Edit guard (was matched by no hook) so it can't target a protected artifact. Helpers is_context_md/is_marker_path/ classify_protected/frontmatter_enabled_text centralized in _hooklib. New behavioral + unit tests for each guard, including the symlink repro. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JqTxpxEkbDHb6AuADohtMR
…161) pre-bash.py gates git commit/push/add by matching the literal Bash command string, so shell indirection (`g=git; c=commit; $g $c -m x`) constructs a real commit the hook never sees and exits 0 — and there was no enforcement below the Claude Code Bash hook (`.git/hooks/` was stock samples). Install repo-level pre-commit/pre-push hooks that run at the git operation itself, where spelling no longer matters: - git-enforce.py enforces H-01 (no commit/push to a protected branch, incl. a detached HEAD on its tip and `HEAD:main` refspecs), H-02 (force / non-ff push), and H-09b/H-10b/H-14 (staged crypto/secret + migration marker coverage). It reuses the SAME _hooklib detection primitives (CRYPTO_RE/SECRET_RE/line_digest/ content_digest/is_migration_path/marker_fresh) so the two enforcement points can't drift, and fails closed on a git read error. - _githooks.py installs a tiny sh shim that selects ONE interpreter (never `python3 X || python X`, which would swallow a BLOCK and drain pre-push stdin), points at the enforcer by absolute path refreshed each session, honors core.hooksPath / worktrees, NEVER clobbers a foreign hook, and is idempotent. - Wired into init-codearbiter.py (scaffold) and session-start.py (every session, so already-initialized repos get it), both best-effort and non-fatal. pre-bash stays as the fast, well-worded first line; this is the spelling-proof backstop underneath. New tests prove a variable-indirected commit on main is blocked at the git layer, foreign hooks are preserved, and dormant repos no-op. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JqTxpxEkbDHb6AuADohtMR
prepareWorktree resolved a task worktree path from the env-controlled
FARM_WORKTREE_ROOT plus a plan-controlled task id, then rm(...,{recursive,force})'d
it BEFORE git validated it as a real worktree. A broad/misconfigured root
(FARM_WORKTREE_ROOT=/Users/alice) plus a plausible id (Desktop) recursively
deleted an arbitrary directory; SAFE_TASK_ID even admitted `.` and `..`.
Two fail-closed defenses, reusing the existing isInside() helper:
- validateWorktreeRoot(): the resolved root must live inside the repository top
level unless FARM_ALLOW_EXTERNAL_WORKTREE_ROOT=1 is set — an out-of-repo root
is refused at run start (the actual blast-radius knob).
- assertContainedWorktree(): every worktree path must be strictly inside that
root (never the root itself), asserted immediately before the recursive delete
in prepareWorktree (which also covers the best-of-N sample and canary paths
that route through it).
validate() also rejects the reserved ids `.` and `..` at the authoring boundary.
Rebuilt farm.js. New unit tests cover the /Users/alice misconfig, the override,
`.`/`..` id rejection, and root-escape containment.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JqTxpxEkbDHb6AuADohtMR
Collaborator
Author
|
Thanks @tg12. these five P1 reports were exceptionally well-researched: precise source evidence, reproductions, and correct duplicate checks on every one. Each validated exactly as written, and #164 fixes all of them (#159–#163), with the rest of the repo swept for the same class in each case. Much appreciated. |
The #162 symlink regression tests failed only on windows-latest: os.symlink to a directory target that doesn't exist yet creates a *file* symlink there, which realpath won't resolve paths beneath (POSIX resolves lexically, so ubuntu/macos passed). The product code is unaffected — this is a test-fixture portability bug. - symlink helper is now directory-aware (target_is_directory), and the marker case pre-creates .codearbiter/.markers so the dir symlink is valid on Windows - skip the symlink cases outright on runners that lack symlink privilege Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JqTxpxEkbDHb6AuADohtMR
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
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.
What & why
Five P1 reports (thanks @tg12) each showed the enforcement layer being enforced against a forgeable or lexical surface rather than the real underlying operation. Each is fixed below, and for every fix the rest of the repo was swept for the same class.
#159 — CONTEXT.md was an unprotected kill switch. Every hook gates on
arbiter: enabledin.codearbiter/CONTEXT.md, but nothing protected that file — writingarbiter: disabledmade every gate dormant.pre-write/pre-editnow block a CONTEXT.md write/edit whose resulting content dropsarbiter: enabledor corrupts the frontmatter (H-18, reusing a newfrontmatter_enabled_text);pre-bashadds the shell flank. Stage bumps that keep it enabled still pass.#160 — gate-pass markers were forgeable.
.codearbiter/.markers/*were writable repo files, so a hand-written marker forged a security/migration/ADR gate pass.pre-write/pre-edit(H-19) block tool writes to any marker;pre-bashblocks shell redirects/verbs namingsecurity-gate-passed/migration-gate-passed. This closes every forge-by-write vector (the demonstrated exploit). Note: blocking the producer scripts themselves is unworkable — the sanctioned gate runs the identical command, so the hook can't distinguish a genuine post-review run from a forged one. That residual is inherent to a single-agent session and is documented in-code; the/adrtouchand the python producers keep working.#161 — shell indirection bypassed the git gates → real git hooks.
pre-bashmatches the literal command string, sog=git; c=commit; $g $cbuilds a real commit it never sees, and there was no enforcement below it. Added repo-levelpre-commit/pre-pushhooks (git-enforce.pyinstalled by_githooks.py) that enforce H-01/H-02/H-09b/H-10b/H-14 at the git operation itself — spelling-proof. They reuse the same_hooklibdetection primitives (no drift), fail closed on git read errors, select one interpreter (neverpython3 X || python X, which would swallow a BLOCK / drain pre-push stdin), honorcore.hooksPath/worktrees, and never clobber a foreign hook. Installed idempotently at/ca:initand every SessionStart.pre-bashstays as the fast first line.#162 — symlink aliases bypassed the Write/Edit path guards.
pre-write/pre-editclassified the rawfile_path, so a symlink whose visible path lacked.codearbiter/slipped past H-05/H-11. All protected-path checks now route through a newclassify_protected()that resolves realpath (the approachpost-write-edit/pre-readalready used) and checks raw + resolved. Also wiredNotebookEditinto the guard (it was matched by no hook).#163 — farm could
rm -rfoutside its workspace.prepareWorktreeresolved a delete path from env-controlledFARM_WORKTREE_ROOT+ plan-controlled task id with no containment (andSAFE_TASK_IDadmitted./..). Now: an out-of-repo root is refused unlessFARM_ALLOW_EXTERNAL_WORKTREE_ROOT=1; every worktree path is asserted strictly inside that root before the recursive delete (reusing the existingisInside()); andvalidate()rejects./..ids.Closes #159. Closes #160. Closes #161. Closes #162. Closes #163.
Type of change
fix: bug fixfeat: new behaviordocs: documentation onlyrefactor: no behavior changechore: deps, tooling, revertsChecklist
main(not a direct write tomain)plugins/ca/hooks/tests/)farm.jsrebuilt in sync)2.6.1is unpublished (nov2.6.1tag), so these ride it and the CI version-bump gate passes; no bump neededCHANGELOG.mdentry added — deferred to the per-release "roll the notes" step, matching repo conventionpreviewwhere appropriate — all new gates run only in arbiter-enabled repos; the git-hook install is idempotent and non-clobberingdocs/hooks.mdupdated — see reviewer notesNotes for the reviewer
git-enforce.py(git-level enforcer),_githooks.py(installer). No network access added —git-enforce.pyshells out only to localgit, preserving the no-network invariant.docs/hooks.mdwas not updated in this PR; happy to follow up, or fold it in here.pre-bash.py/security-pass.pycomments. If you'd prefer the heavier SubagentStop-attestation route, that's a larger follow-up._githooks.pywired intoinit-codearbiter.py+session-start.py) is the one arguably-architectural addition — flagging in case you want an ADR before merge.hooks.json), but the Windowssh-shim path was reasoned about, not run here.Generated by Claude Code