Maintainability pass: correctness fixes, dedup, sealed start* ceremony, docs#59
Merged
Conversation
…otation rotateAtomically previously caught every TransactionCanceledException and returned false, signalling "race lost" to the caller — which scorches the token family. A transient cancellation (throughput, transaction conflict, validation) would therefore be misread as a replay and silently revoke a legitimate family. Now inspect cancellationReasons(): return false only when the parent's freshness condition (index 0) is ConditionalCheckFailed, and rethrow anything else so it maps to a retryable 5xx. Fail closed when the reason is undeterminable. Replay detection is unchanged (a genuinely used/revoked/expired parent still surfaces as ConditionalCheckFailed). Also guard the two full-table scans (refresh deleteExpiredBefore, access-token deleteExpiredBefore) with a server-side begins_with(pk,...) filter so non-matching rows of the shared table stay off the wire, replace the brittle regionMatches(...,3) prefix test with startsWith, route used/ revoked instant fields through the central encode helpers, and remove dead code (UNUSED_CONDITION, an unused local, snake_case helper name). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… branch The no-active-OTP branch computed a throwaway HMAC and then ran MessageDigest.isEqual(dummy, dummy) — comparing a value to itself and discarding the result. The self-compare equalizes nothing the HMAC doesn't already dominate and reads as dead code. Keep the throwaway hmacHash(candidate) (the dominant cost that hides whether an OTP exists for the user), drop the self-compare. Mirrors the dummy-Argon2 verify in BackupCodeService. Timing resistance is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…n tests ADR 0018 removed SonarQube; delete the leftover scan step from the browser job. Add a guard to the build job that fails the gate if PKAUTH_SKIP_TESTCONTAINERS=1 leaks into CI — otherwise the persistence integration and concurrency suites self-skip and the build goes green with zero real-DB coverage. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- OTP hashing is HMAC-SHA256 with a server-side pepper, not Argon2id (DESIGN, GETTING_STARTED, threat-model); rewrite the OTP DoS mitigation to credit the attempt cap + rate limiter rather than hash cost. Argon2id retained only for backup codes. - Document the userVerification=REQUIRED default and the risk of relaxing it (threat-model Spoofing row + operator-guide UV subsection). - Correct the OTP pepper minimum length (>=16 bytes decoded, 32+ recommended) and dev-mode throwaway behavior. - Bump schema version 9 -> 10 and add V10 (amr column) in DESIGN §8. - Give the DESIGN §6 SPI table a Module column and reconcile the §2 prose list with the real owning modules. - Describe the magic-link token as a signed single-use HS256 JWT. - Replace CONTRIBUTING's stale per-module-README claim with a pointer to GETTING_STARTED; de-hardcode the CLAUDE.md @SInCE target. - Add docs/adr/README.md index; bring the micronaut demo to parity with a docker-compose.yml + README persistence note. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The feature-service Config records validated the pepper/baseUrl/non-null fields but not the numeric/duration ranges. A zero or negative maxAttempts, ttl, rateLimit, codeCount, or Argon2 parameter would silently produce never-verifiable or instantly-expired codes. Reject non-positive values in the compact constructors, with tests covering each branch. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…witch startPhoneVerification cast SendResult to Sent after only ruling out RateLimited. A future SendResult variant would throw ClassCastException across the AdminResult boundary instead of failing at compile time. Switch exhaustively over the sealed sum, matching finishPhoneVerification. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…envelope
The three adapters each hardcoded the 503 status and the
{"error":"persistence_failure","operation":...} body in byte-identical
LinkedHashMap blocks. Add PkAuthPersistenceResponse to core spi (next to the
exception it renders) as the single source of the status and body, and have
each adapter bind it while keeping its framework-specific response type and
logging. Guarantees a DB outage can't drift the envelope between adapters or
leak a framework-default 500.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… translation Completes the host->domain config centralization begun in be10176. Add RelyingPartyConfig.from(id,name,origins) carrying the shared required-field validation and canonical "pkauth.relying-party.* are required" message, and CeremonyConfig.from(...) which coalesces each null knob to the conservative core default (UV=REQUIRED, counter=REJECT) — a null never weakens a knob. All three adapters now delegate instead of rebuilding the records field by field; the security-relevant RP/origin/UV validation lives in one place. Dropwizard gains the friendly required-field check it previously lacked. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… result start* ceremonies threw CeremonyRateLimitedException across the adapter boundary — the one place the otherwise-uniform "results, not exceptions" contract was broken, and a trap for any new adapter that forgot the handler. Introduce sealed StartRegistrationResult (Started | RateLimited) and StartAuthenticationResult, mirroring the finish* result sums: - PasskeyAuthenticationService.start* now return the sealed result; the internal limiter check returns the denied bucket instead of throwing, still evaluated before any ChallengeStore interaction (throttle-before-challenge preserved). - Each adapter controller pattern-matches the sum (Started -> 200 with the options envelope, RateLimited -> 429), so a new variant is a compile error in every adapter rather than a runtime 500. The per-adapter exception handlers are removed. - Delete CeremonyRateLimitedException; update CeremonyRateLimiter and CeremonyWireMapper docs. responseOrThrow() is a caller-side convenience for embedded/test callers that never configure a limiter. Also correct a long-standing doc error: all three adapters mount identical /auth/passkeys/... paths (verified in code); fix the false "Dropwizard mounts one segment shorter" claim in CLAUDE.md, DESIGN.md, and the browser SDK README (whose override example would have broken against the real Dropwizard adapter). Includes a google-java-format reflow of the DynamoDB refresh-token repo. BREAKING CHANGE: PasskeyAuthenticationService.startRegistration/ startAuthentication now return StartRegistrationResult/StartAuthenticationResult instead of the bare response envelope, and CeremonyRateLimitedException is removed. The /auth/** wire contract (200/429 bodies) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…odes The atomic single-use race was proven only for refresh tokens; OTP and backup-code consume were tested single-threaded, so a regression from an atomic conditional UPDATE to a read-modify-write in either backend would let two callers redeem the same code (OTP / backup-code reuse) undetected. Add OtpRepositoryScenarios and BackupCodeRepositoryScenarios to the testkit — each fires 8 threads at consume() on one freshly-saved row and asserts exactly one winner — mirroring RefreshTokenScenarios. Run both against real Postgres (JDBI) and DynamoDB Local in the AltFlows integration suites. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…nal SPIs The required CredentialRepository CRUD methods (save/findByCredentialId/ findByUserHandle/updateSignCount) carried no Javadoc — document return-on-miss, the DuplicateCredentialException-on-duplicate contract, and the last-writer-wins / no-CAS semantics of updateSignCount so the SPI is implementable without reading source. Add the missing method-level @SInCE and brief contracts to OriginValidator.isAllowed, AttestationTrustPolicy.evaluate, ClockProvider.now, RevocationCheck (class + isRevoked), and the JwtVerificationResult sealed interface. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The earlier "drop residual SonarQube scan" change over-reached: that step was the TypeScript/browser-SDK scan (projectBaseDir: clients/passkeys-browser), not a JVM scan. SonarQube stays removed for the Java modules (ADR 0018 -> native JaCoCo gates) but is wanted for the JS SDK. Restore the step; sonar-project.properties was never removed. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
wolpert
enabled auto-merge (squash)
June 12, 2026 14:46
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the prioritized roadmap from the multi-agent evaluation of the repo — across three tiers plus the deferred start*-ceremony architectural change. Every change preserves the library's security invariants (verified by a dedicated security review, below); several strengthen them.
Tier 1 — correctness & security-facing
rotateAtomicallynow treats only a parent-freshnessConditionalCheckFailedas race-lost; transientTransactionCanceledExceptions (throughput/conflict/validation) are rethrown to a retryable 5xx instead of being misread as a replay and silently scorching a legitimate token family. Fail-closed on an indeterminable reason. Replay detection unchanged (8-thread concurrent-rotation test still passes).deleteExpiredBefore) with a server-sidebegins_with(pk, …)filter; replace the brittleregionMatches(…,3)prefix test withstartsWith.userVerification=REQUIREDdefault and the OTP pepper ≥16-byte requirement.PKAUTH_SKIP_TESTCONTAINERS=1leaks in (would otherwise green a build with zero persistence/concurrency coverage). SonarQube stays removed for the JVM modules (ADR 0018 → native JaCoCo gates) but is retained for the TypeScript/browser SDK.Tier 2 — testing & deduplication
Configrecords (reject non-positive ttl/attempts/limits).SendResultdowncast with an exhaustive sealed switch.PkAuthPersistenceResponse) and the relying-party/ceremony config translation (RelyingPartyConfig.from/CeremonyConfig.from) — null never weakens a knob.Architectural — start* ceremony sealed result
StartRegistrationResult/StartAuthenticationResult(Started | RateLimited) instead of a thrownCeremonyRateLimitedException. Adapters pattern-match the sum (a new variant is now a compile error in every adapter, not a runtime 500). Throttle-before-challenge ordering and the 429 wire shape are unchanged./auth/passkeys/…paths (verified in code) — fixes the false "Dropwizard one segment shorter" claim in CLAUDE.md, DESIGN.md, and the browser SDK README (whose override example would have broken against the real Dropwizard adapter).Tier 3 — docs & Javadoc
@sinceon the requiredCredentialRepositoryCRUD and several optional SPIs.BREAKING CHANGE
PasskeyAuthenticationService.start{Registration,Authentication}now return the sealed result types instead of the bare response envelope, andCeremonyRateLimitedExceptionis removed. The/auth/**HTTP wire contract (200/429 bodies) is unchanged.Validation
./gradlew check(excluding the Chrome-dependent example demos) passes: spotlessCheck, all unit + Testcontainers integration tests, and JaCoCo coverage on every module.SecureRandom,MessageDigest.isEqualconstant-time compares, JWT alg-pinning, and the account-enumeration guards on start endpoints are all preserved.🤖 Generated with Claude Code