fix: resolve security & correctness findings from review#101
Merged
Conversation
Address the actionable High/Medium/Low findings from the multi-agent review. All fixes land together; `./gradlew check` is green (spotless + tests incl. Testcontainers + JaCoCo). High - magic-link(jwt): stop magic links inheriting the 1h access-token TTL. PkAuthJwtIssuer gains an issue(claims, ttlOverride) overload; MagicLink now issues with its own tokenTtl (default 15m) and enforces consumedJtiTtl >= tokenTtl at construction, closing the replay window where a redeemed link stayed valid after its single-use JTI was evicted. - magic-link: startLogin now delivers ONLY to the address bound to the resolved user (UserLookup#emailFor), never the caller-supplied one — previously an attacker could request a login token for any account by username and have it sent to an address they control. Skips the send (enumeration-resistant Sent) when no bound email exists. - build: reconcile the Spotless version/pin/docs divergence — the catalog comment + dependabot ignore claimed a 7.x pin while the build ran 8.7.0 (the ignore targeted the plugin-marker artifact, not the implementation artifact, so it never matched). Docs now match reality; dead ignore removed. Medium - core: enforce per-request userVerification at finish. The resolved UV is persisted on ChallengeRecord at start and the finish step now requires UV if EITHER the global config OR the per-request value is REQUIRED (max of the two), so a step-up REQUIRED can no longer be silently downgraded. Adds challenges.user_verification (Flyway V11, schema -> "11") and carries it through the JDBI and DynamoDB challenge stores. - dynamodb(refresh): make create() a single atomic TransactWriteItems for the primary + user/family index items, eliminating the orphaned-primary window that revokeFamily/revokeAllForUser would silently skip; duplicate refreshId now surfaces via PkAuthPersistenceException, not a raw escape. - jdbi(credentials): updateSignCount guard relaxed from `< :sc` to `<= :sc` so last_used_at is recorded for sync passkeys that always report signCount=0 (regression is still rejected), per the SPI contract. - spring: pin pkAuthSecurityFilterChain order so a host catch-all chain can't swallow /auth/**, and disable global servlet auto-registration of the JWT filter so it runs only inside the security chain. Low - core: null-guard AAGUID in persistRegistration so a null AAGUID returns a sealed RegistrationResult instead of NPE-ing across the boundary. - jwt: JwtKeyset.es256 stores only public JWKs in verificationSource() so a host that publishes it as JWKS cannot leak the private signing key. - otp: TwilioSmsSender no longer echoes accountSid/fromNumber/authToken into its exception message. Deferred (noted for follow-up): magic-link dedicated-audience split (needs per-adapter validator rewiring) and in-store OTP expiry filtering (needs a ClockProvider threaded through the OtpRepository SPI). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
…P expiry filter)
Both were noted as follow-up in the prior commit; they land here with the
full `./gradlew check` gate green (Spotless + tests incl. Testcontainers +
JaCoCo).
magic-link: dedicated audience (token-confusion defense)
- MagicLinkService.DEFAULT_AUDIENCE ("pkauth:magic-link") + a new
Dependencies.ofDedicatedAudience(keyset, issuerName, ...) factory that
builds a magic-link-scoped JWT issuer + validator (no-op AccessTokenStore,
so magic-link jtis never enter the access-token store). Because magic-link
tokens now carry the magic-link audience, the host's resource-server
validator (application audience) rejects them — a magic-link token can no
longer be replayed as an API bearer/access token.
- All three adapters (Spring autoconfigure, Dropwizard Dagger, Micronaut
factory) wire the magic-link service via ofDedicatedAudience using the
shared keyset + the resource-server issuer name.
- Adds a test proving a magic-link token is rejected by a resource-server
validator yet still consumed by its own service.
otp: in-store expiry filtering
- OtpRepository.findLatestActive gains an `Instant now` parameter so each
backend can filter `expiresAt > now` against the HOST clock (not the DB
wall clock, which would be a second, uncontrollable clock source and broke
fixed-clock tests). OtpService passes clockProvider.now(); the existing
service-level expiry re-check stays as defense-in-depth.
- JDBI (`AND expires_at > :now`), DynamoDB (stream filter on parsed
expiresAt), and the in-memory testkit all filter expiry now.
- OtpServiceTest.expiredOtpIsRejected now drives the service-level re-check
via a non-filtering repository decorator, preserving that branch's coverage.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Author
|
Both originally-deferred items are now implemented in commit
|
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.
Resolves the actionable High / Medium / Low findings from a multi-agent review of the library. All fixes are in this branch;
./gradlew checkis green (Spotless + tests incl. Testcontainers Postgres/DynamoDB + JaCoCo).High
PkAuthJwtIssuergains anissue(claims, ttlOverride)overload; magic links now use their own 15m TTL instead of inheriting the 1h access-token TTL, andconsumedJtiTtl >= tokenTtlis enforced at construction — closing the window where a redeemed link stayed valid after its single-use JTI was evicted.startLoginaccount takeover. Login links are now delivered only to the address bound to the resolved user (UserLookup#emailFor), never the caller-supplied address. Skips the send (enumeration-resistantSent) when no bound email exists.Medium
userVerificationenforced at finish. Resolved UV is persisted onChallengeRecordat start; finish requires UV if either the global config or the per-request value isREQUIRED(max of the two), so a step-upREQUIREDcan no longer be silently downgraded. Addschallenges.user_verification(Flyway V11, schema →11), carried through the JDBI + DynamoDB challenge stores.create()non-atomic write. Now a single atomicTransactWriteItemsfor the primary + user/family index items, eliminating the orphaned-primary window thatrevokeFamily/revokeAllForUserwould silently skip; duplicaterefreshIdsurfaces viaPkAuthPersistenceExceptioninstead of a raw escape.updateSignCount. Guard relaxed< :sc→<= :scsolast_used_atis recorded for sync passkeys that always reportsignCount=0(regression still rejected), per the SPI contract.@Orderso a host catch-all chain can't swallow/auth/**, and the JWT filter's global servlet auto-registration is disabled so it runs only inside the security chain.Low
persistRegistration(sealed result instead of NPE across the boundary).JwtKeyset.es256stores only public JWKs inverificationSource()(no private-key leak if published as JWKS).TwilioSmsSenderno longer echoes credential material into its exception message.Follow-up (in this PR)
The two items originally deferred are being added as follow-up commits:
ClockProviderthreaded through theOtpRepositorySPI.🤖 Generated with Claude Code