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

.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 <it>`
+  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''',
+]

CHANGELOG.md

@@ -62,6 +62,32 @@ predate the plugin rewrite and are grouped by date.
 
 ### Fixed
 
+- A secret in a farm plan no longer persists in the run's permanent receipt.
+  `plan.meta` was serialized verbatim into `.farm/runs/<runId>/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 <plugin>/tools/farm.js`, but the integration launcher ran `farm.ts`

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,

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<void> {
+/** 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<void> {
   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,21 +1273,71 @@ 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/
+// <runId>/` 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<T extends Record<string, unknown>>(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: <output>`) 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)");
 }
 
 // Same bound for the per-task diff-evidence list, which has the identical
 // 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: <raw git stderr>`,
+  // `diffs directory unavailable: <fs error>` and `patch write failed:
+  // <msgOf(e)>` - 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`,