Four LOW findings from the auth-crypto-reviewer pass on the #430 merge. Gate verdict was PASS (0 CRITICAL, 0 HIGH) — none of these blocked, and #430 is already on main. Filing so they are not lost.
1. plan.meta is persisted with no control at all — and the run-scoped change widened it
writeReport serializes plan.meta verbatim (plugins/ca/tools/farm.ts:2301, :2352-2359).
Two corrections to the assumption I made when clearing this:
- The schema is a red herring in both directions.
plan.schema.json is never validated at runtime — farm.ts does JSON.parse(...) as Plan and hand-rolls validate() (the file says so at farm.ts:1710-1715). Even task's additionalProperties: false is enforced only by an LLM reading the schema per writing-plans/references/farm-plan.md:39. Adding additionalProperties: false to meta would fix nothing. There is currently no control — schema or runtime — on what meta carries into the report.
- "Pre-existing, unchanged" is only half true. The run-scoped change altered the exposure:
- Copies 1 → 2:
plan.meta now lands in runs/<runId>/farm-report.json and the .farm/farm-report.json latest pointer, plus twice more in Markdown.
- Retention: overwrite-per-run → permanent. The old
.farm/farm-report.json was clobbered by the next run. .farm/runs/<runId>/ accumulates indefinitely with no prune path in the diff, and the surface docs fresh-id-per-run as intended usage. A secret that landed in plan.meta used to be destroyed by the next run; now it persists.
Stays LOW: plan.json is trusted operator/agent-authored input, .farm/ is gitignored at any depth (.gitignore:73), farm stages only worker.filesWritten (the H-03 declared exception, farm.ts:1576), and no workflow references .farm.
Fix: allowlist-project at the sink rather than trying to close it at the schema —
plan: { name, repo, model, apiBaseUrl, setup } in writeReport. Same positive-allowlist pattern ADR-0017 mandates for the ca-pi models.json projection, and unlike a schema edit it is actually enforced.
2. New artifact error strings bypass the file's own redactSecrets chokepoint
farm.ts:2244, :2251, :2255 (also :2237, :2358). Every other report-bound free-text field in this file is redacted at construction (:302, 1303, 1309, 1320, 1422, 1446, 1520, 1608). The new artifacts block is not: git diff failed: ${d.out.trim().split("\n")[0]} puts raw git output into farm-report.json, and msgOf(e) strings land in artifacts.stream.errors / latest_mirror_errors.
Realistically fs/git error text carrying paths and branch names, so exposure is minimal — but it is the one new class of report content that skips the established chokepoint. redactSecrets is already imported (farm.ts:50) and documented idempotent.
3. atomicWriteFile silently resets destination permission bits
farm.ts:1056-1075. The old writeFile(dest, data) opened an existing destination O_TRUNC and preserved its mode. The new publish creates a temp at default 0o666 & ~umask (→ 0644) and renames over the destination, so the temp's mode wins. An operator who hardened .farm/farm-report.json to 0600 gets it reset to 0644 on every run. Same effect on Windows ACLs (temp inherits the directory ACL; rename carries it).
Nothing in security-controls.md requires restrictive modes on farm reports — that requirement is ca-pi-scoped — hence LOW. Fix: fchmod the temp to the destination's existing mode before rename, or pass an explicit mode; otherwise document that report artifacts are umask-default by design.
4. Temp-file residue on the write/sync failure path
farm.ts:1062-1074. Cleanup is attached only to the rename failure. If fh.writeFile or fh.sync() throws (ENOSPC, EIO), the finally closes the handle and a partially-written .farm-report.json.<pid>-<rand>.tmp is left on disk holding the payload.
The unit test "preserves the prior complete artifact and cleans up when publication fails" only exercises the rename path (destination is a directory), so its title reads broader than its coverage.
Secondary: open(tmp, "w") has no O_EXCL, so it follows a pre-existing symlink at that path. Irrelevant under the documented single-developer model, and the random suffix makes the name unpredictable — but "wx" is free.
Acceptance criteria
- AC-1:
farm-report.json contains only the five allowlisted meta fields; a plan carrying an extra meta key does not propagate it. Pinned by a test.
- AC-2: every free-text field in the
artifacts block passes through redactSecrets.
- AC-3: publishing over a
0600 report leaves it 0600, or the umask-default behavior is documented.
- AC-4: a write/sync failure leaves no
.tmp residue; the existing test is widened to cover that path, not just rename.
Four LOW findings from the
auth-crypto-reviewerpass on the #430 merge. Gate verdict was PASS (0 CRITICAL, 0 HIGH) — none of these blocked, and #430 is already on main. Filing so they are not lost.1.
plan.metais persisted with no control at all — and the run-scoped change widened itwriteReportserializesplan.metaverbatim (plugins/ca/tools/farm.ts:2301,:2352-2359).Two corrections to the assumption I made when clearing this:
plan.schema.jsonis never validated at runtime —farm.tsdoesJSON.parse(...) as Planand hand-rollsvalidate()(the file says so atfarm.ts:1710-1715). Eventask'sadditionalProperties: falseis enforced only by an LLM reading the schema perwriting-plans/references/farm-plan.md:39. AddingadditionalProperties: falsetometawould fix nothing. There is currently no control — schema or runtime — on whatmetacarries into the report.plan.metanow lands inruns/<runId>/farm-report.jsonand the.farm/farm-report.jsonlatest pointer, plus twice more in Markdown..farm/farm-report.jsonwas clobbered by the next run..farm/runs/<runId>/accumulates indefinitely with no prune path in the diff, and the surface docs fresh-id-per-run as intended usage. A secret that landed inplan.metaused to be destroyed by the next run; now it persists.Stays LOW:
plan.jsonis trusted operator/agent-authored input,.farm/is gitignored at any depth (.gitignore:73), farm stages onlyworker.filesWritten(the H-03 declared exception,farm.ts:1576), and no workflow references.farm.Fix: allowlist-project at the sink rather than trying to close it at the schema —
plan: { name, repo, model, apiBaseUrl, setup }inwriteReport. Same positive-allowlist pattern ADR-0017 mandates for the ca-pimodels.jsonprojection, and unlike a schema edit it is actually enforced.2. New artifact error strings bypass the file's own
redactSecretschokepointfarm.ts:2244,:2251,:2255(also:2237,:2358). Every other report-bound free-text field in this file is redacted at construction (:302, 1303, 1309, 1320, 1422, 1446, 1520, 1608). The newartifactsblock is not:git diff failed: ${d.out.trim().split("\n")[0]}puts raw git output intofarm-report.json, andmsgOf(e)strings land inartifacts.stream.errors/latest_mirror_errors.Realistically fs/git error text carrying paths and branch names, so exposure is minimal — but it is the one new class of report content that skips the established chokepoint.
redactSecretsis already imported (farm.ts:50) and documented idempotent.3.
atomicWriteFilesilently resets destination permission bitsfarm.ts:1056-1075. The oldwriteFile(dest, data)opened an existing destinationO_TRUNCand preserved its mode. The new publish creates a temp at default0o666 & ~umask(→0644) and renames over the destination, so the temp's mode wins. An operator who hardened.farm/farm-report.jsonto0600gets it reset to0644on every run. Same effect on Windows ACLs (temp inherits the directory ACL; rename carries it).Nothing in
security-controls.mdrequires restrictive modes on farm reports — that requirement is ca-pi-scoped — hence LOW. Fix:fchmodthe temp to the destination's existing mode before rename, or pass an explicitmode; otherwise document that report artifacts are umask-default by design.4. Temp-file residue on the write/sync failure path
farm.ts:1062-1074. Cleanup is attached only to the rename failure. Iffh.writeFileorfh.sync()throws (ENOSPC, EIO), thefinallycloses the handle and a partially-written.farm-report.json.<pid>-<rand>.tmpis left on disk holding the payload.The unit test "preserves the prior complete artifact and cleans up when publication fails" only exercises the rename path (destination is a directory), so its title reads broader than its coverage.
Secondary:
open(tmp, "w")has noO_EXCL, so it follows a pre-existing symlink at that path. Irrelevant under the documented single-developer model, and the random suffix makes the name unpredictable — but"wx"is free.Acceptance criteria
farm-report.jsoncontains only the five allowlistedmetafields; a plan carrying an extrametakey does not propagate it. Pinned by a test.artifactsblock passes throughredactSecrets.0600report leaves it0600, or the umask-default behavior is documented..tmpresidue; the existing test is widened to cover that path, not just rename.