fix(auth): address PR #617 review feedback on CSRF/rate-limit implementation
All checks were successful
CI / Unit & Component Tests (pull_request) Successful in 3m14s
CI / OCR Service Tests (pull_request) Successful in 20s
CI / Backend Unit Tests (pull_request) Successful in 3m7s
CI / fail2ban Regex (pull_request) Successful in 45s
CI / Semgrep Security Scan (pull_request) Successful in 20s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m0s
All checks were successful
CI / Unit & Component Tests (pull_request) Successful in 3m14s
CI / OCR Service Tests (pull_request) Successful in 20s
CI / Backend Unit Tests (pull_request) Successful in 3m7s
CI / fail2ban Regex (pull_request) Successful in 45s
CI / Semgrep Security Scan (pull_request) Successful in 20s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m0s
- 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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 `<BackButton>` 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).
|
||||
|
||||
---
|
||||
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<string, string> = {};
|
||||
if (cookieParts.length > 0) extraHeaders['Cookie'] = cookieParts.join('; ');
|
||||
const extraHeaders: Record<string, string> = { Cookie: cookieParts.join('; ') };
|
||||
if (xsrfToken) extraHeaders['X-XSRF-TOKEN'] = xsrfToken;
|
||||
|
||||
const modified = new Request(cloned, {
|
||||
|
||||
Reference in New Issue
Block a user