diff --git a/CHANGELOG.md b/CHANGELOG.md index 1df62b6..99bdbbc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,6 +26,19 @@ finding from that review. token from the result (e.g. to render their own email) must instead capture it in a `MessageFormatter` / `EmailSender`. `startLogin`'s enumeration-resistant short-circuit paths continue to return `Sent("")`. +- **Usernames are bounded and the in-memory rate-limiter caches can no longer grow + without limit.** Nothing capped username length (the JDBI column is Postgres + `TEXT`), and both Caffeine caches used `expireAfterWrite` with no + `maximumSize` — so on the `permitAll` start endpoints, a caller varying its + username grew the maps with its own key variety rather than with the number of + real users, and every distinct key was retained for the full window. + `StartRegistrationRequest.MAX_USERNAME_LENGTH` (256, enough for an email + address) now bounds both start requests, and `InMemoryCeremonyRateLimiter` / + `InMemoryWindowCounter` cap their maps at + `DEFAULT_MAX_TRACKED_KEYS` (100 000). An over-long username is refused as a + clean **400** — covered by an adapter-level test asserting the refusal reaches + the client as a 400 and not a 500. `StartAuthenticationRequest` still accepts a + `null` username (the usernameless / discoverable-credential flow). ## [2.2.0] — 2026-06-27 diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/StartAuthenticationRequest.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/StartAuthenticationRequest.java index c40e0e0..8e70aaf 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/StartAuthenticationRequest.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/StartAuthenticationRequest.java @@ -12,4 +12,24 @@ */ @JsonInclude(JsonInclude.Include.NON_NULL) public record StartAuthenticationRequest( - @Nullable String username, @Nullable UserVerificationRequirement userVerification) {} + @Nullable String username, @Nullable UserVerificationRequirement userVerification) { + + /** + * Rejects a username longer than {@link StartRegistrationRequest#MAX_USERNAME_LENGTH}. A {@code + * null} username is still valid — that is the usernameless / discoverable-credential flow. Blank + * is deliberately NOT rejected here (unlike the registration request): an unknown username + * already yields the same empty {@code allowCredentials} shape as a known one, so there is no + * enumeration signal to protect, and tightening it would change existing behaviour for no + * security gain. + * + * @since 2.3.0 + */ + public StartAuthenticationRequest { + if (username != null && username.length() > StartRegistrationRequest.MAX_USERNAME_LENGTH) { + throw new IllegalArgumentException( + "username must be at most " + + StartRegistrationRequest.MAX_USERNAME_LENGTH + + " characters"); + } + } +} diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/StartRegistrationRequest.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/StartRegistrationRequest.java index cf200be..999ea2c 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/StartRegistrationRequest.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/api/StartRegistrationRequest.java @@ -17,10 +17,28 @@ public record StartRegistrationRequest( @Nullable String label, @Nullable UserVerificationRequirement userVerification) { + /** + * Maximum accepted length of {@link #username}, in {@code char}s. + * + *

The username is attacker-chosen on this {@code permitAll} endpoint and is retained well + * beyond the request: it keys the per-username rate-limit bucket for the limiter's whole window, + * and {@code startRegistration} passes it to {@link + * com.codeheadsystems.pkauth.spi.UserLookup#getOrCreateHandle} — which persists a user row before + * any credential exists. Unbounded, both of those grow with whatever the caller sends. 256 + * comfortably covers an email address used as a username. + * + * @since 2.3.0 + */ + public static final int MAX_USERNAME_LENGTH = 256; + public StartRegistrationRequest { Objects.requireNonNull(username, "username"); if (username.isBlank()) { throw new IllegalArgumentException("username must be non-blank"); } + if (username.length() > MAX_USERNAME_LENGTH) { + throw new IllegalArgumentException( + "username must be at most " + MAX_USERNAME_LENGTH + " characters"); + } } } diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ceremony/InMemoryCeremonyRateLimiter.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ceremony/InMemoryCeremonyRateLimiter.java index 1825a76..703e1ef 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ceremony/InMemoryCeremonyRateLimiter.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ceremony/InMemoryCeremonyRateLimiter.java @@ -43,6 +43,18 @@ public final class InMemoryCeremonyRateLimiter implements CeremonyRateLimiter { /** Default window over which the per-IP and per-username counters are tracked. */ public static final Duration DEFAULT_WINDOW = Duration.ofMinutes(1); + /** + * Maximum tracked keys per bucket map. Both maps are keyed by attacker-influenced values (source + * IP, submitted username) on {@code permitAll} endpoints, and {@code expireAfterWrite} alone + * retains every distinct key for the full window — so without a size bound the maps grow with the + * caller's key variety, not with the number of real users. Caffeine evicts near-LRU entries at + * the cap; an evicted counter simply restarts, which costs at most one extra allowance to the + * least-active key and never grants an unbounded budget to an active one. + * + * @since 2.3.0 + */ + public static final int DEFAULT_MAX_TRACKED_KEYS = 100_000; + private static final Logger LOG = LoggerFactory.getLogger(InMemoryCeremonyRateLimiter.class); private final int perIpLimit; @@ -83,8 +95,16 @@ public InMemoryCeremonyRateLimiter(int perIpLimit, int perUsernameLimit, Duratio } this.perIpLimit = perIpLimit; this.perUsernameLimit = perUsernameLimit; - this.ipCounters = Caffeine.newBuilder().expireAfterWrite(window).build(); - this.usernameCounters = Caffeine.newBuilder().expireAfterWrite(window).build(); + this.ipCounters = + Caffeine.newBuilder() + .expireAfterWrite(window) + .maximumSize(DEFAULT_MAX_TRACKED_KEYS) + .build(); + this.usernameCounters = + Caffeine.newBuilder() + .expireAfterWrite(window) + .maximumSize(DEFAULT_MAX_TRACKED_KEYS) + .build(); LOG.warn( "ceremony.rate-limiter InMemoryCeremonyRateLimiter instantiated (perIp={} perUsername={}" + " window={}) — FOR DEV / SINGLE-INSTANCE USE ONLY. Production deployments with more" diff --git a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ratelimit/InMemoryWindowCounter.java b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ratelimit/InMemoryWindowCounter.java index af87d2c..147ae84 100644 --- a/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ratelimit/InMemoryWindowCounter.java +++ b/pk-auth-core/src/main/java/com/codeheadsystems/pkauth/ratelimit/InMemoryWindowCounter.java @@ -31,10 +31,22 @@ */ public final class InMemoryWindowCounter { + /** + * Maximum tracked keys. {@code expireAfterWrite} alone retains every distinct key for the full + * window, so a caller that varies its key (a submitted identifier, a source address) grows this + * map with its own key variety rather than with the number of real users. Caffeine evicts + * near-LRU entries at the cap; an evicted counter restarts, which costs at most one extra + * allowance to the least-active key and never grants an unbounded budget to an active one. + * + * @since 2.3.0 + */ + public static final int DEFAULT_MAX_TRACKED_KEYS = 100_000; + private final Cache counters; /** - * Creates a counter that drops keys after {@code window} has elapsed since their first increment. + * Creates a counter that drops keys after {@code window} has elapsed since their first increment, + * retaining at most {@link #DEFAULT_MAX_TRACKED_KEYS} keys at any moment. * * @param window expiry-after-write window; must be positive */ @@ -43,7 +55,11 @@ public InMemoryWindowCounter(Duration window) { if (window.isZero() || window.isNegative()) { throw new IllegalArgumentException("window must be positive"); } - this.counters = Caffeine.newBuilder().expireAfterWrite(window).build(); + this.counters = + Caffeine.newBuilder() + .expireAfterWrite(window) + .maximumSize(DEFAULT_MAX_TRACKED_KEYS) + .build(); } /** diff --git a/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/api/StartRequestUsernameBoundTest.java b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/api/StartRequestUsernameBoundTest.java new file mode 100644 index 0000000..e96e5ee --- /dev/null +++ b/pk-auth-core/src/test/java/com/codeheadsystems/pkauth/api/StartRequestUsernameBoundTest.java @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: MIT +package com.codeheadsystems.pkauth.api; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; +import static org.assertj.core.api.Assertions.assertThatThrownBy; + +import org.junit.jupiter.api.Test; + +/** + * The username on the two {@code start*} requests is attacker-chosen on a {@code permitAll} + * endpoint and outlives the request: it keys the per-username rate-limit bucket for the limiter's + * whole window, and registration hands it to {@code UserLookup#getOrCreateHandle}, which persists a + * user row before any credential exists. These bounds keep both proportional to real usage rather + * than to whatever the caller sends. + */ +class StartRequestUsernameBoundTest { + + private static String ofLength(int n) { + return "a".repeat(n); + } + + @Test + void registrationRejectsUsernameOverTheBound() { + String tooLong = ofLength(StartRegistrationRequest.MAX_USERNAME_LENGTH + 1); + assertThatThrownBy(() -> new StartRegistrationRequest(tooLong, "x", null, null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("at most"); + } + + @Test + void registrationAcceptsUsernameExactlyAtTheBound() { + String atLimit = ofLength(StartRegistrationRequest.MAX_USERNAME_LENGTH); + assertThatCode(() -> new StartRegistrationRequest(atLimit, "x", null, null)) + .doesNotThrowAnyException(); + } + + @Test + void authenticationRejectsUsernameOverTheBound() { + String tooLong = ofLength(StartRegistrationRequest.MAX_USERNAME_LENGTH + 1); + assertThatThrownBy(() -> new StartAuthenticationRequest(tooLong, null)) + .isInstanceOf(IllegalArgumentException.class) + .hasMessageContaining("at most"); + } + + @Test + void authenticationStillAcceptsNullUsernameForTheUsernamelessFlow() { + assertThatCode(() -> new StartAuthenticationRequest(null, null)).doesNotThrowAnyException(); + assertThat(new StartAuthenticationRequest(null, null).username()).isNull(); + } + + @Test + void authenticationAcceptsUsernameExactlyAtTheBound() { + String atLimit = ofLength(StartRegistrationRequest.MAX_USERNAME_LENGTH); + assertThatCode(() -> new StartAuthenticationRequest(atLimit, null)).doesNotThrowAnyException(); + } +} diff --git a/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/PkAuthCeremonyIntegrationTest.java b/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/PkAuthCeremonyIntegrationTest.java index 570ee7f..8d7b3a2 100644 --- a/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/PkAuthCeremonyIntegrationTest.java +++ b/pk-auth-spring-boot-starter/src/test/java/com/codeheadsystems/pkauth/spring/PkAuthCeremonyIntegrationTest.java @@ -63,6 +63,41 @@ void setUp() { .build(); } + @Test + void overlongUsernameIsRejectedAsBadRequestNotServerError() throws Exception { + // The username bounds a rate-limiter cache key and drives getOrCreateHandle (which persists a + // user row) on a permitAll endpoint, so it must be bounded. Asserting through the adapter, + // not just the record, because what matters is that the refusal reaches the client as a 400 — + // a 500 would mean the validation escaped as an unhandled exception. + String tooLong = "a".repeat(StartRegistrationRequest.MAX_USERNAME_LENGTH + 1); + mockMvc + .perform( + post("/auth/passkeys/registration/start") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"username\":\"" + tooLong + "\",\"displayName\":\"x\"}")) + .andExpect(status().isBadRequest()); + + mockMvc + .perform( + post("/auth/passkeys/authentication/start") + .contentType(MediaType.APPLICATION_JSON) + .content("{\"username\":\"" + tooLong + "\"}")) + .andExpect(status().isBadRequest()); + } + + @Test + void usernameExactlyAtTheBoundIsAccepted() throws Exception { + String atLimit = "a".repeat(StartRegistrationRequest.MAX_USERNAME_LENGTH); + mockMvc + .perform( + post("/auth/passkeys/registration/start") + .contentType(MediaType.APPLICATION_JSON) + .content( + objectMapper.writeValueAsString( + new StartRegistrationRequest(atLimit, "At Limit", null, null)))) + .andExpect(status().isOk()); + } + @Test void registrationThenAssertionMintsValidJwt() throws Exception { // -- 1. Start registration ----------------------------------------------------------------