From 103d454e147559d58e37a5c7b95fbf80e05e4beb Mon Sep 17 00:00:00 2001 From: Marcel Date: Sun, 19 Apr 2026 01:20:11 +0200 Subject: [PATCH] fix(rate-limit): only trust X-Forwarded-For from known reverse proxies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Without this guard any client could send X-Forwarded-For: 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 --- .gitignore | 3 +- .../config/RateLimitInterceptor.java | 25 ++++-- .../config/RateLimitInterceptorTest.java | 88 +++++++++++++++++++ 3 files changed, 109 insertions(+), 7 deletions(-) create mode 100644 backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java diff --git a/.gitignore b/.gitignore index 526bcd0f..de5c12e2 100644 --- a/.gitignore +++ b/.gitignore @@ -11,4 +11,5 @@ gitea/ scripts/large-data.sql .vitest-attachments -**/test-results/.worktrees/ +**/test-results/ +.worktrees/ diff --git a/backend/src/main/java/org/raddatz/familienarchiv/config/RateLimitInterceptor.java b/backend/src/main/java/org/raddatz/familienarchiv/config/RateLimitInterceptor.java index cbcb3b1a..9c495406 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/config/RateLimitInterceptor.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/config/RateLimitInterceptor.java @@ -14,10 +14,10 @@ public class RateLimitInterceptor implements HandlerInterceptor { 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. private final Cache requestCounts = Caffeine.newBuilder() - .expireAfterWrite(1, TimeUnit.MINUTES) + .expireAfterAccess(1, TimeUnit.MINUTES) .maximumSize(10_000) .build(); @@ -35,10 +35,23 @@ public class RateLimitInterceptor implements HandlerInterceptor { } private String resolveClientIp(HttpServletRequest request) { - String forwarded = request.getHeader("X-Forwarded-For"); - if (forwarded != null && !forwarded.isBlank()) { - return forwarded.split(",")[0].trim(); + // Only trust X-Forwarded-For when the direct connection comes from a known + // reverse proxy (loopback or Docker private network). Trusting it unconditionally + // 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."); } } diff --git a/backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java b/backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java new file mode 100644 index 00000000..12a4d89e --- /dev/null +++ b/backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java @@ -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(); + } +}