From 163cde9d42e9b26a179268901b7dc42920ecd196 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 16 Aug 2026 00:59:41 +0000 Subject: [PATCH 1/3] fix(github): bound an error body before redacting it, and collapse every line terminator MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../thrillhousebot/github/GitHubApiError.java | 67 +++++++++++++++---- .../github/GitHubApiErrorTest.java | 54 +++++++++++++++ 2 files changed, 109 insertions(+), 12 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index c223dbef..b6698e19 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -88,8 +88,22 @@ public final class GitHubApiError { "(?i)secondary rate limit|abuse detection|rate limit exceeded" + "|blocked from (?:content creation|creating content)"); - /** Collapses the whitespace of a body so one failure stays on one log line. */ - private static final Pattern WHITESPACE = Pattern.compile("\\s+"); + /** + * Collapses the whitespace of a body so one failure stays on one log line. + * + *

Wider than {@code \s}, which java.util.regex reads as the ASCII six ({@code [ + * \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 survived it (#731) — and a log + * viewer, a terminal, or a JSON/ECS shipper may treat any of them as a record boundary or as a + * screen-control sequence. This class documents a body as attacker-influenced text on its way to + * a log file and already pays for a collapse pass on that basis; this is that pass covering what + * it claims to. + * + *

{@code \p{IsCc}} is the Unicode general category rather than POSIX {@code \p{Cntrl}}, so it + * reaches the C1 controls (U+0080–U+009F, NEL among them) as well as C0 and DEL. + */ + private static final Pattern WHITESPACE = Pattern.compile("[\\s\\p{IsCc}\\u2028\\u2029]+"); /** Backoff used when GitHub throttles without saying for how long. */ static final Duration FALLBACK_DELAY = Duration.ofSeconds(5); @@ -392,20 +406,49 @@ private static String readBody(Response response) { } } + /** + * A response body as one line of loggable text: collapsed, bounded, redacted, then capped at + * {@link #MAX_BODY_CHARS}. + * + *

The bound before the redaction is the point (#731). {@code readBody} reads the entity with + * no size limit of its own, and redaction used to run over all of it, so the cost of explaining a + * failed write was set by whatever the configured API host chose to send. {@link + * #CREDENTIAL_SHAPED_VALUE}'s JWT alternative is quadratic on a body of repeated {@code eyJ} — + * {@code [\w-]} excludes {@code .}, so each greedy run consumes to end-of-input, fails to find + * its separator and backtracks a character at a time, from one in every three positions. + * Measured: 20 000 chars cost 331 ms, 40 000 cost 1 213 ms, 80 000 cost 4 843 ms, 160 000 cost 19 + * 404 ms, and a 1.2 MB body cost about eighteen minutes of CPU — spent on the review's own + * carrier thread, inside the failure path that exists to explain a failed write, before the retry + * decision it feeds is even reached. GitHub's real error bodies are ~300 characters; a GHES or + * reverse-proxy error page, a misconfigured base URL or a compromised endpoint is not. + * + *

Cutting first bounds every regex pass at a constant. The cut is twice {@link + * #MAX_BODY_CHARS} so that redaction, which only ever shortens, still has material to fill the + * 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 that is being masked out. The ellipsis marks + * either cut, so a body shortened here is never mistaken for a body GitHub sent whole. + */ private static String clean(String raw) { if (raw == null) { return ""; } - var collapsed = WHITESPACE.matcher(raw.strip()).replaceAll(" "); - var redacted = redactCredentials(collapsed); - if (redacted.length() <= MAX_BODY_CHARS) { - return redacted; + 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; + } + + /** + * {@code text} cut to at most {@code limit} characters, never through a surrogate pair — a + * dangling high surrogate at the cut point would corrupt a code point. + */ + private static String cutTo(String text, int limit) { + if (text.length() <= limit) { + return text; } - // Never leave a dangling high surrogate at the cut point — that would corrupt a code point. - int keep = - Character.isHighSurrogate(redacted.charAt(MAX_BODY_CHARS - 1)) - ? MAX_BODY_CHARS - 1 - : MAX_BODY_CHARS; - return redacted.substring(0, keep) + "…"; + return text.substring(0, Character.isHighSurrogate(text.charAt(limit - 1)) ? limit - 1 : limit); } } diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java index 8c1cae4c..960518c8 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java @@ -517,6 +517,60 @@ void neverCutsAnOverLongBodyThroughASurrogatePair() { assertTrue(body.endsWith("…")); assertEquals(-1, body.indexOf('\uD834'), "no dangling high surrogate"); } + + /** + * #731. Redaction used to run over the whole body and the cap only afterwards, so the cost of + * explaining one failed write was set by whatever the configured host chose to send: the JWT + * shape backtracks from one position in three, which is quadratic, and the measured curve ran + * 20 000 chars → 331 ms, 40 000 → 1 213 ms, 80 000 → 4 843 ms, 160 000 → 19 404 ms. The bound + * below is enormously slack against the ~1 ms this costs once the body is cut first; it is + * sized to fail only on the quadratic, not on a slow machine. + */ + @Test + void doesNotScanAWholeOversizedBodyLookingForCredentials() { + var body = "eyJ".repeat(66_666); // ~200 KB, as a proxy's error page could be + + var start = System.nanoTime(); + var logged = loggedBody(outbound(403, body)); + var millis = (System.nanoTime() - start) / 1_000_000; + + assertTrue(millis < 2_000, "cleaning a 200 KB body took " + millis + "ms"); + assertEquals(513, logged.length(), logged); + } + + /** + * #731. {@code \s} is the ASCII six in java.util.regex, so the collapse caught CR and LF and + * let every other record separator through — enough for an attacker-influenced body to forge + * what reads as a second log line, or to carry an ANSI sequence into an operator's terminal. + */ + @Test + void collapsesTheLineTerminatorsAndControlsThatAreNotAsciiWhitespace() { + var body = + "a\u0085WARN forged-by-NEL \u2028WARN forged-by-LS \u2029WARN forged-by-PS" + + " \u0000NUL \u001b[2J ansi"; + + var logged = loggedBody(outbound(403, body)); + + assertEquals("a WARN forged-by-NEL WARN forged-by-LS WARN forged-by-PS NUL [2J ansi", logged); + } + + /** The same collapse must not leave a terminator at either end behind as a stray space. */ + @Test + void stripsALineTerminatorAtEitherEndRatherThanLeavingASpace() { + assertEquals("boom", loggedBody(outbound(500, "\u2028 boom \u0085"))); + } + + /** + * A body cut before redaction still says it was cut, even when the mask leaves the result well + * under the cap — otherwise a heavily redacted 200 KB page would read as something GitHub sent + * whole. + */ + @Test + void marksABodyCutBeforeRedactionAsTruncatedEvenWhenTheMaskFitsUnderTheCap() { + var body = loggedBody(outbound(401, "Bearer " + "a".repeat(4_000))); + + assertEquals("***…", body); + } } @Nested From cae073c1eb32e245d43ef45dfad85165d0201259 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 16 Aug 2026 04:10:58 +0000 Subject: [PATCH 2/3] fix(github): keep redaction whole when the bound cuts a token 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/github/GitHubApiError.java | 20 +++++++++++++------ .../github/GitHubApiErrorTest.java | 18 +++++++++++++++++ 2 files changed, 32 insertions(+), 6 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index b6698e19..99141193 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -67,10 +67,17 @@ public final class GitHubApiError { /** * The bearer and JWT shapes — the value half of {@link #CREDENTIAL_SHAPED_PREFIX}'s union, tried * second on a position tie exactly as the one-alternation form tried its alternatives in order. + * + *

The JWT's third segment is optional, so a token the bound below cut mid-payload is still + * masked. Requiring all three made redaction depend on where the cut happened to land: a token + * whose second dot fell past the bound went through unmasked, and its header and the payload + * characters that fit reached the log. The first dot stays mandatory: a run of word characters + * carrying no dot at all is not a JWT, and matching one would mask any long unbroken run, + * blanking the very body this line exists to explain. */ private static final Pattern CREDENTIAL_SHAPED_VALUE = Pattern.compile( - "(?i)(bearer\\s+[\\w.~+/=-]{10,})" + "|(eyJ[\\w-]{8,}\\.[\\w-]{8,}\\.[\\w-]{8,})"); + "(?i)(bearer\\s+[\\w.~+/=-]{10,})" + "|(eyJ[\\w-]{8,}(?:\\.[\\w-]{8,}){1,2})"); /** * The wording GitHub uses when it is throttling rather than refusing. A secondary rate limit and @@ -422,11 +429,12 @@ private static String readBody(Response response) { * decision it feeds is even reached. GitHub's real error bodies are ~300 characters; a GHES or * reverse-proxy error page, a misconfigured base URL or a compromised endpoint is not. * - *

Cutting first bounds every regex pass at a constant. The cut is twice {@link - * #MAX_BODY_CHARS} so that redaction, which only ever shortens, still has material to fill the - * 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 that is being masked out. The ellipsis marks - * either cut, so a body shortened here is never mistaken for a body GitHub sent whole. + *

The collapse above is one linear pass over the whole body; cutting first bounds the + * redaction pass at a constant. The cut is twice {@link #MAX_BODY_CHARS} so that redaction, which + * only ever shortens, still has material to fill the 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 that is being masked out. The ellipsis marks either cut, so a body shortened here is never + * mistaken for a body GitHub sent whole. */ private static String clean(String raw) { if (raw == null) { diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java index 960518c8..d809b36b 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java @@ -417,6 +417,24 @@ void omitsAHeaderThatArrivedEmpty() { @Nested class BodyHandling { + @Test + void masksAJwtTheBoundCutMidPayload() { + // #731 follow-up: bounding before redaction must not narrow what redaction covers. A token + // whose second dot falls past the bound used to go through unmasked, and the cap then logged + // its header and the payload chars that fit — the bound's own doing, on the body of a + // credential the redaction exists to remove. + var prefix = "x".repeat(100); + var header = "a".repeat(50); + var payload = "P".repeat(1200); + var body = prefix + " eyJ" + header + "." + payload + "." + "s".repeat(50); + + var cleaned = loggedBody(outbound(403, body)); + + assertFalse(cleaned.contains("PPPPPPPP"), cleaned); + assertFalse(cleaned.contains(header), cleaned); + assertTrue(cleaned.contains("***"), cleaned); + } + @Test void readsAnInboundResponseWithoutConsumingItForTheCaller() { var response = inbound(403, SECONDARY_LIMIT_BODY, "Retry-After", "60"); From d33b78643fe570f1ecdc6761a80ef78588dad26f Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 16 Aug 2026 04:37:16 +0000 Subject: [PATCH 3/3] fix(github): mask a cut token from its first dot, and collapse format 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/github/GitHubApiError.java | 23 ++++++++----- .../github/GitHubApiErrorTest.java | 34 +++++++++++++++++++ 2 files changed, 48 insertions(+), 9 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index 99141193..c1709722 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -68,16 +68,20 @@ public final class GitHubApiError { * The bearer and JWT shapes — the value half of {@link #CREDENTIAL_SHAPED_PREFIX}'s union, tried * second on a position tie exactly as the one-alternation form tried its alternatives in order. * - *

The JWT's third segment is optional, so a token the bound below cut mid-payload is still - * masked. Requiring all three made redaction depend on where the cut happened to land: a token - * whose second dot fell past the bound went through unmasked, and its header and the payload - * characters that fit reached the log. The first dot stays mandatory: a run of word characters - * carrying no dot at all is not a JWT, and matching one would mask any long unbroken run, - * blanking the very body this line exists to explain. + *

Everything after the first dot is optional in both length and count, so a token the bound + * below cut anywhere past that dot is masked whole — including a cut landing in the first few + * payload characters, which a {@code {8,}} on each segment would have let through. Requiring all + * three segments made redaction depend on where the cut happened to land: a token whose second + * dot fell past the bound went through unmasked, and its header and the payload characters that + * fit reached the log. + * + *

The first dot stays mandatory, so the one shape still not masked is a cut before it: the + * {@code eyJ} header prefix alone, which carries the algorithm and type claims and no secret. + * Matching a dotless run instead would mask every long unbroken run of word characters and blank + * the very body this line exists to explain. */ private static final Pattern CREDENTIAL_SHAPED_VALUE = - Pattern.compile( - "(?i)(bearer\\s+[\\w.~+/=-]{10,})" + "|(eyJ[\\w-]{8,}(?:\\.[\\w-]{8,}){1,2})"); + Pattern.compile("(?i)(bearer\\s+[\\w.~+/=-]{10,})" + "|(eyJ[\\w-]{8,}(?:\\.[\\w-]*){1,2})"); /** * The wording GitHub uses when it is throttling rather than refusing. A secondary rate limit and @@ -110,7 +114,8 @@ public final class GitHubApiError { *

{@code \p{IsCc}} is the Unicode general category rather than POSIX {@code \p{Cntrl}}, so it * reaches the C1 controls (U+0080–U+009F, NEL among them) as well as C0 and DEL. */ - private static final Pattern WHITESPACE = Pattern.compile("[\\s\\p{IsCc}\\u2028\\u2029]+"); + private static final Pattern WHITESPACE = + Pattern.compile("[\\s\\p{IsCc}\\p{IsCf}\\u2028\\u2029]+"); /** Backoff used when GitHub throttles without saying for how long. */ static final Duration FALLBACK_DELAY = Duration.ofSeconds(5); diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java index d809b36b..c5150564 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java @@ -417,6 +417,40 @@ void omitsAHeaderThatArrivedEmpty() { @Nested class BodyHandling { + @Test + void masksAJwtTheBoundCutWithinTheFirstPayloadCharacters() { + // The narrow sibling of the case below: when the cut leaves fewer than a segment's worth of + // payload visible, a per-segment length floor would let the token through. A masked bearer + // run ahead of it shortens the redacted text, so the cap no longer drops the tail and the + // leak lands inside the logged line rather than past its end. + var body = + "Bearer " + + "a".repeat(500) + + "x".repeat(455) + + " eyJ" + + "h".repeat(50) + + "." + + "p".repeat(7) + + "P".repeat(200) + + "." + + "s".repeat(50); + + var cleaned = loggedBody(outbound(403, body)); + + assertFalse(cleaned.contains("h".repeat(50)), cleaned); + assertFalse(cleaned.contains("ppppppp"), cleaned); + } + + @Test + void collapsesBidiOverridesThatCouldReorderTheLoggedLine() { + // Cf format controls are not Cc: a right-to-left override reaching a terminal that honours + // the bidi algorithm reorders what an operator reads, which is the same family of harm as + // the record-splitting this collapse exists to stop. + var cleaned = loggedBody(outbound(500, "before\u202Eafter\u2066isolated")); + + assertEquals("before after isolated", cleaned); + } + @Test void masksAJwtTheBoundCutMidPayload() { // #731 follow-up: bounding before redaction must not narrow what redaction covers. A token