From c32607e1333c7990783d01ba6a81c04e497935cb Mon Sep 17 00:00:00 2001 From: Marcel Date: Mon, 18 May 2026 13:29:36 +0200 Subject: [PATCH] fix(auth): sequential rate-limit check with ipEmail token refund on IP failure MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses Felix (blocker 1): the old implementation consumed from both buckets before checking either result, silently eroding the per-email quota when only the per-IP limit was blocking. The fix checks ipEmail first, then IP; on IP failure it refunds the ipEmail token so legitimate users behind a shared IP are not penalised. Also adds two new test cases: - different_email_from_same_ip_not_blocked_by_sibling_email_exhaustion (Sara) - ip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attempts (red → green) Co-Authored-By: Claude Sonnet 4.6 --- .../familienarchiv/auth/LoginRateLimiter.java | 14 ++++-- .../auth/LoginRateLimiterTest.java | 44 +++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/backend/src/main/java/org/raddatz/familienarchiv/auth/LoginRateLimiter.java b/backend/src/main/java/org/raddatz/familienarchiv/auth/LoginRateLimiter.java index 2571dd06..47928e37 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/auth/LoginRateLimiter.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/auth/LoginRateLimiter.java @@ -36,10 +36,18 @@ public class LoginRateLimiter { .build(key -> newBucket(maxPerIp, windowMinutes)); } + // NOTE: This cache is node-local (in-memory). In a multi-replica deployment, + // effective limits would be multiplied by replica count. + // For the current single-VPS setup this is the correct, simplest implementation. + public void checkAndConsume(String ip, String email) { - boolean ipEmailOk = byIpEmail.get(ip + ":" + email).tryConsume(1); - boolean ipOk = byIp.get(ip).tryConsume(1); - if (!ipEmailOk || !ipOk) { + if (!byIpEmail.get(ip + ":" + email).tryConsume(1)) { + throw DomainException.tooManyRequests(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS, + "Too many login attempts from " + ip); + } + if (!byIp.get(ip).tryConsume(1)) { + // Refund the ipEmail token so IP-level blocking does not erode the per-email quota. + byIpEmail.get(ip + ":" + email).addTokens(1); throw DomainException.tooManyRequests(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS, "Too many login attempts from " + ip); } diff --git a/backend/src/test/java/org/raddatz/familienarchiv/auth/LoginRateLimiterTest.java b/backend/src/test/java/org/raddatz/familienarchiv/auth/LoginRateLimiterTest.java index 77e4ebb6..5664bdf5 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/auth/LoginRateLimiterTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/auth/LoginRateLimiterTest.java @@ -64,4 +64,48 @@ class LoginRateLimiterTest { .satisfies(ex -> org.assertj.core.api.Assertions.assertThat(((DomainException) ex).getCode()) .isEqualTo(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS)); } + + @Test + void different_email_from_same_ip_not_blocked_by_sibling_email_exhaustion() { + for (int i = 0; i < 10; i++) { + rateLimiter.checkAndConsume("1.2.3.4", "user@example.com"); + } + + assertThatThrownBy(() -> rateLimiter.checkAndConsume("1.2.3.4", "user@example.com")) + .isInstanceOf(DomainException.class); + + assertThatNoException().isThrownBy( + () -> rateLimiter.checkAndConsume("1.2.3.4", "other@example.com")); + } + + @Test + void ip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attempts() { + // Use a tighter limiter so the phantom-consumption effect is observable. + // ipEmail=3, IP=3: exhausting IP via one email burns the other email's quota with the old code. + RateLimitProperties props = new RateLimitProperties(); + props.setMaxAttemptsPerIpEmail(3); + props.setMaxAttemptsPerIp(3); + props.setWindowMinutes(15); + LoginRateLimiter tightLimiter = new LoginRateLimiter(props); + + // Exhaust the per-IP bucket using "user@" + for (int i = 0; i < 3; i++) { + tightLimiter.checkAndConsume("1.2.3.4", "user@example.com"); + } + + // Three blocked attempts for "target@" while IP is exhausted + for (int i = 0; i < 3; i++) { + assertThatThrownBy(() -> tightLimiter.checkAndConsume("1.2.3.4", "target@example.com")) + .isInstanceOf(DomainException.class); + } + + // A successful login for "user@" resets the IP bucket but NOT target@'s ipEmail bucket + tightLimiter.invalidateOnSuccess("1.2.3.4", "user@example.com"); + + // After IP reset: "target@" must NOT be blocked by an exhausted ipEmail bucket. + // With the old code, 3 blocked attempts burned all 3 ipEmail tokens → blocked here. + // With the fix, tokens are refunded on each blocked attempt → still has capacity. + assertThatNoException().isThrownBy( + () -> tightLimiter.checkAndConsume("1.2.3.4", "target@example.com")); + } }