From ff2ab388871f813503a92cff1878f7f4b7f6e235 Mon Sep 17 00:00:00 2001 From: SUaDtL Date: Sat, 25 Jul 2026 23:10:28 -0400 Subject: [PATCH] fix(farm): stop a plan secret persisting in the run's permanent receipt `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//` 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: `, `diffs directory unavailable: ` and `patch write failed: ` 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\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. --- .gitleaks.toml | 23 +++ CHANGELOG.md | 45 ++--- plugins/ca/tools/farm.js | 62 ++++++- plugins/ca/tools/farm.ts | 110 +++++++++++-- plugins/ca/tools/farm.unit.test.ts | 253 ++++++++++++++++++++++++++++- plugins/ca/tools/redactor.ts | 32 +++- 6 files changed, 484 insertions(+), 41 deletions(-) diff --git a/.gitleaks.toml b/.gitleaks.toml index 963f9940..2d2b8b42 100644 --- a/.gitleaks.toml +++ b/.gitleaks.toml @@ -153,3 +153,26 @@ high-entropy string with no trigger word, used to assert safeDiagnostic never fragments the audit log. Not a credential for any system. """ regexes = ['''\AZx9qP2mK7vN4wR8tY1uJ6hL3sD5fA0cE\z'''] + +[[allowlists]] +description = """ +The three credential SHAPES the #439 redactor-reach tests assert on, in +plugins/ca/tools/farm.unit.test.ts. An adversarial pass measured four real +shapes reaching a permanently retained farm receipt un-redacted; three are now +matched by the outbound redactor and one (curl's `-u user:pass`) is deliberately +left alone because it collides with `docker run -u 1000:1000`. Each fixture +exists to prove that boundary in a specific direction, so deleting them would +delete the coverage: + sk-proj-LEAK1234567890 - must BE redacted (the api_key keyword misses + OPENAI_KEY, which is why the prefix rule exists) + LEAKPAYLOADNOTAREALJWT123 - must BE redacted, as `Authorization: Bearer ` + ci-bot:Hunter2 - must NOT be redacted, pinning the known gap +None is a credential for any system. Each waiver is the exact value gitleaks +reports for that fixture, measured against the pinned image with +--report-format json, so it can never cover anything but the literal it names. +""" +regexes = [ + '''\Ask-proj-LEAK1234567890\z''', + '''\ALEAKPAYLOADNOTAREALJWT123\z''', + '''\Aci-bot:Hunter2\z''', +] diff --git a/CHANGELOG.md b/CHANGELOG.md index 464714bb..6d8f3475 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -62,25 +62,32 @@ predate the plugin rewrite and are grouped by date. ### Fixed -- site/VOICE.md's punctuation rule is finally enforced. It has banned the - em-dash as a sentence separator in site prose since 2026-07-02, and nothing - checked it: the anti-slop detector's scope covered repo-root docs and - `docs/**` and had never included `site/`. Sixteen of the thirty-six authored - pages violated the rule for twenty-four days while reviewers cited it at - contributors. A rule with no gate is worse than no rule, because people learn - that the style docs are advisory. 118 offending lines across 15 pages are - restructured with a period, a comma, a colon, or parentheses, and - `check_site_voice.py` now blocks the site publish on a new one (issue #338). -- The detector no longer flags a definition-list dash (`- **term** - meaning`). - The rule is about *sentence* separators, and VOICE.md's own Terminology - anchors are written in that exact form, so enforcing it as written would have - made the gate contradict the guide. A real separator later on the same line is - still caught: only the definition dash is dropped, never the term before it. - The first cut of that exemption blanked the whole lead-in and silently - suppressed a genuine finding whenever inline code followed it, which an - adversarial reader caught before it shipped. -- The detector's scope predicate recognises `.mdx` as prose. It keyed on `.md` - alone, so two authored site pages were invisible to a rule about writing. +- A secret in a farm plan no longer persists in the run's permanent receipt. + `plan.meta` was serialized verbatim into `.farm/runs//farm-report.json` + and its Markdown sibling, and the run-scoped change made that permanent: the + old `.farm/farm-report.json` was clobbered by the next run, while + `.farm/runs/` accumulates 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. Every free-text `meta` field is now redacted + at the sink, once, for both receipts. Note this is redaction and not an + allowlist: `checkPlanObject` is already a closed-object check that rejects + unknown `meta` keys at parse, so the allowlist the issue proposed would have + duplicated an existing control and caught nothing — the *allowed* fields were + the exposed ones (issue #439). +- The artifacts block and the diff-evidence reasons now pass through the file's + own redaction chokepoint, so raw `git` stderr and filesystem error text cannot + reach either receipt un-redacted. +- The outbound redactor recognises four credential shapes an adversarial run + proved were reaching a receipt intact: GitLab PATs (`glpat-`), OpenAI project + keys (`sk-proj-`), basic auth embedded in a clone URL, and bearer tokens. A + `curl -u user:pass` rule is deliberately *not* added — that shape collides + with `docker run -u 1000:1000`, and the gap is recorded rather than papered + over. +- `atomicWriteFile` preserves an existing destination's permission bits instead + of resetting them to the umask default, opens its temp file `O_EXCL`, and + cleans up when the *write* fails rather than only when the rename does. The + test that claimed to cover that path only exercised the rename. - The shipped `farm.js` bundle is now executed by the test suite, not merely regenerated and byte-compared. `includes/farm.md` tells operators to run `node /tools/farm.js`, but the integration launcher ran `farm.ts` diff --git a/plugins/ca/tools/farm.js b/plugins/ca/tools/farm.js index 8c9e9de5..cfde4628 100755 --- a/plugins/ca/tools/farm.js +++ b/plugins/ca/tools/farm.js @@ -188,7 +188,33 @@ async function readWorktreeFile(wt, relPath) { } // redactor.ts -var SECRET_LINE = /(api[_-]?key|token|secret|password|BEGIN.*PRIVATE|sk-ant|AKIA[0-9A-Z]{16}|ghp_[A-Za-z0-9]{36})/i; +var SECRET_LINE = new RegExp( + [ + "api[_-]?key", + "token", + "secret", + "password", + "BEGIN.*PRIVATE", + "sk-ant", + "sk-proj-[A-Za-z0-9_-]{8}", + "AKIA[0-9A-Z]{16}", + "ghp_[A-Za-z0-9]{36}", + "glpat-[A-Za-z0-9_-]{8}", + // basic auth in a URL: scheme://user:secret@host - the colon-and-at shape, + // not any "@", so `https://example.com/@scope/pkg` is untouched. + // NOTE the doubled backslashes: these are JS STRING literals, and "\\s" in + // a string is a literal "s". The first cut of this shipped `[^/\s:@]`, + // which silently became `[^/s:@]` and only matched by luck. + "https?://[^/\\s:@]+:[^/\\s@]+@", + // a bearer credential, which is dot-segmented base64url in practice + "Bearer\\s+[A-Za-z0-9_-]{10,}" + // DELIBERATELY NOT ADDED: a `-u user:pass` rule for curl. `-u 1000:1000` + // is an everyday docker/podman uid:gid pair, so the shape collides with + // benign input. Broad is the safe direction for this redactor, but not at + // the cost of firing on ordinary container arguments. + ].join("|"), + "i" +); var PEM_BEGIN = /^-----BEGIN .*-----\s*$/; var PEM_END = /^-----END .*-----\s*$/; var REDACTION_MARKER = "[REDACTED \u2014 secret-pattern match removed before transmission]"; @@ -1069,18 +1095,25 @@ async function renameWithRetry(from, to, attempts = 4) { } } } -async function atomicWriteFile(dest, data) { +async function atomicWriteFile(dest, data, deps = {}) { const tmp = path3.join( path3.dirname(dest), `.${path3.basename(dest)}.${process.pid}-${randomBytes(4).toString("hex")}.tmp` ); - const fh = await open2(tmp, "w"); + const priorMode = await stat(dest).then((s) => s.mode & 511).catch(() => void 0); + const fh = await (deps.open ?? open2)(tmp, "wx"); try { await fh.writeFile(data, "utf8"); await fh.sync(); - } finally { - await fh.close(); + if (priorMode !== void 0) await fh.chmod(priorMode); + } catch (e) { + await fh.close().catch(() => { + }); + await rm(tmp, { force: true }).catch(() => { + }); + throw e; } + await fh.close(); try { await renameWithRetry(tmp, dest); } catch (e) { @@ -1097,14 +1130,20 @@ function newRunArtifactHealth() { cleanup: { failures: [] } }; } +function projectPlanMetaForReport(meta) { + const scrub = (v) => typeof v === "string" ? redactSecrets(v) : Array.isArray(v) ? v.map(scrub) : v; + return Object.fromEntries(Object.entries(meta).map(([k, v]) => [k, scrub(v)])); +} var MAX_RECORDED_ARTIFACT_ERRORS = 10; function noteArtifactError(bucket, message) { - if (bucket.length < MAX_RECORDED_ARTIFACT_ERRORS) bucket.push(message); + const safe = redactSecrets(message); + if (bucket.length < MAX_RECORDED_ARTIFACT_ERRORS) bucket.push(safe); else if (bucket.length === MAX_RECORDED_ARTIFACT_ERRORS) bucket.push("(further errors suppressed)"); } function noteUnavailableDiff(diffs, id, reason) { + const safe = redactSecrets(reason); diffs.unavailableTotal += 1; - if (diffs.unavailable.length < MAX_RECORDED_ARTIFACT_ERRORS) diffs.unavailable.push({ id, reason }); + if (diffs.unavailable.length < MAX_RECORDED_ARTIFACT_ERRORS) diffs.unavailable.push({ id, reason: safe }); else if (diffs.unavailable.length === MAX_RECORDED_ARTIFACT_ERRORS) diffs.unavailable.push({ id: "(suppressed)", @@ -2053,13 +2092,14 @@ async function writeReport(plan, results, blocked, aborted, runId, health) { // report predates the check" without inferring it from an absent key. cleanup: { released: leaks.length === 0, failures: leaks } }; + const reportMeta = projectPlanMetaForReport(plan.meta); const json = JSON.stringify( - { run_id: runId, plan: plan.meta, aborted, tokens: { prompt: pTok, completion: cTok }, results, blocked, artifacts, ts: (/* @__PURE__ */ new Date()).toISOString() }, + { run_id: runId, plan: reportMeta, aborted, tokens: { prompt: pTok, completion: cTok }, results, blocked, artifacts, ts: (/* @__PURE__ */ new Date()).toISOString() }, null, 2 ); const md = [ - `# Farm report \u2014 ${plan.meta.name}`, + `# Farm report \u2014 ${reportMeta.name}`, ``, aborted ? `> **ABORTED by circuit breaker** \u2014 escalation rate exceeded threshold. ` : ``, @@ -2112,6 +2152,7 @@ if (_thisFile === _entryFile) { } export { DEFAULT_API_BASE_URL, + MAX_RECORDED_ARTIFACT_ERRORS, PLAN_SHAPE, SAFE_RUN_ID, SAFE_TASK_ID, @@ -2136,10 +2177,13 @@ export { makeEntitlementProbe, mintRunId, newRunArtifactHealth, + noteArtifactError, + noteUnavailableDiff, numEnv, parseChatCompletion, parseMutationHookOutput, parsePlan, + projectPlanMetaForReport, readSampling, redactSecrets, removeWorktreeVerified, diff --git a/plugins/ca/tools/farm.ts b/plugins/ca/tools/farm.ts index 712709c6..d79e8528 100644 --- a/plugins/ca/tools/farm.ts +++ b/plugins/ca/tools/farm.ts @@ -1176,18 +1176,48 @@ async function renameWithRetry(from: string, to: string, attempts = 4): Promise< // has no portable directory-fsync, and none at all on Windows), so a power loss // immediately after publication may still lose the rename. Do not read the // fh.sync() below as a promise that a published receipt survives a host crash. -export async function atomicWriteFile(dest: string, data: string): Promise { +/** Injectable file-open seam. Production passes nothing; a test supplies a + * handle whose write rejects, which is the only deterministic way to exercise + * the WRITE-path cleanup - Node silently substitutes U+FFFD for an unencodable + * payload rather than throwing, so there is no "bad data" that forces it. Same + * dependency-injection shape this file already uses for RunTaskDeps and the + * worker/gate seams (#439 AC-4). */ +export type AtomicWriteDeps = { open?: typeof open }; + +export async function atomicWriteFile( + dest: string, + data: string, + deps: AtomicWriteDeps = {}, +): Promise { const tmp = path.join( path.dirname(dest), `.${path.basename(dest)}.${process.pid}-${randomBytes(4).toString("hex")}.tmp`, ); - const fh = await open(tmp, "w"); + // #439 AC-3: the destination's mode, if it already exists. The old + // `writeFile(dest, ...)` opened O_TRUNC and PRESERVED the mode; creating a + // temp at the umask default (0644) and renaming over the destination lets the + // temp's mode win, so an operator who hardened a report to 0600 got it reset + // on every run. Read before the write so a mid-publish failure cannot lose it. + const priorMode = await stat(dest).then((s) => s.mode & 0o777).catch(() => undefined); + // "wx" (O_EXCL): the random suffix already makes the name unpredictable, but + // exclusive creation means the temp can never follow a pre-existing symlink + // planted at that path. Free, so there is no reason not to. + const fh = await (deps.open ?? open)(tmp, "wx"); try { await fh.writeFile(data, "utf8"); await fh.sync(); - } finally { - await fh.close(); + if (priorMode !== undefined) await fh.chmod(priorMode); + } catch (e) { + // #439 AC-4: cleanup used to be attached only to the RENAME. A failing + // write or sync (ENOSPC, EIO, an unencodable payload) left a + // partially-written temp file on disk holding the payload - and the test + // that claimed to cover this only exercised the rename path, so its title + // read broader than its coverage. + await fh.close().catch(() => {}); + await rm(tmp, { force: true }).catch(() => {}); + throw e; } + await fh.close(); try { await renameWithRetry(tmp, dest); } catch (e) { @@ -1243,11 +1273,54 @@ export function newRunArtifactHealth(): RunArtifactHealth { }; } +// #439 - what the run's PERMANENT receipt is allowed to carry. +// +// The run-scoped change did not create this exposure, it changed its shape. +// `.farm/farm-report.json` used to be clobbered by the next run; `.farm/runs/ +// /` now accumulates indefinitely with no prune path, so a secret that +// used to be destroyed within one run persists instead. +// +// NOT an allowlist, deliberately. The issue proposed projecting +// `{name, repo, model, apiBaseUrl, setup}` at the sink, but `checkPlanObject` +// is already a CLOSED object check and plan-contract.test.ts pins that an +// unknown `meta` property throws at parse. A sink allowlist would be a second +// copy of a control that already exists, and would have caught nothing. +// +// The exposure is the opposite shape: the ALLOWED fields are the dangerous +// ones. `setup`, `setupEachAttempt` and `setupInputs` are raw shell-command +// arrays and `apiBaseUrl` is a URL that can carry credentials, so a token in a +// setup command is a perfectly legal plan that parses cleanly and lands +// verbatim in the receipt. Redaction - not deletion: a receipt that silently +// dropped the setup it ran would be a worse receipt. +// +// WHAT THIS DOES NOT CLOSE, stated plainly rather than implied. `redactSecrets` +// is keyword- and prefix-driven, so a credential wearing none of its shapes +// still lands in the receipt - measured misses include `glpat-`, `sk-proj-`, +// basic-auth in a clone URL (`https://user:pass@host`), and a bare +// `Authorization: Bearer eyJ...`. Several of those are widened in redactor.ts +// alongside this change, but the class is open-ended: this reduces the blast +// radius of a secret in a plan, it does not make plan.meta a safe place to put +// one. The per-task `.patch` files in the same run directory are also written +// raw, deliberately - a patch that has been redacted no longer applies. +export function projectPlanMetaForReport>(meta: T): T { + const scrub = (v: unknown): unknown => + typeof v === "string" ? redactSecrets(v) : Array.isArray(v) ? v.map(scrub) : v; + return Object.fromEntries(Object.entries(meta).map(([k, v]) => [k, scrub(v)])) as T; +} + // Bound the recorded error list — a dead stream path fails once per settled // task, and the report should carry a diagnosis, not N copies of it. -const MAX_RECORDED_ARTIFACT_ERRORS = 10; -function noteArtifactError(bucket: string[], message: string) { - if (bucket.length < MAX_RECORDED_ARTIFACT_ERRORS) bucket.push(message); +export const MAX_RECORDED_ARTIFACT_ERRORS = 10; +// #439: redact HERE rather than at each of the five call sites. Every other +// report-bound free-text field in this file is redacted at construction; the +// artifacts block was the one new class of report content that skipped the +// chokepoint, and it carries raw git stderr (`git diff failed: `) and +// `msgOf(e)` strings straight into a permanently retained receipt. One sink, +// one rule - a sixth caller cannot forget. `redactSecrets` is documented +// idempotent, so a message that was already redacted upstream is unharmed. +export function noteArtifactError(bucket: string[], message: string) { + const safe = redactSecrets(message); + if (bucket.length < MAX_RECORDED_ARTIFACT_ERRORS) bucket.push(safe); else if (bucket.length === MAX_RECORDED_ARTIFACT_ERRORS) bucket.push("(further errors suppressed)"); } @@ -1255,9 +1328,16 @@ function noteArtifactError(bucket: string[], message: string) { // failure shape (one unusable diffs directory = one entry per task in the // plan). The running total is kept so the report states how many tasks are // actually affected even though it only lists the first few. -function noteUnavailableDiff(diffs: RunArtifactHealth["diffs"], id: string, reason: string) { +export function noteUnavailableDiff(diffs: RunArtifactHealth["diffs"], id: string, reason: string) { + // #439: redacted here for the same reason noteArtifactError is. This is the + // function that actually receives `git diff failed: `, + // `diffs directory unavailable: ` and `patch write failed: + // ` - the three strings the issue names. Those reasons reach BOTH + // receipts: artifacts.diffs.unavailable[].reason in the JSON, and the + // "Diff evidence" list in the Markdown. + const safe = redactSecrets(reason); diffs.unavailableTotal += 1; - if (diffs.unavailable.length < MAX_RECORDED_ARTIFACT_ERRORS) diffs.unavailable.push({ id, reason }); + if (diffs.unavailable.length < MAX_RECORDED_ARTIFACT_ERRORS) diffs.unavailable.push({ id, reason: safe }); else if (diffs.unavailable.length === MAX_RECORDED_ARTIFACT_ERRORS) diffs.unavailable.push({ id: "(suppressed)", @@ -2845,14 +2925,22 @@ async function writeReport( cleanup: { released: leaks.length === 0, failures: leaks }, }; + // One projection, two sinks. #439. + const reportMeta = projectPlanMetaForReport(plan.meta); + const json = JSON.stringify( - { run_id: runId, plan: plan.meta, aborted, tokens: { prompt: pTok, completion: cTok }, results, blocked, artifacts, ts: new Date().toISOString() }, + { run_id: runId, plan: reportMeta, aborted, tokens: { prompt: pTok, completion: cTok }, results, blocked, artifacts, ts: new Date().toISOString() }, null, 2, ); + // #439: BOTH receipts, not just the JSON. `farm-report.md` is written by the + // same tier-1 publish and mirrored to the latest pointer, so redacting the + // JSON alone left the one field it does redact leaking verbatim into the + // Markdown sibling. Projected once, above, and used by both sinks - a second + // call here would be a second place to forget. const md = [ - `# Farm report — ${plan.meta.name}`, + `# Farm report — ${reportMeta.name}`, ``, aborted ? `> **ABORTED by circuit breaker** — escalation rate exceeded threshold.\n` : ``, streamComplete ? `` : `> **Streaming rail incomplete** — ${health.stream.errors.length} write failure(s) on \`farm-results.jsonl\`; this report is authoritative for settled tasks.\n`, diff --git a/plugins/ca/tools/farm.unit.test.ts b/plugins/ca/tools/farm.unit.test.ts index 6269d944..06ac2e54 100644 --- a/plugins/ca/tools/farm.unit.test.ts +++ b/plugins/ca/tools/farm.unit.test.ts @@ -6,12 +6,13 @@ import { describe, it, expect, afterEach, beforeEach, vi } from "vitest"; import path from "node:path"; import { readFileSync } from "node:fs"; import { fileURLToPath } from "node:url"; -import { mkdtemp, writeFile as fsWriteFile, mkdir as fsMkdir, readFile as fsReadFile, rm as fsRm, symlink as fsSymlink, readdir as fsReaddir } from "node:fs/promises"; +import { mkdtemp, writeFile as fsWriteFile, mkdir as fsMkdir, readFile as fsReadFile, rm as fsRm, symlink as fsSymlink, readdir as fsReaddir, chmod as fsChmod, stat as fsStat, open as fsOpen } from "node:fs/promises"; import { tmpdir } from "node:os"; import { extractFileBlocks, extractLiterals, codeLineCount, validate, assertSecureBaseUrl, runTask, httpWorker, DEFAULT_API_BASE_URL, parseChatCompletion, checkDrift, screenEntitlements, makeEntitlementProbe, redactSecrets, run, runGate, mintRunId, parseMutationHookOutput, buildChatBody, readSampling, buildPrompt, captureInScope, createLimiter, validateWorktreeRoot, assertContainedWorktree, allowedWorktreeRoot, _resetAllowedWorktreeRoot, numEnv, atomicWriteFile, assertSafeRunId } from "./farm.ts"; import type { InjectedFile, Sampling } from "./farm.ts"; import type { Worker, WorkerResult, RunTaskDeps, Task } from "./farm.ts"; import { removeWorktreeVerified, deleteBranchVerified, runExitCode, newRunArtifactHealth, cleanupReportLines } from "./farm.ts"; +import { projectPlanMetaForReport, noteArtifactError, noteUnavailableDiff, MAX_RECORDED_ARTIFACT_ERRORS } from "./farm.ts"; import { scrubbedEnv, treeKill, taskkillPath } from "./exec.ts"; import type { RunResult } from "./exec.ts"; import { spawn } from "node:child_process"; @@ -2255,6 +2256,256 @@ describe("atomicWriteFile — publish-or-preserve (#397)", () => { }); }); +// --------------------------------------------------------------------------- +// #439 - what the run's PERMANENT receipt is allowed to contain. +// +// The run-scoped change did not create the exposure, it changed its shape: +// `.farm/farm-report.json` used to be clobbered by the next run, and +// `.farm/runs//` now accumulates indefinitely with no prune path. A +// secret that landed in the report used to be destroyed within one run; now it +// persists. +// +// A CORRECTION to the issue's premise, which changes what the fix must be: the +// issue says there is "no control - schema or runtime - on what meta carries +// into the report". That is wrong. `checkPlanObject` is a CLOSED object check, +// and plan-contract.test.ts already pins that an unknown `meta` property throws +// at parse. An allowlist at the sink would be a second copy of a control that +// already exists, and would have caught nothing. +// +// The real exposure is the opposite shape: the ALLOWED fields are the dangerous +// ones. `meta.setup`, `meta.setupEachAttempt` and `meta.setupInputs` are raw +// shell-command arrays, and `meta.apiBaseUrl` is a URL that can carry +// credentials. A token in a setup command is a perfectly legal plan that parses +// cleanly and lands verbatim in a permanently retained receipt. So the fix is +// REDACTION of the free-text meta fields at the sink, not an allowlist. +// --------------------------------------------------------------------------- +describe("#439 - the permanent receipt redacts what it is allowed to carry", () => { + it("redacts a credential in meta.setup, the field an allowlist would have kept", () => { + const projected = projectPlanMetaForReport({ + name: "p", + setup: ["export API_KEY=sk-ant-REDPROOF-must-not-persist", "npm ci"], + }); + expect(JSON.stringify(projected)).not.toContain("sk-ant-REDPROOF-must-not-persist"); + expect(JSON.stringify(projected)).toContain("REDACTED"); + // The surrounding structure survives: this is redaction, not deletion. A + // receipt that silently drops the setup it ran is a worse receipt. + expect(projected.setup).toHaveLength(2); + expect(projected.setup?.[1]).toBe("npm ci"); + }); + + it("redacts every free-text meta field, not just setup", () => { + const secret = "ghp_" + "a".repeat(36); + const projected = projectPlanMetaForReport({ + name: "p", + apiBaseUrl: `https://token:${secret}@api.example.com`, + setup: [`echo ${secret}`], + setupEachAttempt: [`echo ${secret}`], + setupInputs: [`echo ${secret}`], + }); + expect(JSON.stringify(projected)).not.toContain(secret); + }); + + it("leaves an ordinary plan's meta readable", () => { + // Over-redaction would make the receipt useless for the thing it exists + // for: telling an operator which plan produced this run. + const meta = { name: "nightly", repo: "acme/widgets", model: "gpt-test", setup: ["npm ci"] }; + expect(projectPlanMetaForReport(meta)).toEqual(meta); + }); +}); + +// --------------------------------------------------------------------------- +// #439 AC-2 - the artifacts block is the one NEW class of report content that +// skipped the file's own redaction chokepoint. Every other report-bound +// free-text field is redacted at construction; `git diff failed: ` and `msgOf(e)` strings were not. +// --------------------------------------------------------------------------- +describe("#439 - artifact error text goes through the redaction chokepoint", () => { + it("redacts a credential that appears in an artifact error string", () => { + const bucket: string[] = []; + noteArtifactError(bucket, "git diff failed: fatal: could not read Password for 'https://x:sk-ant-ARTIFACT@h'"); + expect(bucket).toHaveLength(1); + expect(bucket[0]).not.toContain("sk-ant-ARTIFACT"); + }); + + it("still bounds the error list, and says so when it suppresses", () => { + // Redaction must not disturb the existing bound: an unusable diffs + // directory produces one entry per task, and an unbounded list would be + // echoed verbatim into the report. + const bucket: string[] = []; + for (let i = 0; i < 50; i++) noteArtifactError(bucket, `failure ${i}`); + expect(bucket.length).toBeLessThanOrEqual(MAX_RECORDED_ARTIFACT_ERRORS + 1); + expect(bucket[bucket.length - 1]).toContain("suppressed"); + }); +}); + +// --------------------------------------------------------------------------- +// #439 AC-3/AC-4 - the publication primitive must not silently re-permission +// the destination, and must not leave a payload-bearing temp file behind when +// the WRITE fails (the existing test only exercised the RENAME path, so its +// title read broader than its coverage). +// --------------------------------------------------------------------------- +describe("atomicWriteFile - mode preservation and write-path cleanup (#439)", () => { + it("preserves a hardened destination's mode instead of resetting it to 0644", async () => { + const dir = await mkdtemp(path.join(tmpdir(), "farm-mode-")); + const dest = path.join(dir, "report.json"); + await fsWriteFile(dest, '{"prior":true}'); + await fsChmod(dest, 0o600); + const before = (await fsStat(dest)).mode & 0o777; + // An adversarial pass caught this test passing against the UNFIXED + // implementation on Windows: NTFS does not honour POSIX mode bits, so + // chmod(0o600) reports back 0o666 and before === after no matter what + // atomicWriteFile does. A test that cannot fail is worse than no test, so + // it declares that rather than quietly proving nothing. It has teeth on + // POSIX CI, which is where the guarantee is real. + if (before !== 0o600) { + expect(process.platform).toBe("win32"); + return; + } + await atomicWriteFile(dest, '{"new":true}'); + const after = (await fsStat(dest)).mode & 0o777; + expect(after).toBe(before); + expect(JSON.parse(await fsReadFile(dest, "utf8"))).toEqual({ new: true }); + await fsRm(dir, { recursive: true, force: true }); + }); + + it("leaves no temp residue when the WRITE fails, not just the rename", async () => { + const dir = await mkdtemp(path.join(tmpdir(), "farm-writefail-")); + const dest = path.join(dir, "report.json"); + // The failure has to land AFTER the temp exists - that is the window the + // rename-only cleanup left uncovered. There is no "bad data" that forces + // it: Node substitutes U+FFFD for an unencodable payload rather than + // throwing. So the write is failed through the injectable open seam, which + // is how every other failure path in this file is exercised. + const failingOpen = (async (p: string, flags: string) => { + const real = await fsOpen(p, flags); + return Object.assign(Object.create(Object.getPrototypeOf(real)), real, { + writeFile: async () => { throw new Error("ENOSPC: no space left on device"); }, + sync: () => real.sync(), + chmod: (m: number) => real.chmod(m), + close: () => real.close(), + }); + }) as unknown as typeof fsOpen; + await expect(atomicWriteFile(dest, "payload", { open: failingOpen })).rejects.toThrow(/ENOSPC/); + expect(await fsReaddir(dir)).toEqual([]); + await fsRm(dir, { recursive: true, force: true }); + }); +}); + + + +// --------------------------------------------------------------------------- +// #439 (adversarial pass) - the redactor's reach, measured rather than assumed. +// +// The first cut of the receipt fix claimed to "redact every free-text field". +// An adversarial run proved four real credential shapes reached a permanently +// retained receipt anyway, because SECRET_LINE is keyword/prefix driven. Three +// are closed here. The fourth is deliberately NOT, and that is the interesting +// one: `curl -u user:pass` shares its shape with `docker run -u 1000:1000`. +// --------------------------------------------------------------------------- +describe("#439 - redactor reach on shapes a farm plan actually carries", () => { + it("redacts the shapes an adversarial run found leaking", () => { + for (const line of [ + 'git clone https://ci-bot:glpat-LEAKLEAKLEAK123@gitlab.example.com/x/y.git', + 'curl -H "Authorization: Bearer LEAKPAYLOADNOTAREALJWT123"', + "export OPENAI_KEY=sk-proj-LEAK1234567890", + ]) { + expect(redactSecrets(line), line).toContain("REDACTED"); + } + }); + + it("does NOT fire on a uid:gid pair, which is why -u is unhandled", () => { + // The honest limit. A `-u user:pass` rule would catch the fourth shape and + // would also redact every `docker run -u 1000:1000`. Broad is the safe + // direction for this redactor, but not at the cost of ordinary container + // arguments - so the gap is recorded rather than papered over. + for (const line of ["docker run -u 1000:1000 alpine", "podman run -u 0:0 img"]) { + expect(redactSecrets(line)).toBe(line); + } + expect(redactSecrets("curl -u ci-bot:Hunter2 https://x")).not.toContain("REDACTED"); + }); + + it("leaves ordinary plan setup commands and package URLs alone", () => { + for (const line of [ + "npm ci", + "pip install -r requirements.txt", + "see https://example.com/@scope/pkg for details", + 'node -e "0"', + ]) { + expect(redactSecrets(line), line).toBe(line); + } + }); +}); + + +// --------------------------------------------------------------------------- +// #439 (adversarial pass) - BOTH receipts, not just the JSON. +// +// The first cut wrapped the JSON sink and left `farm-report.md` reading raw +// plan.meta three lines later. An end-to-end run with +// meta.name = "nightly sk-ant-MDPROOF-9999" produced a redacted JSON receipt +// and a Markdown sibling whose first line was +// # Farm report - nightly sk-ant-MDPROOF-9999 +// verbatim, in the same never-pruned run directory, on a fully green run. +// Redacting one of two tier-1 artifacts is not a fix, so the projection is now +// computed once and both sinks read it. +// --------------------------------------------------------------------------- +describe("#439 - the Markdown receipt is redacted too", () => { + it("projects meta once so a second sink cannot be forgotten", () => { + // A unit-level guard on the source itself: `plan.meta` must not be read + // directly at either sink. This is deliberately a source assertion - the + // end-to-end proof is expensive, and the failure mode is precisely "someone + // added a third sink and read the raw object again". + const src = readFileSync(new URL("./farm.ts", import.meta.url), "utf8"); + const reportSection = src.slice(src.indexOf("const reportMeta = projectPlanMetaForReport")); + const header = reportSection.slice(0, reportSection.indexOf("## Diff evidence")); + expect(header).toContain("reportMeta.name"); + expect(header).not.toContain("plan.meta.name"); + }); + + it("redacts a credential wherever it sits in meta, for both sinks", () => { + const projected = projectPlanMetaForReport({ + name: "nightly sk-ant-MDPROOF-9999", + setup: ["npm ci"], + }); + // The name is what the Markdown H1 interpolates. + expect(projected.name).not.toContain("sk-ant-MDPROOF-9999"); + expect(projected.name).toContain("REDACTED"); + }); +}); + +// --------------------------------------------------------------------------- +// #439 (adversarial pass) - the diff-evidence reasons are the strings the issue +// actually named, and they went through a DIFFERENT function. +// +// `git diff failed: `, `diffs directory unavailable: ` and `patch write failed: ` are all passed to +// noteUnavailableDiff, not noteArtifactError. Redacting only the latter left +// the cited example un-redacted in BOTH receipts: artifacts.diffs.unavailable +// in the JSON and the "Diff evidence" list in the Markdown. Reproduced with a +// blocked diffs directory in a repo whose path carried a token-shaped segment. +// --------------------------------------------------------------------------- +describe("#439 - diff-evidence reasons go through the chokepoint", () => { + it("redacts a credential in an unavailable-diff reason", () => { + const health = newRunArtifactHealth(); + noteUnavailableDiff( + health.diffs, + "task-a", + "diffs directory unavailable: EEXIST: mkdir 'C:\\farm-ghp_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa\\.farm'", + ); + expect(health.diffs.unavailable).toHaveLength(1); + expect(health.diffs.unavailable[0]!.reason).not.toContain("ghp_aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + }); + + it("keeps the running total exact past the retention bound", () => { + // Redaction must not disturb the property that made this list trustworthy: + // the bounded list never understates how many tasks lack diff evidence. + const health = newRunArtifactHealth(); + for (let i = 0; i < 40; i++) noteUnavailableDiff(health.diffs, `t${i}`, `reason ${i}`); + expect(health.diffs.unavailableTotal).toBe(40); + expect(health.diffs.unavailable.length).toBeLessThanOrEqual(MAX_RECORDED_ARTIFACT_ERRORS + 1); + }); +}); + // --------------------------------------------------------------------------- // #397 — a caller-supplied run id becomes a directory name, so it must be a // single safe path segment. diff --git a/plugins/ca/tools/redactor.ts b/plugins/ca/tools/redactor.ts index 4e28c6cd..8554a5b1 100644 --- a/plugins/ca/tools/redactor.ts +++ b/plugins/ca/tools/redactor.ts @@ -46,7 +46,37 @@ // must flag and benign lines both must pass, asserted against SECRET_LINE here // (farm.unit.test.ts) and against SECRET_RE in test_hooklib.py, so neither side // can silently regress on it. -const SECRET_LINE = /(api[_-]?key|token|secret|password|BEGIN.*PRIVATE|sk-ant|AKIA[0-9A-Z]{16}|ghp_[A-Za-z0-9]{36})/i; +// #439 widened the prefix set after an adversarial pass measured four shapes +// that reached a permanently retained farm receipt un-redacted: a GitLab PAT +// (`glpat-`), an OpenAI project key (`sk-proj-`, which the `api[_-]?key` +// keyword misses because the surrounding text says `OPENAI_KEY`), basic auth +// embedded in a clone URL (`https://user:pass@host`), and a bare +// `Authorization: Bearer eyJ...` JWT. Each is a literal, high-signal shape, so +// widening here costs nothing in false positives on the benign corpus - the +// URL and Bearer rules both require credential-shaped material, not merely the +// word "Bearer" or an "@" in a URL. +const SECRET_LINE = new RegExp( + [ + "api[_-]?key", "token", "secret", "password", "BEGIN.*PRIVATE", + "sk-ant", "sk-proj-[A-Za-z0-9_-]{8}", + "AKIA[0-9A-Z]{16}", + "ghp_[A-Za-z0-9]{36}", + "glpat-[A-Za-z0-9_-]{8}", + // basic auth in a URL: scheme://user:secret@host - the colon-and-at shape, + // not any "@", so `https://example.com/@scope/pkg` is untouched. + // NOTE the doubled backslashes: these are JS STRING literals, and "\\s" in + // a string is a literal "s". The first cut of this shipped `[^/\s:@]`, + // which silently became `[^/s:@]` and only matched by luck. + "https?://[^/\\s:@]+:[^/\\s@]+@", + // a bearer credential, which is dot-segmented base64url in practice + "Bearer\\s+[A-Za-z0-9_-]{10,}", + // DELIBERATELY NOT ADDED: a `-u user:pass` rule for curl. `-u 1000:1000` + // is an everyday docker/podman uid:gid pair, so the shape collides with + // benign input. Broad is the safe direction for this redactor, but not at + // the cost of firing on ordinary container arguments. + ].join("|"), + "i", +); // PEM-style armor delimiters. BEGIN opens a span; END closes it. Matched // independently of SECRET_LINE so even a `-----BEGIN CERTIFICATE-----` (no // trigger word) is span-redacted — armored material is opaque, redact it whole.