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"); + } +}