From e0d22d26f1e116d9c71485236cdfca41eeb854aa Mon Sep 17 00:00:00 2001 From: Ned Wolpert Date: Wed, 5 Aug 2026 08:04:24 -0700 Subject: [PATCH] fix(core): bound username length and cap the in-memory rate-limiter caches MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two halves of the same unbounded-growth problem on the permitAll start endpoints. Nothing capped username length anywhere — StartRegistrationRequest checked only non-blank and the JDBI column is Postgres TEXT — while both Caffeine caches were built with expireAfterWrite and no maximumSize. Every distinct key is retained for the whole window, so the maps grow with the caller's key variety rather than with the number of real users, and each entry is as large as the username sent. The username also outlives the request in a second way: startRegistration passes it to UserLookup#getOrCreateHandle, which persists a user row before any credential exists, on an endpoint that requires no authentication. StartRegistrationRequest.MAX_USERNAME_LENGTH (256 — comfortably covers an email address used as a username) now bounds both start requests, and both cache maps cap at DEFAULT_MAX_TRACKED_KEYS (100_000). Caffeine evicts near-LRU 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. Validation goes in the record's compact constructor, matching the non-blank check already there rather than adding a variant to the sealed start-result sums (which would break every adapter's exhaustive switch, including hosts'). That path was assumed to yield 400 rather than 500, so the assumption is now tested through the Spring adapter instead of asserted: overlongUsernameIsRejectedAsBadRequestNotServerError drives both start endpoints and expects 400. StartAuthenticationRequest keeps accepting a null username — that is the usernameless / discoverable-credential flow — and deliberately does not adopt the non-blank rule, since an unknown username already yields the same empty allowCredentials shape as a known one. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 13 +++++ .../api/StartAuthenticationRequest.java | 22 ++++++- .../pkauth/api/StartRegistrationRequest.java | 18 ++++++ .../ceremony/InMemoryCeremonyRateLimiter.java | 24 +++++++- .../ratelimit/InMemoryWindowCounter.java | 20 ++++++- .../api/StartRequestUsernameBoundTest.java | 57 +++++++++++++++++++ .../spring/PkAuthCeremonyIntegrationTest.java | 35 ++++++++++++ 7 files changed, 184 insertions(+), 5 deletions(-) create mode 100644 pk-auth-core/src/test/java/com/codeheadsystems/pkauth/api/StartRequestUsernameBoundTest.java 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 ----------------------------------------------------------------