diff --git a/.github/workflows/security-audit.yaml b/.github/workflows/security-audit.yaml index 701aa0a6f..d9f1644ef 100644 --- a/.github/workflows/security-audit.yaml +++ b/.github/workflows/security-audit.yaml @@ -493,6 +493,46 @@ jobs: echo "- **The audit wrote no verdict.** \`audit-status.txt\` was absent, empty, or not \`PASS\`/\`FAIL\`, so the run ended without deciding. Start at the report's own markers: \`UNVERIFIABLE\` is a check the agent could not determine, \`_Incomplete …_\` heads a domain that was cut off mid-report, and a domain section reading \`_No report …_\` is one that never reported — which is what an expired wait deadline looks like." >> "$NOTES" fi + # The notes above index the report by marker, and the clamp below + # keeps the head — so on a report too long to post, the markers they + # send the reader to are the part that gets cut. Run 35842217451 + # posted `VERDICT: INCONCLUSIVE` naming `audit-ci-secrets.md`, and + # its one `UNVERIFIABLE` line, along with two whole later domains + # and their verdicts, fell past 32,000 characters of a 226,302- + # character body. Lift every line that carries a verdict or a + # non-passing finding into the head, where the clamp cannot reach + # it; the full sections still follow for whatever survives. + # + # Verdicts and findings are collected separately so the cap can fall + # on the findings alone: one verdict line per fragment is bounded by + # the fragment count, while a single domain's findings are not, and + # a shared cap would let the first domain's findings push the last + # domain's verdict out — the loss this lift exists to prevent. + # + # Matched at line start after an optional heading or bullet marker, + # which is what keeps `### FAIL IF results` and every `- PASS:` + # bullet quoting a **FAIL IF** clause out of the lift. `FAIL:` takes + # its colon for the same reason; a domain-level failure arrives as + # `VERDICT: FAIL` and is caught by the verdict pass. + # + # `|| true` on both: `grep` exits 1 when it matches nothing, and + # under this step's `set -eo pipefail` that would end the step here, + # before `audit-comment.md` is composed and before any `gh issue` + # call — turning a truncated report into no report at all. A + # nonempty fragment with no marker line is exactly the state the + # verdict loop above already reports as unreadable. + VERDICTS=$(mktemp) + FINDINGS=$(mktemp) + # Unquoted on purpose: `AUDIT_FRAGMENTS` is a filename list. + # shellcheck disable=SC2086 + for f in $AUDIT_FRAGMENTS; do + [ -s "$f" ] || continue + { grep -hE '^[[:space:]]*(#+[[:space:]]*)?([-*][[:space:]]*)?VERDICT:' "$f" || true; } | + cut -c1-500 | sed "s|^|- \`$f\`: |" >> "$VERDICTS" + { grep -hE '^[[:space:]]*(#+[[:space:]]*)?([-*][[:space:]]*)?(FAIL:|UNVERIFIABLE|BLOCKER|WARNING)' "$f" || true; } | + cut -c1-500 | sed "s|^|- \`$f\`: |" >> "$FINDINGS" + done + if [ "$STATUS" = "FAIL" ]; then TITLE="[security-audit] FAIL on $(date -u +%Y-%m-%d)" HEADLINE="Audit failed at $DATE." @@ -516,6 +556,19 @@ jobs: echo "- **Inconclusive for a reason this step could not name.** That is itself a bug in the reporting step." echo fi + if [ -s "$VERDICTS" ] || [ -s "$FINDINGS" ]; then + echo "### Lines that decided this verdict" + echo + echo "_Lifted out of the fragments so truncation cannot cut them. Each domain's full section follows for as far as the body reaches._" + echo + cat "$VERDICTS" + head -n 40 "$FINDINGS" + LIFTED=$(wc -l < "$FINDINGS") + if [ "$LIFTED" -gt 40 ]; then + echo "- _…and $((LIFTED - 40)) more findings; read them in the transcript._" + fi + echo + fi if [ -s audit-report.md ]; then cat audit-report.md else @@ -557,7 +610,7 @@ jobs: done fi } > audit-comment.md - rm -f "$NOTES" + rm -f "$NOTES" "$VERDICTS" "$FINDINGS" # Truncate before posting, non-fatally. See docs/specs/security-audit.md -> "Outcomes and reporting". node scripts/clamp-issue-body.mjs audit-comment.md \ diff --git a/docs/specs/security-audit.md b/docs/specs/security-audit.md index 6ca52be14..aaa8b5f9e 100644 --- a/docs/specs/security-audit.md +++ b/docs/specs/security-audit.md @@ -81,6 +81,7 @@ Source of truth: `2. Wait without ending your turn`, `3. Merge`, and `4. The ver - **Must return `VERDICT: INCONCLUSIVE` from a domain with any undetermined check unless it found a failure.** Only all-determined passing checks permit `VERDICT: PASS`; a domain's inconclusive verdict prevents a merged pass. - **Never write a `FAIL IF` condition no audit run can read**: audit the readable half; stage the rest under `## Future` only while it is unbuilt, and otherwise state it beside the rule. `AUDIT_PAT`-readable GitHub state stays audited (rationale). - **`STATUS` is assigned in exactly two places**: where the status file is parsed, and in the single escalation block, **which orders `FAIL` > `MISSING` > `PASS`** — a dissent can raise `MISSING` to `FAIL` and never the reverse, and a `FAIL` alongside missing or unreadable fragments still reports them. +- **Every fragment's verdict line is lifted into the head ahead of the report, then every fragment's `UNVERIFIABLE`, `FAIL:`, `BLOCKER` and `WARNING` lines** — two blocks, not one per domain. Both passes match at line start after an optional heading or bullet marker, so the fragments' `FAIL IF` vocabulary is not read as a finding, and both cut lines to 500 characters. **Only the findings block carries the 40-line cap**, so no domain's findings can push another's verdict out. **Neither pass may fail the step when it matches nothing** — under `set -eo pipefail` that would post no report at all (rationale). - **The report is truncated to 32,000 characters before posting**, head kept, by `scripts/clamp-issue-body.mjs` (self-tested by `scripts/clamp-issue-body-selftest.mjs`). The call is non-fatal; the `audit-transcript` artifact holds the report in full; `.github/workflows/workflow-audit.yaml` truncates its commit list the same way (rationale). - **Every run uploads the `audit-transcript` artifact, which is world-readable and not secret-masked** — 14-day retention, deep-linked from failure issues (rationale). diff --git a/docs/specs/security-audit.rationale.md b/docs/specs/security-audit.rationale.md index 9ccbb0515..373b66820 100644 --- a/docs/specs/security-audit.rationale.md +++ b/docs/specs/security-audit.rationale.md @@ -66,6 +66,8 @@ A `FAIL IF` condition no audit run can read makes the verdict a coin flip, becau GitHub rejects an over-long issue body outright; that rejection lands on a `set -e` step *after* the verdict is decided, and the finding then reaches no issue and no comment — only a red run and an artifact that expires. Truncation keeps the head because that is where the verdict and the links are, and the clamp call is non-fatal so a failure of the helper cannot reopen the window it closes. +Keeping the head is not the same as keeping what decides. Run 35842217451 (2026-09-23) composed a 226,302-character body: the posted issue carried `VERDICT: INCONCLUSIVE` for `audit-ci-secrets.md` and a note sending the reader to its `UNVERIFIABLE` checks, while that domain's one `UNVERIFIABLE` line sat past the cut along with the whole of `## Application security`, `## Hosted accounts` and `## Summary`. The deciding lines are not positionally predictable, so the clamp cannot be taught to keep them; lifting them into the head, which the clamp keeps by construction, is what makes the notes' pointers resolve. The lift matches at line start after an optional heading or bullet marker because every fragment is written in `FAIL IF` vocabulary — `### FAIL IF results` heads the passing list, and a `- PASS:` bullet quotes the clause it passed — so a bare `FAIL` match would lift the passing evidence as findings. The findings are bounded at 40 lines, and every lifted line at 500 characters, so the lift cannot itself exhaust the budget it protects; the real report produced 8 lines and 1,571 characters. The cap falls on the findings alone because one verdict line per fragment is bounded by the fragment count while a single domain's findings are not, so a shared cap would let the first domain's findings push the last domain's verdict out — the loss the lift exists to prevent. The first draft of the lift piped `grep` without `|| true`: a nonempty fragment carrying no marker line — the unreadable-verdict state the guard loop above already reports — made the pipeline exit 1, and `set -eo pipefail` ended the step before `audit-comment.md` was composed, so nothing was posted at all. Caught by the review on the PR that introduced it and pinned by a markerless-fragment case. + Issue prose per combination of conditions cannot be kept correct by fixing combinations. Four consecutive review rounds found the same defect in different clothes — an arm whose text was true only of the states that could reach it, made false by the next gate that widened. A note claiming nothing about the other conditions cannot be invalidated by a new one. Gating a fragment guard on the status produced the same defect three times: gated on `PASS`, one empty fragment silenced the dissent check; widened to `!= FAIL`, an orchestrator that wrote `FAIL` itself silenced both, so a domain that left no report beside a real finding appeared nowhere at all. Recording what is true of a run and deciding its verdict are separate jobs. diff --git a/scripts/security-audit.test.mjs b/scripts/security-audit.test.mjs index 50deac2c6..fb1cc5262 100644 --- a/scripts/security-audit.test.mjs +++ b/scripts/security-audit.test.mjs @@ -75,6 +75,46 @@ const cases = [ notes: ['the merge never ran', '## audit-supply-chain.md', 'VERDICT: PASS', '## audit-application.md', '_No report — this domain produced no fragment._', '## audit-ci-secrets.md\n\n_Incomplete — this domain never closed its report'], counts: { '_Incomplete — this domain never closed its report': 1 } }, + // Run 35842217451 composed a 226,302-character body; the clamp keeps the + // head, so what reached the issue was `VERDICT: INCONCLUSIVE` for + // `audit-ci-secrets.md` without the one `UNVERIFIABLE` line the note sends + // the reader to, and without two later domains' sections at all. Every + // verdict and non-passing finding is lifted into the head ahead of the + // report, where the clamp cannot reach it. + { name: 'the lines that decided the verdict outlive truncation', status: 'PASS', + verdicts: ['PASS', 'INCONCLUSIVE', 'PASS', 'PASS'], + evidence: [null, '- UNVERIFIABLE: token scope needs a live credential', null, null], + report: `# Fixture report\n${'filler paragraph. '.repeat(3000)}\n`, + expected: 'INCONCLUSIVE', clamped: true, + notes: ['- `audit-ci-secrets.md`: - UNVERIFIABLE: token scope needs a live credential', + '- `audit-hosted.md`: VERDICT: PASS'] }, + // The lift reads line starts, so the `FAIL IF` vocabulary every fragment is + // written in must not read as a finding: a passing clause quoting one, and + // the heading the list sits under, are both PASS evidence. + { name: 'a passing FAIL IF clause is not lifted as a finding', status: 'PASS', + verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], + evidence: ['### FAIL IF results\n- PASS: **FAIL IF** a secret leaks — none does.', null, null, null], + unfinished: [0], expected: 'INCONCLUSIVE', + counts: { 'FAIL IF': 0 } }, + // A fragment carrying no marker line at all — the unreadable-verdict state + // the guard loop above already reports. The lift's `grep` matches nothing + // and exits 1; without `|| true` this step's `set -eo pipefail` ends it + // before the body is composed, so the reader gets a red run and an artifact + // instead of a truncated report. `raw` bypasses the verdict-line prefix + // every other fixture fragment carries. + { name: 'a fragment with no marker line still gets reported', status: 'PASS', + verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], + raw: [null, null, '# audit-application.md\n\nI reviewed the specs but could not finish.\n', null], + expected: 'INCONCLUSIVE', posts: true, + notes: ["A domain's verdict could not be read", '- `audit-hosted.md`: VERDICT: PASS'] }, + // The cap falls on the findings alone, so one domain's findings cannot push + // a later domain's verdict out of the head — the loss the lift exists to + // prevent. 42 findings in the first fragment is two past the cap. + { name: 'findings past the cap do not push out a later verdict', status: 'PASS', + verdicts: ['PASS', 'PASS', 'PASS', 'PASS'], + evidence: [Array.from({ length: 42 }, (_, i) => `WARNING: finding ${i}`).join('\n'), null, null, null], + unfinished: [0], expected: 'INCONCLUSIVE', + notes: ['- `audit-hosted.md`: VERDICT: PASS', 'more findings; read them in the transcript'] }, ]; for (const scenario of cases) { test(`reporting: ${scenario.name}`, (t) => { @@ -86,21 +126,30 @@ for (const scenario of cases) { if (args[0] === 'issue' && args[1] === 'list') process.stdout.write('23\\n'); `); if (scenario.status !== undefined) writeFileSync(join(dir, 'audit-status.txt'), scenario.status); - if (scenario.report !== null) writeFileSync(join(dir, 'audit-report.md'), '# Fixture report\n'); + if (scenario.report !== null) writeFileSync(join(dir, 'audit-report.md'), scenario.report ?? '# Fixture report\n'); scenario.verdicts.forEach((verdict, i) => { if (verdict === null) return; const sentinel = scenario.unfinished?.includes(i) ? '' : `${SENTINEL}\n${scenario.trailingBlank ? '\n' : ''}`; - writeFileSync(join(dir, fragments[i]), `VERDICT: ${verdict}\nEvidence\n${sentinel}`); + const evidence = scenario.evidence?.[i] ?? 'Evidence'; + writeFileSync(join(dir, fragments[i]), + scenario.raw?.[i] ?? `VERDICT: ${verdict}\n${evidence}\n${sentinel}`); }); const result = spawnSync('bash', ['-c', reporting], { cwd: dir, env, encoding: 'utf8' }); assert.equal(result.status, scenario.expected === 'PASS' ? 0 : 1, result.stderr); const calls = readFileSync(join(dir, 'gh-calls.jsonl'), 'utf8').trim().split('\n').map(JSON.parse); assert.equal(calls.some((args) => args[0] === 'issue' && args[1] === 'close'), scenario.expected === 'PASS'); + // The step aborting before it posts leaves every `notes` assertion below + // unreachable, so name the post itself. + if (scenario.posts) { + assert.ok(calls.some((args) => args[0] === 'issue' && args[1] === 'comment'), 'no issue comment was posted'); + } if (scenario.expected !== 'PASS') { const body = readFileSync(join(dir, 'audit-comment.md'), 'utf8'); assert.match(body, scenario.expected === 'FAIL' ? /Audit failed/ : /Audit reached no usable verdict/); + // Without this the truncation case passes vacuously on a body that fit. + if (scenario.clamped) assert.match(body, /_Truncated to fit: the full body is \d+ characters\./); for (const note of scenario.notes ?? []) assert.ok(body.includes(note), `missing note: ${note}`); for (const [note, n] of Object.entries(scenario.counts ?? {})) { assert.equal(body.split(note).length - 1, n, `wrong occurrence count for: ${note}`); diff --git a/scripts/spec-word-budgets.json b/scripts/spec-word-budgets.json index 6011d7705..f9b0d72ea 100644 --- a/scripts/spec-word-budgets.json +++ b/scripts/spec-word-budgets.json @@ -18,7 +18,7 @@ "docs/specs/relay.md": 10150, "docs/specs/remote-api.md": 4700, "docs/specs/remote-security-model.md": 4750, - "docs/specs/security-audit.md": 2000, + "docs/specs/security-audit.md": 2100, "docs/specs/security-ci.md": 2750, "docs/specs/security-hosted.md": 600, "docs/specs/security-local.md": 3150,