fix: resolve security, correctness, and maintenance review findings#99
Merged
Conversation
Address issues surfaced by a multi-agent review of the library. HIGH - jwt: reject reserved claim names in JwtClaims so caller-supplied additionalClaims can no longer overwrite iss/sub/aud/exp/jti/pkauth.* (impersonation / TTL-bypass via the documented claim-merge feature). - otp: fix off-by-one that allowed only maxAttempts-1 code comparisons. After the unconditional increment, reject with `>` (not `>=`) so exactly maxAttempts guesses are checked while keeping the anti-bypass behavior. - admin: map OTP AttemptsExceeded to RateLimited (429) instead of a 200 whose failure was buried in the body, so brute-force lockout is visible at the HTTP layer. Ordinary mismatch/expiry keep their typed 200 body. MEDIUM - magic-link: enforce the pkauth.purpose claim at the consume boundary (finishVerification(token, requiredPurpose) + ConsumeResult.WrongPurpose) so a login token can't be replayed at the email-verify endpoint; the single-use JTI is not burned on a cross-purpose attempt. - dynamodb: revokeFamily/revokeAllForUser now use a scalar-only conditional UpdateItem instead of a read-modify-write putItem, so a concurrent usedAt mark set by rotateAtomically can no longer be clobbered. - testkit: InMemoryRefreshTokenRepository.rotateAtomically no longer does a nested cross-key write inside ConcurrentHashMap.compute (contract violation). The successor is inserted first and rolled back on a lost race. - docs(CLAUDE): list pk-auth-admin-api, place AdminResult in its real module, and add the missing SPIs (ConsumedJtiStore, CeremonyRateLimiter, RevocationCheck, MessageFormatter). LOW - core: mark the lifecycle package @NullMarked (matches the other 10). - core: correct InMemoryWindowCounter doc (fixed-window, not sliding). - jdbi: close Update statements via try-with-resources in JdbiCredentialRepository.save and the backup-code audit insert. - dynamodb: order OTP findLatestActive by parsed Instant, not the variable-precision ISO string. - magic-link: document the consumedJtiTtl >= token-TTL invariant. - dropwizard: drop the stale TODO reference in AltFlowsModule javadoc. - build: add error_prone_annotations to the version catalog and replace 8 hardcoded coordinates across 4 build files. Deferred to its own commit: ceremony 429 Retry-After header, which is a wire-contract change to the exported CeremonyResponse record across all three adapters and warrants an @since/ADR of its own. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The admin endpoints already emit Retry-After on their rate-limit refusals, but the unauthenticated ceremony endpoints (registration/authentication start + finish) returned a bare 429, so clients and proxies got no backoff hint on the most attack-exposed surface. Add a `headers` component to the exported CeremonyWireMapper.CeremonyResponse record (with a backward-compatible two-arg `(status, body)` constructor) and emit `Retry-After: 60` on every ceremony 429. The value is a conservative constant matching InMemoryCeremonyRateLimiter.DEFAULT_WINDOW; the CeremonyRateLimiter SPI is boolean-only and exposes no per-call window, so an exact remaining-time can't be computed — Retry-After is advisory, so a stale hint is permitted. All three adapters (Spring/Micronaut/Dropwizard) now copy CeremonyResponse.headers() onto the native HTTP response. This was split out of the review-findings commit because it changes the public wire-contract record across all adapters. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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.
Summary
Addresses findings from a multi-agent review of the library (security, correctness, test, maintainability). One commit; the full
./gradlew checkgate passes (spotless, unit + Testcontainers integration tests, JaCoCo).HIGH
JwtClaimsrejects reserved claim names, so caller-suppliedadditionalClaimscan no longer overwriteiss/sub/aud/exp/jti/pkauth.*(impersonation / TTL-bypass via the documented claim-merge feature).>(not>=) so exactlymaxAttemptscode comparisons happen while keeping the anti-bypass behavior. +2 regression tests.AttemptsExceeded→RateLimited(429) so brute-force lockout is visible at the HTTP layer; ordinary mismatch/expiry keep their typed 200 body. +1 test.MEDIUM
finishVerification(token, requiredPurpose)+ConsumeResult.WrongPurpose); a login token can't be replayed at the email-verify endpoint and the single-use JTI isn't burned on a cross-purpose attempt. +1 test.revokeFamily/revokeAllForUseruse a scalar-only conditionalUpdateIteminstead of read-modify-writeputItem, so a concurrentusedAtmark can't be clobbered.InMemoryRefreshTokenRepository.rotateAtomicallyno longer does a nested cross-key write insideConcurrentHashMap.compute.pk-auth-admin-api, placeAdminResultin its real module, add the missing SPIs.LOW
lifecyclepackage@NullMarked;InMemoryWindowCounterfixed-window doc; JDBIUpdatetry-with-resources (×2); DynamoDB OTP latest-by-parsed-Instant; magic-link TTL invariant doc; AltFlowsModule stale-TODO comment;error_prone_annotationsinto the version catalog (8 hardcoded coords removed).Deferred to its own commit (in this PR)
Retry-Afterheader. This is a wire-contract change to the exportedCeremonyResponserecord across all three adapters, so it lands as a separate commit with its own@since.Test plan
./gradlew check(spotless + all tests + JaCoCo), Docker up for Testcontainers🤖 Generated with Claude Code