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.