Skip to content

fix: resolve SonarQube reliability bugs and null-contract violations#54

Merged
wolpert merged 1 commit into
mainfrom
fix/sonar-reliability-bugs
Jun 11, 2026
Merged

fix: resolve SonarQube reliability bugs and null-contract violations#54
wolpert merged 1 commit into
mainfrom
fix/sonar-reliability-bugs

Conversation

@wolpert

@wolpert wolpert commented Jun 11, 2026

Copy link
Copy Markdown
Contributor

Summary

Addresses the SonarQube findings driving the failed quality gate, focusing on genuine correctness issues rather than gaming the metric. Two tiers:

Tier 1 — Reliability (the rating mover)

Four S2095 BLOCKER resource leaks. The E new-code reliability rating requires at least one BLOCKER reliability bug — these are it.

  • JdbiBackupCodeRepository / JdbiRefreshTokenRepository: JDBI Update was built and execute()-d without being closed if execute() threw → wrapped in try-with-resources.
  • testkit ChallengeStoreScenarios / RefreshTokenScenarios: ExecutorService converted to try-with-resources (JDK 21 AutoCloseable), removing the manual finally.

Tier 2 — JSpecify null-contract corrections (S2583 / S2637)

These were flagged because defensive null-handling met a @NullMarked @NonNull contract. Fixed by making the contract honest, not by deleting safety checks:

  • @Nullable on bindNullable(Object value) (its whole purpose is binding nullable values), Transport.fromWire(String) (javadoc documents null handling), and the adapter clientIp / attachedUserHandle helpers that already returned null.
  • @Nullable on Spring @RequestBody params the handlers already null-guard (Spring genuinely injects null for absent bodies).
  • @Nullable on PkAuthJwtAuthenticationToken.token (+ getToken/getCredentials), which eraseCredentials() already nulls.
  • Removed dead body == null guards in the email/SMS senders whose params are @NonNull by SPI contract.

Deliberately not changed

To keep every change high-quality, these Sonar findings are better resolved in the SonarQube UI than by editing code:

  • jti == null guards in the access-token stores — intentional defense-in-depth in security-sensitive validation; keep and mark intentional.
  • OtpPepperResolver S2119 "reuse Random" — false positive; it's a one-shot SecureRandom at startup.
  • S6218 array equals/hashCode and S8692 test-clock — out of scope (lower-value tiers).

Verification

  • ./gradlew check passes — compile, Error Prone, JSpecify nullness across all modules, Spotless.
  • All unit tests + Docker-backed JDBI/DynamoDB integration tests pass, including the concurrentTakeOnce / concurrentRotation TCK race scenarios that exercise the changed ExecutorService cleanup.
  • Security-reviewed: no exploitable change. The only behavioral shift (Spring @Nullable @RequestBody relaxes required=true) is non-exploitable — the empty-input path is already reachable via {} and the service layer rejects blank tokens/codes.

🤖 Generated with Claude Code

Tier 1 — reliability (BLOCKER S2095 resource leaks):
- Wrap JDBI Update in try-with-resources in JdbiBackupCodeRepository and
  JdbiRefreshTokenRepository so the statement is closed if execute() throws.
- Convert the testkit concurrency-scenario ExecutorService usages to
  try-with-resources (JDK 21 AutoCloseable), dropping the manual finally.

Tier 2 — JSpecify null-contract corrections (S2583/S2637):
- @nullable on bindNullable(Object value) params (the method exists to bind
  nullable values), Transport.fromWire(String), and the adapter clientIp /
  attachedUserHandle helpers that already returned null.
- @nullable on Spring @RequestBody params that the existing handlers already
  null-guard (Spring can inject a null body); service layer still validates
  blank input, so no behavioral regression on the unauthenticated endpoints.
- @nullable on PkAuthJwtAuthenticationToken.token (+ getToken/getCredentials),
  which eraseCredentials() already nulls.
- Drop dead body==null guards in the email/SMS senders whose params are
  @nonnull by SPI contract.

Verified: ./gradlew check passes; JDBI + DynamoDB integration and testkit
concurrency TCK tests green. Security-reviewed: no exploitable changes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@wolpert
wolpert enabled auto-merge (rebase) June 11, 2026 14:57
@wolpert
wolpert merged commit a9b46f0 into main Jun 11, 2026
5 checks passed
@wolpert
wolpert deleted the fix/sonar-reliability-bugs branch June 11, 2026 15:01
@sonarqubecloud

Copy link
Copy Markdown

Quality Gate Failed Quality Gate failed

Failed conditions
64.3% Coverage on New Code (required ≥ 80%)
8.7% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant