Repository navigation
v0.6.13: taint snapshot durability + symlink refusal + relay liveness - #4
Merged
Merged
Conversation
Closes the alternate-name self-approval bypass flagged as a ship-blocker
in the v0.6.11 round-6 implementation audit. `isAdminCliInvocation`
relied on `basenameOf(peeled[0]) === "patchwork"`, so an agent could
self-approve through a symlinked alternate name:
ln -s "$(command -v patchwork)" ./pw
script -q -c './pw approve <pending-id>' /dev/null
`isPatchworkExecutable` now runs the basename match first (cheap, and
unchanged behaviour for direct invocations), then falls back to realpath
identity against the running CLI's own canonical path. That catches
`./pw`-style symlinks because realpath canonicalises both sides. The
canonical realpath defaults to `process.argv[1]` and can be overridden
with `PATCHWORK_CANONICAL_REALPATH` for tests and unusual install
layouts.
Honest residual, documented in the code and in the test corpus: literal
binary copies (`cp $(which patchwork) /tmp/pw`) and wrapper scripts are
NOT caught, because their realpath is themselves rather than the
canonical CLI. Those stay residual until a root-owned approval authority
replaces argv-based identity entirely in Protected Mode (v0.7). The
corpus includes one explicit DOCUMENTED-RESIDUAL test asserting current
behaviour, so that future change has a clean inversion point.
Tests added under "F1 / R6-002: alternate-name (symlink) execution" in
packages/agents/tests/claude-code/dangerous-shell-combos.test.ts:
- denies `<symlinkName> approve <id>` via realpath identity
- denies alternate-name + clear-taint under taint
- denies a symlink wrapped in `exec`
- does not match a non-patchwork executable with a similar name
- does not crash on a non-existent path (fail-safe)
- documented residual: literal binary copies still bypass
The new describe block uses top-level ESM imports rather than inline
require("node:*") calls, which is the biome-clean form and the standard
vitest pattern in this file.
Validated 2026-10-06: clean build, full suite 1515 passing,
dangerous-shell-combos.test.ts 58 passing.
Refs: REVIEWS/2026-05-12-gpt55-v0.6.11-impl-audit-round6.json
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
….6.12
Per the release strategy, v0.6.12 ships the R6-002 self-approval fix
together with an honest security-model disclosure, so the fix lands in
the same release as the admission that the prior model is bypassable.
- README: replaces the "Tamper-resistant ... impossible for non-admin
users to remove" overclaim with an accurate "Tamper-evident" line plus
a prominent "Security model -- read this" callout. v0.6.x is Advisory
and tamper-evident; hook enforcement runs at the agent's own
privilege; a same-UID process can route around it; this is not an OS
sandbox; true tamper-resistance is Protected Mode in v0.7. Points at
SECURITY.md for vulnerability reporting, not public issues.
- README roadmap: the "root-owned approval daemon (v0.6.12)" bullet
became "Protected Mode (v0.7)". A daemon fed by a same-privilege hook
is tamper-evident logging, not an enforcement boundary; the boundary
is OS-level containment with the daemon as its control plane. The
v0.6.11 bullet's forward reference and the URL-allowlist bullet were
corrected to match.
- docs/v0.6.11/threat-model.md: dated banner marking the forward-looking
v0.6.12-daemon claims as superseded. Statements about v0.6.11's actual
behaviour and residuals are unchanged.
- docs/v0.6.11/{migration,index}.md: `patchwork init --upgrade` never
existed. The guide told upgraders to run it to install the admin-CLI
deny rule, so anyone who followed it never got the rule and has the
in-CLI TTY check as their only gate, which a PTY wrapper passes. Both
pages now give the manual policy edit and say to verify with
`patchwork policy show`.
- .github/workflows/ci.yml: `pnpm/action-setup@v4.1` is not a real tag,
so every CI run since 2026-05-12 failed at "Set up job". Pinned to
@v4 (which publish.yml already used, hence releases still worked) and
moved `pnpm lint` into its own non-blocking job so the pre-existing
biome backlog stops masking real build and test failures.
- README test counts refreshed to 1515.
- .changeset: patch bump across the fixed group.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The v0.6.12 README callout said Patchwork is Advisory and tamper-evident,
while the documentation site still told visitors the opposite. Shipping
both would have had the release note publicly contradicted by the
project's own homepage.
Corrected, 25 edits across 16 files:
- docs/index.md: the homepage feature card claimed a 5-layer architecture
'makes it impossible for the AI to disable its own monitoring'. It does
not, and that was the single most visible overclaim in the project.
- docs/security/threat-model.md: the AI-agent-tampering section asserted
'the agent cannot access or modify the hook configuration, the audit
log, or the policy files'. The hooks run at the agent's own UID, so it
can modify its own user-level hook config, and it can act entirely
outside the hooks' view via grandchild processes, package lifecycle
scripts or 'python -c'. Rewritten to state what is actually protected
(root-owned system policy, relay copy, chain detection) and what is
not, plus the same Advisory banner the README now carries.
- docs/concepts/tamper-proof-layers.md: retitled 'Tamper-Evident Layers'.
The URL is deliberately unchanged so inbound links keep working, with a
note explaining the rename.
- docs/concepts/compliance.md: the EU AI Act Art. 15(1) row claimed
'tamper-proof logging'. A compliance mapping should be the most
conservative claim in the repo, not the loosest.
- docs/team-mode-architecture.md: 'root-owned and tamper-proof, a
malicious user cannot modify it' now also states the two things it does
not cover: omitted events, and fabricated events submitted over the
socket by a same-UID process.
- Remaining prose, headings, nav label and link text moved from
tamper-proof to tamper-evident across guide, guides/team-mode,
security/architecture, concepts/how-it-works, concepts/seals-and-
witnesses, getting-started/{installation,quickstart,configuration},
.vitepress/config.mts and README.
- packages/cli/src/commands/setup.ts: four user-facing strings, including
the install prompt, described the relay as tamper-proof.
Site builds clean. Full suite still 1515 passing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Ran the repo's own verification gate (`pnpm test:log:write`) over the v0.6.12 branch. Overall PASS, 1404/1404 across 299 suites. Previous entry was 2026-04-28, so the log had not been refreshed across the whole v0.6.11 cycle. Note on the two test counts, since they look contradictory: `test:log` covers core + agents + cli only (1404), while `pnpm test` adds team (99) and web (12) for 1515. The README quotes the all-packages figure, which is consistent with the 1509 it quoted at v0.6.11, plus the six new R6-002 cases. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ak docs build Two problems found by actually walking the v0.6.11 upgrade path on a real hardened install rather than reading it. 1. The documented remediation was impossible. The system-level install flags /Library/Patchwork/policy.yml immutable (chflags schg on macOS, chattr +i on Linux, attrib +R on Windows), so writing it fails as root with 'Operation not permitted' — not a permissions message, and nothing in the docs mentioned the flag despite the README advertising it as a feature. So between the non-existent 'init --upgrade' flag and this, there was NO working documented way to install the R6 admin-CLI deny rule on a hardened install. The migration guide now gives the three-step sequence, insists the flag is restored afterwards (leaving it cleared silently downgrades the install), and notes that 'policy validate' reports on the file you point it at, so a failed write followed by a successful validate is not evidence the edit landed. An installation.md warning cross-references it. It also records that a policy change needs no relay restart, since the hook loads policy per invocation — worth saying, because an unnecessary restart drops any event in flight. 2. docs/TEST_LOG.md broke the docs build, which I introduced earlier today by regenerating it. VitePress compiles every .md under docs/ through the Vue compiler, and the new R6-002 test names contain angle-bracket placeholders that read as unclosed HTML. The build then fails with 'Element is missing end tag' pointing at an unrelated line 67 characters long, which is why it was not obvious. TEST_LOG is a generated record and is not linked from the nav or sidebar, so it is now srcExcluded. That also stops any future 'test:log:write' from breaking the docs deploy when the corpus gains such a name. This would have failed the Deploy Docs workflow on push to main. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ands The migration guide now tells people to install this rule, so it should also tell them its one sharp edge. The rule is a regex over the raw command string and cannot distinguish an invocation from the phrase appearing in text, so it also denies writing documentation about the admin CLI, commit messages describing it, greps searching for it, and test fixtures containing it. Found the hard way: within minutes of installing the rule on a real machine, its first block was a command writing a code comment that merely quoted the verb phrase as a placeholder example. It fails closed, which is the safe direction, and the in-process isAdminCliInvocation check is argv-aware and unaffected. But it is a genuine nuisance, worst for anyone working on Patchwork itself, so it belongs in the guide rather than being discovered. The note gives the workaround and says a proper fix will evaluate policy against parsed argv rather than text. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both items are filesystem tamper-evidence hardening on the taint store, which is the state the sink classifier reads to decide whether to deny. F2 (R1-011) — durable write in writeTaintSnapshot. tmp + rename already gave atomicity but not durability. Without fsync, a crash or power loss between the write and the OS flushing its page cache can leave the snapshot absent or truncated, and a reader that then finds an older-but-still-valid snapshot silently rolls taint BACKWARDS. That is the dangerous direction: taint that was recorded disappears and a sink that should deny allows. Now opens the temp file by descriptor, writes, fsyncs, then renames, then fsyncs the parent directory so the rename itself survives. The directory fsync is best-effort and wrapped, because opening a directory throws on Windows; the file fsync is the load-bearing half, and a lost rename leaves the previous snapshot intact, which the reader already handles. F3 — refuse to follow symlinks on taint-store paths. Mirrors the existing installer.ts defence for .claude/ and settings.json. An agent that can create a symlink in its own home directory could otherwise redirect the taint store: a symlink AT the snapshot path makes readFileSync return an attacker-chosen file, so the agent picks the taint state the classifier sees, and a symlink at the taint DIRECTORY redirects every snapshot, pending marker and lockfile at once, plus where the temp file lands on write. The two sides deliberately differ. Read fails closed by returning null, which commit 8 collapses to 'all kinds active', the same path every other form of doubt takes. Write throws, because the PostToolUse caller already wraps this in try/catch and a loud storage failure is better than silently writing taint state somewhere the agent chose. symlinkState() distinguishes 'absent' from 'could not determine'. An indeterminate answer about a security-relevant path is treated as unsafe, not safe. lstatSync is used rather than existsSync because existsSync follows symlinks, so a dangling symlink would report absent and skip the check entirely. Only the snapshot path and its immediate parent are checked, deliberately: walking every ancestor would reject legitimate installs, since macOS reaches /var through a symlink. Tests: 8 new, full suite 1515 -> 1523. - 5 in taint-store.test.ts for F3. Each uses VALID content behind the symlink, because the question is not 'is this snapshot well-formed' but 'is this the file we wrote'. Includes assertions that the decoy target is untouched, so the refusal demonstrably precedes any write, and a negative test that a normal snapshot still round-trips. - 3 in a new taint-store-durability.test.ts for F2. Separate file because the node:fs module mock is hoisted and would otherwise apply to the whole of taint-store.test.ts. fsyncSync is spied rather than replaced, so every other fs call behaves normally. Asserts at least two fsync calls, which confirms both the file and the directory are synced on this platform. Known gap against the tracker's F3 acceptance criteria: the criteria also ask for an audit event on each refusal. taint-store.ts has no emit mechanism and adding one means plumbing through adapter.ts, which is caller-side work rather than part of this fix. The refusal itself, which is the security property, is complete. The event is not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`patchwork relay verify` reported `Integrity: PASS` whenever every line in the relay log parsed as JSON. That is a real check, but only on the lines that ARE there. It said nothing about whether the relay is still receiving, so a relay that stopped accepting events months ago reports PASS forever. Silence was indistinguishable from health. That is not hypothetical. In v0.6.10 the relay socket was tightened from 0777 to 0660 while its group stayed wheel; hook processes run as the user, typically staff, so every delivery returned connect EACCES while verify kept reporting PASS for days. New `assessRelayHealth()` in @patchwork/core separates the three questions that were being conflated: 1. Is what we have intact? corrupt line count 2. Is the daemon still alive? heartbeat recency 3. Is delivery working? the divergence marker Only the first was previously checked, and the divergence marker was already being written and already surfaced by `relay status` — verify simply ignored it. It is a pure function over already-read inputs, so it is testable without a daemon, a socket or root. Three statuses: pass, stale (what we hold may be incomplete) and fail (what we hold is damaged). fail outranks stale but both sets of reasons are kept, because an operator should see the stronger statement first without losing the weaker one. `relay verify` now also sets a process exit code, which it never did before: 0 pass, 1 damaged, 2 degraded. Previously it printed FAIL on corrupt lines and still exited 0, so any CI step or cron check piping it saw success. A missing relay log is also now exit 2 rather than a cheerful message, since 'never received anything' is not health. Adds --max-silence <minutes> (default 5, which is ten consecutive missed 30s heartbeats) and --json. The degraded output states the distinction explicitly, because it is the one users are most likely to get wrong: DEGRADED means the log may be incomplete, not that it was edited. Patchwork can prove the log was not altered. It cannot prove it is complete. Tests: 12 new in packages/core/tests/relay/health.test.ts, all clock-injected rather than wall-clock dependent. Covers the v0.6.10 regression shape directly (every line parses, deliveries failing, must not report pass), heartbeat staleness, caller-supplied thresholds, empty logs, unparseable heartbeat timestamps, corrupt lines, fail-outranks-stale precedence, blank-line handling, a zero-count divergence marker being healthy, and newest-heartbeat rather than last-line selection. Full suite 1523 -> 1535. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Covers F2, F3 and F4 at patch across the fixed group. The note calls out that upgraders may see relay verify flip from PASS to DEGRADED, and that this is the new check working rather than a new fault: the condition was already present and unreported. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
F4 as first written degraded the status whenever the divergence marker held any failures at all. The marker is cumulative and never self-clears, so every install that has ever restarted its relay would read DEGRADED forever. That is worse than the spurious PASS it replaced, because an alarm that is always on teaches people to dismiss it. Verified against this machine: 3,529 accumulated failures whose most recent entry is 48 days old, against a daemon that is currently healthy. The first recorded failure matches a daemon restart to the minute, which is the expected shape — restarting the relay removes its socket briefly, so any delivery in flight at that moment fails. 24 restarts over six months. A non-zero count therefore mostly records transient, long-past and largely unavoidable events. Divergence is now judged on the age of last_failure_at: - Outside the window: a NOTE, status stays PASS. The note still states the count, the age, and that those events are missing from the relay copy, so the information is not lost, just not alarmed on. - Inside the window, daemon alive: the sharp case, reported as 'the relay was reachable and still did not take the event'. That is a genuine fault, distinct from a restart or a sleeping host. - Inside the window, no fresh heartbeat: 'hooks cannot reach the relay'. - No usable timestamp: a note saying the age is unknown, rather than guessing current or historical. The report gains divergenceAgeMs and splits reasons (affect status) from notes (informational). New --divergence-window, default 60 minutes. Confirmed on the live relay: PASS, exit 0, with the historical note, where the previous version reported DEGRADED exit 2. Also corrects the module doc comment, which cited host sleep as the cause. That was wrong for a reason worth recording: if the host is asleep no hook is running either, so there is nothing to fail. Delivery failure needs an active agent AND an unreachable relay. Tests: 5 new (17 total in health.test.ts), including a regression guard built from the real measured shape — 3,529 failures 48 days old plus a healthy daemon must read PASS with a note. Suite 1535 -> 1540. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three style/useTemplate violations introduced by the F2/F3 work. The remaining named-rule hits in these files (5 noNonNullAssertion, 2 useTemplate) are all on pre-existing lines and belong to X2, the repo-wide biome backlog, not here. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
JonoGitty
changed the title
JonoGitty
marked this pull request as draft
October 6, 2026 10:13
JonoGitty
marked this pull request as ready for review
October 6, 2026 10:22
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.
v0.6.13: filesystem tamper-evidence hardening + relay liveness
Three items from the hardening ladder, all touching state the sink classifier depends on.
F2 — durable taint snapshot writes (R1-011)
writeTaintSnapshotused tmp + rename, which gives atomicity but not durability. Withoutfsync, a crash or power loss between the write and the OS flushing its page cache can leave the snapshot absent or truncated, and a reader that then finds an older-but-still-valid snapshot silently rolls taint backwards. That is the dangerous direction: taint that was recorded disappears, and a sink that should deny allows.The temp file is now fsynced before the rename, and the parent directory afterwards so the rename itself survives. The directory fsync is best-effort and wrapped, because opening a directory throws on Windows; the file fsync is the load-bearing half, and a lost rename leaves the previous snapshot intact, which the reader already handles.
F3 — symlink refusal on taint-store paths
Mirrors the existing
patchwork initdefence for.claude/andsettings.json. An agent able to create a symlink in its own home directory could otherwise redirect the taint store: a symlink at the snapshot path makes the reader return an attacker-chosen file, letting the agent select the taint state the classifier sees, and a symlink at the taint directory redirects every snapshot, pending marker and lockfile at once, plus where the temp file lands on write.Reads fail closed (null, which the composer collapses to "all kinds active"); writes throw loudly, because silently writing taint state somewhere the agent chose is worse than a reported storage failure.
symlinkState()distinguishes absent from could not determine and treats the latter as unsafe. Only the snapshot path and its immediate parent are checked, deliberately — walking every ancestor would reject legitimate macOS installs, since/varis itself a symlink.Known gap: the tracker's acceptance criteria also ask for an audit event on each refusal.
taint-store.tshas no emit mechanism and adding one is caller-side work inadapter.ts. The refusal, which is the security property, is complete. The event is not.F4 — relay liveness and delivery checks
patchwork relay verifyreportedIntegrity: PASSwhenever every line in the relay log parsed as JSON. That is a real check, but only on the lines that are there. It said nothing about whether the relay was still receiving, so a relay that stopped accepting events months ago reported PASS indefinitely.Not hypothetical: in v0.6.10 the socket was tightened from 0777 to 0660 while its group stayed
wheel, so every delivery returnedconnect EACCESwhile verify reported PASS for days.New
assessRelayHealth()in core separates the three questions that were conflated — is what we have intact (corrupt lines), is the daemon alive (heartbeat recency), is delivery working (the divergence marker, which was already being recorded and which verify simply ignored). It is a pure clock-injected function, testable without a daemon, a socket or root.Divergence is judged on recency, not cumulative count. The marker never self-clears, so a raw
count > 0test would degrade forever after a single historical failure — measured on one real machine: 3,529 accumulated failures whose most recent entry was 48 days old, against a completely healthy daemon. An alarm that is always on teaches people to dismiss it. Historical divergence is now a note that leaves the status PASS; only recent divergence degrades; and "the daemon was alive and still did not take the event" is reported as a distinctly sharper fault than "hooks cannot reach the relay".Behaviour change worth noting in release notes:
relay verifynow sets a process exit code (0 pass, 1 damaged, 2 degraded). It previously printedFAILon corrupt lines and still exited 0, so any CI step or cron check piping it saw success regardless. Anyone with it in a pipeline may see it start failing — that is the fix working, not a new fault.New options:
--max-silence <minutes>(default 5),--divergence-window <minutes>(default 60),--json.Verification
node:fsmock is hoisted, 17 for F4 including a regression guard built from the real measured 3,529-failure shape