fix(github): bound an error body before redacting it, and collapse every line terminator

[devops-thiago]

Stacked on #738 (fix/730-retry-after-floor). Based against that branch so the diff reads clean; re-target to main once #738 merges. Only the last commit (154d172) belongs to this PR. Note that ci.yml triggers on pull requests into main/develop/release/** only, so the full CI run happens on re-target — the gates below were run locally on this exact tree.

What type of PR is this?

• 🐛 Bug fix
• 🔒 Security

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

readBody reads the entity with no size bound of its own, and clean() 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 is eyJ[\w-]{8,}\.[\w-]{8,}\.[\w-]{8,}. [\w-] excludes ., so on a body of repeated eyJ each 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:

body size	time
20 000	331 ms
40 000	1 213 ms
80 000	4 843 ms
160 000	19 404 ms
1.2 MB	~1 078 623 ms (≈ 18 minutes)

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.

var collapsed = WHITESPACE.matcher(raw).replaceAll(" ").strip();
var bounded = cutTo(collapsed, MAX_BODY_CHARS * 2);
var redacted = redactCredentials(bounded);
var capped = cutTo(redacted, MAX_BODY_CHARS);
return capped.length() < redacted.length() || bounded.length() < collapsed.length()
    ? capped + "…"
    : capped;

Every regex pass is now bounded at a constant. The pre-cut is twice MAX_BODY_CHARS rather 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 into cutTo and now protects both cuts.

2. \s is the ASCII six, so most line terminators survived the collapse

WHITESPACE was \\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

• The finding.file() interpolation (ReviewPublisher.java:752 and :814) — the third item on the issue. ReviewPublisher is being changed by separate work in this round, so touching it here would conflict; it needs MarkdownSafe.oneLine at both warn sites and is left to that change.
• Bounding readEntity itself. 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.
• A5's "match on more than the log-shaped body". That is a separate finding about which string the retry decision reads, not about the cost of producing it.

Related Issues

Fixes #731

How Has This Been Tested?

• Unit tests

Four tests added to GitHubApiErrorTest.BodyHandling, all reusing the audit's probes:

• doesNotScanAWholeOversizedBodyLookingForCredentials — 200 KB of eyJ (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.
• stripsALineTerminatorAtEitherEndRatherThanLeavingASpace
• marksABodyCutBeforeRedactionAsTruncatedEvenWhenTheMaskFitsUnderTheCap — a 4 000-char bearer value masks to ***, and the result must still say it was cut.

The existing capsAnOverLongBodySoOneFailureCannotFloodTheLog and neverCutsAnOverLongBodyThroughASurrogatePair are unchanged and still pass, which is what pins the cap and the surrogate guard through the restructuring.

Verbatim red output on unfixed code

[ERROR] Tests run: 15, Failures: 4, Errors: 0, Skipped: 0, Time elapsed: 95.25 s <<< FAILURE! -- in dev.thiagogonzaga.thrillhousebot.github.GitHubApiErrorTest$BodyHandling
[ERROR]   GitHubApiErrorTest.doesNotScanAWholeOversizedBodyLookingForCredentials cleaning a 200 KB body took 92877ms ==> expected: <true> but was: <false>
[ERROR]   GitHubApiErrorTest.collapsesTheLineTerminatorsAndControlsThatAreNotAsciiWhitespace expected: <a WARN forged-by-NEL WARN forged-by-LS WARN forged-by-PS NUL [2J ansi> but was: <a?WARN forged-by-NEL ?WARN forged-by-LS ?WARN forged-by-PS  NUL  ansi>
[ERROR]   GitHubApiErrorTest.stripsALineTerminatorAtEitherEndRatherThanLeavingASpace expected: <boom> but was: <boom ?>
[ERROR]   GitHubApiErrorTest.marksABodyCutBeforeRedactionAsTruncatedEvenWhenTheMaskFitsUnderTheCap expected: <***?> but was: <***>

(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 BodyHandling nest runs in 3.4 s, and all four go green:

[INFO] Tests run: 15, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 3.393 s -- in dev.thiagogonzaga.thrillhousebot.github.GitHubApiErrorTest$BodyHandling

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: 0
• jacoco ∩ git diff -U0 on 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

• My code follows the project's coding standards
• I have performed a self-review of my own code
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly
• My changes generate no new warnings or errors

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.

[thrillhousebot]

🤖 ThrillhouseBot PR Summary

What this PR does

Bounds 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.

⚠️ Description vs. Implementation

Every mismatch found between the description and the change is reported as a finding below, so it is not repeated here.

Control-Flow Diagram

🔀 Show diagram

flowchart TD
  A["raw body from response entity"] --> B{"raw == null?"}
  B -- "yes" --> Z["return empty string"]
  B -- "no" --> C["collapse controls, separators and ASCII whitespace to single spaces"]
  C --> D["strip leading and trailing spaces"]
  D --> E["cut to 1024 chars, never through a surrogate pair"]
  E --> F["redact credential-shaped values"]
  F --> G["cut to 512 chars, never through a surrogate pair"]
  G --> H{"either cut happened?"}
  H -- "yes" --> I["append ellipsis"]
  H -- "no" --> J["return capped text"]
  I --> K["return capped text plus ellipsis"]

Loading

Changes Overview

• Files changed: 2
• Lines added: +109
• Lines removed: -12

Changed Files

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]

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 before redactCredentials(bounded) (line 436) runs. The JWT alternative of CREDENTIAL_SHAPED_VALUE, quoted verbatim in this PR's description and Javadoc as eyJ[\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 ran redactCredentials(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.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[thrillhousebot]

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]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[devops-thiago]

@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:

org.opentest4j.AssertionFailedError: xxxx…xxxx eyJaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa.PPPPPPPP…PPPP? ==> expected: <false> but was: <true>

The JWT's third segment is now optional, so a token the bound cut mid-payload is masked from its eyJ header onward. The first dot stays mandatory deliberately: my first attempt made both tail segments optional, which matched any long run of word characters and swallowed an entire oversized body into *** — doesNotScanAWholeOversizedBodyLookingForCredentials caught it, and blanking 512 characters of diagnostic body would have been a worse trade than the hole being closed.

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 (:424, javadoc overstating that cutting bounds every regex pass) is also fixed in the same commit — the whitespace collapse is one linear pass over the whole body, and the text now says so.

New test: masksAJwtTheBoundCutMidPayload. Suite 3302 green, BugInstance 0, changed lines and branches fully covered.

---

Generated by Claude Code

[thrillhousebot]

The next review will close every previous finding this comment names by its path:line and title; anything it does not name stays open.

[thrillhousebot]

🤖 ThrillhouseBot — changes since the last review

• New findings this round: 3
• Previous findings resolved: 2
  • 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
  • src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:424 — Javadoc says every regex pass is bounded at a constant; the collapse pass still scans the whole body
• Previous findings still open: 0

[thrillhousebot]

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.

[devops-thiago]

@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 \p{IsCf} alongside \p{IsCc}. Red first, verbatim:

org.opentest4j.AssertionFailedError: expected: <before after isolated> but was: <before?after?isolated>

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: collapsesBidiOverridesThatCouldReorderTheLoggedLine.

---

Generated by Claude Code

[thrillhousebot]

The next review will close every previous finding this comment names by its path:line and title; anything it does not name stays open.

[thrillhousebot]

🤖 ThrillhouseBot — changes since the last review

• New findings this round: 0
• Previous findings resolved: 3
  • src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:80 — Pre-cut still splits a JWT whose visible segment is under 8 chars, leaking header and payload prefix
  • src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java:71 — Javadoc claims any mid-payload cut is masked; segments shorter than 8 chars pass unmasked
  • 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
• Previous findings still open: 0

[thrillhousebot]

ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:

• Check trivy is pending
• Check test is pending
• Check format is pending
• Check dependency-review is pending
• Check frontend is pending

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud