fix(review): collapse every model-supplied value a log line interpolates - #760
Conversation
🤖 ThrillhouseBot PR SummaryWhat this PR doesThis change lifts the Unicode-aware whitespace/control/format collapsing pattern from GitHubApiError into a shared LogSafe.oneLine helper and applies it to eight log lines that interpolate model-supplied finding titles, file paths, and verdict reasons, so characters like NEL, U+2028/U+2029, NUL, ESC, and bidi overrides cannot forge or corrupt log records. GitHubApiError is refactored to reuse the same helper instead of keeping a private duplicate of the pattern. Description vs. ImplementationNo mismatch found between the PR description and the change. Changes Overview
Changed Files
Risk Assessment
Everything's coming up Thrillhouse! 🎉 No issues found in this PR. Automated review by ThrillhouseBot. Reply with |
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
thrillhousebot
Bot
added
java
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
38df0c2 to
f096db8
Compare
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: LogSafe.oneLine leaves Zs space separators (EM SPACE, IDEOGRAPHIC SPACE) unsplit despite the javadoc's "every run of whitespace" contract (
src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java:61)
Input not in the diff: a model-supplied title or path containing U+2003 EM SPACE, U+3000 IDEOGRAPHIC SPACE, U+2000–U+2006, U+2008–U+200A or U+205F (all Unicode category Zs). The pattern on line 61 — Pattern.compile("[\s\p{IsCc}\p{IsCf}\u2028\u2029]+") — combines the ASCII six from \s with the Cc and Cf categories, but no class in it matches Zs, so a value like "a\u2003b" passes through replaceAll(" ") unchanged and the raw separator reaches the log line. The javadoc at lines 66–67 states "every run of whitespace, control and format characters becomes a single space", which the code contradicts for interior runs: String.strip() on line 75 uses Character.isWhitespace (which returns true for U+2003 and U+3000, trimming them at the ends), but the pattern does not collapse them in the middle. NBSP-family U+00A0/U+2007/U+202F are likewise left intact. The gap cannot forge a record boundary or reorder text, so impact is limited, but the documented contract of the one helper all eight sites route through is wrong for these inputs; verify whether downstream log shippers treat Zs as whitespace before deciding to widen with \p{Zs}.
…hind 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.
|
@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java:61 — LogSafe.oneLine leaves Zs space separators (EM SPACE, IDEOGRAPHIC SPACE) unsplit despite the javadoc's "every run of whitespace" contract Correct, and fixed in The gap is slightly wider than the finding states.
So the inconsistency was not only interior-versus-edge; three of the seven were never removed at all. The class is now `[\s\p{IsZs}\p{IsCc}\p{IsCf} ]+ (The On the point about downstream shippers: no, they are not treated as whitespace, and that is the argument for collapsing rather than against. These cannot forge a boundary — which is why this is the cheapest of the four classes to justify — but two values differing only by an NBSP render identically to the operator reading the line, which is the same harm the Full suite green at 3364 tests, BugInstance size 0. Generated by Claude Code |
|
The next review will close every previous finding this comment names by its |
🤖 ThrillhouseBot — changes since the last review
|
There was a problem hiding this comment.
ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):
- LOW: LogSafe javadoc overclaims that every untrusted value in every log line goes through it (
src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java:22)
The class javadoc states "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". The PR's own description discloses that the three ReviewPublisher DEBUG lines (:720, :728, :874) carry the same model-supplied values and were deliberately left unsanitized for a separate pass, so the "every ... goes through here" claim is not currently true. Those lines are outside the diff hunks, so verify at ReviewPublisher.java:720/728/874 that they interpolate title/file as the description states; if so, qualify the javadoc so a maintainer reading it does not assume the class covers every log statement.
1 participant
What type of PR is this?
Description
A log record is something an operator reads and acts on, and the model chooses the finding titles
and paths that get spliced into these lines. A value carrying a line terminator splits the record
and forges a second one; a value carrying a bidi override or an ANSI escape reorders or erases what
is left of the first.
#744 routed the two
ReviewPublisherwarn lines throughMarkdownSafe.oneLine, whose collapseclass is
Pattern.compile("\\s+")— the ASCII six ([ \t\n\x0B\f\r]), becausejava.util.regexreads a bare
\sthat way unless the pattern asks for Unicode character classes. NEL (U+0085),LINE SEPARATOR (U+2028), PARAGRAPH SEPARATOR (U+2029), NUL, ESC and RLO all pass straight through
it, and
String.strip()does not catch them either — it trims the ends and these are interior. Thevector those two lines were sanitized against is therefore still open on them.
GitHubApiErrormet the same threat in #740 and installed the class that does cover it:[\s\p{IsCc}\p{IsCf} ]+. That reasoning never crossed over.The fix
Lift that class — and the javadoc that argues for it — into a new
LogSafe.oneLine, a helper forlog-destined strings, and route all eight sites through it:
ReviewPublisher.java:770file(wasMarkdownSafe.oneLine)ReviewPublisher.java:832file(wasMarkdownSafe.oneLine)FindingQuoteValidator.java:83title,fileFindingVerificationService.java:1229title,file,verdict.reason()FrameworkFalsePositiveFilter.java:74title,fileFindingPipeline.java:1370title,fileFindingDeduplicator.java:55file,titleFollowUpAnalyzer.java:633title,file,duplicateOf.title()All six INFO lines are live in a default install — the repository configures no
quarkus.loglevels anywhere. Each line number was checked against the tree before editing; all six still held.
GitHubApiErrornow calls the helper rather than keeping a private duplicate of the pattern, sothere is one class and one explanation rather than two that can drift.
MarkdownSafe.oneLineis deliberately left aloneIt also feeds markdown the bot posts to GitHub, where the wider class has a real rendering cost —
\p{IsCf}splits an emoji ZWJ sequence or an Indic conjunct apart. Abody=field or a warn lineis a diagnostic identity and pays that cost gladly; a posted comment is a rendering surface and
should not. Widening
WHITESPACE_RUNin place would have changed what the bot posts. One patternper destination, two intents kept apart.
The moved javadoc carries both halves of the bargain with it: the accepted
\p{IsCf}cost, and whythe replacement is a space rather than a deletion (deleting would let
admin<ZWSP>istratorclose upinto a different real word; a space cannot).
Related Issues
Fixes #755
How Has This Been Tested?
ModelSuppliedTextInLogLinesTestdrives all eight sites through their real production paths andcaptures the
java.util.logging.LogRecordeach class emits — the object every handler (console,file, syslog, JSON/ECS shipper) is handed — so the assertion does not depend on which handler
happens to be installed. Each test feeds one crafted
titleorfilecarrying NEL, U+2028, U+2029,NUL, ESC and RLO, then asserts the record still names the finding and carries none of the six.
LogSafeTestpins the helper itself: sixteen characters a reader could take for a control (eachbuilt by code point, not referenced from the production pattern), run collapsing, end trimming, the
space-not-deletion rule, an ordinary value left alone, and a
nullvalue.Red before the fix
All seven tests fail on the unfixed tree, each on the first forbidden character (U+0085), with the
remaining five visible in the rendered message:
<NEL>and the rest are the test's own rendering, applied only when building the failure message —the assertion runs
joined.contains(forbidden)against the raw record text with the literal codepoint. All six are present in every line; the assertion trips on the first.
Green after
Tests run: 3356, Failures: 0, Errors: 0, Skipped: 0.The first assertion of every test is that the log line ran at all and that the record still names
the finding, so a fix that simply dropped the line or blanked the value would not pass.
Checklist
Screenshots / Logs
Gates, in order:
./mvnw -B spotless:apply— BUILD SUCCESS./mvnw -B clean compile spotbugs:check spotless:check— BugInstance size is 0, BUILD SUCCESS./mvnw -B clean test— Tests run: 3356, Failures: 0, Errors: 0, Skipped: 0Coverage over
git diff -U0 73cc33d...HEADintersected with the jacoco report: zero uncoveredlines and zero uncovered branches across all nine changed main files.
Additional Notes
Milestone v0.6.5, which is not tagged yet.
The DEBUG lines in
ReviewPublisher(:720,:728,:874) carry the same values but are off in adefault install; they are left for a separate pass rather than widening this diff.