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 913d0529..7ac63d82 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; @@ -82,15 +83,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+)|(github_pat_\\w+)"); /** * The bearer shape, and with {@link #JWT_SHAPED_VALUE} below it the value half of {@link @@ -124,9 +129,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 +140,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.~+/=-]+", Pattern.CASE_INSENSITIVE); /** * The JWT shape, kept apart from {@link #BEARER_SHAPED_VALUE} rather than alternated with it: one @@ -162,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); @@ -428,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); @@ -441,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)); } } @@ -584,7 +567,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..456c114b 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; @@ -719,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); } @@ -727,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 @@ -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); } @@ -786,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()); } /** @@ -829,7 +832,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; } } @@ -876,7 +879,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); 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/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")); + } +} 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); + } } /** 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"); + } +}