fix(auth): sequential rate-limit check with ipEmail token refund on IP failure
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 <noreply@anthropic.com>
This commit is contained in:
@@ -36,10 +36,18 @@ public class LoginRateLimiter {
|
|||||||
.build(key -> newBucket(maxPerIp, windowMinutes));
|
.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) {
|
public void checkAndConsume(String ip, String email) {
|
||||||
boolean ipEmailOk = byIpEmail.get(ip + ":" + email).tryConsume(1);
|
if (!byIpEmail.get(ip + ":" + email).tryConsume(1)) {
|
||||||
boolean ipOk = byIp.get(ip).tryConsume(1);
|
throw DomainException.tooManyRequests(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS,
|
||||||
if (!ipEmailOk || !ipOk) {
|
"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,
|
throw DomainException.tooManyRequests(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS,
|
||||||
"Too many login attempts from " + ip);
|
"Too many login attempts from " + ip);
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -64,4 +64,48 @@ class LoginRateLimiterTest {
|
|||||||
.satisfies(ex -> org.assertj.core.api.Assertions.assertThat(((DomainException) ex).getCode())
|
.satisfies(ex -> org.assertj.core.api.Assertions.assertThat(((DomainException) ex).getCode())
|
||||||
.isEqualTo(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS));
|
.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"));
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user