From f096db81251237f91ceb2c1b3a9c411e7e326d0a Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Sun, 16 Aug 2026 23:32:16 +0000 Subject: [PATCH 1/2] fix(review): collapse every model-supplied value a log line interpolates MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A log record is a thing an operator reads and acts on, and the model chooses the finding titles and paths spliced into these lines. A value 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 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. NEL (U+0085), U+2028, U+2029, NUL, ESC and a bidi override all go through it untouched, and String.strip() does not catch them either since it trims ends and these are interior. So the vector those lines were sanitized against is still open on them. GitHubApiError met the same threat in #740 and installed the class that does cover it. Lift that class, and the reasoning that chose it, into LogSafe — a helper for log-destined strings — and route every site through it: - the two ReviewPublisher warn lines, replacing MarkdownSafe.oneLine - FindingQuoteValidator, FindingVerificationService, FrameworkFalsePositiveFilter, FindingPipeline, FindingDeduplicator and FollowUpAnalyzer, which interpolate raw title/file/reason at INFO — on in a default install, since the repository configures no log levels - GitHubApiError itself, which now calls the helper rather than keeping a private duplicate of the pattern MarkdownSafe.oneLine is deliberately left as it is. It also feeds markdown the bot posts to GitHub, where the wider class has a real rendering cost — an emoji ZWJ sequence comes apart — that a log line pays gladly and a posted comment should not. ModelSuppliedTextInLogLinesTest drives all eight sites through their real production paths and asserts on the LogRecord each emits, so the proof does not depend on which handler is installed. Fixes #755 --- .../thiagogonzaga/thrillhousebot/LogSafe.java | 77 ++++ .../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 | 99 +++++ .../ModelSuppliedTextInLogLinesTest.java | 405 ++++++++++++++++++ 11 files changed, 608 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..3ffb304d --- /dev/null +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java @@ -0,0 +1,77 @@ +/* + * 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{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{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..190a9117 --- /dev/null +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/LogSafeTest.java @@ -0,0 +1,99 @@ +/* + * 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")); + } + + @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 697d7c29ad69c1fbf102ba0a92509be39bbb9afe Mon Sep 17 00:00:00 2001 From: Thiago Gonzaga Date: Mon, 17 Aug 2026 00:29:24 +0000 Subject: [PATCH 2/2] fix(review): collapse the space separators the log class also left behind MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The class covered the ASCII six plus the control and format categories, but not Zs, so NBSP, the EM/EN and figure spaces, the narrow NBSP, the medium mathematical space and the ideographic space reached the log line intact — against a javadoc promising that every run of whitespace becomes a single space. The trim was no backstop, and inconsistently so: it reads 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 removed at the ends yet never collapsed inside, and the other three survived at every position. These forge no record boundary, which makes them the cheapest of the four classes to justify. They are worth collapsing for the same reason the invisible characters already were: two values differing only by one of them render identically to the operator reading the line. --- .../thiagogonzaga/thrillhousebot/LogSafe.java | 12 ++++++- .../thrillhousebot/LogSafeTest.java | 32 +++++++++++++++++++ 2 files changed, 43 insertions(+), 1 deletion(-) diff --git a/src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java b/src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java index 3ffb304d..925e09e2 100644 --- a/src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java +++ b/src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java @@ -47,6 +47,16 @@ public final class LogSafe { *

{@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 @@ -58,7 +68,7 @@ public final class LogSafe { * 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]+"); + Pattern.compile("[\\s\\p{IsZs}\\p{IsCc}\\p{IsCf}\\u2028\\u2029]+"); private LogSafe() {} diff --git a/src/test/java/dev/thiagogonzaga/thrillhousebot/LogSafeTest.java b/src/test/java/dev/thiagogonzaga/thrillhousebot/LogSafeTest.java index 190a9117..68f5abed 100644 --- a/src/test/java/dev/thiagogonzaga/thrillhousebot/LogSafeTest.java +++ b/src/test/java/dev/thiagogonzaga/thrillhousebot/LogSafeTest.java @@ -86,6 +86,38 @@ 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"));