Skip to content

fix(farm): stop a plan secret persisting in the run's permanent receipt#483

Closed
SUaDtL wants to merge 2 commits into
mainfrom
fix/439-farm-receipt-redaction
Closed

fix(farm): stop a plan secret persisting in the run's permanent receipt#483
SUaDtL wants to merge 2 commits into
mainfrom
fix/439-farm-receipt-redaction

Conversation

@SUaDtL

@SUaDtL SUaDtL commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator

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

The issue 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:139 already pins that an unknown meta property 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.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 doesn't fire on OPENAI_KEY), basic auth in a clone URL, and a bare Bearer eyJ.... Three are closed. 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 doesn't 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.

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 is forced through an injectable open seam, 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:

mutation result
identity projector 3 failed
Markdown reads raw plan.meta 1 failed
noteUnavailableDiff un-redacted 1 failed
noteArtifactError un-redacted 1 failed

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.

Closes #439.

SUaDtL added 2 commits July 25, 2026 21:18
`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.

Closes #439.
…scan

The three credential SHAPES the redactor-reach tests assert on are, by design,
shaped like real credentials - that is what makes the assertions meaningful -
so gitleaks reported them and the secret-scan gate went red.

Waived by exact anchored VALUE, per .gitleaks.toml's own contract: 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, not guessed: the pinned gitleaks image was run locally
with --report-format json to read back the exact `Secret` field for each
finding, because a waiver derived from the source text rather than the reported
secret is the failure mode that config's header documents at length.

One fixture changed to make it waivable. The Bearer token was written as a
realistic JWT (`eyJ...LEAKPAYLOAD.sig`), and a JWT contains dots - a banned
metacharacter in an anchored waiver body. It is now a dotless
`LEAKPAYLOADNOTAREALJWT123`, which still exercises the redactor's
`Bearer\s+[A-Za-z0-9_-]{10,}` rule and still trips gitleaks' curl-auth-header,
so nothing about the coverage weakens - only the shape of the literal changes.

Verified against the pinned image after the waiver: tracked findings 3 -> 0. The
three that remain are untracked, git-ignored local files (.claude/settings.local
.json, .codex/config.toml, a .pyc) that a CI checkout never contains. ci-impact
41 tests OK, #439 suite 14 OK, farm.js rebuilt.
@SUaDtL

SUaDtL commented Jul 26, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #487, which is the identical change as a single clean commit.

The tree scan was green, but the commit-range scan on this branch was not: the first cut committed a realistic JWT-shaped fixture (eyJ...LEAKPAYLOAD.sig), and that scan correctly reports a secret introduced by a commit even when a later commit removes it. It cannot be waived narrowly either — a JWT contains dots, and a dot is a banned metacharacter in an anchored .gitleaks.toml waiver body.

Force-pushing to rewrite the history is prohibited here, so the change was rebuilt on a fresh branch instead. That is the honest fix: the literal should never have been committed in that shape.

@SUaDtL SUaDtL closed this Jul 26, 2026
@SUaDtL
SUaDtL deleted the fix/439-farm-receipt-redaction branch July 26, 2026 03:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

farm run-scoped receipts: allowlist plan.meta at the sink, redact artifact errors, preserve report mode

1 participant