fix: resolve security, correctness, and maintenance review findings

CLAUDE.md

@@ -31,14 +31,14 @@ JDK 21 is required (Gradle's toolchain fetches one if absent). Node ≥ 20 + npm
 Dependency arrows always point **inward**. Adapters depend on core; core depends on no adapter, no framework, no servlet/HTTP API, no JDBC/DynamoDB.
 
 1. **`pk-auth-core`** — framework- and persistence-neutral. Knows WebAuthn (WebAuthn4J), the wire contract, and declares the SPIs. `PasskeyAuthenticationService` is the ceremony entry point. The exported packages are `api`, `ceremony`, `config`, `credential`, `error`, `json`, `lifecycle`, `metrics`, and `spi` (enforced via `module-info.java`); everything else is module-internal.
-2. **SPIs (ports)** — narrow interfaces the host implements: `UserLookup`, `CredentialRepository`, `ChallengeStore` (required); `BackupCodeRepository`, `OtpRepository`, `EmailSender`, `SmsSender`, `RefreshTokenRepository`, `AccessTokenStore`, `TokenTtlPolicy`, `UserDeletionListener`, `AttestationTrustPolicy`, `OriginValidator`, `ClockProvider` (optional / feature-gated). See `DESIGN.md` §6 for the required-vs-optional table.
+2. **SPIs (ports)** — narrow interfaces the host implements: `UserLookup`, `CredentialRepository`, `ChallengeStore` (required); `BackupCodeRepository`, `OtpRepository`, `EmailSender`, `SmsSender`, `RefreshTokenRepository`, `AccessTokenStore`, `TokenTtlPolicy`, `RevocationCheck`, `UserDeletionListener`, `AttestationTrustPolicy`, `OriginValidator`, `ClockProvider`, `ConsumedJtiStore`, `CeremonyRateLimiter`, `MessageFormatter` (optional / feature-gated). See `DESIGN.md` §6 for the required-vs-optional table.
 3. **Adapters** — `pk-auth-spring-boot-starter` (Spring Boot 4 / Security 7 autoconfigure), `pk-auth-dropwizard` (Dropwizard 5 `ConfiguredBundle` + Dagger 2), `pk-auth-micronaut` (Micronaut 4 `@Factory` + `@Filter`, deliberately **not** Micronaut Security). Each mounts the same `/auth/**` JSON contract and pattern-matches the core's sealed result sums into HTTP status codes.
 
-Feature modules (`pk-auth-backup-codes`, `pk-auth-magic-link`, `pk-auth-otp`, `pk-auth-refresh-tokens`) and persistence modules (`pk-auth-persistence-jdbi`, `pk-auth-persistence-dynamodb`, in-memory `pk-auth-testkit`) implement core-declared SPIs and are wired in by the host.
+Feature modules (`pk-auth-backup-codes`, `pk-auth-magic-link`, `pk-auth-otp`, `pk-auth-refresh-tokens`, `pk-auth-admin-api`) and persistence modules (`pk-auth-persistence-jdbi`, `pk-auth-persistence-dynamodb`, in-memory `pk-auth-testkit`) implement core-declared SPIs and are wired in by the host. (`pk-auth-admin-api` hosts `AdminService` / `AdminResult<T>` and the account/credential/backup-code/email/phone admin operations mounted by all three adapters at `/auth/admin/**`.)
 
 ### Things that bite if you don't know them
 
-- **Sealed result sums, not exceptions.** Ceremony and admin operations return sealed interfaces — `AdminResult<T>` (`Success | NotFound | Forbidden | ValidationFailed | Conflict | RateLimited`), `RegistrationResult`, `AssertionResult`, `RotateResult`, `JwtVerificationResult`. Adapters map these to HTTP; never throw across that boundary. When you add a variant, every adapter's `*ResultMapper` must handle it.
+- **Sealed result sums, not exceptions.** Ceremony and admin operations return sealed interfaces — `AdminResult<T>` (`Success | NotFound | Forbidden | ValidationFailed | Conflict | RateLimited`, declared in `pk-auth-admin-api`), `RegistrationResult`, `AssertionResult` (core), `RotateResult` (`pk-auth-refresh-tokens`), `JwtVerificationResult` (`pk-auth-jwt`). Adapters map these to HTTP; never throw across that boundary. When you add a variant, every adapter's `*ResultMapper` must handle it.
 - **Wire bytes are base64url, no padding** (RFC 4648 §5). Jackson 3 adapters get this from `PkAuthObjectMappers.pkAuthModule()`; the Dropwizard adapter is still on Jackson 2 and uses the `PkAuthJacksonBridge`.
 - **`finish` endpoints are not idempotent** — challenges are single-use via `ChallengeStore.takeOnce`. There is **no shared transaction across SPIs**: `takeOnce` is consumed before `CredentialRepository.save`; a failed save forces a ceremony restart. This is intentional — see [`docs/transactional-semantics.md`](./docs/transactional-semantics.md).
 - **All three adapters mount identical `/auth/**` paths** (`/auth/passkeys/**`, `/auth/refresh`, `/auth/admin/**`) — Dropwizard's Jersey resources use the same `@Path("/auth/passkeys")` etc. as Spring/Micronaut, so the TS SDK targets one path scheme everywhere (no per-client path override).

examples/dropwizard-demo/build.gradle.kts

@@ -47,8 +47,8 @@ dependencies {
     implementation(libs.slf4j.api)
     runtimeOnly(libs.logback.classic)
 
-    compileOnly("com.google.errorprone:error_prone_annotations:2.50.0")
-    testCompileOnly("com.google.errorprone:error_prone_annotations:2.50.0")
+    compileOnly(libs.build.errorprone.annotations)
+    testCompileOnly(libs.build.errorprone.annotations)
 
     testImplementation(libs.dropwizard.testing)
 }

gradle/libs.versions.toml

@@ -157,6 +157,7 @@ h2 = { module = "com.h2database:h2", version.ref = "h2" }
 build-spotless-plugin = { module = "com.diffplug.spotless:spotless-plugin-gradle", version.ref = "spotless" }
 build-errorprone-plugin = { module = "net.ltgt.gradle:gradle-errorprone-plugin", version.ref = "errorprone-plugin" }
 build-errorprone-core = { module = "com.google.errorprone:error_prone_core", version.ref = "errorprone-core" }
+build-errorprone-annotations = { module = "com.google.errorprone:error_prone_annotations", version.ref = "errorprone-core" }
 
 # Test bundle ingredients
 junit-jupiter = { module = "org.junit.jupiter:junit-jupiter", version.ref = "junit-jupiter" }

pk-auth-admin-api/src/main/java/com/codeheadsystems/pkauth/admin/DefaultAdminService.java

@@ -206,7 +206,12 @@ public AdminResult<UserHandle> finishEmailVerification(String token) {
       return new AdminResult.ValidationFailed<>("token must be non-blank");
     }
     if (magicLinkService == null) return notConfigured("email verification");
-    ConsumeResult result = magicLinkService.finishVerification(token);
+    // Demand the email-verify purpose: a login-flow magic-link token must not be replayable here to
+    // mark an address verified (cross-purpose token confusion). MagicLinkService checks the purpose
+    // before consuming the single-use JTI, so a rejected cross-purpose token stays usable for
+    // login.
+    ConsumeResult result =
+        magicLinkService.finishVerification(token, MagicLinkService.PURPOSE_EMAIL_VERIFY);
     if (result instanceof ConsumeResult.Success success) {
       // Host apps own the users table; we report success and let the adapter persist the
       // emailVerified flag via UserLookup's host-app-specific update path (out of scope here).
@@ -216,6 +221,9 @@ public AdminResult<UserHandle> finishEmailVerification(String token) {
     if (result instanceof ConsumeResult.AlreadyConsumed) {
       return new AdminResult.Conflict<>("token already consumed");
     }
+    if (result instanceof ConsumeResult.WrongPurpose) {
+      return new AdminResult.ValidationFailed<>("token is not an email-verification token");
+    }
     return new AdminResult.ValidationFailed<>("invalid token");
   }
 
@@ -249,6 +257,15 @@ public AdminResult<PhoneVerificationResult> finishPhoneVerification(
     }
     if (otpService == null) return notConfigured("phone verification");
     OtpService.VerifyResult result = otpService.finishVerification(target, phoneE164, code);
+    // AttemptsExceeded is a brute-force lockout signal, not a "soft" verification outcome, so it
+    // must surface at the HTTP layer as 429 (RateLimited) rather than a 200 the failure of which is
+    // buried in the body — otherwise a client/proxy keying off status (the documented contract for
+    // these sums) can't see that guessing has been throttled. A new OTP must be requested.
+    // Ordinary mismatch/expiry stay as a typed 200 body on purpose: the privacy-neutral
+    // remainingAttempts is part of the verification UX, not an error.
+    if (result instanceof OtpService.VerifyResult.AttemptsExceeded) {
+      return new AdminResult.RateLimited<>(Duration.ofMinutes(15));
+    }
     return new AdminResult.Success<>(
         switch (result) {
           case OtpService.VerifyResult.Success s -> new PhoneVerificationResult.Verified();

pk-auth-admin-api/src/test/java/com/codeheadsystems/pkauth/admin/DefaultAdminServiceTest.java

@@ -330,6 +330,20 @@ void finishPhoneVerificationFlow() {
             s -> assertThat(s.value()).isInstanceOf(PhoneVerificationResult.Expired.class));
   }
 
+  @Test
+  void finishPhoneVerificationExhaustedAttemptsMapsToRateLimited() {
+    // maxAttempts == 3: three wrong guesses are compared (soft 200 mismatch), and the fourth is
+    // refused. A brute-force lockout must surface as RateLimited (429), not a 200 whose failure is
+    // hidden in the body, so a client/proxy can see guessing was throttled.
+    admin.startPhoneVerification(alice, alice, "+15551234567");
+    for (int i = 0; i < 3; i++) {
+      assertThat(admin.finishPhoneVerification(alice, alice, "+15551234567", "000000"))
+          .isInstanceOf(AdminResult.Success.class);
+    }
+    assertThat(admin.finishPhoneVerification(alice, alice, "+15551234567", "000000"))
+        .isInstanceOf(AdminResult.RateLimited.class);
+  }
+
   // -- Authorizer override --
 
   @Test

pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/CeremonyWireMapper.java

@@ -37,10 +37,51 @@ public final class CeremonyWireMapper {
 
   private CeremonyWireMapper() {}
 
-  /** Carries a wire-format response: HTTP status code + a JSON-serializable body. */
-  public record CeremonyResponse(int status, Map<String, Object> body) {
+  /**
+   * {@code Retry-After} hint (seconds) emitted on every ceremony 429, mirroring the {@code
+   * Retry-After} the admin endpoints already send on their rate-limit refusals. The value is a
+   * conservative constant matching {@code InMemoryCeremonyRateLimiter.DEFAULT_WINDOW} (1 minute):
+   * the {@link com.codeheadsystems.pkauth.spi.CeremonyRateLimiter} SPI is boolean-only and exposes
+   * no per-call window, so an exact remaining-time cannot be computed here. A host that tightens or
+   * widens its limiter window may therefore serve a slightly stale hint, which {@code Retry-After}
+   * explicitly permits (it is advisory). Hosts wanting an exact value can override the header.
+   */
+  private static final Map<String, String> RATE_LIMIT_HEADERS = Map.of("Retry-After", "60");
+
+  /**
+   * Canonical 429 ceremony refusal: {@code {"outcome":"rate_limited"}} body + {@code Retry-After}.
+   */
+  private static CeremonyResponse rateLimitedResponse() {
+    return new CeremonyResponse(429, Map.of("outcome", "rate_limited"), RATE_LIMIT_HEADERS);
+  }
+
+  /**
+   * Carries a wire-format response: HTTP status code, a JSON-serializable body, and response
+   * headers. Adapters MUST copy {@link #headers()} onto the native HTTP response (e.g. {@code
+   * Retry-After} on a 429); a body-only adapter silently drops them.
+   *
+   * <p>The {@code headers} component was added in 2.1.0; the two-arg constructor preserves the
+   * prior {@code (status, body)} call sites with no headers.
+   *
+   * @param status the HTTP status code
+   * @param body the JSON-serializable response body
+   * @param headers response headers to copy onto the native HTTP response (since 2.1.0)
+   */
+  public record CeremonyResponse(
+      int status, Map<String, Object> body, Map<String, String> headers) {
     public CeremonyResponse {
       body = Map.copyOf(body);
+      headers = Map.copyOf(headers);
+    }
+
+    /**
+     * Convenience constructor for a response with no extra headers.
+     *
+     * @param status the HTTP status code
+     * @param body the JSON-serializable response body
+     */
+    public CeremonyResponse(int status, Map<String, Object> body) {
+      this(status, body, Map.of());
     }
   }
 
@@ -68,8 +109,7 @@ public static CeremonyResponse forRegistration(RegistrationResult result) {
                   Base64Url.encode(dc.credentialId().value())));
       case RegistrationResult.InvalidPayload ip ->
           new CeremonyResponse(400, errorBody("invalid_payload", "detail", ip.detail()));
-      case RegistrationResult.RateLimited rl ->
-          new CeremonyResponse(429, Map.of("outcome", "rate_limited"));
+      case RegistrationResult.RateLimited rl -> rateLimitedResponse();
     };
   }
 
@@ -78,11 +118,12 @@ public static CeremonyResponse forRegistration(RegistrationResult result) {
    * controllers use this for the {@code RateLimited} variant of {@code StartRegistrationResult} /
    * {@code StartAuthenticationResult}.
    *
-   * @return canonical rate-limited response (HTTP 429, body {@code {"outcome": "rate_limited"}})
+   * @return canonical rate-limited response (HTTP 429, body {@code {"outcome": "rate_limited"}},
+   *     plus a {@code Retry-After} header since 2.1.0)
    * @since 0.9.1
    */
   public static CeremonyResponse rateLimited() {
-    return new CeremonyResponse(429, Map.of("outcome", "rate_limited"));
+    return rateLimitedResponse();
   }
 
   /**
@@ -137,8 +178,7 @@ public static CeremonyResponse forAssertionError(AssertionResult result) {
           new CeremonyResponse(401, Map.of("outcome", "user_verification_required"));
       case AssertionResult.InvalidSignature is ->
           new CeremonyResponse(401, Map.of("outcome", "invalid_signature"));
-      case AssertionResult.RateLimited rl ->
-          new CeremonyResponse(429, Map.of("outcome", "rate_limited"));
+      case AssertionResult.RateLimited rl -> rateLimitedResponse();
     };
   }
 

pk-auth-core/src/main/java/com/codeheadsystems/pkauth/lifecycle/package-info.java

@@ -9,4 +9,5 @@
  *
  * @since 1.1.0
  */
+@org.jspecify.annotations.NullMarked
 package com.codeheadsystems.pkauth.lifecycle;

pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ratelimit/InMemoryWindowCounter.java

@@ -10,7 +10,12 @@
 import java.util.concurrent.atomic.AtomicInteger;
 
 /**
- * Caffeine-backed sliding-window counter shared by the backup-codes and magic-link rate limiters.
+ * Caffeine-backed fixed-window counter shared by the backup-codes and magic-link rate limiters.
+ *
+ * <p>The window is fixed from a key's first increment, not sliding: {@code expireAfterWrite} starts
+ * the clock when the key's counter is created, and the in-place {@link AtomicInteger} increments
+ * that follow do not reset it, so the whole window expires at once {@code window} after the first
+ * increment (the next increment then starts a fresh window at {@code 1}).
  *
  * <p>Each rate-limiter SPI in this project (e.g. {@code BackupCodeRateLimiter}, {@code
  * MagicLinkRateLimiter}, {@code CeremonyRateLimiter}) defines its own key signature. The

pk-auth-core/src/test/java/com/codeheadsystems/pkauth/api/CeremonyWireMapperTest.java

@@ -111,6 +111,7 @@ void registrationRateLimitedIs429() {
         CeremonyWireMapper.forRegistration(new RegistrationResult.RateLimited("register"));
     assertThat(r.status()).isEqualTo(429);
     assertThat(r.body()).containsEntry("outcome", "rate_limited");
+    assertThat(r.headers()).containsEntry("Retry-After", "60");
   }
 
   @Test
@@ -185,13 +186,15 @@ void assertionRateLimitedIs429() {
         CeremonyWireMapper.forAssertionError(new AssertionResult.RateLimited("authenticate"));
     assertThat(r.status()).isEqualTo(429);
     assertThat(r.body()).containsEntry("outcome", "rate_limited");
+    assertThat(r.headers()).containsEntry("Retry-After", "60");
   }
 
   @Test
   void startCeremonyRateLimitedIs429() {
     CeremonyResponse r = CeremonyWireMapper.rateLimited();
     assertThat(r.status()).isEqualTo(429);
     assertThat(r.body()).containsEntry("outcome", "rate_limited");
+    assertThat(r.headers()).containsEntry("Retry-After", "60");
   }
 
   @Test

pk-auth-dropwizard/build.gradle.kts

@@ -50,8 +50,8 @@ dependencies {
 
     // Dropwizard's transitive Jersey/Jakarta dependencies surface annotations whose enclosing
     // packages reference com.google.errorprone.annotations.* — same pattern as the JDBI module.
-    compileOnly("com.google.errorprone:error_prone_annotations:2.50.0")
-    testCompileOnly("com.google.errorprone:error_prone_annotations:2.50.0")
+    compileOnly(libs.build.errorprone.annotations)
+    testCompileOnly(libs.build.errorprone.annotations)
 
     implementation(libs.slf4j.api)
 

pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/dagger/AltFlowsModule.java

@@ -43,11 +43,10 @@
  * configuration shape. This module is used only by {@link PkAuthFullComponent}; the slim {@link
  * PkAuthComponent} (passkey-ceremony-only) does not include it.
  *
- * <p>Fail-fast policy (matches the maintainer decision pinned in the TODO): no adapter-level
- * defaults. The host must hand in the OTP pepper, the magic-link base URL, and an explicit {@link
- * EmailSender} / {@link SmsSender} (or accept the {@link LoggingEmailSender} / {@link
- * LoggingSmsSender} dev-only fallback by passing {@code null} senders, which only activates when
- * the {@link AltFlowOptions#devMode()} flag is true).
+ * <p>Fail-fast policy: no adapter-level defaults. The host must hand in the OTP pepper, the
+ * magic-link base URL, and an explicit {@link EmailSender} / {@link SmsSender} (or accept the
+ * {@link LoggingEmailSender} / {@link LoggingSmsSender} dev-only fallback by passing {@code null}
+ * senders, which only activates when the {@link AltFlowOptions#devMode()} flag is true).
  *
  * @since 0.9.1
  */

pk-auth-dropwizard/src/main/java/com/codeheadsystems/pkauth/dropwizard/resource/PkAuthCeremonyResource.java

@@ -76,7 +76,9 @@ public Response finishAuthentication(
   }
 
   private static Response toResponse(CeremonyResponse wire) {
-    return Response.status(wire.status()).entity(wire.body()).build();
+    Response.ResponseBuilder builder = Response.status(wire.status()).entity(wire.body());
+    wire.headers().forEach(builder::header);
+    return builder.build();
   }
 
   private static String clientIp(HttpServletRequest httpRequest) {

pk-auth-jwt/src/main/java/com/codeheadsystems/pkauth/jwt/JwtClaims.java

@@ -7,6 +7,7 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
+import java.util.Set;
 import org.jspecify.annotations.Nullable;
 
 /**
@@ -29,10 +30,41 @@ public record JwtClaims(
     @Nullable Map<String, Object> additionalClaims,
     @Nullable String audience) {
 
+  /**
+   * Claim names the issuer sets itself and that {@link #additionalClaims} must never carry. Because
+   * the issuer merges {@code additionalClaims} into the same JWT body after writing these, allowing
+   * a caller to supply one would let it silently overwrite a security-critical value (e.g. a future
+   * {@code exp}, an impersonating {@code sub}, or a forged {@code aud}/{@code iss}). The RFC 7519
+   * registered set plus the {@code pkauth.*} private claims mirror {@code
+   * PkAuthJwtValidator.removeKnownClaims}.
+   *
+   * @since 2.1.0
+   */
+  private static final Set<String> RESERVED_CLAIM_NAMES =
+      Set.of(
+          "iss",
+          "sub",
+          "aud",
+          "iat",
+          "nbf",
+          "exp",
+          "jti",
+          "pkauth.method",
+          "pkauth.cred",
+          "pkauth.amr");
+
   public JwtClaims {
     Objects.requireNonNull(userHandle, "userHandle");
     Objects.requireNonNull(method, "method");
     Objects.requireNonNull(amr, "amr");
+    if (additionalClaims != null) {
+      for (String key : additionalClaims.keySet()) {
+        if (RESERVED_CLAIM_NAMES.contains(key)) {
+          throw new IllegalArgumentException(
+              "additionalClaims must not contain the reserved claim '" + key + "'");
+        }
+      }
+    }
     if (method == AuthMethod.PASSKEY && credentialId == null) {
       throw new IllegalArgumentException("credentialId is required when method == PASSKEY");
     }