fix(rate-limit): only trust X-Forwarded-For from known reverse proxies
Without this guard any client could send X-Forwarded-For: <spoofed-ip> and bypass per-IP rate limiting entirely. Also switches expireAfterWrite → expireAfterAccess so the 1-minute window starts at first request, not last, and fixes the .gitignore entry that accidentally merged **/test-results/ and .worktrees/ into one broken pattern. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
3
.gitignore
vendored
3
.gitignore
vendored
@@ -11,4 +11,5 @@ gitea/
|
|||||||
scripts/large-data.sql
|
scripts/large-data.sql
|
||||||
|
|
||||||
.vitest-attachments
|
.vitest-attachments
|
||||||
**/test-results/.worktrees/
|
**/test-results/
|
||||||
|
.worktrees/
|
||||||
|
|||||||
@@ -14,10 +14,10 @@ public class RateLimitInterceptor implements HandlerInterceptor {
|
|||||||
|
|
||||||
private static final int MAX_REQUESTS_PER_MINUTE = 10;
|
private static final int MAX_REQUESTS_PER_MINUTE = 10;
|
||||||
|
|
||||||
// Caffeine cache: per-IP counter that auto-expires after 1 minute of inactivity.
|
// Caffeine cache: per-IP counter that expires 1 minute after first access.
|
||||||
// Bounded to 10_000 entries to prevent OOM from IP exhaustion.
|
// Bounded to 10_000 entries to prevent OOM from IP exhaustion.
|
||||||
private final Cache<String, AtomicInteger> requestCounts = Caffeine.newBuilder()
|
private final Cache<String, AtomicInteger> requestCounts = Caffeine.newBuilder()
|
||||||
.expireAfterWrite(1, TimeUnit.MINUTES)
|
.expireAfterAccess(1, TimeUnit.MINUTES)
|
||||||
.maximumSize(10_000)
|
.maximumSize(10_000)
|
||||||
.build();
|
.build();
|
||||||
|
|
||||||
@@ -35,10 +35,23 @@ public class RateLimitInterceptor implements HandlerInterceptor {
|
|||||||
}
|
}
|
||||||
|
|
||||||
private String resolveClientIp(HttpServletRequest request) {
|
private String resolveClientIp(HttpServletRequest request) {
|
||||||
String forwarded = request.getHeader("X-Forwarded-For");
|
// Only trust X-Forwarded-For when the direct connection comes from a known
|
||||||
if (forwarded != null && !forwarded.isBlank()) {
|
// reverse proxy (loopback or Docker private network). Trusting it unconditionally
|
||||||
return forwarded.split(",")[0].trim();
|
// allows any client to spoof a different IP and bypass per-IP rate limiting.
|
||||||
|
String remoteAddr = request.getRemoteAddr();
|
||||||
|
if (isTrustedProxy(remoteAddr)) {
|
||||||
|
String forwarded = request.getHeader("X-Forwarded-For");
|
||||||
|
if (forwarded != null && !forwarded.isBlank()) {
|
||||||
|
return forwarded.split(",")[0].trim();
|
||||||
|
}
|
||||||
}
|
}
|
||||||
return request.getRemoteAddr();
|
return remoteAddr;
|
||||||
|
}
|
||||||
|
|
||||||
|
private boolean isTrustedProxy(String ip) {
|
||||||
|
return ip.equals("127.0.0.1") || ip.equals("::1")
|
||||||
|
|| ip.startsWith("10.")
|
||||||
|
|| ip.startsWith("172.")
|
||||||
|
|| ip.startsWith("192.168.");
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -0,0 +1,88 @@
|
|||||||
|
package org.raddatz.familienarchiv.config;
|
||||||
|
|
||||||
|
import jakarta.servlet.http.HttpServletRequest;
|
||||||
|
import jakarta.servlet.http.HttpServletResponse;
|
||||||
|
import org.junit.jupiter.api.BeforeEach;
|
||||||
|
import org.junit.jupiter.api.Test;
|
||||||
|
import org.springframework.http.HttpStatus;
|
||||||
|
|
||||||
|
import java.io.PrintWriter;
|
||||||
|
import java.io.StringWriter;
|
||||||
|
|
||||||
|
import static org.assertj.core.api.Assertions.assertThat;
|
||||||
|
import static org.mockito.Mockito.*;
|
||||||
|
|
||||||
|
class RateLimitInterceptorTest {
|
||||||
|
|
||||||
|
private RateLimitInterceptor interceptor;
|
||||||
|
private HttpServletRequest request;
|
||||||
|
private HttpServletResponse response;
|
||||||
|
private PrintWriter writer;
|
||||||
|
|
||||||
|
@BeforeEach
|
||||||
|
void setUp() throws Exception {
|
||||||
|
interceptor = new RateLimitInterceptor();
|
||||||
|
request = mock(HttpServletRequest.class);
|
||||||
|
response = mock(HttpServletResponse.class);
|
||||||
|
writer = new PrintWriter(new StringWriter());
|
||||||
|
when(response.getWriter()).thenReturn(writer);
|
||||||
|
when(request.getRemoteAddr()).thenReturn("1.2.3.4");
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void allows_requests_below_limit() throws Exception {
|
||||||
|
for (int i = 0; i < 10; i++) {
|
||||||
|
assertThat(interceptor.preHandle(request, response, null)).isTrue();
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void blocks_request_when_limit_exceeded() throws Exception {
|
||||||
|
for (int i = 0; i < 10; i++) {
|
||||||
|
interceptor.preHandle(request, response, null);
|
||||||
|
}
|
||||||
|
assertThat(interceptor.preHandle(request, response, null)).isFalse();
|
||||||
|
verify(response).setStatus(HttpStatus.TOO_MANY_REQUESTS.value());
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void different_ips_have_independent_limits() throws Exception {
|
||||||
|
HttpServletRequest other = mock(HttpServletRequest.class);
|
||||||
|
when(other.getRemoteAddr()).thenReturn("9.9.9.9");
|
||||||
|
|
||||||
|
for (int i = 0; i < 10; i++) {
|
||||||
|
interceptor.preHandle(request, response, null);
|
||||||
|
}
|
||||||
|
// first IP is at limit — second IP is not
|
||||||
|
assertThat(interceptor.preHandle(other, response, null)).isTrue();
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void ignores_x_forwarded_for_from_untrusted_remote_addr() throws Exception {
|
||||||
|
when(request.getRemoteAddr()).thenReturn("5.5.5.5"); // not a trusted proxy
|
||||||
|
when(request.getHeader("X-Forwarded-For")).thenReturn("99.99.99.99");
|
||||||
|
|
||||||
|
// exhaust the limit for the real remote IP (5.5.5.5), not the spoofed one
|
||||||
|
for (int i = 0; i < 10; i++) {
|
||||||
|
interceptor.preHandle(request, response, null);
|
||||||
|
}
|
||||||
|
assertThat(interceptor.preHandle(request, response, null)).isFalse();
|
||||||
|
|
||||||
|
// spoofed IP (99.99.99.99) should not have been rate-limited
|
||||||
|
HttpServletRequest clean = mock(HttpServletRequest.class);
|
||||||
|
when(clean.getRemoteAddr()).thenReturn("99.99.99.99");
|
||||||
|
assertThat(interceptor.preHandle(clean, response, null)).isTrue();
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void trusts_x_forwarded_for_from_localhost() throws Exception {
|
||||||
|
when(request.getRemoteAddr()).thenReturn("127.0.0.1"); // trusted proxy
|
||||||
|
when(request.getHeader("X-Forwarded-For")).thenReturn("20.20.20.20");
|
||||||
|
|
||||||
|
for (int i = 0; i < 10; i++) {
|
||||||
|
interceptor.preHandle(request, response, null);
|
||||||
|
}
|
||||||
|
// the rate-limited IP should be 20.20.20.20 (from header), not 127.0.0.1
|
||||||
|
assertThat(interceptor.preHandle(request, response, null)).isFalse();
|
||||||
|
}
|
||||||
|
}
|
||||||
Reference in New Issue
Block a user