From 164a917d95bf707e81300cb66ef315bf36d93f5d Mon Sep 17 00:00:00 2001 From: Marcel Date: Tue, 19 May 2026 09:08:26 +0200 Subject: [PATCH] fix(auth): tighten API URL match, add Retry-After header, and add missing tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - frontend/hooks.server.ts: replace request.url.includes('/api/') with new URL(request.url).pathname.startsWith('/api/') so a page named /my-api/something cannot accidentally match the API gate - DomainException: add optional retryAfterSeconds field and a new tooManyRequests() factory overload that carries the value - LoginRateLimiter: pass windowMinutes * 60 as retryAfterSeconds when throwing TOO_MANY_LOGIN_ATTEMPTS (RFC 6585 §4 SHOULD) - GlobalExceptionHandler: emit Retry-After header when retryAfterSeconds is set on a DomainException - RateLimitInterceptor: emit Retry-After: 60 on 429 responses (1-min window matches the existing MAX_REQUESTS_PER_MINUTE logic) - LoginRateLimiterTest: assert retryAfterSeconds equals window duration - RateLimitInterceptorTest: assert Retry-After header is set on 429 - JdbcSessionRevocationAdapterIntegrationTest: new @SpringBootTest + Testcontainers test verifying revokeAll deletes all spring_session rows and revokeOther leaves the current session intact Co-Authored-By: Claude Sonnet 4.6 --- .../familienarchiv/auth/LoginRateLimiter.java | 5 +- .../config/RateLimitInterceptor.java | 1 + .../exception/DomainException.java | 19 +++ .../exception/GlobalExceptionHandler.java | 8 +- ...ssionRevocationAdapterIntegrationTest.java | 136 ++++++++++++++++++ .../auth/LoginRateLimiterTest.java | 12 ++ .../config/RateLimitInterceptorTest.java | 9 ++ frontend/src/hooks.server.ts | 2 +- 8 files changed, 186 insertions(+), 6 deletions(-) create mode 100644 backend/src/test/java/org/raddatz/familienarchiv/auth/JdbcSessionRevocationAdapterIntegrationTest.java 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 1dde6e31..31ba5b6f 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/auth/LoginRateLimiter.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/auth/LoginRateLimiter.java @@ -42,16 +42,17 @@ public class LoginRateLimiter { // For the current single-VPS setup this is the correct, simplest implementation. public void checkAndConsume(String ip, String email) { + long retryAfterSeconds = windowMinutes * 60L; String key = ip + ":" + email.toLowerCase(Locale.ROOT); if (!byIpEmail.get(key).tryConsume(1)) { throw DomainException.tooManyRequests(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS, - "Too many login attempts from " + ip); + "Too many login attempts from " + ip, retryAfterSeconds); } if (!byIp.get(ip).tryConsume(1)) { // Refund the ipEmail token so IP-level blocking does not erode the per-email quota. byIpEmail.get(key).addTokens(1); throw DomainException.tooManyRequests(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS, - "Too many login attempts from " + ip); + "Too many login attempts from " + ip, retryAfterSeconds); } } 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 ed53494c..7cd18b23 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/config/RateLimitInterceptor.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/config/RateLimitInterceptor.java @@ -28,6 +28,7 @@ public class RateLimitInterceptor implements HandlerInterceptor { AtomicInteger count = requestCounts.get(ip, k -> new AtomicInteger(0)); if (count.incrementAndGet() > MAX_REQUESTS_PER_MINUTE) { response.setStatus(HttpStatus.TOO_MANY_REQUESTS.value()); + response.setHeader("Retry-After", "60"); response.getWriter().write("{\"code\":\"RATE_LIMIT_EXCEEDED\",\"message\":\"Too many requests\"}"); return false; } diff --git a/backend/src/main/java/org/raddatz/familienarchiv/exception/DomainException.java b/backend/src/main/java/org/raddatz/familienarchiv/exception/DomainException.java index c65ad5b8..3f38cddc 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/exception/DomainException.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/exception/DomainException.java @@ -10,11 +10,21 @@ public class DomainException extends RuntimeException { private final ErrorCode code; private final HttpStatus status; + /** Seconds until the rate-limit window resets; {@code null} when not applicable. */ + private final Long retryAfterSeconds; public DomainException(ErrorCode code, HttpStatus status, String developerMessage) { super(developerMessage); this.code = code; this.status = status; + this.retryAfterSeconds = null; + } + + private DomainException(ErrorCode code, HttpStatus status, String developerMessage, Long retryAfterSeconds) { + super(developerMessage); + this.code = code; + this.status = status; + this.retryAfterSeconds = retryAfterSeconds; } public ErrorCode getCode() { @@ -25,6 +35,11 @@ public class DomainException extends RuntimeException { return status; } + /** Returns the {@code Retry-After} value in seconds, or {@code null} if not set. */ + public Long getRetryAfterSeconds() { + return retryAfterSeconds; + } + // --- Static factories for common cases --- public static DomainException notFound(ErrorCode code, String message) { @@ -59,4 +74,8 @@ public class DomainException extends RuntimeException { public static DomainException tooManyRequests(ErrorCode code, String message) { return new DomainException(code, HttpStatus.TOO_MANY_REQUESTS, message); } + + public static DomainException tooManyRequests(ErrorCode code, String message, long retryAfterSeconds) { + return new DomainException(code, HttpStatus.TOO_MANY_REQUESTS, message, retryAfterSeconds); + } } diff --git a/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java b/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java index aa33094d..87838d5c 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/exception/GlobalExceptionHandler.java @@ -23,9 +23,11 @@ public class GlobalExceptionHandler { @ExceptionHandler(DomainException.class) public ResponseEntity handleDomain(DomainException ex) { - return ResponseEntity - .status(ex.getStatus()) - .body(new ErrorResponse(ex.getCode(), ex.getMessage())); + var builder = ResponseEntity.status(ex.getStatus()); + if (ex.getRetryAfterSeconds() != null) { + builder = builder.header("Retry-After", String.valueOf(ex.getRetryAfterSeconds())); + } + return builder.body(new ErrorResponse(ex.getCode(), ex.getMessage())); } @ExceptionHandler(MethodArgumentNotValidException.class) diff --git a/backend/src/test/java/org/raddatz/familienarchiv/auth/JdbcSessionRevocationAdapterIntegrationTest.java b/backend/src/test/java/org/raddatz/familienarchiv/auth/JdbcSessionRevocationAdapterIntegrationTest.java new file mode 100644 index 00000000..cb094be2 --- /dev/null +++ b/backend/src/test/java/org/raddatz/familienarchiv/auth/JdbcSessionRevocationAdapterIntegrationTest.java @@ -0,0 +1,136 @@ +package org.raddatz.familienarchiv.auth; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.raddatz.familienarchiv.PostgresContainerConfig; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.boot.test.context.SpringBootTest; +import org.springframework.context.annotation.Import; +import org.springframework.jdbc.core.JdbcTemplate; +import org.springframework.test.context.ActiveProfiles; +import org.springframework.test.context.bean.override.mockito.MockitoBean; +import org.springframework.transaction.support.TransactionTemplate; +import software.amazon.awssdk.services.s3.S3Client; + +import java.time.Instant; +import java.util.UUID; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Integration test for {@link JdbcSessionRevocationAdapter} that verifies + * session rows are actually written to / removed from the {@code spring_session} + * table backed by a real PostgreSQL container. + * + *

Sessions are inserted via raw JDBC to avoid the module-access restriction on + * {@code JdbcIndexedSessionRepository.JdbcSession}. The {@link SessionRevocationPort} + * bean injected here is the real {@link JdbcSessionRevocationAdapter} wired by Spring. + */ +@SpringBootTest(webEnvironment = SpringBootTest.WebEnvironment.RANDOM_PORT) +@ActiveProfiles("test") +@Import(PostgresContainerConfig.class) +class JdbcSessionRevocationAdapterIntegrationTest { + + @MockitoBean S3Client s3Client; + + @Autowired SessionRevocationPort adapter; + @Autowired JdbcTemplate jdbcTemplate; + @Autowired TransactionTemplate transactionTemplate; + + private static final String PRINCIPAL = "revocation-it@test.de"; + + @BeforeEach + void clearSessions() { + // spring_session_attributes cascades on delete + transactionTemplate.execute(status -> { + jdbcTemplate.update("DELETE FROM spring_session"); + return null; + }); + } + + // ── helper ───────────────────────────────────────────────────────────────── + + /** + * Inserts a minimal {@code spring_session} row attributed to {@value #PRINCIPAL} + * and returns its opaque primary-key ID (the value the repository uses as the + * session identifier, not the {@code SESSION_ID} column which holds the public token). + * + *

Column layout mirrors the Flyway-managed schema shipped with the app: + * PRIMARY_ID, SESSION_ID, CREATION_TIME, LAST_ACCESS_TIME, MAX_INACTIVE_INTERVAL, + * EXPIRY_TIME, PRINCIPAL_NAME. + */ + /** + * Inserts a persisted session row for {@value #PRINCIPAL} and returns the + * {@code SESSION_ID} column value — this is the opaque identifier that + * {@link JdbcIndexedSessionRepository} uses as the session's public key + * (returned by {@code JdbcSession.getId()} and expected by + * {@link JdbcIndexedSessionRepository#deleteById}). + * + *

The inserts run inside a {@link TransactionTemplate} so the rows are + * committed before {@code findByPrincipalName} opens its own transaction and + * can see the data via Read Committed isolation. + */ + private String insertSession() { + String primaryId = UUID.randomUUID().toString(); + // SESSION_ID is the value used by JdbcSession.getId() and findByPrincipalName map keys. + String sessionId = UUID.randomUUID().toString(); + long now = Instant.now().toEpochMilli(); + long expiry = now + 8L * 3600 * 1000; // 8-hour TTL + transactionTemplate.execute(status -> { + jdbcTemplate.update(""" + INSERT INTO spring_session + (PRIMARY_ID, SESSION_ID, CREATION_TIME, LAST_ACCESS_TIME, + MAX_INACTIVE_INTERVAL, EXPIRY_TIME, PRINCIPAL_NAME) + VALUES (?, ?, ?, ?, ?, ?, ?) + """, + primaryId, sessionId, now, now, 28800, expiry, PRINCIPAL); + // Spring Session's listSessionsByPrincipalName query joins spring_session_attributes; + // insert a minimal attribute row so the session appears in the result set. + jdbcTemplate.update(""" + INSERT INTO spring_session_attributes + (SESSION_PRIMARY_ID, ATTRIBUTE_NAME, ATTRIBUTE_BYTES) + VALUES (?, ?, ?) + """, + primaryId, "test_attr", new byte[]{0}); + return null; + }); + return sessionId; // the public key used by JdbcSession.getId() and deleteById() + } + + // ── tests ────────────────────────────────────────────────────────────────── + + @Test + void revokeAllSessions_removes_every_row_from_spring_session_table() { + insertSession(); + insertSession(); + + int count = adapter.revokeAllSessions(PRINCIPAL); + + assertThat(count).isEqualTo(2); + assertThat(jdbcTemplate.queryForObject( + "SELECT COUNT(*) FROM spring_session WHERE PRINCIPAL_NAME = ?", + Long.class, PRINCIPAL)) + .isZero(); + } + + @Test + void revokeOtherSessions_deletes_non_current_rows_and_keeps_current_session() { + String keepId = insertSession(); + insertSession(); + insertSession(); + + int count = adapter.revokeOtherSessions(keepId, PRINCIPAL); + + assertThat(count).isEqualTo(2); + // The current session row must still be present (keyed by SESSION_ID) + assertThat(jdbcTemplate.queryForObject( + "SELECT COUNT(*) FROM spring_session WHERE SESSION_ID = ?", + Long.class, keepId)) + .isEqualTo(1L); + // The total for this principal is now exactly 1 + assertThat(jdbcTemplate.queryForObject( + "SELECT COUNT(*) FROM spring_session WHERE PRINCIPAL_NAME = ?", + Long.class, PRINCIPAL)) + .isEqualTo(1L); + } +} 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 4872ecc8..c2b178de 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/auth/LoginRateLimiterTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/auth/LoginRateLimiterTest.java @@ -42,6 +42,18 @@ class LoginRateLimiterTest { .isEqualTo(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS)); } + @Test + void blocked_attempt_carries_retry_after_seconds_equal_to_window_duration() { + 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) + .satisfies(ex -> assertThat(((DomainException) ex).getRetryAfterSeconds()) + .isEqualTo(15 * 60L)); // windowMinutes=15 → 900 seconds + } + @Test void success_after_10_failures_resets_ip_email_bucket() { for (int i = 0; i < 10; i++) { diff --git a/backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java b/backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java index 12a4d89e..eda7835d 100644 --- a/backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java +++ b/backend/src/test/java/org/raddatz/familienarchiv/config/RateLimitInterceptorTest.java @@ -45,6 +45,15 @@ class RateLimitInterceptorTest { verify(response).setStatus(HttpStatus.TOO_MANY_REQUESTS.value()); } + @Test + void blocked_response_includes_retry_after_header() throws Exception { + for (int i = 0; i < 10; i++) { + interceptor.preHandle(request, response, null); + } + interceptor.preHandle(request, response, null); + verify(response).setHeader("Retry-After", "60"); + } + @Test void different_ips_have_independent_limits() throws Exception { HttpServletRequest other = mock(HttpServletRequest.class); diff --git a/frontend/src/hooks.server.ts b/frontend/src/hooks.server.ts index 124c1634..96d3543c 100644 --- a/frontend/src/hooks.server.ts +++ b/frontend/src/hooks.server.ts @@ -111,7 +111,7 @@ const PUBLIC_API_PATHS = [ export const handleFetch: HandleFetch = async ({ event, request, fetch }) => { const apiUrl = env.API_INTERNAL_URL || 'http://localhost:8080'; - const isApi = request.url.startsWith(apiUrl) || request.url.includes('/api/'); + const isApi = request.url.startsWith(apiUrl) || new URL(request.url).pathname.startsWith('/api/'); if (!isApi) return fetch(request);