fix(security-audit): clamp the issue body so a FAIL is never reported nowhere - #480
fix(security-audit): clamp the issue body so a FAIL is never reported nowhere#480dormouse-bot wants to merge 5 commits into
Conversation
… nowhere GitHub rejects an issue or comment body over 65536 characters, and that rejection lands on a set -e step after the verdict is decided — so a report that trips the limit reaches no issue and no comment at all. Run 33249330988 did exactly that: VERDICT: FAIL with a BLOCKER finding, a 68 KB audit-report.md, and nothing filed. Add scripts/clamp-issue-body.mjs (dependency-free, in-place, no-op under the limit) and call it from both security-audit.yaml and workflow-audit.yaml, which composes its body the same unbounded way. The clamp keeps the head — verdict, run and transcript links, dissent notes — and points at the audit-transcript artifact, which always holds the report untruncated. scripts/clamp-issue-body-selftest.mjs reproduces the 68 KB body that caused this and asserts the result is postable, plus the ways a clamp could be under the limit and still useless: dropping the head, splitting a code point, stacking footers on re-run, or leaving a fence open.
Deploying mouseterm with
|
| Latest commit: |
8ac6d23
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://693f8402.mouseterm.pages.dev |
| Branch Preview URL: | https://fix-audit-report-body-limit.mouseterm.pages.dev |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
Two things worth fixing before this lands. The clamp itself holds up — I ran it over a tilde-fenced body, an emoji-dense body, and one enormous single line, and all six root gates plus the new selftest pass locally.
The clamp is now the last thing that can kill the reporting step. node scripts/clamp-issue-body.mjs runs under set -e ahead of every gh issue create / gh issue comment in both workflows, so any failure of the helper — node absent from a future runner image, an unreadable path, an unexpected throw — lands in exactly the window this PR exists to close: red run, no issue, no comment. It also fails closed for the common case, where the body was under the limit and would have posted untouched. Since the script rewrites in place only when it is over budget, a failure leaves the body exactly as composed, so falling back to posting unclamped is strictly better than not posting at all. Sharper in workflow-audit.yaml, whose own added comment notes there is no setup-node step and it is relying on the runner image's preinstalled node.
A ~~~ fence is not closed. unclosedFence always emits a backtick fence, which does not close a tilde-fenced block, so on a report that cuts inside one the truncation footer and its artifact link render as literal text inside the code block — the outcome the function's own doc comment says it prevents ("a missing one hides the footer entirely"). The report is machine-merged from three subagents' markdown, so the fence character is not ours to assume.
One more without a suggestion attached, since both call sites pass a short constant note: the room <= 0 branch returns footer.trimStart(), which is itself over the limit whenever the note is long enough to reach that branch — so "Ship the footer rather than an over-long body" does not hold, gh rejects it, and the step dies the same way. A .slice(0, limit) there would make the comment true.
Verification
Fatal-before-filing, reproduced against the real script:
$ cat /tmp/t.sh
set -eo pipefail
node scripts/clamp-issue-body.mjs /tmp/nope.md --note "x" 2>/dev/null
echo "REACHED gh issue create"
$ bash /tmp/t.sh; echo "step-exit=$?"
step-exit=1
Tilde fence, current behavior — the closer is a backtick fence, so the footer stays inside the block:
tail: "...code line in a tilde fence\n```\n\n---\n\n_Truncated here: the full body is 135010 characters..."
With the suggested unclosedFence, the closer matches (~~~), and the existing selftest still passes, as do the three opener shapes I checked (tilde, three-backtick, four-backtick) — every result under the limit with the footer outside the fence.
Root gates on the merged tree: spec-lint, xterm-lint, loopback-lint, deploy-lint, deploy-lint-selftest, clamp-issue-body-selftest all OK.
Three follow-ups from review of this PR. The clamp was itself the last thing that could kill the reporting step. It runs under `set -e` ahead of every `gh issue create` / `gh issue comment` in both workflows, so any failure of the helper landed in exactly the window this PR exists to close, including for the common case where the body was under the limit and would have posted untouched. It rewrites the file only when over budget, so on failure the body is still the one composed above; both call sites now fall back with `|| echo ... >&2`. `unclosedFence` always emitted a backtick fence, which does not close a `~~~` block, so a cut inside a tilde-fenced section left the truncation footer and its artifact link rendering as literal text inside the code block -- the outcome the function exists to prevent. The report is machine-merged from three subagents' markdown, so the marker is not ours to assume; the closer now comes from the unmatched opener. The `room <= 0` branch returned the whole footer, which is itself over the limit whenever the note is long enough to reach that branch, so `gh` rejected it and the step died the same way. It is now cut to budget. Selftest gains a fence-marker case over four opener shapes and a case for the over-long-note branch; both fail against the pre-fix helper.
|
All three suggestions applied in The Both new selftest cases are load-bearing — run against a copy of the helper with the two fixes reverted they fail 4 checks, three of them the backtick closer emitted for a non-backtick opener and one the 65656-character body from the pathological branch; against the fixed helper the whole selftest is OK. VerificationFatal-before-filing, the shape you reproduced, now against the pushed workflow line: New selftest cases against the pre-fix helper (both fixes reverted in a Root gates on the pushed tree: |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
The tilde fix closes one of the two ways unclosedFence can miss an open block; the other is nesting, and it's the shape this repo's own comment-formatting guidance prescribes (a longer outer fence around a shorter inner one). At a cut inside the inner block the parity count is even — the inner opener is read as the outer closer — so nothing is emitted and the footer, with the audit-transcript download link in it, renders as literal text inside the still-open block. That's exactly the outcome case 5 and case 6 exist to prevent, reached by a different route.
Tracking the open marker instead of counting lines fixes nesting and mismatched pairs together, and it's about the same size as the parity count. It also drops the assumption that any fence-shaped line closes a block: inside a fenced block a line like ```js is literal content, not a closer, so a CLOSER regex that requires nothing after the marker is what makes the state machine agree with how GitHub will actually render the body. Suggestions inline, plus a selftest case for the nested shape.
Everything else from the last round checks out — the || echo fallback, the tilde closer, and the slice(0, budget) on the room <= 0 branch all do what the replies say, and the three threads are resolved. One stale comment rides along with the fix: room = budget - footer.length - 4 reserves four characters for the closer, which has been an under-reserve since the marker stopped being hardcoded to ``` (a ```` opener needs five). SAFETY_MARGIN absorbs it, so it's the comment that's wrong rather than the arithmetic — worth a word if you touch that line.
Verification
Against the pushed helper, the footer lands inside the open block for two shapes the selftest doesn't cover (printing the 10 characters before the footer separator, which should be the closing fence):
CURRENT nested -> "nner code line" # ```` outer, ``` inner
CURRENT infoStr -> "line\nline\nline" # ``` block containing a ```js line
With the suggested unclosedFence, every shape closes with the right marker and stays under the limit:
nested "\n````" true
tilde "\n~~~" true
plain "\n```" true
mismatch "\n```" true # ``` block containing a ~~~ line — literal, per CommonMark
balanced "" true # no closer emitted, correctly
The existing selftest passes unchanged against the suggested helper. The new case 6b is load-bearing: against the pushed helper it fails with tail "er code line"; against the suggested one the whole selftest is OK. spec-lint OK on the merged tree.
`unclosedFence` counted fence lines and emitted a closer on an odd count. Two shapes defeat that count, and both end with the truncation footer -- including the `audit-transcript` link that is the whole point of the footer -- rendering as literal text inside a still-open code block: - Nested fences (a longer outer marker around a shorter inner one, the shape AGENTS.md's own comment-formatting guidance prescribes) leave two blocks open at the cut, but the parity count is even because the inner opener is read as the outer closer. - A fence-shaped line carrying an info string (```js) is literal content inside an open block, not a closer, but the count treats it as one. Track the open marker instead: a fence closes only with its own character, at least as long, and nothing after it. That fixes nesting and info strings together and keeps the mismatched-pair handling the previous commit added, at about the same size as the parity count. Selftest cases 6b and 6c cover the two shapes; both fail against the parity count and pass against the state machine. Also corrects a stale comment: the four characters reserved for the closer under-reserve for a ```` opener. `SAFETY_MARGIN` absorbs it, so the arithmetic stands and only the comment needed the word.
|
@dormouse-bot this got ridiculously complex. Just truncate at 32,000 chars, don't worry about nested fences. This has way too many comments and explanation, it should be much more concise. Add a note to AGENTS.md about being concise with comments, and don't duplicate in a comment something which is already explained in a spec (or vice-versa) |
…hinery Per review: the clamp had grown a fence state machine, a safety margin, a surrogate guard, and a pathological-note branch to defend a limit nobody was near. Truncating at 32,000 characters — half GitHub's ceiling — needs none of it. Helper and self-test drop from 369 lines to 134, and the explanation in the workflows, SECURITY.md, and AGENTS.md is stated once rather than four times.
|
Cut in The prose came down with it: the 22-line comment in On the AGENTS.md note — it is already there, and I did not add a second one. VerificationSelf-test still load-bearing — against a copy of the helper with Root gates on the pushed tree: |
dormouse-bot
left a comment
dormouse-bot
left a comment
There was a problem hiding this comment.
The simplification landed cleanly and the clamp is correct — both call sites cover every gh issue create / gh issue comment path in the two workflows, the CLI is a no-op below the limit so it can run unconditionally, and the six root gates plus the new self-test pass on the merged tree.
One half of nedtwigg's ask is still open: the rationale for the clamp now lives in three places, not one. SECURITY.md owns it (the script header already points there with a single line, which is the right shape), but both workflow comments restate it before pointing at it — AGENTS.md -> **Say it once.** allows "at most a one-line pointer" everywhere but the owning doc. Suggestions inline trim each to its pointer plus the fact the code can't show: for workflow-audit.yaml that's why $REPORT grows unboundedly and why the job needs no setup-node; for security-audit.yaml there's nothing local left to say.
Verification
Merged tree, refs/pull/480/merge:
$ node scripts/clamp-issue-body-selftest.mjs
clamp-issue-body-selftest: OK
$ node scripts/spec-lint.mjs
spec-lint: OK (24 specs, 25 files checked)
Call-site coverage — every body source in the two workflows, and each one is downstream of a clamp:
security-audit.yaml:496 gh issue comment "$EXISTING" --body-file audit-comment.md (clamp at :489)
security-audit.yaml:509 gh issue create ... --body-file audit-comment.md (clamp at :489)
workflow-audit.yaml:385 gh issue create ... --body-file "$BODY" (clamp at :379)
Both are single-job workflows with actions/checkout as step one, so scripts/clamp-issue-body.mjs is on disk when the reporting step runs, and Redact secrets from agent output only rewrites the transcript, audit-report.md, and $AUDIT_FRAGMENTS — it can't remove the helper out from under the clamp.
On the AGENTS.md question: your reply checks out. **Say it once.** is at AGENTS.md line 64 of the merged tree, carrying both halves of the ask, so not adding a second note was right.
SECURITY.md owns the rationale for the clamp; both workflow comments restated it before pointing there. Trim each to its pointer plus the facts local to the call site.
2 participants
The security audit found a BLOCKER on 2026-08-29 and reported it to nobody:
gh issue createrejected the 68 KB body withGraphQL: Body is too long (maximum is 65536 characters), that rejection landed on aset -estep after the verdict was decided, and the run went red with no issue and no comment.scripts/clamp-issue-body.mjstruncates the composed body to 32,000 characters before it is posted, keeping the head (verdict, links, dissent notes) and pointing at theaudit-transcriptartifact for the rest..github/workflows/workflow-audit.yamluses the same helper for its own unbounded commit list.The call is non-fatal (
|| echo …) so the truncation step cannot itself become the silencer it exists to prevent, and the script rewrites the file only when over the limit, so a normal-sized report is byte-identical afterwards.Verified by
scripts/clamp-issue-body-selftest.mjs(wired into the rootpnpm test): its first case reproduces the 68 KB body from run 33249330988 and asserts the result is postable with the head intact. Against a helper with the length check removed, 6 of its assertions go red. All six root gates pass, both workflows re-parse as YAML, and both touchedrun:blocks passbash -n.What is still silenced
audit-application.mdfrom that run opensVERDICT: FAILwith BLOCKER-1 against the iframe proxy: a hostile page's<iframe src="http://127.0.0.1:<port>/">satisfies theHostcheck (browsers omitOriginon a frame navigation), and the response path grants the stranger framing rights anyway via the unconditionalx-frame-options/content-security-policystrip iniframe-proxy-rewrite.ts, plus apostMessage(m, '*')URL side-channel towindow.parent. Stripping those headers is what makesdor iframework at all, so the threat model is your call, not something to slip into a reporting fix. This PR only ensures you get told; once it merges, tonight's audit files it properly on its own.