fix(github): bound an error body before redacting it, and collapse every line terminator - #740
Conversation
🤖 ThrillhouseBot PR SummaryWhat this PR doesBounds the error-body logging path: clean() now collapses Unicode controls and line/paragraph separators, pre-cuts the collapsed body to 1024 chars before credential redaction, caps at 512 chars after, and appends an ellipsis when either cut fired; factors the surrogate-pair guard into a shared cutTo helper and adds four BodyHandling tests covering cost, control collapsing, edge stripping and the ellipsis semantics.
|
| File | Change | Summary |
|---|---|---|
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java |
Modified | Reorder clean() collapse/pre-cut/redact/cap pipeline; widen WHITESPACE to Unicode controls; add cutTo. |
src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java |
Modified | Four new BodyHandling tests: bounded redaction cost, Unicode terminator collapse, edge strip, pre-cut ellipsis. |
Risk Assessment
| Risk | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟠 High | 0 |
| 🟡 Medium | 1 |
| 🔵 Low | 1 |
Key Findings
- LOW: Javadoc says every regex pass is bounded at a constant; the collapse pass still scans the whole body (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:424)
Things to double-check
1 lower-confidence finding
- MEDIUM: Pre-cut can split an unlabeled JWT so redaction misses it and payload chars reach the log (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:435) (low confidence — verify before acting)
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- MEDIUM: Pre-cut can split an unlabeled JWT so redaction misses it and payload chars reach the log (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:435)
Input not in the diff: a collapsed body longer than 1024 chars in which an unlabeled JWT (no "Bearer " prefix) starts within the first ~512 chars and is long enough that the pre-cut lands inside its middle (in the payload segment before the second dot, or within the first 7 chars after the final dot).var bounded = cutTo(collapsed, MAX_BODY_CHARS * 2);truncates the token beforeredactCredentials(bounded)(line 436) runs. The JWT alternative of CREDENTIAL_SHAPED_VALUE, quoted verbatim in this PR's description and Javadoc aseyJ[\w-]{8,}\.[\w-]{8,}\.[\w-]{8,}, requires all three dot-separated segments with 8+ chars after each dot inside the 1024-char window. When the window ends inside the payload before its closing dot, or with fewer than 8 chars after the final dot, the alternative fails and the truncated token is returned unredacted; the 512-char cap then logs the prefix plus the JWT header and a large part of the payload verbatim (e.g. ~100 chars of prefix + a ~1100-char JWT whose second dot sits beyond index 1024 exposes header + ~387 payload chars). The removed code ranredactCredentials(collapsed)over the whole collapsed body first, so the complete token matched and was masked before the cap — this is a regression of the redaction guarantee on the very path this PR hardens, under the same threat model the PR cites (hostile bodies from a compromised endpoint). The in-diff tests do not exercise this shape: the 200 KB test body is dotless "eyJ" runs that match in neither version, and the bearer test is rescued by the "Bearer" label alternative. Verify the full CREDENTIAL_SHAPED_VALUE definition — if another alternative matches a bare word-char run (e.g. a generic[\w-]+without the dot structure), the leak is closed. Otherwise the pre-cut must not split a token: redact the tail before cutting, or add a truncation-tolerant alternative that masks an unterminated eyJ-shaped prefix at the window boundary.
thrillhousebot
Bot
added
bug
…ery line terminator clean() redacted the whole body and capped it only afterwards, so the credential patterns scanned however much the configured API host chose to send. The JWT alternative backtracks from one position in three, which is quadratic: 20 000 chars cost 331ms, 160 000 cost 19 404ms, and a 1.2 MB body about eighteen minutes of CPU inside GitHubApiError.from — on the review's own carrier thread, in the path that exists to explain a failed write. Cutting to twice the 512-char cap first bounds every pass at a constant; the ellipsis now marks either cut. The collapse pass also used \s, which java.util.regex reads as the ASCII six, so NEL, LINE SEPARATOR, PARAGRAPH SEPARATOR, NUL and the ANSI escape reached the log line intact — enough to forge what reads as a second log record. It now covers the Unicode Cc category and the two separators. Fixes #731
154d172 to
163cde9
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:
- Check format is pending
- Check test is pending
- Check frontend is pending
- Check trivy is pending
- Check dependency-review is pending
Additionally, No new issues in this revision, but 2 previous finding(s) remain unresolved — fix them, or reply on their review thread with why they are deferred. A finding listed only under "Things to double-check" has no thread: clear it by commenting @thrillhousebot resolved path/to/File.java:42 — <the finding's title> on this PR.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Bounding the body before redacting narrowed what redaction covers: a JWT whose second dot fell past the bound no longer matched the three-segment shape, so it went through unmasked and the cap logged its header and the payload characters that fit. The third segment is now optional, which masks a token cut mid-payload; the first dot stays mandatory, since a run of word characters carrying no dot is not a JWT and matching one would blank the body this line exists to explain. Also corrects the clean() javadoc: the whitespace collapse is one linear pass over the whole body, and it is the cut that bounds the redaction pass, not every pass. Refs #731
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:435 — Pre-cut can split an unlabeled JWT so redaction misses it and payload chars reach the log Confirmed and fixed, not deferred — the finding was right that bounding before redaction narrowed what redaction covers, and that this PR introduced it. Reproduced verbatim before the fix: The JWT's third segment is now optional, so a token the bound cut mid-payload is masked from its One correction to my own reasoning in passing: with the mandatory first dot this does not eliminate the quadratic backtracking, so the 1024-char bound remains the thing that keeps the cost constant. The javadoc says that now rather than claiming otherwise. The LOW on the same file ( New test: Generated by Claude Code |
|
The next review will close every previous finding this comment names by its |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: Bidi format controls (U+202E/U+2066, category Cf) survive the collapse and can reorder log text (
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:113)
Input not in the diff: a 4xx body containing U+202E (RIGHT-TO-LEFT OVERRIDE) or U+2066 (LEFT-TO-RIGHT ISOLATE), Unicode category Cf rather than Cc. The new collapse class[\s\p{IsCc}\u2028\u2029]+covers ASCII whitespace, C0/C1 controls (NUL, ESC, NEL, ...) and U+2028/U+2029, but not Cf format controls, so bidi overrides reach the WARN line unchanged. The Javadoc's own contract for this pass is that a "log viewer, a terminal, or a JSON/ECS shipper" must not receive record boundaries or screen-control sequences; a terminal that honors the Unicode bidi algorithm (iTerm2, Windows Terminal; xterm-class terminals typically ignore it) renders the text after the override right-to-left, which can reorder or hide log content — the same log-forgery family the PR fixes, though weaker (visual reordering, not record splitting). Verify whether the deployment's log consumers honor bidi overrides; if they do, extend the class with\p{Cf}, which also collapses ZWSP/ZWNJ/BOM — harmless in a one-line log.
… controls A per-segment length floor left a token unmasked when the bound cut within the first few payload characters, and a masked bearer run ahead of it shortens the text so the leak lands inside the cap rather than past it. Everything after the first dot is now optional in length and count. The first dot stays mandatory, so the one unmasked shape is a cut before it - the header prefix, which carries no secret; the javadoc says that instead of claiming every cut is covered. The collapse also takes Cf format controls: a bidi override is not Cc, and reordering what an operator reads is the same family of harm as splitting the record. Refs #731
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:113 — Bidi format controls (U+202E/U+2066, category Cf) survive the collapse and can reorder log text Fixed rather than deferred. The collapse class now takes Reasoning for taking it rather than gating it on whether the deployment's consumers honour bidi: the javadoc's contract for this pass is that no log viewer, terminal or shipper receives control sequences, and that contract shouldn't depend on which terminal happens to read the line. Collapsing ZWSP/ZWNJ/BOM along with it is harmless on a single-line log. New test: Generated by Claude Code |
|
The next review will close every previous finding this comment names by its |
🤖 ThrillhouseBot — changes since the last review
|
|
1 participant
What type of PR is this?
Description
Two defects in the same method, both in the path that exists to explain a failed write.
1. Redaction ran before the cap, and the JWT shape is quadratic — the primary fix
readBodyreads the entity with no size bound of its own, andclean()redacted the whole of it, capping at 512 characters only afterwards. So the cost of logging one 4xx was set by whatever the configured API host chose to send.CREDENTIAL_SHAPED_VALUE's JWT alternative iseyJ[\w-]{8,}\.[\w-]{8,}\.[\w-]{8,}.[\w-]excludes., so on a body of repeatedeyJeach greedy run consumes to end-of-input, fails to find its separator and backtracks one character at a time — from one position in three. That is quadratic, and it was measured as such:Every doubling costs about 4×. That time is spent on the review's own carrier thread, inside
GitHubApiError.from, before the retry decision the cleaned body feeds is even reached.The fix is the one the audit called for: cut, then redact, then cap.
Every regex pass is now bounded at a constant. The pre-cut is twice
MAX_BODY_CHARSrather than exactly it because redaction only ever shortens, so the wider window leaves material to fill the 512-char cap with; past that the output was going to be truncated anyway, and what a wider window would pull into view is more of the token-shaped run being masked out. The ellipsis now marks either cut, so a body shortened before redaction is never mistaken for one GitHub sent whole. The surrogate-pair guard that protected the 512 cut is factored intocutToand now protects both cuts.2.
\sis the ASCII six, so most line terminators survived the collapseWHITESPACEwas\\s+, which java.util.regex reads as[ \t\n\x0B\f\r]unless the pattern asks for Unicode character classes. CR and LF being collapsed closes the classic forged-record vector, but NEL (U+0085), LINE SEPARATOR (U+2028), PARAGRAPH SEPARATOR (U+2029), NUL and the ANSI escape all reached the warn line intact — and a log viewer, a terminal, or a JSON/ECS shipper may treat any of them as a record boundary or a screen-control sequence. The class already documents a body as attacker-influenced text on its way to a log file and already pays for a collapse pass on that basis; the pass simply did not cover the class it claimed to.Now
[\\s\\p{IsCc}\\u2028\\u2029]+—\p{IsCc}is the Unicode general category rather than POSIX\p{Cntrl}, so it reaches the C1 controls (U+0080–U+009F, NEL among them) as well as C0 and DEL. The strip moved after the collapse so a terminator at either end does not survive as a stray space.Not in this PR
finding.file()interpolation (ReviewPublisher.java:752and:814) — the third item on the issue.ReviewPublisheris being changed by separate work in this round, so touching it here would conflict; it needsMarkdownSafe.oneLineat both warn sites and is left to that change.readEntityitself. The audit lists it as optional. Every regex pass is bounded now, and the remaining cost of a huge body is one linear collapse pass plus the read that already happened.Related Issues
Fixes #731
How Has This Been Tested?
Four tests added to
GitHubApiErrorTest.BodyHandling, all reusing the audit's probes:doesNotScanAWholeOversizedBodyLookingForCredentials— 200 KB ofeyJ(probe P2c), bounded at 2 000 ms. The bound is enormously slack against what this costs once the body is cut first; it is sized to fail only on the quadratic, never on a slow machine.collapsesTheLineTerminatorsAndControlsThatAreNotAsciiWhitespace— probe P3b's body, asserted as one clean line.stripsALineTerminatorAtEitherEndRatherThanLeavingASpacemarksABodyCutBeforeRedactionAsTruncatedEvenWhenTheMaskFitsUnderTheCap— a 4 000-char bearer value masks to***, and the result must still say it was cut.The existing
capsAnOverLongBodySoOneFailureCannotFloodTheLogandneverCutsAnOverLongBodyThroughASurrogatePairare unchanged and still pass, which is what pins the cap and the surrogate guard through the restructuring.Verbatim red output on unfixed code
(The
?are the terminal's rendering of the surviving U+0085 / U+2028 / U+2029 and of the ellipsis.)92 877 ms for one 200 KB body — this machine is slower than the one the audit measured 24 s on, which only sharpens the point. After the fix the whole 15-test
BodyHandlingnest runs in 3.4 s, and all four go green:Gates
./mvnw -B spotless:apply→ clean./mvnw -B clean compile spotbugs:check spotless:check→BugInstance size is 0, BUILD SUCCESS./mvnw -B clean test→Tests run: 3290, Failures: 0, Errors: 0, Skipped: 0git diff -U0on this commit's main code → 0 uncovered lines, 0 uncovered branches (both conditions of the ellipsis test and both arms of the surrogate guard are exercised)Checklist
Screenshots / Logs
See the verbatim red output above.
Additional Notes
Severity is latent, not live: GitHub's real error bodies are around 300 characters, and no path was found by which a PR author gets GitHub itself to echo hundreds of kilobytes into a 4xx body. It needs a large body from the configured API host — a GHES or reverse-proxy error page, a misconfigured base URL, a compromised endpoint. The redaction half is pre-existing from #704 (v0.6.2); #723 made the same
clean()output load-bearing for a retry decision, and promoted a debug line to warn, which is what widened the exposure of the second half.