From 613007164ef87d032997b998beb7c395c8dd6f99 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 16 Aug 2026 23:11:57 +0000 Subject: [PATCH 1/5] fix(github): take the credential value floors to a single character MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The bound before redaction cuts a response body at 1024 characters, and a token severed below a shape's length floor stops matching and reaches the warn line. #746 lowered the three floors from ten to four on that basis but stopped there, which narrows the window rather than closing it: a cut that leaves one, two or three value characters is still under four, so the sigil and up to three characters of the secret still reach the log. Lower all three shapes to one — gh[pousr]_, github_pat_ and bearer. The sigil is the whole discriminator, which is the argument the javadoc already makes; none of the three occurs in prose, so the length was never carrying the discrimination. One is where the argument ends, since a cut leaving no value characters strands the sigil alone. The javadoc described the severed-token case as closed while three characters still went through; it now states the residual it actually has. Pins the boundary in ACredentialTheBoundCutJustShortOfItsLengthFloor at one surviving character for each shape alongside the nine it already covered, plus the zero-character case the javadoc names. Every over-masking control (gh_2.40.0, ghost_writer, the ghp_ prefix in prose) stays green. Fixes #757 --- .../thrillhousebot/github/GitHubApiError.java | 26 ++++---- .../github/GitHubApiErrorTest.java | 59 +++++++++++++++---- 2 files changed, 63 insertions(+), 22 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index 913d0529..4fe50e1b 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -82,15 +82,19 @@ public final class GitHubApiError { * across every pattern — split apart, prefixes here and the value shapes beside it, only because * one alternation of all four shapes is more than the regex complexity budget allows. * - *

Four value characters, not ten (#746). The sigil is the whole discriminator here — {@code - * ghp_} and {@code github_pat_} do not occur in prose, so the length floor was buying nothing the - * sigil did not already buy, while costing the one thing that matters: a token the bound before - * redaction severs below the floor stops matching and reaches the log with its first nine - * characters intact. This is the same reasoning #740 applied to the JWT alternative below and did - * not carry across to the shapes that still needed it. + *

One value character, not ten (#746, #757). The sigil is the whole discriminator here — + * {@code ghp_} and {@code github_pat_} do not occur in prose, so the length floor was buying + * nothing the sigil did not already buy, while costing the one thing that matters: a token the + * bound before redaction severs below the floor stops matching and reaches the log with whatever + * the cut left of it intact. #746 made that argument and then stopped at four, which leaves the + * same window three characters wide instead of closing it: a cut leaving one, two or three value + * characters is still under the floor and still reaches the warn line. One is where the argument + * ends, because a cut that leaves no value characters at all strands the sigil by itself, and a + * sigil is not secret. This is the same reasoning #740 applied to the JWT alternative below and + * did not carry across to the shapes that still needed it. */ private static final Pattern CREDENTIAL_SHAPED_PREFIX = - Pattern.compile("(?i)(gh[pousr]_\\w{4,})|(github_pat_\\w{4,})"); + Pattern.compile("(?i)(gh[pousr]_\\w{1,})|(github_pat_\\w{1,})"); /** * The bearer shape, and with {@link #JWT_SHAPED_VALUE} below it the value half of {@link @@ -124,9 +128,9 @@ public final class GitHubApiError { * matched, since widening the anchor would mask ordinary prose beginning {@code ey} for a shape * no issuer emits. * - *

The value takes the same four-character floor as {@link #CREDENTIAL_SHAPED_PREFIX}, for the - * same reason: the {@code Bearer } that precedes it is the discriminator, and ten characters only - * meant the bound could sever a header value into something that no longer looked like one. + *

The value takes the same one-character floor as {@link #CREDENTIAL_SHAPED_PREFIX}, for the + * same reason: the {@code Bearer } that precedes it is the discriminator, and any floor above one + * only meant the bound could sever a header value into something that no longer looked like one. * *

The word {@code bearer} is consumed with the value rather than left in the line, which does * mask the noun in prose such as {@code missing bearer token}. Masking only the value was tried @@ -135,7 +139,7 @@ public final class GitHubApiError { * leftmost-match design above exists to swallow. */ private static final Pattern BEARER_SHAPED_VALUE = - Pattern.compile("bearer\\s+[\\w.~+/=-]{4,}", Pattern.CASE_INSENSITIVE); + Pattern.compile("bearer\\s+[\\w.~+/=-]{1,}", Pattern.CASE_INSENSITIVE); /** * The JWT shape, kept apart from {@link #BEARER_SHAPED_VALUE} rather than alternated with it: one diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java index ced9d5ec..e6503954 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java @@ -762,10 +762,14 @@ void marksABodyCutBeforeRedactionAsTruncatedEvenWhenTheMaskFitsUnderTheCap() { } /** - * #746. The bound before redaction is the only cut that can sever a token — the cap runs after - * the mask — and a shape with a ten-character floor stops matching once it has been severed + * #746, #757. The bound before redaction is the only cut that can sever a token — the cap runs + * after the mask — and a shape with a length floor stops matching once it has been severed * below it. A run of credential-shaped material ahead of the token compresses to {@code ***}, * so what the bound left of the secret survives the cap and reaches the warn line whole. + * + *

Each shape is pinned at nine surviving value characters and at one, because a floor is + * only closed at its own boundary: #746's {@code {4,}} masks nine and still strands one, two or + * three. */ @Nested class ACredentialTheBoundCutJustShortOfItsLengthFloor { @@ -773,16 +777,17 @@ class ACredentialTheBoundCutJustShortOfItsLengthFloor { /** Enough credential-shaped material to redact to {@code ***} and clear the cap for us. */ private static final String COMPRESSIBLE = "Bearer " + "a".repeat(900); - /** A body whose {@code sigil} lands so that the 1024-char bound leaves nine value chars. */ - private static String cutAfterNineCharactersOf(String sigil, String value) { - var padding = 1_024 - sigil.length() - 9 - COMPRESSIBLE.length(); + /** + * A body whose {@code sigil} lands so the 1024-char bound leaves {@code visible} value chars. + */ + private static String cutLeaving(int visible, String sigil, String value) { + var padding = 1_024 - sigil.length() - visible - COMPRESSIBLE.length(); return COMPRESSIBLE + ",".repeat(padding) + sigil + value; } @Test void isStillMaskedForATokenPrefix() { - var logged = - loggedBody(outbound(403, cutAfterNineCharactersOf("ghp_", "A1b2C3d4E5f6G7h8I9j0K1"))); + var logged = loggedBody(outbound(403, cutLeaving(9, "ghp_", "A1b2C3d4E5f6G7h8I9j0K1"))); assertFalse(logged.contains("ghp_A1b2C3d4E"), logged); } @@ -790,19 +795,51 @@ void isStillMaskedForATokenPrefix() { @Test void isStillMaskedForAFineGrainedPersonalAccessToken() { var logged = - loggedBody( - outbound(403, cutAfterNineCharactersOf("github_pat_", "A1b2C3d4E5f6G7h8I9j0K1"))); + loggedBody(outbound(403, cutLeaving(9, "github_pat_", "A1b2C3d4E5f6G7h8I9j0K1"))); assertFalse(logged.contains("github_pat_A1b2C3d4E"), logged); } @Test void isStillMaskedForABearerValue() { - var logged = - loggedBody(outbound(403, cutAfterNineCharactersOf("Bearer ", "S3cr3tV4lu3W1thM0re"))); + var logged = loggedBody(outbound(403, cutLeaving(9, "Bearer ", "S3cr3tV4lu3W1thM0re"))); assertFalse(logged.contains("Bearer S3cr3tV4l"), logged); } + + @Test + void isStillMaskedForATokenPrefixCutToASingleCharacter() { + var logged = loggedBody(outbound(403, cutLeaving(1, "ghp_", "A1b2C3d4E5f6G7h8I9j0K1"))); + + assertFalse(logged.contains("ghp_A"), logged); + } + + @Test + void isStillMaskedForAFineGrainedPersonalAccessTokenCutToASingleCharacter() { + var logged = + loggedBody(outbound(403, cutLeaving(1, "github_pat_", "A1b2C3d4E5f6G7h8I9j0K1"))); + + assertFalse(logged.contains("github_pat_A"), logged); + } + + @Test + void isStillMaskedForABearerValueCutToASingleCharacter() { + var logged = loggedBody(outbound(403, cutLeaving(1, "Bearer ", "S3cr3tV4lu3W1thM0re"))); + + assertFalse(logged.contains("Bearer S"), logged); + } + + /** + * The floor cannot go below one, so this is the residual the javadoc names rather than a + * proof of the fix: a cut leaving no value characters at all strands the sigil, which carries + * no secret. Green before and after — it pins the boundary, it does not demonstrate it moved. + */ + @Test + void leavesTheBareSigilBehindWhenTheCutLandsBeforeTheFirstValueCharacter() { + var logged = loggedBody(outbound(403, cutLeaving(0, "ghp_", "A1b2C3d4E5f6G7h8I9j0K1"))); + + assertTrue(logged.endsWith("ghp_…"), logged); + } } /** From 4ce31b1aff7370887217a130a3e1f428b5b96f6a Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 16 Aug 2026 23:42:26 +0000 Subject: [PATCH 2/5] style(github): write the credential value floors as + rather than {1,} The two forms are the same quantifier; the concise one is what the rest of the file already uses, and static analysis flags the long form. --- .../thiagogonzaga/thrillhousebot/github/GitHubApiError.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index 4fe50e1b..7bbc3d25 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -94,7 +94,7 @@ public final class GitHubApiError { * did not carry across to the shapes that still needed it. */ private static final Pattern CREDENTIAL_SHAPED_PREFIX = - Pattern.compile("(?i)(gh[pousr]_\\w{1,})|(github_pat_\\w{1,})"); + Pattern.compile("(?i)(gh[pousr]_\\w+)|(github_pat_\\w+)"); /** * The bearer shape, and with {@link #JWT_SHAPED_VALUE} below it the value half of {@link @@ -139,7 +139,7 @@ public final class GitHubApiError { * leftmost-match design above exists to swallow. */ private static final Pattern BEARER_SHAPED_VALUE = - Pattern.compile("bearer\\s+[\\w.~+/=-]{1,}", Pattern.CASE_INSENSITIVE); + Pattern.compile("bearer\\s+[\\w.~+/=-]+", Pattern.CASE_INSENSITIVE); /** * The JWT shape, kept apart from {@link #BEARER_SHAPED_VALUE} rather than alternated with it: one From f0f87b9784f9c673c61dc131640de0c880f27f8c Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga <2332561+devops-thiago@users.noreply.github.com> Date: Mon, 17 Aug 2026 08:09:47 -0300 Subject: [PATCH 3/5] fix(review): collapse every model-supplied value a log line interpolates (#760) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > **Stacked on #758.** The base is `fix/757-credential-floors` because both PRs touch > `GitHubApiError.java`. This re-targets `main` once #758 merges; only the last two commits of the > compare view belong to this PR. ## What type of PR is this? - [x] 🐛 Bug fix - [ ] ✨ Feature - [ ] 📝 Documentation - [ ] 🔧 Refactor - [ ] 🚀 Performance - [ ] ✅ Test - [x] 🔒 Security - [ ] 📦 Dependency update - [ ] 🏗️ CI/CD ## Description A log record is something an operator reads and acts on, and the model chooses the finding titles and paths that get spliced into these lines. A value carrying a line terminator splits the record and forges a second one; a value carrying a bidi override or an ANSI escape reorders or erases what is left of the first. #744 routed the two `ReviewPublisher` warn lines through `MarkdownSafe.oneLine`, whose collapse class is `Pattern.compile("\\s+")` — the ASCII six (`[ \t\n\x0B\f\r]`), because `java.util.regex` reads a bare `\s` that way unless the pattern asks for Unicode character classes. NEL (U+0085), LINE SEPARATOR (U+2028), PARAGRAPH SEPARATOR (U+2029), NUL, ESC and RLO all pass straight through it, and `String.strip()` does not catch them either — it trims the ends and these are interior. The vector those two lines were sanitized against is therefore still open on them. `GitHubApiError` met the same threat in #740 and installed the class that does cover it: `[\s\p{IsCc}\p{IsCf} ]+`. That reasoning never crossed over. ### The fix Lift that class — and the javadoc that argues for it — into a new `LogSafe.oneLine`, a helper for **log-destined** strings, and route all eight sites through it: | site | level | model-supplied values | |---|---|---| | `ReviewPublisher.java:770` | WARN | `file` (was `MarkdownSafe.oneLine`) | | `ReviewPublisher.java:832` | WARN | `file` (was `MarkdownSafe.oneLine`) | | `FindingQuoteValidator.java:83` | INFO | `title`, `file` | | `FindingVerificationService.java:1229` | INFO | `title`, `file`, `verdict.reason()` | | `FrameworkFalsePositiveFilter.java:74` | INFO | `title`, `file` | | `FindingPipeline.java:1370` | INFO | `title`, `file` | | `FindingDeduplicator.java:55` | INFO | `file`, `title` | | `FollowUpAnalyzer.java:633` | INFO | `title`, `file`, `duplicateOf.title()` | All six INFO lines are live in a default install — the repository configures no `quarkus.log` levels anywhere. Each line number was checked against the tree before editing; all six still held. `GitHubApiError` now calls the helper rather than keeping a private duplicate of the pattern, so there is one class and one explanation rather than two that can drift. ### `MarkdownSafe.oneLine` is deliberately left alone It also feeds markdown the bot posts to GitHub, where the wider class has a real rendering cost — `\p{IsCf}` splits an emoji ZWJ sequence or an Indic conjunct apart. A `body=` field or a warn line is a diagnostic identity and pays that cost gladly; a posted comment is a rendering surface and should not. Widening `WHITESPACE_RUN` in place would have changed what the bot posts. One pattern per destination, two intents kept apart. The moved javadoc carries both halves of the bargain with it: the accepted `\p{IsCf}` cost, and why the replacement is a space rather than a deletion (deleting would let `administrator` close up into a different real word; a space cannot). ## Related Issues Fixes #755 ## How Has This Been Tested? - [x] Unit tests - [ ] Integration tests - [ ] Manual testing `ModelSuppliedTextInLogLinesTest` drives all eight sites through their **real production paths** and captures the `java.util.logging.LogRecord` each class emits — the object every handler (console, file, syslog, JSON/ECS shipper) is handed — so the assertion does not depend on which handler happens to be installed. Each test feeds one crafted `title` or `file` carrying NEL, U+2028, U+2029, NUL, ESC and RLO, then asserts the record still names the finding and carries none of the six. `LogSafeTest` pins the helper itself: sixteen characters a reader could take for a control (each built by code point, not referenced from the production pattern), run collapsing, end trimming, the space-not-deletion rule, an ordinary value left alone, and a `null` value. ### Red before the fix All seven tests fail on the unfixed tree, each on the first forbidden character (U+0085), with the remaining five visible in the rendered message: ``` [ERROR] Tests run: 7, Failures: 7, Errors: 0, Skipped: 0 ``` ``` ### aCraftedPathCannotForgeARecordFromEitherPublisherRejectionWarning U+0085 reached the log line: GitHub rejected inline comment for app.js2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever:2 (status=422 body=) — filing it on the file instead GitHub rejected the file-level thread for app.js2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever (status=422 body=) — the finding keeps no thread at all ==> expected: but was: ### aCraftedTitleCannotForgeARecordFromTheQuoteValidatorDemotion U+0085 reached the log line: Finding 'Missing null check2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever' (src/Main.java:2) quotes code that does not appear in the diff — dropping its suggestion and capping confidence ==> expected: but was: ### aCraftedTitleCannotForgeARecordFromTheVerifierRejection U+0085 reached the log line: Verifier rejected finding 'Missing null check2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever' (src/Main.java:10): fp Finding verification: 0 kept, 0 downgraded, 1 rejected ==> expected: but was: ### aCraftedTitleCannotForgeARecordFromTheFrameworkFilterDrop U+0085 reached the log line: Dropping finding 'Missing no-arg constructor2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever' (src/main/java/dev/example/PrSummaryGenerator.java:12) — it claims a missing no-arg constructor but the diff shows an injection-annotated constructor; constructor injection needs no no-arg constructor in CDI/Spring ==> expected: but was: ### aCraftedPathAndTitleCannotForgeARecordFromTheAnchorBackfill U+0085 reached the log line: Populating missing content anchor for finding 'Missing null check2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever' (app.js2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever:1) ==> expected: but was: ### aCraftedPathAndTitleCannotForgeARecordFromTheDeduplicatorMerge U+0085 reached the log line: Merging 2 duplicate findings at app.js2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever:42 ('Missing null check2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever') ==> expected: but was: ### aCraftedTitleCannotForgeARecordFromTheRepliedDuplicateDrop U+0085 reached the log line: Dropping re-raised finding 'Missing null check2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever' (src/B.java:5) — a maintainer already replied to the prior finding 'Missing null check2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findingsforged-by-line-separatorforged-by-paragraph-separatorafter-nul[2Kescapeddesrever' at the same location ==> expected: but was: ``` `` and the rest are the test's own rendering, applied only when building the failure message — the assertion runs `joined.contains(forbidden)` against the raw record text with the literal code point. All six are present in every line; the assertion trips on the first. ### Green after `Tests run: 3356, Failures: 0, Errors: 0, Skipped: 0`. The first assertion of every test is that the log line ran at all and that the record still names the finding, so a fix that simply dropped the line or blanked the value would not pass. ## Checklist - [x] My code follows the project's coding standards - [x] I have performed a self-review of my own code - [x] I have added tests that prove my fix is effective or that my feature works - [x] New and existing unit tests pass locally with my changes - [x] I have updated the documentation accordingly - [x] My changes generate no new warnings or errors ## Screenshots / Logs Gates, in order: - `./mvnw -B spotless:apply` — BUILD SUCCESS - `./mvnw -B clean compile spotbugs:check spotless:check` — **BugInstance size is 0**, BUILD SUCCESS - `./mvnw -B clean test` — **Tests run: 3356, Failures: 0, Errors: 0, Skipped: 0** Coverage over `git diff -U0 73cc33d...HEAD` intersected with the jacoco report: **zero uncovered lines and zero uncovered branches** across all nine changed main files. ## Additional Notes Milestone v0.6.5, which is not tagged yet. The DEBUG lines in `ReviewPublisher` (`:720`, `:728`, `:874`) carry the same values but are off in a default install; they are left for a separate pass rather than widening this diff. --- .../thiagogonzaga/thrillhousebot/LogSafe.java | 87 ++++ .../thrillhousebot/github/GitHubApiError.java | 31 +- .../review/FindingDeduplicator.java | 6 +- .../review/FindingPipeline.java | 3 +- .../review/FindingQuoteValidator.java | 5 +- .../review/FollowUpAnalyzer.java | 6 +- .../review/FrameworkFalsePositiveFilter.java | 3 +- .../review/ReviewPublisher.java | 5 +- .../review/ai/FindingVerificationService.java | 6 +- .../thrillhousebot/LogSafeTest.java | 131 ++++++ .../ModelSuppliedTextInLogLinesTest.java | 405 ++++++++++++++++++ 11 files changed, 650 insertions(+), 38 deletions(-) create mode 100644 src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java create mode 100644 src/test/java/dev/thiagogonzaga/thrillhousebot/LogSafeTest.java create mode 100644 src/test/java/dev/thiagogonzaga/thrillhousebot/review/ModelSuppliedTextInLogLinesTest.java diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java new file mode 100644 index 00000000..925e09e2 --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java @@ -0,0 +1,87 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot; + +import java.util.regex.Pattern; + +/** + * The single place untrusted text is flattened before it is interpolated into a log line. Every + * value a log statement splices in that the bot did not choose itself — a GitHub error body, a + * model-supplied finding title or path — goes through here, so the "a field forges a record" class + * of defect is fixed in one place rather than re-litigated at each new call site (#731, #740, + * #742). + * + *

This is the log-destined counterpart to {@code MarkdownSafe}, and the two are deliberately + * separate. {@code MarkdownSafe.oneLine} feeds text the bot posts to GitHub, where the wider class + * below has a real rendering cost; a log record is a diagnostic identity rather than a rendering + * surface, and pays that cost gladly. Widening the markdown collapser instead would change what the + * bot posts. + */ +public final class LogSafe { + + /** + * Collapses the whitespace of an untrusted string so one value 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. A caller that reaches for this class has already decided its value is + * attacker-influenced text on its way to a log file and is already paying 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. + * + *

{@code \p{IsZs}} covers the space separators {@code \s} leaves behind — NBSP (U+00A0), the + * EM/EN and figure spaces (U+2000–U+200A), the narrow NBSP (U+202F), the medium mathematical + * space (U+205F) and the ideographic space (U+3000). Without it the class contradicted the + * contract this method's javadoc states, and inconsistently: {@code String.strip()} reads {@code + * Character.isWhitespace}, which is true for U+2003 and U+3000 but false for U+00A0, U+2007 and + * U+202F, so the first pair were trimmed at the ends yet never collapsed inside, and the second + * three survived at every position. They forge no boundary, which is why this is the cheapest of + * the four classes to justify; they are here because two values differing only by one of them + * render identically in a log, which is the same harm as the invisible characters below. + * + *

{@code \p{IsCf}} is here for the same harm rather than for line integrity: bidi overrides + * and isolates (RLO, LRM, LRI) reorder what an operator reads, and the invisible joiners and + * spaces (ZWJ, ZWNJ, ZWSP, the BOM, the soft hyphen) let two different values render identically + * — both forge a record's meaning as surely as a forged boundary forges its extent. The accepted + * cost is that an echoed user string loses its grapheme clusters: an emoji ZWJ sequence or an + * Indic conjunct is split apart. A logged value is a diagnostic identity rather than a rendering + * surface, and which characters arrived is the question it exists to answer. Replacing with a + * space rather than deleting is part of the same bargain — deletion would let {@code + * administrator} close up into a different real word, a space cannot. + */ + private static final Pattern WHITESPACE = + Pattern.compile("[\\s\\p{IsZs}\\p{IsCc}\\p{IsCf}\\u2028\\u2029]+"); + + private LogSafe() {} + + /** + * An untrusted string flattened to one log-safe line: every run of whitespace, control and format + * characters becomes a single space, and the ends are trimmed so a value that began or ended with + * such a run does not leave a stray space in the line. A {@code null} value flattens to the empty + * string, so a caller never has to guard for one. + */ + public static String oneLine(String value) { + if (value == null) { + return ""; + } + return WHITESPACE.matcher(value).replaceAll(" ").strip(); + } +} diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index 7bbc3d25..d9d0c6d0 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -15,6 +15,7 @@ */ package dev.thiagogonzaga.thrillhousebot.github; +import dev.thiagogonzaga.thrillhousebot.LogSafe; import jakarta.ws.rs.WebApplicationException; import jakarta.ws.rs.core.Response; import java.time.DateTimeException; @@ -166,34 +167,6 @@ 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. - * - *

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

{@code \p{IsCf}} is here for the same harm rather than for line integrity: bidi overrides - * and isolates (RLO, LRM, LRI) reorder what an operator reads, and the invisible joiners and - * spaces (ZWJ, ZWNJ, ZWSP, the BOM, the soft hyphen) let two different bodies render identically - * — both forge a record's meaning as surely as a forged boundary forges its extent. The accepted - * cost is that an echoed user string loses its grapheme clusters: an emoji ZWJ sequence or an - * Indic conjunct is split apart. A {@code body=} field is a diagnostic identity rather than a - * rendering surface, and which characters arrived is the question it exists to answer. Replacing - * with a space rather than deleting is part of the same bargain — deletion would let {@code - * administrator} close up into a different real word, a space cannot. - */ - 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); @@ -588,7 +561,7 @@ private static Body clean(String raw) { if (raw == null) { return Body.UNREADABLE; } - var collapsed = WHITESPACE.matcher(raw).replaceAll(" ").strip(); + var collapsed = LogSafe.oneLine(raw); var bounded = cutTo(collapsed, MAX_BODY_CHARS * 2); var redacted = redactCredentials(bounded); var capped = cutTo(redacted, MAX_BODY_CHARS); diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingDeduplicator.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingDeduplicator.java index b4fbab80..a31b70a3 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingDeduplicator.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingDeduplicator.java @@ -15,6 +15,7 @@ */ package dev.thiagogonzaga.thrillhousebot.review; +import dev.thiagogonzaga.thrillhousebot.LogSafe; import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import io.quarkus.logging.Log; @@ -54,7 +55,10 @@ public ReviewResponse dedupe(ReviewResponse response) { if (cluster.size() > 1) { Log.infof( "Merging %d duplicate findings at %s:%d ('%s')", - cluster.size(), cluster.get(0).file(), cluster.get(0).line(), cluster.get(0).title()); + cluster.size(), + LogSafe.oneLine(cluster.get(0).file()), + cluster.get(0).line(), + LogSafe.oneLine(cluster.get(0).title())); } merged.add(merge(cluster)); } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java index 45e85c84..009a10c5 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingPipeline.java @@ -17,6 +17,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; +import dev.thiagogonzaga.thrillhousebot.LogSafe; import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; import dev.thiagogonzaga.thrillhousebot.dashboard.ReviewSession; import dev.thiagogonzaga.thrillhousebot.github.GitHubPullRequestClient; @@ -1369,7 +1370,7 @@ ReviewResponse populateMissingAnchors(ReviewResponse response, DiffLineResolver if (fallback != null && !fallback.isBlank()) { Log.infof( "Populating missing content anchor for finding '%s' (%s:%d)", - finding.title(), finding.file(), finding.line()); + LogSafe.oneLine(finding.title()), LogSafe.oneLine(finding.file()), finding.line()); adjusted.add( new ReviewResponse.Finding( finding.risk(), diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingQuoteValidator.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingQuoteValidator.java index a18ecc47..dafcb15c 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingQuoteValidator.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FindingQuoteValidator.java @@ -15,6 +15,7 @@ */ package dev.thiagogonzaga.thrillhousebot.review; +import dev.thiagogonzaga.thrillhousebot.LogSafe; import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import io.quarkus.logging.Log; @@ -82,8 +83,8 @@ public ReviewResponse validate(ReviewResponse response, String diff) { } Log.infof( "Finding '%s' (%s:%d) %s" + DEMOTION_SUFFIX, - finding.title(), - finding.file(), + LogSafe.oneLine(finding.title()), + LogSafe.oneLine(finding.file()), finding.line(), reason); kept.add(withoutSuggestion(finding)); diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java index 3c128cd5..db2342eb 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FollowUpAnalyzer.java @@ -17,6 +17,7 @@ import com.fasterxml.jackson.core.JsonProcessingException; import com.fasterxml.jackson.databind.ObjectMapper; +import dev.thiagogonzaga.thrillhousebot.LogSafe; import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.github.GitHubCommentClient; @@ -633,7 +634,10 @@ public ReviewResponse dropRepliedDuplicates( Log.infof( "Dropping re-raised finding '%s' (%s:%d) — a maintainer already replied to the prior" + " finding '%s' at the same location", - finding.title(), finding.file(), finding.line(), duplicateOf.title()); + LogSafe.oneLine(finding.title()), + LogSafe.oneLine(finding.file()), + finding.line(), + LogSafe.oneLine(duplicateOf.title())); } if (!dropped) { return response; diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FrameworkFalsePositiveFilter.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FrameworkFalsePositiveFilter.java index 721f1c03..e200e5a8 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FrameworkFalsePositiveFilter.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/FrameworkFalsePositiveFilter.java @@ -15,6 +15,7 @@ */ package dev.thiagogonzaga.thrillhousebot.review; +import dev.thiagogonzaga.thrillhousebot.LogSafe; import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService; import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; import io.quarkus.logging.Log; @@ -75,7 +76,7 @@ public ReviewResponse filter(ReviewResponse response, String diff) { "Dropping finding '%s' (%s:%d) — it claims a missing no-arg constructor but the diff" + " shows an injection-annotated constructor; constructor injection needs no no-arg" + " constructor in CDI/Spring", - finding.title(), finding.file(), finding.line()); + LogSafe.oneLine(finding.title()), LogSafe.oneLine(finding.file()), finding.line()); changed = true; continue; } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index 71be21a9..8a42b4f6 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -15,6 +15,7 @@ */ package dev.thiagogonzaga.thrillhousebot.review; +import dev.thiagogonzaga.thrillhousebot.LogSafe; import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.github.GitHubApiError; @@ -767,7 +768,7 @@ private boolean postFindingCommentRoutes( } Log.warnf( "GitHub rejected inline comment for %s:%d (%s) — filing it on the file instead", - MarkdownSafe.oneLine(finding.file()), finding.line(), reason); + LogSafe.oneLine(finding.file()), finding.line(), reason); return postFileLevelComment(target, finding, findingId); } @@ -829,7 +830,7 @@ private boolean postFileLevelComment(CommentTarget target, Finding finding, int } catch (RuntimeException e) { Log.warnf( "GitHub rejected the file-level thread for %s (%s) — the finding keeps no thread at all", - MarkdownSafe.oneLine(finding.file()), rejectionReason(e)); + LogSafe.oneLine(finding.file()), rejectionReason(e)); return false; } } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java index fbf68f5d..8610c4c5 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java @@ -18,6 +18,7 @@ import com.fasterxml.jackson.annotation.JsonProperty; import com.fasterxml.jackson.databind.ObjectMapper; import dev.langchain4j.service.Result; +import dev.thiagogonzaga.thrillhousebot.LogSafe; import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; import dev.thiagogonzaga.thrillhousebot.review.Confidence; import dev.thiagogonzaga.thrillhousebot.review.PromptTemplateEscaper; @@ -1228,7 +1229,10 @@ ReviewResponse apply(ReviewResponse response, VerificationResponse verification) rejected++; Log.infof( "Verifier rejected finding '%s' (%s:%d): %s", - finding.title(), finding.file(), finding.line(), verdict.reason()); + LogSafe.oneLine(finding.title()), + LogSafe.oneLine(finding.file()), + finding.line(), + LogSafe.oneLine(verdict.reason())); } case "downgraded" -> { downgraded++; diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/LogSafeTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/LogSafeTest.java new file mode 100644 index 00000000..68f5abed --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/LogSafeTest.java @@ -0,0 +1,131 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * Covers {@link LogSafe} — the collapse every untrusted value goes through on its way into a log + * line. The characters are named and built by code point here rather than referenced from the + * production pattern, so these tests pin the class of harm independently of the expression that + * implements it. + */ +class LogSafeTest { + + private static String between(int codePoint) { + return "a" + (char) codePoint + "b"; + } + + /** + * The survivors of a bare {@code \s}, which java.util.regex reads as the ASCII six: each of these + * is a record boundary or a screen control to some reader downstream, so each must come out as an + * ordinary space (#731, #742). The last four are the ASCII six itself, which must not regress. + */ + private static Stream charactersAReaderCouldTakeForAControl() { + return Stream.of( + arguments("NEL (U+0085)", between(0x0085)), + arguments("LINE SEPARATOR (U+2028)", between(0x2028)), + arguments("PARAGRAPH SEPARATOR (U+2029)", between(0x2029)), + arguments("NUL", between(0x0000)), + arguments("ESC", between(0x001B)), + arguments("DEL", between(0x007F)), + arguments("a C1 control (U+0090)", between(0x0090)), + arguments("RIGHT-TO-LEFT OVERRIDE", between(0x202E)), + arguments("ZERO WIDTH SPACE", between(0x200B)), + arguments("ZERO WIDTH JOINER", between(0x200D)), + arguments("the byte order mark", between(0xFEFF)), + arguments("SOFT HYPHEN", between(0x00AD)), + arguments("LF", between('\n')), + arguments("CR", between('\r')), + arguments("TAB", between('\t')), + arguments("a plain space", between(' '))); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("charactersAReaderCouldTakeForAControl") + void flattensEveryCharacterAReaderCouldTakeForAControl(String name, String value) { + assertEquals("a b", LogSafe.oneLine(value), name); + } + + @Test + void collapsesARunOfThemIntoOneSpaceRatherThanOnePerCharacter() { + assertEquals("a b", LogSafe.oneLine("a\r\n \t" + (char) 0x0085 + "b")); + } + + @Test + void trimsSuchARunAtEitherEndRatherThanLeavingAStraySpace() { + assertEquals("boom", LogSafe.oneLine((char) 0x2028 + " boom " + (char) 0x0085)); + } + + /** + * A space rather than a deletion: {@code administrator} must not close up into a different + * real word, which is half of why the invisible characters are worth collapsing at all. + */ + @Test + void separatesRatherThanDeletesSoTwoHalvesCannotCloseUpIntoOneWord() { + assertEquals("admin istrator", LogSafe.oneLine("admin" + (char) 0x200B + "istrator")); + } + + /** + * The space separators a bare {@code \s} also leaves behind. None of these forges a boundary, so + * they are the cheapest of the four classes to justify; they are here because two values + * differing only by one of them render identically in a log line. + */ + private static Stream spaceSeparatorsThatRenderLikeASpace() { + return Stream.of( + arguments("NO-BREAK SPACE (U+00A0)", between(0x00A0)), + arguments("EN QUAD (U+2000)", between(0x2000)), + arguments("EM SPACE (U+2003)", between(0x2003)), + arguments("FIGURE SPACE (U+2007)", between(0x2007)), + arguments("NARROW NO-BREAK SPACE (U+202F)", between(0x202F)), + arguments("MEDIUM MATHEMATICAL SPACE (U+205F)", between(0x205F)), + arguments("IDEOGRAPHIC SPACE (U+3000)", between(0x3000))); + } + + @ParameterizedTest(name = "{0}") + @MethodSource("spaceSeparatorsThatRenderLikeASpace") + void flattensEverySpaceSeparatorThatRendersLikeAnOrdinarySpace(String name, String value) { + assertEquals("a b", LogSafe.oneLine(value), name); + } + + /** + * The trim is no backstop for these: it reads {@code Character.isWhitespace}, which is false for + * U+00A0, U+2007 and U+202F, so a value bounded by them kept them at every position until the + * class covered {@code \p{IsZs}}. + */ + @Test + void trimsASpaceSeparatorTheWhitespaceTestDoesNotRecognise() { + assertEquals("boom", LogSafe.oneLine((char) 0x00A0 + "boom" + (char) 0x202F)); + } + + @Test + void leavesAnOrdinaryValueAlone() { + assertEquals("src/main/java/App.java", LogSafe.oneLine("src/main/java/App.java")); + } + + /** A caller logging an absent value should not have to guard for it. */ + @Test + void flattensAnAbsentValueToTheEmptyString() { + assertEquals("", LogSafe.oneLine(null)); + } +} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ModelSuppliedTextInLogLinesTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ModelSuppliedTextInLogLinesTest.java new file mode 100644 index 00000000..aa55bb4a --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/review/ModelSuppliedTextInLogLinesTest.java @@ -0,0 +1,405 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.review; + +import static dev.thiagogonzaga.thrillhousebot.review.ai.AiResults.aiOk; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.anyInt; +import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import com.fasterxml.jackson.databind.ObjectMapper; +import dev.thiagogonzaga.thrillhousebot.config.BotIdentity; +import dev.thiagogonzaga.thrillhousebot.config.ThrillhouseConfig; +import dev.thiagogonzaga.thrillhousebot.github.GitHubCommentClient; +import dev.thiagogonzaga.thrillhousebot.github.GitHubReviewClient; +import dev.thiagogonzaga.thrillhousebot.github.ReviewThreadService; +import dev.thiagogonzaga.thrillhousebot.review.ai.AiReviewService; +import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerificationService; +import dev.thiagogonzaga.thrillhousebot.review.ai.FindingVerifier; +import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewResponse; +import dev.thiagogonzaga.thrillhousebot.review.ai.ReviewTokenLedger; +import dev.thiagogonzaga.thrillhousebot.review.ai.TokenCounter; +import dev.thiagogonzaga.thrillhousebot.review.ai.TruncatedResponseSalvager; +import jakarta.ws.rs.WebApplicationException; +import java.util.ArrayList; +import java.util.List; +import java.util.Map; +import java.util.logging.Handler; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import org.junit.jupiter.api.Test; + +/** + * #742/#755. Every log line that interpolates a model-supplied {@code title} or {@code file} is one + * record an operator reads, and the model chooses those strings. A path or title carrying a line + * terminator splits the record and forges a second one; one carrying a bidi override or an ANSI + * escape reorders or erases what the operator is shown. + * + *

#744 routed the two {@link ReviewPublisher} warn lines through {@link MarkdownSafe#oneLine}, + * whose collapse class is the ASCII {@code \s} six — so NEL (U+0085), LINE SEPARATOR (U+2028), + * PARAGRAPH SEPARATOR (U+2029), NUL, ESC and RLO went straight through it — and left the six INFO + * lines that interpolate the same model strings untouched. Every one of those is on in a default + * install: there is no {@code quarkus.log} level configuration in the repository. + * + *

Each test drives the real production path and captures the {@link LogRecord} the class emits — + * the object every handler (console, file, syslog, JSON/ECS shipper) is handed — so the assertion + * does not depend on which handler happens to be installed. + */ +class ModelSuppliedTextInLogLinesTest { + + private static String ch(int codePoint) { + return String.valueOf((char) codePoint); + } + + private static final String NEL = ch(0x85); + private static final String LS = ch(0x2028); + private static final String PS = ch(0x2029); + private static final String NUL = ch(0x00); + private static final String ESC = ch(0x1B); + private static final String RLO = ch(0x202E); + + private static final List FORGERY_CHARACTERS = List.of(NEL, LS, PS, NUL, ESC, RLO); + + /** A model string that forges a second record and then reorders what is left of the first. */ + private static final String FORGED = + NEL + + "2026-08-16 12:00:00 WARN [thrillhousebot] approved the pull request, 0 findings" + + LS + + "forged-by-line-separator" + + PS + + "forged-by-paragraph-separator" + + NUL + + "after-nul" + + ESC + + "[2Kescaped" + + RLO + + "desrever"; + + /** A crafted path: a real-looking prefix so the record stays plausible, then the forgery. */ + private static final String FORGED_PATH = "app.js" + FORGED; + + /** A crafted finding title, the value the six INFO lines all interpolate. */ + private static final String FORGED_TITLE = "Missing null check" + FORGED; + + private static String visible(String text) { + return text.replace(NEL, "") + .replace(LS, "") + .replace(PS, "") + .replace(NUL, "") + .replace(ESC, "") + .replace(RLO, ""); + } + + /** + * The record as a handler sees it: the message and every parameter the formatter would splice. + */ + private static String text(LogRecord entry) { + var joined = new StringBuilder(String.valueOf(entry.getMessage())); + var parameters = entry.getParameters(); + if (parameters != null) { + for (var parameter : parameters) { + joined.append(' ').append(parameter); + } + } + return joined.toString(); + } + + /** + * Runs {@code body} with a handler attached to {@code source}'s logger and returns its records. + */ + private static List logsOf(Class source, Runnable body) { + var captured = new ArrayList(); + var logger = Logger.getLogger(source.getName()); + var handler = + new Handler() { + @Override + public void publish(LogRecord entry) { + captured.add(text(entry)); + } + + @Override + public void flush() { + // nothing is buffered + } + + @Override + public void close() { + // nothing to release + } + }; + logger.addHandler(handler); + try { + body.run(); + } finally { + logger.removeHandler(handler); + } + return captured; + } + + /** + * The line must have been emitted, must still identify the finding, and must carry no forgery. + */ + private static void assertRecordCannotBeForged(List captured, String anchor) { + assertFalse(captured.isEmpty(), "the log line under test did not run"); + var joined = String.join("\n", captured); + assertTrue(joined.contains(anchor), "the line must still name the finding: " + visible(joined)); + for (var forbidden : FORGERY_CHARACTERS) { + assertFalse( + joined.contains(forbidden), + "U+" + + String.format("%04X", (int) forbidden.charAt(0)) + + " reached the log line:\n" + + visible(joined)); + } + } + + private static ReviewResponse.Finding finding( + String file, int line, String title, String suggestionOld) { + return new ReviewResponse.Finding( + "medium", "high", file, line, title, "description", suggestionOld, "new"); + } + + private static ReviewResponse response(ReviewResponse.Finding... findings) { + return new ReviewResponse( + List.of(findings), + List.of(), + new ReviewResponse.Summary( + findings.length, 0, 0, findings.length, 0, "assessment", "purpose", List.of())); + } + + /** + * Both {@link ReviewPublisher} warn lines at once: the line-anchored comment is refused, so the + * first fires, and the file-level fallback is refused too, so the second does. + */ + @Test + void aCraftedPathCannotForgeARecordFromEitherPublisherRejectionWarning() { + var reviewClient = mock(GitHubReviewClient.class); + var config = mock(ThrillhouseConfig.class); + var reviewConfig = mock(ThrillhouseConfig.ReviewConfig.class); + var formatter = mock(SuggestionFormatter.class); + when(config.review()).thenReturn(reviewConfig); + when(reviewConfig.maxReviewComments()).thenReturn(10); + when(formatter.formatReviewComment(any(), anyBoolean(), anyInt())).thenReturn("body"); + when(reviewClient.createPullRequestComment( + anyString(), anyString(), anyString(), anyString(), anyInt(), any())) + .thenThrow(new WebApplicationException("nope", 422)); + var publisher = + new ReviewPublisher( + reviewClient, + mock(GitHubCommentClient.class), + mock(ReviewThreadService.class), + formatter, + mock(FollowUpAnalyzer.class), + mock(PrLabeler.class), + config, + BotIdentity.of("thrillhousebot")); + var result = + new ReviewResult( + List.of( + new Finding(RiskLevel.HIGH, FORGED_PATH, 2, "title", "description", null, null)), + 0, + 1, + 0, + 0, + RiskLevel.HIGH, + ReviewState.REQUEST_CHANGES, + true, + "summary", + List.of(), + List.of(), + 0); + var resolver = new DiffLineResolver(Map.of(FORGED_PATH, "@@ -0,0 +1,3 @@\n+a\n+b\n+c\n")); + + var captured = + logsOf( + ReviewPublisher.class, + () -> publisher.postInlineComments("auth", "o", "r", 1, "sha", result, resolver)); + + assertRecordCannotBeForged(captured, "app.js"); + } + + /** {@link FindingQuoteValidator} demoting a finding whose quote is nowhere in the diff. */ + @Test + void aCraftedTitleCannotForgeARecordFromTheQuoteValidatorDemotion() { + var diff = + """ + diff --git a/src/Main.java b/src/Main.java + --- a/src/Main.java + +++ b/src/Main.java + @@ -1,3 +1,3 @@ + public class Main { + + var repos = new ArrayList(snapshot); + } + """; + var validator = new FindingQuoteValidator(); + var response = + response(finding("src/Main.java", 2, FORGED_TITLE, "nothing like this is in the diff")); + + var captured = logsOf(FindingQuoteValidator.class, () -> validator.validate(response, diff)); + + assertRecordCannotBeForged(captured, "Missing null check"); + } + + /** {@link FrameworkFalsePositiveFilter} dropping a no-arg-constructor claim the diff refutes. */ + @Test + void aCraftedTitleCannotForgeARecordFromTheFrameworkFilterDrop() { + var diff = + """ + ### src/main/java/dev/example/PrSummaryGenerator.java (modified, +6 -0) + ```diff + @@ -10,3 +10,9 @@ + public class PrSummaryGenerator { + + private final AiReviewService aiReviewService; + + + + @Inject + + public PrSummaryGenerator(AiReviewService aiReviewService) { + + this.aiReviewService = aiReviewService; + + } + } + ``` + """; + var filter = new FrameworkFalsePositiveFilter(); + var claim = + new ReviewResponse.Finding( + "medium", + "medium", + "src/main/java/dev/example/PrSummaryGenerator.java", + 12, + "Missing no-arg constructor" + FORGED, + "CDI requires a bean to be proxyable; add a no-arg constructor.", + null, + null); + + var captured = + logsOf(FrameworkFalsePositiveFilter.class, () -> filter.filter(response(claim), diff)); + + assertRecordCannotBeForged(captured, "Missing no-arg constructor"); + } + + /** {@link FindingDeduplicator} merging a cluster — it names the cluster's file and title. */ + @Test + void aCraftedPathAndTitleCannotForgeARecordFromTheDeduplicatorMerge() { + var deduplicator = new FindingDeduplicator(); + var response = + response( + finding(FORGED_PATH, 42, FORGED_TITLE, "old"), + finding(FORGED_PATH, 43, FORGED_TITLE, "old")); + + var captured = logsOf(FindingDeduplicator.class, () -> deduplicator.dedupe(response)); + + assertRecordCannotBeForged(captured, "Missing null check"); + } + + /** {@link FollowUpAnalyzer} dropping a re-raise a maintainer already answered. */ + @Test + void aCraftedTitleCannotForgeARecordFromTheRepliedDuplicateDrop() { + var analyzer = new FollowUpAnalyzer(new ObjectMapper()); + var priorJson = + "{\"findings\": [{\"risk\": \"medium\", \"file\": \"src/B.java\", \"line\": 5," + + " \"title\": " + + new ObjectMapper().valueToTree(FORGED_TITLE) + + ", \"description\": \"d\"}]}"; + var botComment = + new GitHubReviewClient.PullRequestComment( + 100L, + null, + "src/B.java", + "**MEDIUM — " + FORGED_TITLE + "**", + new GitHubReviewClient.ReviewResponse.User("thrillhousebot"), + "MEMBER"); + var maintainerReply = + new GitHubReviewClient.PullRequestComment( + 101L, + 100L, + "src/B.java", + "Declining.", + new GitHubReviewClient.ReviewResponse.User("maintainer"), + "MEMBER"); + var reRaised = response(finding("src/B.java", 5, FORGED_TITLE, null)); + + var captured = + logsOf( + FollowUpAnalyzer.class, + () -> + analyzer.dropRepliedDuplicates( + reRaised, + List.of(priorJson), + List.of(botComment, maintainerReply), + BotIdentity.of("thrillhousebot"))); + + assertRecordCannotBeForged(captured, "Missing null check"); + } + + /** {@link FindingPipeline} filling in a content anchor the model left blank. */ + @Test + void aCraftedPathAndTitleCannotForgeARecordFromTheAnchorBackfill() { + var pipeline = + new FindingPipeline( + mock(AiReviewService.class), + mock(FindingQuoteValidator.class), + mock(FrameworkFalsePositiveFilter.class), + mock(FindingDeduplicator.class), + mock(FindingVerificationService.class), + mock(FollowUpAnalyzer.class), + new ObjectMapper(), + BotIdentity.of("thrillhousebot"), + mock(DiffBudgetPlanner.class), + new TokenCounter(), + mock(ReviewTokenLedger.class), + new TruncatedResponseSalvager(new ObjectMapper())); + var response = response(finding(FORGED_PATH, 1, FORGED_TITLE, null)); + var resolver = new DiffLineResolver(Map.of(FORGED_PATH, "@@ -0,0 +1,1 @@\n+var a = 1;\n")); + + var captured = + logsOf(FindingPipeline.class, () -> pipeline.populateMissingAnchors(response, resolver)); + + assertRecordCannotBeForged(captured, "Missing null check"); + } + + /** {@link FindingVerificationService} logging the verdict that rejected a finding. */ + @Test + void aCraftedTitleCannotForgeARecordFromTheVerifierRejection() { + var mapper = new ObjectMapper(); + var verifier = mock(FindingVerifier.class); + var config = mock(ThrillhouseConfig.class); + var reviewConfig = mock(ThrillhouseConfig.ReviewConfig.class); + when(config.review()).thenReturn(reviewConfig); + when(reviewConfig.verifierEnabled()).thenReturn(true); + when(verifier.verify(anyString(), anyString(), anyString(), anyString(), anyString())) + .thenReturn( + aiOk("{\"verdicts\": [{\"id\": 1, \"verdict\": \"rejected\", \"reason\": \"fp\"}]}")); + var service = + new FindingVerificationService( + verifier, + config, + mapper, + mock(ReviewTokenLedger.class), + new TruncatedResponseSalvager(mapper)); + var response = response(finding("src/Main.java", 10, FORGED_TITLE, "old")); + + var captured = + logsOf( + FindingVerificationService.class, + () -> service.verify(42L, response, "diff", "stack", "")); + + assertRecordCannotBeForged(captured, "Missing null check"); + } +} From 271efd6e1a6a706aa2a61a83f83dcc3d5b80e89e Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 17 Aug 2026 12:14:31 +0000 Subject: [PATCH 4/5] fix(review): route the debug rejection lines through the log collapse too The three debug lines interpolate the same model-supplied path the warn lines do, and were left out on the grounds that debug is off in a default install. That left LogSafe's own javadoc claiming every value a log statement splices in goes through it while three did not, which is the kind of claim that stops the next reader checking. Widening the coverage rather than qualifying the claim: the sanitizing call is the same one the warn lines already make, and a debug-enabled install has exactly the forgery surface the warn lines were fixed for. --- .../thrillhousebot/review/ReviewPublisher.java | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index 8a42b4f6..20bd0f7e 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -720,7 +720,7 @@ private boolean postFindingCommentRoutes( if (line.isEmpty()) { Log.debugf( "Line for %s:%d is outside the PR diff — filing the finding on the file instead", - finding.file(), finding.line()); + LogSafe.oneLine(finding.file()), finding.line()); return postFileLevelComment(target, finding, findingId); } @@ -728,7 +728,7 @@ private boolean postFindingCommentRoutes( if (resolvedLine != finding.line()) { Log.debugf( "Adjusted inline comment line for %s from %d to %d", - finding.file(), finding.line(), resolvedLine); + LogSafe.oneLine(finding.file()), finding.line(), resolvedLine); } // A GitHub suggestion overwrites the whole commented range, so multi-line old code needs a @@ -877,7 +877,7 @@ private Optional tryPostInlineComment( Log.debugf( e, "Inline comment rejected for %s:%d (suggestion=%s): %s", - finding.file(), + LogSafe.oneLine(finding.file()), endLine, includeSuggestion, reason); From 7d46425e9e8ded938d19e55af81710b949480c25 Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 17 Aug 2026 13:15:32 +0000 Subject: [PATCH 5/5] fix(github): collapse the rate-limit headers into the diagnostics line too MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit diagnostics() reads four values straight off the response and interpolates them verbatim, one field away from a body it collapses. A header is response data from the configured API host, so on the hosts this class already names as its threat model — a GHES or reverse-proxy error page, a misconfigured base URL, a compromised endpoint — it is as untrusted as the body beside it, and it reaches the same warn line. The collapse goes in the one place every header passes through, so a header added later is covered without anyone remembering to. The reason a rejection carries when the exception brought no response was raw for the same reason, and takes the same treatment. --- .../thrillhousebot/github/GitHubApiError.java | 8 +- .../review/ReviewPublisher.java | 6 +- .../GitHubApiErrorHeaderCollapseTest.java | 76 +++++++++++++++++++ 3 files changed, 87 insertions(+), 3 deletions(-) create mode 100644 src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorHeaderCollapseTest.java diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java index d9d0c6d0..7ac63d82 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java @@ -405,6 +405,12 @@ private boolean blocksContentCreation() { /** * One line naming everything that separates one GitHub failure from another: the status, the * throttling headers when present, and the body. This is the line that was missing in #568. + * + *

The headers go through {@link LogSafe} in {@link #append} for the reason the body already + * does. A header is response data from the configured API host, so on the hosts this class's + * threat model names — a GHES or reverse-proxy error page, a misconfigured base URL, a + * compromised endpoint — it is as attacker-influenced as the body beside it, and it was reaching + * this line verbatim while the body was being collapsed one field away. */ public String diagnostics() { var text = new StringBuilder("status=").append(status); @@ -418,7 +424,7 @@ public String diagnostics() { private static void append(StringBuilder text, String name, String value) { if (value != null && !value.isBlank()) { - text.append(' ').append(name).append('=').append(value); + text.append(' ').append(name).append('=').append(LogSafe.oneLine(value)); } } diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java index 20bd0f7e..456c114b 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java @@ -787,9 +787,11 @@ private boolean postFindingCommentRoutes( */ private static String rejectionReason(RuntimeException e) { if (e instanceof WebApplicationException w) { - return GitHubApiError.of(w).map(GitHubApiError::diagnostics).orElseGet(e::toString); + return GitHubApiError.of(w) + .map(GitHubApiError::diagnostics) + .orElseGet(() -> LogSafe.oneLine(e.toString())); } - return e.toString(); + return LogSafe.oneLine(e.toString()); } /** diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorHeaderCollapseTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorHeaderCollapseTest.java new file mode 100644 index 00000000..bd5b60b3 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorHeaderCollapseTest.java @@ -0,0 +1,76 @@ +/* + * Copyright 2026 Thiago Gonzaga + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package dev.thiagogonzaga.thrillhousebot.github; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.params.provider.Arguments.arguments; + +import jakarta.ws.rs.core.Response; +import java.util.stream.Stream; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +/** + * The rate-limit headers reach {@link GitHubApiError#diagnostics()} the same way the body does, and + * on the hosts this class's threat model names — a GHES or reverse-proxy error page, a + * misconfigured base URL, a compromised endpoint — they are response data from the same untrusted + * place. They were being interpolated verbatim while the body one field away was collapsed. + */ +class GitHubApiErrorHeaderCollapseTest { + + private static String diagnosticsWithHeader(String name, String value) { + return GitHubApiError.from( + Response.status(403).header(name, value).entity("{\"message\":\"no\"}").build()) + .diagnostics(); + } + + private static Stream headersAReaderCouldTakeForAControl() { + return Stream.of( + arguments("Retry-After", "NEL", "30" + (char) 0x0085 + "forged WARN approved"), + arguments("x-ratelimit-remaining", "ESC", "0" + (char) 0x001B + "[2Kcleared"), + arguments("x-ratelimit-reset", "LINE SEPARATOR", "0" + (char) 0x2028 + "forged"), + arguments("x-ratelimit-resource", "NUL", "core" + (char) 0x0000 + "forged")); + } + + @ParameterizedTest(name = "{0} carrying {1}") + @MethodSource("headersAReaderCouldTakeForAControl") + void collapsesEveryRateLimitHeaderOnItsWayIntoTheLine(String header, String name, String value) { + var line = diagnosticsWithHeader(header, value); + assertFalse( + line.codePoints().anyMatch(c -> Character.getType(c) == Character.CONTROL) + || line.indexOf(0x2028) >= 0, + name + " survived into the diagnostics line: " + line); + } + + /** The collapse must not blank the header — its value is what the operator came for. */ + @Test + void keepsTheHeaderValueItCollapsed() { + assertEquals( + "status=403 retry-after=30 forged body={\"message\":\"no\"}", + diagnosticsWithHeader("Retry-After", "30" + (char) 0x0085 + "forged")); + } + + /** A control: an ordinary header is untouched. */ + @Test + void leavesAnOrdinaryHeaderAlone() { + assertEquals( + "status=403 retry-after=30 body={\"message\":\"no\"}", + diagnosticsWithHeader("Retry-After", "30")); + } +}