From 687a2590c796ac3f8d8cc2b264e50d1a7d62de5c Mon Sep 17 00:00:00 2001 From: Marcel Date: Tue, 19 May 2026 07:41:04 +0200 Subject: [PATCH] fix(auth): address PR #617 review feedback on CSRF/rate-limit implementation - Remove unreachable `&& !xsrfToken` condition from `handleFetch` guard; simplify the redundant `cookieParts.length > 0` check that follows it - Add `TOO_MANY_LOGIN_ATTEMPTS` to both Error Handling sections in CLAUDE.md (backend and frontend) so LLMs are aware of the code without looking it up - Add reverse-proxy IP trust and IPv6 address-cycling caveats to ADR-022 Consequences section Co-Authored-By: Claude Sonnet 4.6 --- CLAUDE.md | 4 ++-- docs/adr/022-csrf-session-revocation-rate-limiting.md | 9 +++++++++ frontend/src/hooks.server.ts | 5 ++--- 3 files changed, 13 insertions(+), 5 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 544b1253..10a3c368 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -160,7 +160,7 @@ Input DTOs live flat in the domain package. Response types are the model entitie → See [CONTRIBUTING.md §Error handling](./CONTRIBUTING.md#error-handling) -**LLM reminder:** use `DomainException.notFound/forbidden/conflict/internal()` from service methods — never throw raw exceptions. When adding a new `ErrorCode`: (1) add to `ErrorCode.java`, (2) add to `ErrorCode` type in `frontend/src/lib/shared/errors.ts`, (3) add a `case` in `getErrorMessage()`, (4) add i18n keys in `messages/{de,en,es}.json`. +**LLM reminder:** use `DomainException.notFound/forbidden/conflict/internal()` from service methods — never throw raw exceptions. When adding a new `ErrorCode`: (1) add to `ErrorCode.java`, (2) add to `ErrorCode` type in `frontend/src/lib/shared/errors.ts`, (3) add a `case` in `getErrorMessage()`, (4) add i18n keys in `messages/{de,en,es}.json`. Valid error codes include: `TOO_MANY_LOGIN_ATTEMPTS` (returned by `LoginRateLimiter` as HTTP 429 when a brute-force threshold is exceeded). ### Security / Permissions @@ -267,7 +267,7 @@ Back button pattern — use the shared `` component from `$lib/share → See [CONTRIBUTING.md §Error handling](./CONTRIBUTING.md#error-handling) -**LLM reminder:** when adding a new `ErrorCode`: (1) add to `ErrorCode.java`, (2) add to `ErrorCode` type in `frontend/src/lib/shared/errors.ts`, (3) add a `case` in `getErrorMessage()`, (4) add i18n keys in `messages/{de,en,es}.json`. +**LLM reminder:** when adding a new `ErrorCode`: (1) add to `ErrorCode.java`, (2) add to `ErrorCode` type in `frontend/src/lib/shared/errors.ts`, (3) add a `case` in `getErrorMessage()`, (4) add i18n keys in `messages/{de,en,es}.json`. Valid error codes include: `TOO_MANY_LOGIN_ATTEMPTS` (returned by `LoginRateLimiter` as HTTP 429 when a brute-force threshold is exceeded). --- diff --git a/docs/adr/022-csrf-session-revocation-rate-limiting.md b/docs/adr/022-csrf-session-revocation-rate-limiting.md index be886d9d..ff1f02f0 100644 --- a/docs/adr/022-csrf-session-revocation-rate-limiting.md +++ b/docs/adr/022-csrf-session-revocation-rate-limiting.md @@ -104,3 +104,12 @@ source. because `@WebMvcTest` slices exclude `JacksonAutoConfiguration`. The response only serialises a fixed String key (`"code"`) so naming strategy and custom modules are irrelevant. +- IP extraction uses `HttpServletRequest.getRemoteAddr()`. In deployments behind + a reverse proxy the `X-Forwarded-For` header is not trusted — doing so would + let clients spoof their IP and trivially bypass the per-IP limit. Trusting + proxy headers requires separate work (e.g. Spring's `ForwardedHeaderFilter` + with an allowlist of trusted proxy addresses). +- IPv6 and IPv4-mapped addresses (e.g. `::ffff:1.2.3.4`) are not normalised to + a canonical form. An attacker with access to multiple IPv6 addresses could + rotate addresses to bypass the per-IP bucket. This is a known limitation of + address-based rate limiting and is acceptable for the current deployment. diff --git a/frontend/src/hooks.server.ts b/frontend/src/hooks.server.ts index 3db312bc..124c1634 100644 --- a/frontend/src/hooks.server.ts +++ b/frontend/src/hooks.server.ts @@ -131,14 +131,13 @@ export const handleFetch: HandleFetch = async ({ event, request, fetch }) => { if (sessionId) cookieParts.push(`fa_session=${sessionId}`); if (xsrfToken) cookieParts.push(`XSRF-TOKEN=${xsrfToken}`); - if (cookieParts.length === 0 && !xsrfToken) { + if (cookieParts.length === 0) { return fetch(request); } // Clone first so the body stream is preserved on the new Request. const cloned = request.clone(); - const extraHeaders: Record = {}; - if (cookieParts.length > 0) extraHeaders['Cookie'] = cookieParts.join('; '); + const extraHeaders: Record = { Cookie: cookieParts.join('; ') }; if (xsrfToken) extraHeaders['X-XSRF-TOKEN'] = xsrfToken; const modified = new Request(cloned, {