Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 54 additions & 1 deletion .github/workflows/security-audit.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -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."
Expand All @@ -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
Expand Down Expand Up @@ -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 \
Expand Down
1 change: 1 addition & 0 deletions docs/specs/security-audit.md
Original file line number Diff line number Diff line change
Expand Up @@ -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).

Expand Down
2 changes: 2 additions & 0 deletions docs/specs/security-audit.rationale.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
53 changes: 51 additions & 2 deletions scripts/security-audit.test.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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'] },
Comment thread
dormouse-bot marked this conversation as resolved.
// 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) => {
Expand All @@ -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}`);
Expand Down
2 changes: 1 addition & 1 deletion scripts/spec-word-budgets.json
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading