fix(review): floor unmitigated-injection-sink findings at high risk - #594
Conversation
The same planted stored-XSS defect published as CRITICAL inline behind Angular's escape hatch and as MEDIUM in a collapsed low-confidence block behind React's. Severity is a property of the defect class, so nothing downstream enforced the prompt's own floor: the review call could rate the class differently per framework, and the verifier could take it down again because a rendering or query-dialect semantic "is not verifiable here". Apply the floor deterministically to the audit's output. A finding that names an injection sink AND states that nothing sanitizes, escapes, validates, encodes or parameterizes the value reaching it publishes at no less than high risk. Only risk moves, and only upward: confidence stays exactly where the model or the verifier put it, so the hedge survives while the misclassification does not. Placement follows, since a high-risk finding opens an inline thread however low its confidence. Read the hedging demotion clause by clause too. The prompt requires a demonstrated-sink finding to name the unshown layer to verify, which put a hedge word into the description of every such finding by construction; a whole-body scan then read that mandated clause as a hedge of the claim and stripped the finding's blocking confidence for saying what it was told to say. Fixes #570
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
🤖 ThrillhouseBot PR SummaryWhat this PR doesWraps the verification pipeline's output in a deterministic severity floor that publishes any finding whose text names an injection sink while claiming no sanitization at no less than HIGH risk, regardless of the model's rating, a verifier downgrade, or fail-open returns, and narrows the hedging-confidence demotion so a hedge word inside a clause that names something to verify no longer strips blocking confidence.
|
| File | Change | Summary |
|---|---|---|
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java |
Modified | Adds a deterministic high-risk floor for unmitigated injection-sink findings applied in verify() over the extracted audit() path, plus clause-scoped hedging detection with a verification-cue exemption. |
src/test/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationServiceTest.java |
Modified | Adds seven tests: the #570 XSS-pair regression with matching severity and placement, string-built-SQL and verifier-downgrade flooring, clause-scoped hedge demotion, and no-op pinning. |
Risk Assessment
| Risk | Count |
|---|---|
| 🔴 Critical | 0 |
| 🟠 High | 0 |
| 🟡 Medium | 1 |
| 🔵 Low | 1 |
Key Findings
- MEDIUM: Floor trigger matches negated sinks and positive mitigation claims (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:378)
Things to double-check
1 lower-confidence finding
- LOW: RiskLevel.fromString(...).ordinal() dereferenced without null guard (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:371) (low confidence — verify before acting)
⚠️ CI Checks Status
Some checks are still pending or have failed:
| Check | Type | Status | Detail |
|---|---|---|---|
| frontend | check-run | ⏳ Pending | - |
| format | check-run | ⏳ Pending | - |
| trivy | check-run | ⏳ Pending | - |
| test | check-run | ⏳ Pending | - |
| actionlint | check-run | ⏳ Pending | - |
| changes | check-run | ⏳ Pending | - |
| dependency-review | check-run | ⏳ Pending | - |
Automated review by ThrillhouseBot. Reply with /review to re-run.
![thrillhousebot[bot]](https://avatars.githubusercontent.com/in/3996985?s=60&v=4){kind=small}
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: RiskLevel.fromString(...).ordinal() dereferenced without null guard (
src/main/java/dev/thiagogonzaga/thrillhousebot/review/ai/FindingVerificationService.java:371)
belowInjectionSinkFloor runs RiskLevel.fromString(finding.risk()).ordinal() for every finding in every review, with no null check. The pre-existing same-file isBlockingEligible deliberately handles the same call null-tolerantly (risk == RiskLevel.CRITICAL || risk == RiskLevel.HIGH), and this file's strictRisk returns null for unrecognized labels — an asymmetry suggesting fromString might also be able to yield null (or throw) for a risk string the pipeline did not normalize. If that is possible, one malformed model label would NPE the whole verify() path instead of failing soft, dropping the review. This cannot be confirmed from the provided material: verify RiskLevel.fromString's contract (does it throw, default, or return null on an unrecognized label?) and whether the review-response parser normalizes risk labels before constructing ReviewResponse.Finding. If null is possible, guard the call — e.g., treat an unparseable risk as 'do not floor' (return false) — before calling .ordinal().
thrillhousebot
Bot
added
bug
…the sink out
Both floor signals are token matches scanned over the whole finding text, so a
sentence that DENIES the sink satisfied them out of its own negation: "sql
injection" matched inside "so no SQL injection is possible", and the absence
group matched "not concatenated but parameterized" — the very mitigation the
sentence asserts. The floor then raised that finding to high, and at high
confidence it would go on to block the merge. That is this floor's own failure
mode pointed the other way, and it contradicted the trigger's documented
narrowness.
Add two defeaters, either of which suppresses the floor anywhere in the text: a
denied sink ("no XSS", "not vulnerable to SQL injection"), and a mitigation the
finding says IS present ("it is escaped on render", "React escapes them").
Negated forms are excluded from the second so "is not escaped" stays an absence
claim rather than defeating itself, and the denial gap is one word so "no
protection against SQL injection" is still read as an assertion of the defect.
Under-firing leaves a finding where this class already stood; over-firing
escalates a non-defect. The defeaters take the safe direction.
|
🤖 ThrillhouseBot — changes since the last review
|
| "\\b(unsanitiz\\w*|unescaped|unvalidated|unparameteriz\\w*|non-parameteriz\\w*" | ||
| + "|(no|not|without|missing|never|lacks?|absent)\\s+(\\w+[\\s-]+){0,3}" |
There was a problem hiding this comment.
🟡 MEDIUM — NO_MITIGATION cannot match "Nothing sanitizes …" — the floor misses its own trigger phrasing
The floor javadoc defines the trigger as a finding that "states that nothing sanitizes, escapes, validates, encodes or parameterizes the value reaching it publishes at no less than high", and the NO_MITIGATION comment says it captures "the finding's own statement that nothing neutralizes the tainted value". But the pattern's absence-word list is (no|not|without|missing|never|lacks?|absent): the compound words "nothing"/"nobody" are absent, and \bno\b cannot match inside "Nothing" because there is no word boundary after "no". A finding whose body is "Nothing sanitizes the channel value before it reaches innerHTML" matches INJECTION_SINK (innerhtml) but not NO_MITIGATION, so belowInjectionSinkFloor returns false and the finding still publishes at the model's below-high rating — the exact #570 drift this PR exists to prevent. Adding "nothing|nobody" to the negation group fixes it and does not change any in-diff test (the literal-template fixture only contains "nothing user-authored reaches it", which has no sanitize/escape/validate/parameterize root after the negation).
| "\\b(unsanitiz\\w*|unescaped|unvalidated|unparameteriz\\w*|non-parameteriz\\w*" | |
| + "|(no|not|without|missing|never|lacks?|absent)\\s+(\\w+[\\s-]+){0,3}" | |
| "\\b(unsanitiz\\w*|unescaped|unvalidated|unparameteriz\\w*|non-parameteriz\\w*" | |
| + "|(no|not|without|missing|never|nothing|nobody|lacks?|absent)\\s+(\\w+[\\s-]+){0,3}" |
| "\\b(no|not|never)\\s+(\\w+[\\s-]+){0,1}" | ||
| + "(xss|cross[- ]site scripting|sql injection|command injection|shell injection" |
There was a problem hiding this comment.
🟡 MEDIUM — SINK_DENIED swallows "does not prevent SQL injection" — an assertion, not a denial
The SINK_DENIED javadoc claims "an assertion of the defect that merely contains a negation ("no protection against SQL injection") is not mistaken for a denial", and belowInjectionSinkFloor suppresses the floor whenever SINK_DENIED matches. But the pattern \b(no|not|never)\s+(\w+[\s-]+){0,1} plus the sink alternation matches "not prevent SQL injection" (one bridge word between the negation and the sink name). A genuine defect finding — title "SQL injection in findPendingByChannel via unsanitized channel", description "The code does not prevent SQL injection because the channel is concatenated into the query text" — satisfies namesInjectionSink and NO_MITIGATION, then is exempted by SINK_DENIED and never floored. The one-word gap only excludes the two-word "no protection against SQL injection"; assertions phrased as "not prevent/stop/block ... SQL injection" are swallowed, so the under-rated publication #570 describes survives in a very common phrasing. The in-diff denial test uses "not concatenated but parameterized" and does not cover this shape.
| "\\b(no|not|never)\\s+(\\w+[\\s-]+){0,1}" | |
| + "(xss|cross[- ]site scripting|sql injection|command injection|shell injection" | |
| "\\b(no|not|never)\\s+(?!(prevent\\w*|stop\\w*|block\\w*|avoid\\w*|mitigat\\w*|defend\\w*)\\b)" | |
| + "(\\w+[\\s-]+){0,1}" | |
| + "(xss|cross[- ]site scripting|sql injection|command injection|shell injection" |
1 participant
What type of PR is this?
Description
Round 3 reproduced #570's controlled pair with brand new code: the same planted
stored-XSS defect published as CRITICAL, inline, top Key Finding behind
Angular's
bypassSecurityTrustHtmland as MEDIUM, collapsed, low confidencebehind React's
dangerouslySetInnerHTML. A string-concatenated SQL tenantfilter published the same way — asserted as fact in the walkthrough row,
hedged at MEDIUM/low-confidence in the finding for the same line.
#575 addressed this in
PrReviewPrompts.SYSTEMalone. The floor it added isprose, and nothing downstream enforced it, so two separate mechanisms could
still undo it:
FindingVerificationService.downgradeapplies whatever risk the verifier proposes, in the lowering direction,
with no floor. The verifier's own rejection grounds name "routing and
rendering semantics" as remembered framework behavior capped at
"medium"risk with"low"confidence — which is precisely how ademonstrated
dangerouslySetInnerHTMLsink is described. That producesexactly the observed
MEDIUM+ low-confidence pair, and explains why thewalkthrough (never verified) asserts what the finding (verified) hedges.
demoteHedgedBlockingFindingsscans the whole title and description for
may|might|could|potentially|possibly.fix(review): keep security severity on the defect class, not on uncertainty #575's own instruction — "lower the confidence, name in the description the
exact layer to verify" — puts one of those words into the description of
every demonstrated-sink finding by construction. The guard then read that
mandated clause as a hedge of the claim and took the finding's confidence
from
hightomedium, which costs it its blocking eligibility(
BlockingStrictness.BALANCEDrequiresConfidence.HIGH) and stamps a"verify before acting" disclaimer on a defect the diff demonstrates.
Production logs from the round-3 corpus show it firing on
SQL injection in findPendingByChannel via unsanitized channel.What changed
A deterministic severity floor, applied to the audit's output. A finding
that names an injection sink and states that nothing sanitizes, escapes,
validates, encodes or parameterizes the value reaching it publishes at no less
than
highrisk. It is applied inverify, wrapping every path — the model'sown rating, a verifier downgrade, and every fail-open return — and the summary
counts are recomputed when it fires.
Only risk moves, and only upward. Confidence is left exactly where the model or
the verifier put it, which is the separation #570 asks for: severity is a
property of the defect class and its blast radius, while "some layer I was not
shown might neutralize this" is a statement about confidence. Placement
follows severity:
Finding.postsInlinekeeps ahighfinding on an inlinethread however low its confidence, so the same defect class stops landing in
the collapsed "Things to double-check" block in one framework and on the diff
in another.
The trigger requires both halves stated in the finding's own words, so it does
not fire on a sink mentioned in passing (an escape hatch rendering a literal
template) or on an unrelated finding that happens to say "unvalidated". A
criticalkeeps its level — the floor lifts, never lowers.Both halves are token matches, though, so a sentence that rules the sink out
could satisfy them out of its own negation — "sql injection" matches inside "so
no SQL injection is possible", and the absence group matches "not concatenated
but parameterized", the very mitigation that sentence asserts. Two defeaters,
either of which suppresses the floor anywhere in the text, close that: a
denied sink ("no XSS", "not vulnerable to SQL injection") and a
mitigation the finding says IS present ("it is escaped on render", "React
escapes them"). Negated forms are excluded from the second, so "is not escaped"
stays an absence claim rather than defeating itself, and the denial gap is one
word so "no protection against SQL injection" still reads as an assertion of
the defect. Under-firing leaves a finding where this class already stood;
over-firing escalates a non-defect and, at high confidence, would block the
merge on it — the defeaters take the safe direction.
The hedging demotion now reads clause by clause. A hedge word inside a
clause that names something to verify is the verification request the prompt
mandates next to a demonstrated defect, not a hedge of the defect. A hedge
anywhere else still demotes, so the guard's intent — a claim that hedges itself
cannot request changes on its own — is untouched.
Note on the collapsed-block routing
Routing was investigated and is not itself broken.
Finding.postsInline()keys onConfidence.LOWonly, somediumconfidencestill posts inline; a
critical/highfinding posts inline at any confidence.The collapsed block therefore requires both a below-high risk and low
confidence, which is why the severity floor — not a routing change — is the
fix. No renderer file is touched by this PR.
Related Issues
Fixes #570
How Has This Been Tested?
Eight new tests in
FindingVerificationServiceTest, including the regressiontest #570 asks for (the same class in two frameworks must publish the same
severity and the same placement).
Red output on unfixed code
leavesCriticalInjectionFindingsAndUnrelatedRatingsAlonepasses before andafter — it pins what the floor must not do.
Red output for the two defeaters
Both defeater tests fail on the floor as first written, each raising a finding
that rules the sink out to
high(noteconfidence=highon the first: itwould have become blocking-eligible):
Gates
Coverage — jacoco ∩
git diff -U0 fda4bc7...HEADon changed main code: 60instrumented changed lines, zero uncovered lines and zero uncovered
branches.
Checklist
Additional Notes
The verifier's own prompt (
FindingVerifierPrompts.SYSTEM) still lists"routing and rendering semantics" as a downgrade ground with no carve-out for a
sink whose tainted value is visible in the diff, the way it already carves out
config/IaC, mock-fidelity, heuristic and producer→consumer findings. That file
is outside this change's scope; the deterministic floor makes the outcome
correct regardless of what the verifier proposes, but a matching carve-out
there would stop the verifier spending a downgrade on it in the first place.