fix(auth): tighten API URL match, add Retry-After header, and add missing tests
Some checks failed
CI / Unit & Component Tests (pull_request) Successful in 3m9s
CI / OCR Service Tests (pull_request) Successful in 24s
CI / fail2ban Regex (pull_request) Has been cancelled
CI / Semgrep Security Scan (pull_request) Has been cancelled
CI / Compose Bucket Idempotency (pull_request) Has been cancelled
CI / Backend Unit Tests (pull_request) Has been cancelled
Some checks failed
CI / Unit & Component Tests (pull_request) Successful in 3m9s
CI / OCR Service Tests (pull_request) Successful in 24s
CI / fail2ban Regex (pull_request) Has been cancelled
CI / Semgrep Security Scan (pull_request) Has been cancelled
CI / Compose Bucket Idempotency (pull_request) Has been cancelled
CI / Backend Unit Tests (pull_request) Has been cancelled
- 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 <noreply@anthropic.com>
This commit is contained in:
@@ -42,16 +42,17 @@ public class LoginRateLimiter {
|
|||||||
// For the current single-VPS setup this is the correct, simplest implementation.
|
// 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) {
|
||||||
|
long retryAfterSeconds = windowMinutes * 60L;
|
||||||
String key = ip + ":" + email.toLowerCase(Locale.ROOT);
|
String key = ip + ":" + email.toLowerCase(Locale.ROOT);
|
||||||
if (!byIpEmail.get(key).tryConsume(1)) {
|
if (!byIpEmail.get(key).tryConsume(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, retryAfterSeconds);
|
||||||
}
|
}
|
||||||
if (!byIp.get(ip).tryConsume(1)) {
|
if (!byIp.get(ip).tryConsume(1)) {
|
||||||
// Refund the ipEmail token so IP-level blocking does not erode the per-email quota.
|
// Refund the ipEmail token so IP-level blocking does not erode the per-email quota.
|
||||||
byIpEmail.get(key).addTokens(1);
|
byIpEmail.get(key).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, retryAfterSeconds);
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -28,6 +28,7 @@ public class RateLimitInterceptor implements HandlerInterceptor {
|
|||||||
AtomicInteger count = requestCounts.get(ip, k -> new AtomicInteger(0));
|
AtomicInteger count = requestCounts.get(ip, k -> new AtomicInteger(0));
|
||||||
if (count.incrementAndGet() > MAX_REQUESTS_PER_MINUTE) {
|
if (count.incrementAndGet() > MAX_REQUESTS_PER_MINUTE) {
|
||||||
response.setStatus(HttpStatus.TOO_MANY_REQUESTS.value());
|
response.setStatus(HttpStatus.TOO_MANY_REQUESTS.value());
|
||||||
|
response.setHeader("Retry-After", "60");
|
||||||
response.getWriter().write("{\"code\":\"RATE_LIMIT_EXCEEDED\",\"message\":\"Too many requests\"}");
|
response.getWriter().write("{\"code\":\"RATE_LIMIT_EXCEEDED\",\"message\":\"Too many requests\"}");
|
||||||
return false;
|
return false;
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -10,11 +10,21 @@ public class DomainException extends RuntimeException {
|
|||||||
|
|
||||||
private final ErrorCode code;
|
private final ErrorCode code;
|
||||||
private final HttpStatus status;
|
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) {
|
public DomainException(ErrorCode code, HttpStatus status, String developerMessage) {
|
||||||
super(developerMessage);
|
super(developerMessage);
|
||||||
this.code = code;
|
this.code = code;
|
||||||
this.status = status;
|
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() {
|
public ErrorCode getCode() {
|
||||||
@@ -25,6 +35,11 @@ public class DomainException extends RuntimeException {
|
|||||||
return status;
|
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 ---
|
// --- Static factories for common cases ---
|
||||||
|
|
||||||
public static DomainException notFound(ErrorCode code, String message) {
|
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) {
|
public static DomainException tooManyRequests(ErrorCode code, String message) {
|
||||||
return new DomainException(code, HttpStatus.TOO_MANY_REQUESTS, 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);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -23,9 +23,11 @@ public class GlobalExceptionHandler {
|
|||||||
|
|
||||||
@ExceptionHandler(DomainException.class)
|
@ExceptionHandler(DomainException.class)
|
||||||
public ResponseEntity<ErrorResponse> handleDomain(DomainException ex) {
|
public ResponseEntity<ErrorResponse> handleDomain(DomainException ex) {
|
||||||
return ResponseEntity
|
var builder = ResponseEntity.status(ex.getStatus());
|
||||||
.status(ex.getStatus())
|
if (ex.getRetryAfterSeconds() != null) {
|
||||||
.body(new ErrorResponse(ex.getCode(), ex.getMessage()));
|
builder = builder.header("Retry-After", String.valueOf(ex.getRetryAfterSeconds()));
|
||||||
|
}
|
||||||
|
return builder.body(new ErrorResponse(ex.getCode(), ex.getMessage()));
|
||||||
}
|
}
|
||||||
|
|
||||||
@ExceptionHandler(MethodArgumentNotValidException.class)
|
@ExceptionHandler(MethodArgumentNotValidException.class)
|
||||||
|
|||||||
@@ -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.
|
||||||
|
*
|
||||||
|
* <p>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).
|
||||||
|
*
|
||||||
|
* <p>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}).
|
||||||
|
*
|
||||||
|
* <p>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);
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -42,6 +42,18 @@ class LoginRateLimiterTest {
|
|||||||
.isEqualTo(ErrorCode.TOO_MANY_LOGIN_ATTEMPTS));
|
.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
|
@Test
|
||||||
void success_after_10_failures_resets_ip_email_bucket() {
|
void success_after_10_failures_resets_ip_email_bucket() {
|
||||||
for (int i = 0; i < 10; i++) {
|
for (int i = 0; i < 10; i++) {
|
||||||
|
|||||||
@@ -45,6 +45,15 @@ class RateLimitInterceptorTest {
|
|||||||
verify(response).setStatus(HttpStatus.TOO_MANY_REQUESTS.value());
|
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
|
@Test
|
||||||
void different_ips_have_independent_limits() throws Exception {
|
void different_ips_have_independent_limits() throws Exception {
|
||||||
HttpServletRequest other = mock(HttpServletRequest.class);
|
HttpServletRequest other = mock(HttpServletRequest.class);
|
||||||
|
|||||||
@@ -111,7 +111,7 @@ const PUBLIC_API_PATHS = [
|
|||||||
|
|
||||||
export const handleFetch: HandleFetch = async ({ event, request, fetch }) => {
|
export const handleFetch: HandleFetch = async ({ event, request, fetch }) => {
|
||||||
const apiUrl = env.API_INTERNAL_URL || 'http://localhost:8080';
|
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);
|
if (!isApi) return fetch(request);
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user