fix(farm): stop a plan secret persisting in the run's permanent receipt - #487
Merged
Conversation
`writeReport` serialized `plan.meta` verbatim, and the run-scoped change made
that permanent rather than transient: `.farm/farm-report.json` used to be
clobbered by the next run, while `.farm/runs/<runId>/` accumulates indefinitely
with no prune path. `meta.setup`, `setupEachAttempt` and `setupInputs` are raw
shell-command arrays and `apiBaseUrl` is a URL, so a token in a setup command
was a perfectly legal plan that landed on disk forever.
REDACTION, NOT AN ALLOWLIST - the issue's premise was wrong in a way that
changes the fix. It states there is "no control, schema or runtime, on what meta
carries into the report". There is: `checkPlanObject` is a CLOSED object check
and plan-contract.test.ts already pins that an unknown `meta` property throws at
parse. The proposed sink allowlist would have been a second copy of an existing
control and caught nothing. The exposure is the opposite shape - the ALLOWED
fields are the dangerous ones - so the free-text fields are redacted instead.
WHAT THE ADVERSARIAL PASS BROKE, all reproduced end-to-end before being fixed:
* THE MARKDOWN RECEIPT WAS NOT REDACTED AT ALL. The first cut wrapped the JSON
sink and left `farm-report.md` reading raw `plan.meta` three lines later. A
run with meta.name = "nightly sk-ant-MDPROOF-9999" produced a redacted JSON
receipt and a Markdown sibling whose H1 carried the token verbatim, in the
same never-pruned directory, on a fully green run. The projection is now
computed once and both sinks read it.
* AC-2 MISSED THE STRINGS IT CITED. `git diff failed: <raw git stderr>`,
`diffs directory unavailable: <fs error>` and `patch write failed:
<msgOf(e)>` all go to `noteUnavailableDiff` - a different function the first
cut never touched - and land in BOTH receipts. Reproduced with a blocked
diffs directory inside a repo path carrying a token-shaped segment.
* THE REDACTOR MISSED SHAPES A PLAN ACTUALLY CARRIES. Measured misses:
`glpat-`, `sk-proj-` (the `api[_-]?key` keyword does not fire on
`OPENAI_KEY`), basic auth in a clone URL, and a bare `Bearer eyJ...`. Three
are closed in redactor.ts. The fourth is deliberately NOT: a `curl -u
user:pass` rule collides with `docker run -u 1000:1000`, so the gap is
recorded in the code rather than papered over.
* THE MODE TEST WAS VACUOUS ON WINDOWS. NTFS does not honour POSIX mode bits,
so chmod(0o600) reads back 0o666 and the assertion passed against the
UNFIXED implementation on the maintainer's own platform. It now declares
that instead of proving nothing, and keeps its teeth on POSIX CI.
Also fixed, from the original four ACs: `atomicWriteFile` preserves an existing
destination's mode instead of resetting it to the umask default, opens its temp
`O_EXCL`, and cleans up when the WRITE fails rather than only on rename - the
test that claimed to cover that path only exercised rename. The write-failure
path is exercised through an injectable `open` seam, because Node substitutes
U+FFFD for an unencodable payload rather than throwing, so there is no "bad
data" that forces the failure.
Verified by mutation, after the fixes: identity projector -> 3 failed; Markdown
reading raw meta -> 1 failed; noteUnavailableDiff un-redacted -> 1 failed;
noteArtifactError un-redacted -> 1 failed. Every half of the fix has a test that
notices its absence.
14 new tests. Full farm suite 323 passed / 1 skipped across 5 files; typecheck
clean; the shared secret-detection corpus still agrees across redactor.ts and
_hooklib (test_hooklib 19 + 80 OK); farm.js rebuilt.
NOT CLOSED, stated rather than implied: per-task `.patch` files in the same run
directory are written raw and stay that way - a redacted patch no longer
applies. And redaction reduces the blast radius of a secret in a plan; it does
not make `plan.meta` a safe place to put one.
HOSTED SECRET SCAN. The three credential SHAPES the redactor-reach tests assert
on are, by design, shaped like real credentials - that is what makes the
assertions mean anything - so gitleaks reports them. They are waived in
.gitleaks.toml by exact anchored VALUE, per that file's own contract: a
description naming the file and why the literal is benign, `\A<value>\z`, and no
regex metacharacter in the body, so a waiver can never cover anything but the
literal it names. The values were MEASURED against the pinned image with
--report-format json, not guessed from the source text, which is the failure
mode that config's header documents at length. Verified: tracked findings 3 -> 0.
The Bearer fixture is deliberately NOT a realistic JWT. A JWT contains dots, and
a dot is a banned metacharacter in an anchored waiver body, so a realistic one
could not be waived narrowly. `LEAKPAYLOADNOTAREALJWT123` still exercises the
redactor's `Bearer\s+[A-Za-z0-9_-]{10,}` rule and still trips gitleaks'
curl-auth-header, so no coverage is lost.
This branch is a single clean commit rather than the two it was developed as:
the first cut committed the realistic JWT, and the commit-range scan correctly
reports a secret introduced by a commit even when a later commit removes it.
Rewriting history with a force-push is prohibited here, so the change was
rebuilt on a fresh branch instead - which is the honest fix, since that literal
should never have been committed in that shape.
Closes #439.
SUaDtL
added a commit
that referenced
this pull request
Jul 26, 2026
MAIN WAS SHIPPING STALE BUNDLES. #487 widened the shared outbound redactor (GitLab PATs, OpenAI project keys, basic auth in a clone URL, bearer tokens) and rebuilt `farm.js`. It did not rebuild the ca-pi extensions - and those EMBED the same redactor, because `tools/src/redaction.ts` imports `../../../ca/tools/redactor.ts`. So a Pi install would have kept redacting on the narrower pattern set, silently, while the repo believed the widening had shipped. The parent's baked child fingerprint is regenerated with them; verified locally that the sha256 of the rebuilt child appears in the rebuilt parent, and it is the same value CI computed (817a2a7a4789f0ff...). AND THE PATH FILTER THAT LET IT THROUGH. The `ca-pi` filter watched `plugins/ca-pi/**` and `core/**`, so a change to a file the bundles embed but that lives under `plugins/ca/` triggered nothing. Main therefore stayed red-free until the next ca-pi-touching PR happened to run the job and fail on staleness - which is exactly how this was found, on #490. A cross-plugin import needs a cross-plugin filter, so the filter now watches `plugins/ca/tools/redactor.ts` directly. This is a prerequisite for both PRs currently in flight: #490 and #491 each touch ca-pi paths, so both were failing the bundle-staleness gate on a defect neither introduced. pi-security 12/12 pass; test_pi_package 24 OK; ci-impact 41 OK; docs contract clean. ca-pi advances to 0.1.27. Co-authored-by: SUaDtL <SUaDtL@users.noreply.github.com>
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.
writeReportserializedplan.metaverbatim, and the run-scoped change made that permanent rather than transient:.farm/farm-report.jsonused to be clobbered by the next run, while.farm/runs/<runId>/accumulates indefinitely with no prune path.meta.setup,setupEachAttemptandsetupInputsare raw shell-command arrays andapiBaseUrlis a URL — so a token in a setup command was a perfectly legal plan that landed on disk forever.Redaction, not an allowlist — the issue's premise was wrong
The issue states there is "no control — schema or runtime — on what
metacarries into the report." There is.checkPlanObjectis a closed-object check, andplan-contract.test.ts:139already pins that an unknownmetaproperty throws at parse.The sink allowlist the issue proposed (
{name, repo, model, apiBaseUrl, setup}) would have been a second copy of an existing control and caught nothing. The exposure is the opposite shape: the allowed fields are the dangerous ones. So the free-text fields are redacted instead.What the adversarial pass broke — all reproduced end-to-end
I ran a four-lens refutation workflow against my own first cut. It found three real defects and one hollow test.
🔴 The Markdown receipt was not redacted at all. The first cut wrapped the JSON sink and left
farm-report.mdreading rawplan.metathree lines later. A run withmeta.name = "nightly sk-ant-MDPROOF-9999"produced a redacted JSON receipt and a Markdown sibling whose H1 carried the token verbatim, in the same never-pruned directory, on a fully green run. The projection is now computed once and both sinks read it.🟠 AC-2 missed the strings it cited.
git diff failed: <raw git stderr>,diffs directory unavailable: <fs error>andpatch write failed: <msgOf(e)>all go tonoteUnavailableDiff— a different function the first cut never touched — and land in both receipts. Reproduced with a blocked diffs directory inside a repo path carrying a token-shaped segment.🟠 The redactor missed shapes a plan actually carries. Measured misses:
glpat-,sk-proj-(theapi[_-]?keykeyword doesn't fire onOPENAI_KEY), basic auth in a clone URL, and a bareBearer eyJ.... Three are closed. The fourth is deliberately not: acurl -u user:passrule collides withdocker run -u 1000:1000, so the gap is recorded in the code rather than papered over.🟡 The mode test was vacuous on Windows. NTFS doesn't honour POSIX mode bits, so
chmod(0o600)reads back0o666and the assertion passed against the unfixed implementation on the maintainer's own platform. It now declares that instead of proving nothing, and keeps its teeth on POSIX CI.The original four ACs
atomicWriteFilepreserves an existing destination's mode instead of resetting it to the umask default, opens its tempO_EXCL, and cleans up when the write fails rather than only on rename — the test that claimed to cover that path only exercised rename. The write failure is forced through an injectableopenseam, because Node substitutes U+FFFD for an unencodable payload rather than throwing, so there is no "bad data" that triggers it.Verification
Mutation-tested after the fixes, since a test that can't fail is worse than none:
plan.metanoteUnavailableDiffun-redactednoteArtifactErrorun-redacted14 new tests. Full farm suite 323 passed / 1 skipped across 5 files; typecheck clean; the shared secret-detection corpus still agrees across
redactor.tsand_hooklib(test_hooklib19 + 80 OK);farm.jsrebuilt.Not closed, stated rather than implied
Per-task
.patchfiles in the same run directory are written raw and stay that way — a redacted patch no longer applies. And redaction reduces the blast radius of a secret in a plan; it does not makeplan.metaa safe place to put one.Hosted secret scan, and why this is a fresh branch
The three credential shapes the redactor-reach tests assert on are, by design, shaped like real credentials — that is what makes the assertions mean anything — so gitleaks reports them. They are waived in
.gitleaks.tomlby exact anchored value, per that file's own contract: a description naming the file and why the literal is benign,\A<value>\z, and no regex metacharacter in the body, so a waiver can never cover anything but the literal it names. The values were measured against the pinned image with--report-format json, not guessed from the source text — which is the failure mode that config's header documents at length. Verified: tracked findings 3 → 0.The Bearer fixture is deliberately not a realistic JWT. A JWT contains dots, and a dot is a banned metacharacter in an anchored waiver body, so a realistic one could not be waived narrowly.
LEAKPAYLOADNOTAREALJWT123still exercises the redactor'sBearer\s+[A-Za-z0-9_-]{10,}rule and still trips gitleaks'curl-auth-header, so no coverage is lost.This is a single clean commit on a fresh branch, replacing #483. The first cut committed the realistic JWT, and the commit-range scan correctly reports a secret introduced by a commit even when a later commit removes it — the tree was clean, the range was not. Force-pushing to rewrite that history is prohibited here, so the change was rebuilt instead. That is the honest fix: the literal should never have been committed in that shape.
Closes #439.