fix(github): take the credential value floors to a single character

[devops-thiago]

What type of PR is this?

• 🐛 Bug fix
• ✨ Feature
• 📝 Documentation
• 🔧 Refactor
• 🚀 Performance
• ✅ Test
• 🔒 Security
• 📦 Dependency update
• 🏗️ CI/CD

Description

clean() cuts a response body to 1024 characters before redactCredentials runs, so a
credential the cut severs can fall below its shape's length floor, stop matching, and reach the
warn line with whatever survived the cut intact.

#746 lowered the three floors from {10,} to {4,} on exactly that reasoning — but four is not
where the reasoning ends. A cut that leaves one, two or three value characters is still under the
floor, so the sigil plus up to three characters of the secret still reach the log. The window is a
third of its old width, not closed.

This takes all three shapes to {1,}:

• gh[pousr]_\w{1,} and github_pat_\w{1,} (GitHubApiError.java:93)
• bearer\s+[\w.~+/=-]{1,} (GitHubApiError.java:138)

The sigil is the whole discriminator, which is the argument the javadoc above these patterns
already makes: ghp_, github_pat_ and Bearer do not occur in prose, so the length floor was
never carrying the discrimination. {4,} was a stopping point rather than a reasoned bound — no
test pinned it. One is where the argument genuinely ends: a cut that leaves no value characters
strands the sigil by itself, and a sigil is not secret.

The second half of the issue is the javadoc. GitHubApiError.java:85-90 described the
severed-token case as closed while three characters were still going through. It now states the
residual it actually has, so the next reader starts from the true premise. The bearer shape's
paragraph at :127-129 is updated to match.

Over-masking

Every existing over-masking control stays green with no edit: gh_2.40.0_linux_amd64.tar.gz,
ghost_writer, the ghp_ prefix and the ghs_ prefix, the JWT-lookalike hostname and the
mid-run eyJ cases. Nothing in the suite regressed — full run below.

Related Issues

Fixes #757

How Has This Been Tested?

• Unit tests
• Integration tests
• Manual testing

GitHubApiErrorTest.ACredentialTheBoundCutJustShortOfItsLengthFloor already pinned nine surviving
value characters for each of the three shapes. Its fixture helper is generalised to take the
surviving-character count, and three tests are added at one surviving character — the tightest
point of the floor, and the case the fix has to close.

Red before the fix

The three new tests on the unfixed tree at 73cc33d (verbatim, from the surefire XML report — the
console renders the trailing … as ? because the surefire console stream is not UTF-8 here):

[ERROR] Tests run: 67, Failures: 3, Errors: 0, Skipped: 0

isStillMaskedForATokenPrefixCutToASingleCharacter
***,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,ghp_A… ==> expected: <false> but was: <true>

isStillMaskedForAFineGrainedPersonalAccessTokenCutToASingleCharacter
***,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,github_pat_A… ==> expected: <false> but was: <true>

isStillMaskedForABearerValueCutToASingleCharacter
***,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,,Bearer S… ==> expected: <false> but was: <true>

That is the claimed mechanism exactly: the credential-shaped prefix ahead of the token compresses
to ***, so the stranded sigil and its one surviving character clear the 512-character cap and
land in the body= field.

Green after

Tests run: 3328, Failures: 0, Errors: 0, Skipped: 0.

One further test, leavesTheBareSigilBehindWhenTheCutLandsBeforeTheFirstValueCharacter, is a
control rather than proof — it is green before and after. It pins the residual the javadoc now
names: a cut landing before any value character leaves ghp_… in the line, which is the floor's
true boundary and carries nothing secret.

Checklist

• My code follows the project's coding standards
• I have performed a self-review of my own code
• I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly
• My changes generate no new warnings or errors

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: 3328, Failures: 0, Errors: 0, Skipped: 0

Coverage over git diff -U0 73cc33d...HEAD intersected with the jacoco report: zero uncovered
lines and zero uncovered branches in the changed main code.

Additional Notes

Milestone v0.6.5, which is not tagged yet — this belongs in the release rather than on top of it.

#755 stacks on this branch, since it touches the same file; it re-targets main once this merges.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.

Scanned Files

None

[thrillhousebot]

🤖 ThrillhouseBot PR Summary

What this PR does

Lowers the credential-shape length floors in GitHub API error redaction from {4,} to {1,} so a token severed by the 1024-character pre-redaction cut still matches and is masked, and pins the new boundary with tests at one surviving value character for each shape plus a zero-character control test.

⚠️ Description vs. Implementation

The PR description does not fully match the change:

• PR description claims ghp_, github_pat_ and Bearer "do not occur in prose" and that the length floor was never carrying discrimination; the code's own javadoc above BEARER_SHAPED_VALUE says the word bearer "does mask the noun in prose such as missing bearer token", so the premise is false for the bearer shape and the new {1,} floor newly masks short prose phrases such as "bearer of" that the {4,} floor previously left intact.

Changes Overview

• Files changed: 2
• Lines added: +63
• Lines removed: -22

Changed Files

File	Change	Summary
src/main/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiError.java	Modified	Lowers value floors to {1,} for gh[pousr]/github_pat and bearer patterns; javadoc updated to describe the new boundary and the bare-sigil residual.
src/test/java/dev/thiagogonzaga/thrillhousebot/github/GitHubApiErrorTest.java	Modified	Generalises the cut fixture to a parameterised surviving-char count; adds one-char tests for all three shapes and a zero-char control pinning the bare sigil.

Risk Assessment

Risk	Count
🔴 Critical	0
🟠 High	0
🟡 Medium	0
🔵 Low	0

No new issues found in this PR, but the review cannot be approved until required CI is confirmed green.

⚠️ Required CI Checks Status

Some required checks are still pending or have failed:

Check	Type	Status	Detail
format	check-run	⏳ Pending	-
test	check-run	⏳ Pending	-
frontend	check-run	⏳ Pending	-
trivy	check-run	⏳ Pending	-
dependency-review	check-run	⏳ Pending	-

---

Automated review by ThrillhouseBot. Reply with /review to re-run.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[thrillhousebot]

ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:

• Check dependency-review is pending
• Check format is pending
• Check test is pending
• Check frontend is pending
• Check trivy is pending

[thrillhousebot]

ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:

• Check format is pending
• Check trivy is pending
• Check test is pending
• Check frontend is pending
• Check dependency-review is pending

[devops-thiago]

@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/LogSafe.java:22 — LogSafe javadoc overclaims that every untrusted value in every log line goes through it

Raised on #760, which merged into this branch carrying it open. Verified and fixed here in 271efd6.

The claim was false as written. All three lines interpolate finding.file() raw:

line	statement
ReviewPublisher.java:721	Line for %s:%d is outside the PR diff — filing the finding on the file instead
:729	Adjusted inline comment line for %s from %d to %d
:877	Inline comment rejected for %s:%d (suggestion=%s): %s

Widened the coverage rather than qualifying the claim. The sanitizing call is the one the warn lines beside them already make, and "debug is off by default" bounds who is exposed, not whether the vector exists — a debug-enabled install has precisely the surface the warn lines were fixed for. Qualifying the javadoc would have left a helper documenting its own gaps, which is the failure mode this PR exists to remove: MarkdownSafe.oneLine promised a collapse it did not perform, and #742 read as closed for a week because of it.

Grep confirms no Log.*f call in ReviewPublisher now interpolates finding.file() or finding.title() without going through the helper, so the class javadoc's "every value" is now true rather than aspirational.

This push also merges main (picking up #759) and carries the Zs widening from the earlier finding on #760.

Gates: BugInstance size is 0, spotless clean, full suite 3370 tests, 0 failures.

---

Generated by Claude Code

[thrillhousebot]

The next review will close every previous finding this comment names by its path:line and title; anything it does not name stays open.

[thrillhousebot]

🤖 ThrillhouseBot — changes since the last review

• New findings this round: 1
• Previous findings resolved: 0
• Previous findings still open: 0

[thrillhousebot]

ThrillhouseBot noted 1 lower-confidence item(s) under Things to double-check in the PR summary (not posted as inline threads):

• LOW: Log line reason slots interpolate rejectionReason raw despite LogSafe sweep (src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:771)
  The changed log lines in ReviewPublisher sanitize finding.file() but still interpolate the rejection-derived value raw: line 771 LogSafe.oneLine(finding.file()), finding.line(), reason);, line 833 LogSafe.oneLine(finding.file()), rejectionReason(e)); in the file-level warn, and the trailing : %s slot (line 883) in tryPostInlineComment. LogSafe's own javadoc claims it is "The single place untrusted text is flattened before it is interpolated into a log line" and lists "a GitHub error body" among the values that go through it; a rejection reason drawn from a GitHub rejection response is exactly that class of value. If rejectionReason(e) or reason can propagate any part of the raw response body — the #757 threat model explicitly includes a compromised endpoint — the forged-record vector this PR closes for title/file stays open through the reason slot. rejectionReason's implementation is not in the diff: verify it cannot return response-body text (or wrap the value in LogSafe.oneLine, which also handles null). The new ModelSuppliedTextInLogLinesTest cannot distinguish this: it drives the path with a constant WebApplicationException reason ("nope") containing no control characters.

[devops-thiago]

@thrillhousebot resolved src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:771 — Log line reason slots interpolate rejectionReason raw despite LogSafe sweep

Investigated as asked. The finding pointed at the right slot; the mechanism turned out to be one field over, and there was a real defect there. Fixed in 7d46425.

On the question actually posed — can rejectionReason propagate raw response body text? No. rejectionReason returns GitHubApiError.diagnostics(), whose body component is body.logged(), and clean() collapses that through LogSafe before redacting and capping it. The body reaches the reason slot already flattened.

But diagnostics() also interpolates four response headers, and those were verbatim:

append(text, "retry-after", retryAfter);
append(text, "x-ratelimit-remaining", rateLimitRemaining);
append(text, "x-ratelimit-reset", rateLimitReset);
append(text, "x-ratelimit-resource", rateLimitResource);

They come from response.getHeaderString(...) and went into the line untouched — one field away from a body being collapsed for exactly this reason. GitHubApiErrorHeaderCollapseTest, red before the fix:

[ERROR] Tests run: 3, Failures: 3, Errors: 0, Skipped: 0
NEL survived into the diagnostics line:
  status=403 x-ratelimit-resource=core?forged WARN approved body={"message":"no"}
ESC survived into the diagnostics line:
  status=403 x-ratelimit-remaining=0?[2Kcleared body={"message":"no"}
LINE SEPARATOR survived into the diagnostics line:
  status=403 retry-after=30?forged body={"message":"no"}

The collapse now goes in append, the one place every header passes through, so a header added later is covered without anyone having to remember. Two further tests pin that the value is kept rather than blanked, and that an ordinary header is untouched.

The orElseGet(e::toString) fallback — used when the exception carried no response at all — was raw for the same reason and now takes the same treatment, as does the non-WebApplicationException return.

Worth recording that your note about the new test was correct and is why this needed a separate one: ModelSuppliedTextInLogLinesTest drives the path with a constant reason containing no control characters, so it could never have caught this.

Full suite 3376 tests, 0 failures; BugInstance size is 0.

---

Generated by Claude Code

[thrillhousebot]

The next review will close every previous finding this comment names by its path:line and title; anything it does not name stays open.

[thrillhousebot]

🤖 ThrillhouseBot — changes since the last review

• New findings this round: 0
• Previous findings resolved: 1
  • src/main/java/dev/thiagogonzaga/thrillhousebot/review/ReviewPublisher.java:771 — Log line reason slots interpolate rejectionReason raw despite LogSafe sweep
• Previous findings still open: 0

[thrillhousebot]

ThrillhouseBot found no issues in this PR, but some checks are still pending or failed:

• Check format is pending
• Check test is pending
• Check frontend is pending
• Check trivy is pending
• Check dependency-review is pending

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud