Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,28 @@ public record StartRegistrationRequest(
@Nullable String label,
@Nullable UserVerificationRequirement userVerification) {

/**
* Maximum accepted length of {@link #username}, in {@code char}s.
*
* <p>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");
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String, AtomicInteger> 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
*/
Expand All @@ -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();
}

/**
Expand Down
Original file line number Diff line number Diff line change
@@ -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();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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 ----------------------------------------------------------------
Expand Down
Loading