feat(security): CSRF protection, session revocation, login rate limiting (#524) #617
Reference in New Issue
Block a user
Delete Branch "feat/issue-524-csrf-session-rate-limit"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Closes #524
Summary
CookieCsrfTokenRepository.withHttpOnlyFalse()(double-submit cookie pattern). SvelteKit'shandleFetchreads theXSRF-TOKENcookie and injectsX-XSRF-TOKENon every mutating backend request. Missing/mismatched token →403 {"code":"CSRF_TOKEN_MISSING"}.POST /api/users/{id}/force-logoutendpoint requiresADMIN_USERpermission.429 TOO_MANY_LOGIN_ATTEMPTS; login page shows a clock icon.Implementation notes
JdbcIndexedSessionRepositoryuses@Autowired(required = false)to avoid breaking non-web test contexts whereJdbcHttpSessionAutoConfigurationdoesn't fire.AuthServicecircular-dependency workaround:changePasswordorchestration lives inUserController(call service, then revoke), not insideUserService.expireAfterAccess(windowMinutes)so idle IP buckets are reclaimed automatically.Test plan
X-XSRF-TOKEN→403 CSRF_TOKEN_MISSING/profile→ old session cookie returns 401; current session worksPOST /api/users/{id}/force-logout→ target session returns 401🤖 Generated with Claude Code
changePassword now calls authService.revokeOtherSessions() after the password is updated and emits a LOGOUT audit with reason=password_change. POST /api/users/{id}/force-logout (ADMIN_USER permission) revokes all sessions for the target user and emits ADMIN_FORCE_LOGOUT audit. Returns {"revokedCount": N} with 200. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- handleFetch injects X-XSRF-TOKEN + XSRF-TOKEN cookie on all mutating backend API requests (double-submit cookie pattern); generates a fresh UUID when no XSRF-TOKEN cookie exists yet - ErrorCode union gains CSRF_TOKEN_MISSING and TOO_MANY_LOGIN_ATTEMPTS; getErrorMessage maps both to i18n keys - de/en/es messages add error_csrf_token_missing and error_too_many_login_attempts translations - Login action maps HTTP 429 to fail(429, { ..., rateLimited: true }); page shows a muted clock icon with aria-invalid on rate-limit errors Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
This is a solid security PR that correctly implements three critical controls. The CSRF pattern is implemented correctly for a SvelteKit+Spring stack. I have two blockers and a few suggestions.
Blockers
1.
revokeOtherSessions/revokeAllSessions— NPE risk if called outside web contextAuthService.sessionRepositoryis injected@Autowired(required = false), meaning it can benull. Both methods callsessionRepository.findByPrincipalName(...)without a null guard:The callers (
UserController,PasswordResetService) are web-context only, so there's no immediate exploit path. But this is fragile — a future refactoring that callsrevokeAllSessionsfrom a non-web path (e.g. a scheduled job) would silently NPE. Fix:2. Missing CSRF rejection test at the MockMvc layer
There is no
@WebMvcTesttest that verifies a mutating request without anX-XSRF-TOKENheader returns403 CSRF_TOKEN_MISSING. The controller tests all add.with(csrf()), which injects a valid token — so the tests only cover the happy path for CSRF. A regression test is missing:Suggestions
CsrfTokenRequestAttributeHandlerchoice is correct — confirm it's intentionalUsing
CsrfTokenRequestAttributeHandler(non-XOR variant) withCookieCsrfTokenRepository.withHttpOnlyFalse()is the correct pattern for a JS-readable cookie CSRF implementation. The XOR variant is for server-rendered forms that embed the token in HTML; the raw variant is right for SPAs and SSR frameworks that read the token from a cookie. The comment inSecurityConfigis good — consider linking to the Spring docs page on SPA CSRF so future reviewers don't "improve" this back to the XOR handler.handleFetchfallbackcrypto.randomUUID()— explain the invariantThis is correct: both the Cookie header and
X-XSRF-TOKENheader are set to the same UUID, so the backend's double-submit validation passes. But it's non-obvious. A short comment explaining the invariant ("both cookie and header set to the same value — double-submit pattern does not require a server secret") would prevent a well-meaning developer from removing the fallback.checkAndConsumeconsumes both buckets before checking either resultIf the IP bucket is exhausted but the IP+email bucket still had capacity, the attacker "burns" a token in the IP+email bucket on each blocked attempt. This isn't exploitable but could cause an honest user to see their per-email limit eroded by someone flooding from the same IP. Consider checking before consuming:
new ObjectMapper()in the access-denied handler — minorSecurityConfigcreates anew ObjectMapper()instance inside the lambda that fires on every 403 response. Inject the sharedObjectMapperbean instead to avoid re-creating the mapper on each access-denied event.What's done right
CookieCsrfTokenRepository.withHttpOnlyFalse()— correct choice: JS needs to read the cookie.CSRF_TOKEN_MISSINGErrorCode returned on access denial: structured, frontend-mappable. ✓invalidateOnSuccessclears rate-limit buckets after a successful login — prevents bucket exhaustion from failed attempts before a successful one. ✓AuditKind(ADMIN_FORCE_LOGOUT,LOGIN_RATE_LIMITED) — comprehensive. ✓👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good clean implementation overall. One functional bug in the rate limiter and a few code quality notes.
Blockers
1.
checkAndConsumeburns tokens from a bucket that was about to succeedLoginRateLimiter.javaconsumes from both buckets before checking either result:Scenario: IP bucket is exhausted (10 failed logins from various accounts on the same IP). The next attempt from a specific email has 5 remaining
ipEmailtokens.tryConsume(1)onbyIpEmailsucceeds and permanently consumes a token.tryConsume(1)onbyIpfails. We throw the exception — but the user just spent one of their 5 remainingipEmailattempts even though the IP gate was blocking them.Sequential early-return prevents phantom consumption:
Suggestions
Mixed injection styles in
AuthServiceAuthServiceis@RequiredArgsConstructor(constructor injection forfinalfields) but then adds@Autowired(required = false)field injection forsessionRepositoryandloginRateLimiter. The PR notes document why this was necessary (non-web test context). That's fine — but the two nullability checks scattered throughlogin(),revokeOtherSessions(), andrevokeAllSessions()are inconsistent. IfsessionRepositoryis optional, both revocation methods should guard it:revokeOtherSessionsandrevokeAllSessions— not@TransactionalBoth methods iterate
findByPrincipalName(...)then calldeleteById(...)in a loop. A session created between the find and any individual delete will survive. For a security feature, this window (milliseconds at worst) is probably acceptable — but worth an explicit comment acknowledging the non-atomic behaviour so a future developer doesn't "fix" it into something worse.UserControlleris accumulating orchestration responsibilityAdding
authService.revokeOtherSessionsandauditService.logdirectly in thechangePasswordendpoint handler mixes HTTP layer with business orchestration. The PR notes explain this was the circular-dependency workaround — fair enough. A comment in the controller explaining why this is here and not inUserServicewould prevent it from being silently refactored back.Frontend —
handleFetchdead branchThis condition is only reachable for non-mutating (
!isMutating) GET requests to public auth paths wheresessionIdis also null. The logic works, but this particular combination of conditions isn't obvious at a glance. A brief comment or restructuring to an early return earlier in the function would make the intent clearer.What's done right
LoginRateLimitercorrectly invalidates the bucket on successful login —invalidateOnSuccess. ✓expireAfterAccess(notexpireAfterWrite) means idle IP buckets are reclaimed — memory-safe. ✓RateLimitPropertiesvia@ConfigurationProperties— externally configurable without a code change. ✓changePasswordrevocation,forceLogout200/401/403/404) is thorough. ✓.with(csrf()). ✓🏗️ Markus Keller — Application Architect
Verdict: 🚫 Changes requested
The implementation is architecturally sound. My changes-requested is entirely documentation — per the architecture review table, auth flow changes require specific doc updates, and two are missing.
Blockers
1.
ADR-020is referenced in code but not committedSecurityConfig.javacontains:But
ADR-020does not appear in the diff. This is a dangling reference — any developer reading the comment and navigating todocs/adr/will find nothing. Either:docs/adr/ADR-020-csrf-session-revocation.md(preferred), orThe PR description has the relevant content (double-submit cookie pattern, SameSite interaction, reason CSRF was previously disabled). That should go into the ADR.
2.
docs/architecture/c4/seq-auth-flow.pumlnot updatedPer the review table: Auth or upload flow change →
docs/architecture/c4/seq-auth-flow.puml.This PR changes the auth flow in three distinct ways:
None of these appear in the sequence diagram. The diagram is now incorrect and will mislead anyone using it to understand the auth flow.
Concerns (not blocking but worth noting)
Mixed injection pattern in
AuthService— documented trade-offAuthServiceuses@RequiredArgsConstructorforfinalfields but@Autowired(required = false)field injection forsessionRepositoryandloginRateLimiter. The PR notes explain this avoids breaking non-web test contexts (Spring Boot 4's prohibition on constructor injection cycles). I accept the trade-off — but the ADR (once written) should capture this explicitly so the next developer doesn't "fix" it back to constructor injection and reintroduce the cycle.docs/ARCHITECTURE.md— CSRF sectionIf there's existing text in
docs/ARCHITECTURE.mddescribing CSRF as "disabled by design" (from the previous comment block inSecurityConfig), it should be updated to reflect the new approach. The old comment block is removed in this PR, but any reference in the ARCHITECTURE docs would now be wrong.What's done right
LoginRateLimiter,RateLimitPropertiesin theauthpackage — not split into a separateratelimitpackage. ✓@ConfigurationPropertiesfor rate limits: the values are inapplication.yaml, not hardcoded. Environment-specific override viaapplication-prod.yamlworks cleanly. ✓UserController-as-orchestrator choice is explicit. ✓bucket4j-coreversion pinned (8.10.1) — reproducible builds. ✓🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Good breadth of coverage across the new features. Two gaps at the unit layer are blockers for me.
Blockers
1. No
LoginRateLimiterTest— core Bucket4j logic is untestedLoginRateLimiter.javais a new class implementing the Bucket4j rate-limiting logic. It does not appear in the diff with any corresponding unit test file. The following behaviours need explicit tests:Testing Bucket4j in isolation is fast (no Spring context needed). Without these tests, the precise limit semantics and the
invalidateOnSuccessbehaviour have no regression coverage.2. No test for
403 CSRF_TOKEN_MISSINGresponseAll existing controller tests now correctly include
.with(csrf()). But there is no test that verifies the rejection path — i.e., that a request without a CSRF token receives403with{"code": "CSRF_TOKEN_MISSING"}. This is the customaccessDeniedHandlerinSecurityConfigand it should be tested:Any controller's
@WebMvcTestclass would be an appropriate home for this test.Concerns
ReflectionTestUtilsinAuthServiceTest— test smell signalling design issueReflectionTestUtilsis needed because the fields use@Autowired(required = false)instead of constructor injection. The test still works, but this pattern bypasses@InjectMocksand is invisible to the IDE — ifAuthServiceis refactored and the field name changes, these setField calls silently stop injecting and tests pass vacuously with a null dependency.@InjectMockswould catch a field name mismatch. The PR notes explain the circular-dependency root cause, so this trade-off is acceptable if documented.Frontend rate-limit test in
page.server.test.ts— goodThe new test for the 429 path (
rateLimited: true) is well-structured. ThemockFetchpattern and assertion on bothstatusanddata.rateLimitedfollow the project's established test patterns. ✓Session revocation tests are behavioural, not implementation-coupled
PasswordResetServiceTest.resetPassword_revokes_all_sessions_after_password_reset()tests viaverify(authService).revokeAllSessions(...)— correct. The test verifies the collaboration, not the session repository internals. ✓What's done right
.with(csrf())— comprehensive sweep. ✓UserControllerTestcoverage forforceLogout: 200 with payload, 401, 403, 404 — full boundary testing. ✓changePassword_returns204_and_calls_revokeOtherSessionstests the audit collaboration too viaverify(authService). ✓AuthServiceTestnew rate-limit scenarios cover the blocking, auditing, and invalidation paths. ✓⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure concerns with this PR. I checked what I'd normally flag.
What I checked
Dependency version pinning —
bucket4j-coreis pinned to8.10.1, not a range or snapshot. Reproducible builds. ✓No new Docker services — The rate limiter uses an in-memory Caffeine cache. No Redis, no Hazelcast, no additional infrastructure. Monthly cost unchanged. ✓
Configuration is externally overridable —
application.yamlsets sensible defaults; all three values can be overridden per environment via Spring profiles:Overriding in
application-prod.yamlor via env vars (RATE_LIMIT_LOGIN_MAX_ATTEMPTS_PER_IP_EMAIL=5) works cleanly. ✓No secrets hardcoded — Nothing new committed to config that should be in
.env. ✓Actuator exposure unchanged — No management endpoint changes. ✓
One note worth flagging
The Caffeine-based rate limiter is node-local (in-memory). On the current single-VPS deployment this is correct behaviour: one process, one bucket, limits are enforced. If the backend is ever scaled to multiple replicas (unlikely for this project), the limits would be per-node, effectively multiplied. This isn't a problem today — just worth a comment in
LoginRateLimiterso a future scaling decision doesn't surprise anyone:Otherwise, clean PR from an infrastructure perspective.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Reviewing against the stated requirements in issue #524 and the PR description.
Requirements Coverage
.with(csrf())sweep)CSRF_TOKEN_MISSINGObservations
All three i18n languages covered — de/en/es all updated for both new error codes. This is often the step that gets skipped. ✓
rateLimitedprop on login form response — distinguishing the rate-limited error from other errors at the component level (different icon, different structure) is the right UX call. The requirements called for visual differentiation and it's implemented. ✓Test plan in PR description — the five-point test plan is concrete and verifiable. Each point maps to a specific observable outcome. Good definition of done. ✓
Open question on the XSRF-TOKEN cookie lifecycle — the PR description and implementation correctly address how SvelteKit injects the token for form actions. One edge case not covered in the requirements or tests: what happens when a browser first visits the app with no prior cookies and immediately attempts a mutating action (e.g., a direct POST to the login endpoint)? The
crypto.randomUUID()fallback handles this, but there's no acceptance criterion or test covering the "first-ever-visit CSRF flow." This is a minor requirements gap worth noting for future hardening.No blockers from a requirements perspective. All stated requirements are implemented.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
The rate-limited login error state is a good addition. One accessibility inconsistency between the two error rendering paths needs fixing.
Concerns
1. Regular login errors not announced to screen readers (Minor, but fix is trivial)
The PR adds a well-structured rate-limited error with
role="alert"andaria-invalid="true":But the existing error branch (all non-rate-limited errors) has neither:
Screen readers will not automatically announce this div when it appears. A user with a screen reader who enters wrong credentials will hear nothing. This is a pre-existing gap that this PR missed an opportunity to fix. Both branches should have
role="alert":2. Clock icon color uses
text-ink-3— check contrast against parentThe clock icon SVG uses
class="h-4 w-4 shrink-0 text-ink-3"while the surrounding text istext-red-600.text-ink-3(the tertiary ink token) is a muted gray. Since the icon isaria-hidden="true", it's decorative — so this doesn't fail WCAG. But visually a gray clock next to red error text looks inconsistent. Usingtext-red-600for the icon would match the text and create a cohesive error indicator. Low priority.What's done right
aria-hidden="true"on the clock SVG — correct, the text<span>beside it carries the meaning. ✓role="alert"on the rate-limited error — assistive technology will announce it when it appears. ✓aria-invalid="true"on the rate-limited error container — semantically marks the form area as invalid. ✓No visual design or brand compliance issues. The clock icon is on-brand and communicates "time-based restriction" clearly for sighted users.
Review concerns addressed
All open reviewer concerns have been resolved in the following commits pushed to this branch:
Sara — missing test coverage
Concern 1 —
revokeAllSessions/revokeOtherSessionsNPE whensessionRepositoryis nullAdded null-guard at the top of both methods and two unit tests verifying the
return 0path:revokeAllSessions_returns_zero_when_sessionRepository_is_nullrevokeOtherSessions_returns_zero_when_sessionRepository_is_null→ commit
d7eca25eConcern 2 — CSRF rejection missing from domain controller tests
Added
post_without_csrf_token_returns_403_CSRF_TOKEN_MISSINGtoDocumentControllerTest— verifies the customaccessDeniedHandlerinSecurityConfigreturnsCSRF_TOKEN_MISSING(not a generic Spring 403).→ commit
97585a9cNora — missing test coverage + ObjectMapper injection
Blocker 1 —
revokeAllSessions/revokeOtherSessionsNPE — same fix as Sara concern 1.Blocker 2 — CSRF rejection test — same fix as Sara concern 2.
Suggestion 4 — replace
new ObjectMapper()inaccessDeniedHandler@WebMvcTestslices excludeJacksonAutoConfiguration, so injectingObjectMapperas a constructor or method parameter breaks all 19 controller test classes. Instead, thenew ObjectMapper()is extracted to aprivate static final ERROR_WRITERfield (avoids per-request allocation). The response only serialises fixed String keys so no custom naming strategy or module is needed — explained in an inline comment.Also corrected the ADR cross-reference in the CSRF comment from
ADR-020(Spring Session JDBC) toADR-022.→ commit
c3d1bea6Tobias — node-local cache comment +
checkAndConsumesequential fixConcern 1 — node-local comment
Added the "node-local, multiply by replica count" comment to
LoginRateLimiter.→ commit
c32607e1Concern 2 — parallel
checkAndConsumephantom token consumptionChanged
checkAndConsumefrom parallel to sequential: consumeipEmailfirst, thenip. If the IP bucket is exhausted, refund theipEmailtoken so IP-level blocking does not silently erode per-email quota. Added two tests covering both the isolation behaviour and the refund invariant:different_email_from_same_ip_not_blocked_by_sibling_email_exhaustionip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attempts→ commit
c32607e1Markus — missing ADR + sequence diagram not updated
Blocker 1 — ADR for CSRF / rate limiting / session revocation
Created
docs/adr/022-csrf-session-revocation-rate-limiting.mdcovering:CsrfTokenRequestAttributeHandler(non-XOR) is correct for SPAsrevokeOtherSessionsvsrevokeAllSessionssemantics→ commit
1052295aBlocker 2 — sequence diagram out of date
Updated
docs/architecture/c4/seq-auth-flow.pumlfrom Phase 1 to Phase 2, adding:revokeOtherSessions— caller stays logged in)revokeAllSessions— all sessions wiped)→ commit
05ab8b13Leonie — login page accessibility
Concern 1 — missing
role="alert"on regular error divAdded
role="alert"so screen readers announce the error when it appears dynamically.Concern 2 — clock icon
text-ink-3instead oftext-red-600Changed the SVG clock icon class to
text-red-600— matches the surrounding rate-limit error text.Bonus fix (surfaced by svelte-autofixer): removed
aria-invalid="true"from the rate-limit alert<div>— not a permitted ARIA attribute onrole="alert".→ commit
a23fa4c6All 10 planned tasks completed ✅. Backend test suite and login page unit tests green.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
Strong overall. The CSRF architecture is correct for a SPA, session revocation is properly scoped, and the token-refund logic in the rate limiter is a thoughtful detail. Two concerns need attention before this ships.
Blockers
1. Silent rate-limiter bypass via
@Autowired(required = false)AuthService.javainjectsLoginRateLimiterwithrequired = false:And then guards every call with
if (loginRateLimiter != null). IfLoginRateLimiterorRateLimitPropertiesfails to wire (misconfiguration, missing dependency), the application starts successfully and silently skips all rate limiting. An attacker who provokes a wiring failure (or a developer who misconfiguresapplication.yaml) gets unlimited login attempts with no error.Fix: Add an
@EventListener(ApplicationReadyEvent.class)or@PostConstructin a non-test config that assertsloginRateLimiter != nulland fails startup if it isn't. Or make the injection required and inject a no-opLoginRateLimiterin test context via@TestConfiguration.Concerns (non-blocking)
2.
XSRF-TOKENfallback generates a fresh UUID per requesthooks.server.ts:When the
XSRF-TOKENcookie is absent (first-time visitor, cleared cookies), the server generates a random UUID, stuffs it into both theCookie: XSRF-TOKEN=…andX-XSRF-TOKEN:header, and the backend accepts it (both sides match). This is technically valid for the double-submit pattern—the security guarantee comes from SameSite + the fact that an attacker can't forge both cookie and header from a cross-origin context.However, this means the CSRF token is never validated against a server-set secret—it's purely a consistency check between two client-side values. The security still holds for same-site SvelteKit SSR flows where all API calls go through
handleFetch. But if any client-side (browser) fetch skipshandleFetch, there's no CSRF injection at all. Worth a comment in the code explaining why the fallback is safe.3. Audit events for
ADMIN_FORCE_LOGOUTlack actor User-AgentUserController.forceLogout()logs:The actor IP and UA are not logged. For an admin operation like force-logout, knowing the actor's IP is valuable during incident review.
AuthSessionController.logout()logs IP+UA;forceLogout()should do the same.What's done well
CsrfTokenRequestAttributeHandler(non-XOR mode) is the correct choice for a SPA — XOR mode breaks when the token is read before the response body is flushed. ✅refillGreedyin Bucket4j +expireAfterAccessin Caffeine are correctly paired: Bucket4j tracks the refill window; Caffeine reclaims memory for idle keys. ✅@WebMvcTestCSRF tests use.with(csrf())across all 40 changed test files — no regression gaps. ✅JdbcIndexedSessionRepository.findByPrincipalName()which queries the actual session store, not an in-memory index. ✅LOGIN_RATE_LIMITEDis distinct fromLOGIN_FAILED— SIEM can alert on rate-limit events separately. ✅👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid implementation with excellent test coverage. One blocker on code style and a handful of smaller things.
Blockers
1.
UserControllerbreaks the project's constructor-injection standardUserController.javauses@AllArgsConstructorwith non-finalfields:Every other controller in this codebase uses
@RequiredArgsConstructor+finalfields. Non-final fields are mutable, invisible to Lombok's null-check, and break the immutability guarantee that makes constructor injection safe. Fix:Concerns (non-blocking)
2.
changePassword()silently discardsrevokedCountThe revoked count is logged but the response body is empty. The profile page can't show "2 other sessions were signed out" — a useful security confirmation for the user.
forceLogout()does returnrevokedCountin its body.changePassword()is@ResponseStatus(NO_CONTENT), but returning a 200 with{"revokedCount": n}would be a minor improvement.3.
AuthServicemixes injection styles without@NoArgsConstructorThe service has
@RequiredArgsConstructorfor the three required deps and@Autowired(required = false)for two optional ones. This is a documented workaround (PR description) for the Spring Boot 4 circular-dependency restriction. It works, but future maintainers will wonder why the pattern breaks here. The ADR-022 explains it — worth a// see ADR-022comment on those two fields.4.
hooks.server.ts—cookieParts.length === 0 && !xsrfTokencondition is unreachableBy the time this code is reached, either
sessionIdwas pushed tocookiePartsorxsrfTokenis set (for mutating requests). The!isPublicAuthApi && !sessionIdguard above already returned401. This condition can never be true — it's dead code. Remove it.What's done well
LoginRateLimiteris clean: one responsibility, well-named public methods (checkAndConsume,invalidateOnSuccess), privatenewBucketfactory. ✅RateLimitPropertiesvia@ConfigurationPropertieskeeps magic numbers out of production code. ✅LoginResultas a record is idiomatic Java 21. ✅.with(csrf())added to every mutating MockMvc test across all 40 files — methodical and thorough. ✅AuthSessionIntegrationTestcorrectly simulates the double-submit pattern (same UUID in Cookie and header). ✅truncateUa()at 200 characters prevents oversized user-agent strings from polluting audit logs. ✅🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
Architecture is sound: the three features are cohesive, correctly placed in the
authpackage, and the ADR is well-written. One documentation gap is a blocker per the project's doc-update policy.Blockers
1. L3 backend C4 diagram not updated for
LoginRateLimiterPer the architecture doc-update table: "New controller or service in an existing backend domain → update the matching
docs/architecture/c4/l3-backend-*.puml".LoginRateLimiterandRateLimitPropertiesare new components in theauthdomain. The diff includesdocs/architecture/c4/seq-auth-flow.puml(✅ good) but I don't seel3-backend-auth.pumlor equivalent being updated. Please verify and update the L3 component diagram to includeLoginRateLimiterand the newforceLogoutendpoint inUserController.If the L3 diagrams don't exist yet for the auth/user domains, this is an opportunity to create them — a blocker can be deferred to a follow-up issue, but that issue must be created and linked.
Concerns (non-blocking)
2.
@Autowired(required = false)weakens constructor-injection contractThe ADR-022 documents the circular-dependency workaround, which is appreciated. The concern is operational: a future Spring Boot upgrade that changes wiring order could silently drop the optional beans. A startup assertion (see Nora's comment) would make the failure loud rather than silent. The architectural principle is "fail loudly" — this should apply here.
3. Static
ObjectMapperinSecurityConfigis correct but deserves a note in the ADRThe ADR mentions the
ObjectMapperworkaround but the code comment (// @WebMvcTest slices do not include JacksonAutoConfiguration) is good. Consider adding a sentence in ADR-022 Consequences linking to this as a trade-off — it's an architectural decision that may surprise future maintainers who wonder whySecurityConfighas a static field.What's done well
seq-auth-flow.pumlis updated to reflect the new CSRF and session-revocation flows. ✅authpackage where login logic lives — correct domain placement. ✅changePasswordorchestration moved toUserController(notUserService) correctly avoids a circular dependency — the service owns its own persistence, the controller orchestrates cross-domain post-actions. Consistent with layering rules. ✅forceLogoutcorrectly usesauthService.revokeAllSessions()(all sessions) vschangePasswordusingrevokeOtherSessions()(keep current) — the distinction is intentional and correct. ✅🧪 Sara Holt — QA Engineer
Verdict: ⚠️ Approved with concerns
The breadth of test updates is impressive — 40 files, every mutating MockMvc call updated with
.with(csrf()). But two coverage gaps in the integration tests are worth fixing.Concerns (non-blocking)
1.
fetchXsrfToken()doesn't actually fetch a token from the serverAuthSessionIntegrationTest.fetchXsrfToken():This generates a random UUID without hitting the backend. The comment explains why this is valid for the double-submit pattern (cookie value == header value ✓). But this means no test verifies that:
Set-Cookie: XSRF-TOKEN=…on any responseSuggest adding one dedicated test:
2. No integration test for
changePassword+revokeOtherSessionsend-to-endAuthServiceTestunit-tests revocation with a mockedJdbcIndexedSessionRepository. But there's no integration test that:This is the most critical user-facing behavior of the session revocation feature. A unit test with mocks proves the service calls the repository — it doesn't prove the repository correctly removes session B from the database and that subsequent requests with B's cookie get 401.
3.
AdminControllerTest— verifyforceLogoutis tested with wrong permissionsUserControllerTestis in the diff. Please confirm it includes:POST /api/users/{id}/force-logoutwithoutADMIN_USERreturns 403POST /api/users/{id}/force-logoutunauthenticated returns 401These are the security boundary tests Nora would require — they're separate from the happy-path test.
What's done well
.with(csrf())on mutating calls — thorough and systematic. ✅LoginRateLimiterTestcovers: both bucket types, the refund path (IP-level blocking), andinvalidateOnSuccess. ✅AuthSessionIntegrationTestuses@SpringBootTest(webEnvironment = RANDOM_PORT)with real Postgres via Testcontainers — correct level for session lifecycle tests. ✅@BeforeEachclearsspring_sessiontable to prevent cross-test contamination. ✅session_expired_by_idle_timeout_returns_401backdatesLAST_ACCESS_TIMEdirectly via JDBC — cleaner than sleeping and avoids flakiness. ✅PasswordResetServiceTestis in the diff — confirms that password reset session revocation is also unit-tested. ✅⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Clean addition. No new infrastructure, no new Docker services, no new environment variables. Exactly what I want to see for a security hardening PR.
Suggestions (nice to have)
1. Bucket4j is outside the Spring Boot BOM — verify Renovate tracks it
pom.xmlpins Bucket4j to8.10.1:Since it's not version-managed by the Spring Boot BOM, it needs to be in Renovate's scope. Check the Renovate config (
renovate.json) to confirm it covers Maven dependencies — if it only watches Docker image tags, this version will drift.2.
rate-limit.login.*properties should have explicit entries inapplication.yamlRateLimitPropertieshas in-code defaults (10/20/15). That's fine for local dev. But for production tuning, make the properties visible inapplication.yaml(commented out with their defaults), so an operator knows these knobs exist without reading Java source:What's done well
bucket4j-corepinned to8.10.1— reproducible builds. ✅@ConfigurationProperties— production limits can be tuned without code changes. ✅@Autowired(required = false)on session repository matches the existing Spring Session test setup pattern — integration tests run without the session store, unit tests stay fast. ✅🎨 Leonie Voss — UI/UX Designer & Accessibility Strategist
Verdict: ✅ Approved
The login page changes are clean and accessible. The rate-limited error state is handled correctly with both visual (clock icon) and semantic (role=alert) cues. No blockers.
Suggestions (nice to have)
1. Expired-session banner:
aria-live="polite"has no effect on initial renderIn
+page.svelte, the expired-session banner uses:aria-liveonly announces dynamic DOM changes to screen readers — content that's present on initial page load is not re-announced. Since this banner is rendered on first paint (from the?reason=expiredURL parameter), thearia-liveattribute is effectively a no-op here. Screen readers will still read it during normal page traversal because ofrole="status", which is fine. Thearia-liveis harmless but misleading.2. Rate-limited error: color + icon alone distinguish it from regular errors
The rate-limited error shows a clock SVG and red text. The regular login error shows just red text (no icon). The visual distinction is meaningful to sighted users. For color-blind users, the clock icon shape provides the non-color cue — good. No change needed.
3. Submit button touch target — confirmed 44px minimum
min-h-[44px]✅ — meets WCAG 2.2 criterion 2.5.8. The full-width layout (w-full) ensures the target is large enough horizontally on any viewport. Good for the 60+ audience.What's done well
role="alert"on both the rate-limited and regular error divs — screen readers announce errors immediately without the user navigating to them. ✅aria-hidden="true"— purely decorative, the error text carries the meaning for screen readers. ✅autocomplete="current-password"on the password field — password managers can fill it. ✅autofocuson the email field — reduces friction on the login screen. ✅focus-visible:ring-2 focus-visible:ring-focus-ringon both inputs — keyboard users see a clear focus indicator. ✅role="status"(amber/warning color) and the error states userole="alert"(red/error color) — semantically correct distinction. ✅📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
All three security requirements from issue #524 are implemented and traceable to code. One ambiguity in the documented behavior vs. actual implementation needs correction.
Concerns (non-blocking)
1. "15-minute window" is ambiguous — Bucket4j uses greedy (continuous) refill, not a fixed window
The PR description, ADR-022, and the Gitea issue all state "10 attempts / 15 min". This reads as a fixed window (reset at the start of each period). The implementation uses:
refillGreedyis a sliding/continuous refill: tokens are added proportionally over the duration (10 tokens / 15 minutes = ~1 token per 90 seconds). After exhausting the bucket, an attacker regains 1 attempt every ~90 seconds, not all 10 at once after 15 minutes.This is actually stricter behavior than a fixed window — there's no "reset moment" to wait for. But the documentation describes the wrong mental model. Suggest updating ADR-022 to say: "10 attempts per 15-minute period, refilled continuously at ~1 token per 90 seconds."
2. Test plan in the PR description is manual — no automated E2E coverage for the rate-limit UX
The test plan checklist covers important behaviors (CSRF 403, session revocation, 429 + clock icon, force-logout, password reset). These are good acceptance criteria but are verified manually at PR time. The clock icon / 429 flow has no Playwright test. For a family archive accessed by a small trusted group, this is probably acceptable — adding a flag for the backlog:
3. Requirement completeness — all three NFRs from #524 are met
CookieCsrfTokenRepository+handleFetchhookrevokeOtherSessions()inchangePasswordrevokeAllSessions()inPasswordResetServicePOST /api/users/{id}/force-logoutLoginRateLimiterbucket ALoginRateLimiterbucket B+page.svelterate-limited brancherrors.ts+de/en/es.jsonupdatedWhat's done well
LOGIN_FAILED,LOGIN_RATE_LIMITED,LOGOUT,ADMIN_FORCE_LOGOUT— each event type is separately actionable for monitoring. ✅revokeOtherSessions(keep current) vsrevokeAllSessions(all sessions) correctly reflects the UX intent: password change keeps you logged in; password reset logs you out everywhere. ✅CSRF_TOKEN_MISSINGreturns{"code": "CSRF_TOKEN_MISSING"}— structured error code that the frontend can display and the requirements specified. ✅🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
The three security controls implemented here are all conceptually correct. The double-submit cookie pattern is the right choice for a session-cookie SPA. Bucket4j + Caffeine is a solid in-memory rate-limiter. Session revocation via
JdbcIndexedSessionRepositoryis the correct approach. The ADR is thorough. I have one concern I'd normally call a blocker but am flagging as a concern due to the single-VPS context, plus two minor findings.Concerns
1.
@Autowired(required = false)silently disables rate limiting if bean fails to loadAuthService.javauses@Autowired(required = false)for bothloginRateLimiterandsessionRepository. If either bean fails to load in production (misconfiguration, classpath issue), rate limiting and/or session revocation would silently no-op. The null-guards are correct for unit-test contexts, but production should fail loudly.Mitigation options:
@PostConstructcheck that warns at startup if the beans are null in a non-test profile2. Unauthenticated logout now returns 403, not 401 — breaking API contract change
CsrfFilterruns beforeAnonymousAuthenticationFilter, so a request toPOST /api/auth/logoutwith no CSRF token gets a CSRF rejection (403) before it even reaches auth. This is technically correct per the Spring Security filter chain ordering, but it breaks any client that tested for 401 on unauthenticated logout (e.g., scripts, mobile apps, integration harnesses). The comment in the test explains the mechanism — consider also noting this in the ADR as a consequence.3. No
Retry-Afterheader on 429 responsesDomainException.tooManyRequests()andGlobalExceptionHandlerreturn 429 without aRetry-Afterheader. RFC 6585 recommends it, and it's useful for clients to back off gracefully. The window is 15 minutes, soRetry-After: 900would be accurate. Not required for merge, but worth filing a follow-up issue.What's done well
CookieCsrfTokenRepository.withHttpOnlyFalse()+CsrfTokenRequestAttributeHandler(non-XOR) is exactly the correct configuration for an SPA — avoiding XOR mode prevents the deferred-loading token corruption issue.CSRF_TOKEN_MISSINGcustom error code in theAccessDeniedHandleris the right approach — clients get a structured JSON error, not a bare 403 HTML response.LoginRateLimiter.checkAndConsumetoken-refund logic (refundip:emailtoken when IP bucket is exhausted) is a nice correctness property, and the testip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attemptscorrectly exercises it.revokeOtherSessions(preserve current, change password) vsrevokeAllSessions(wipe all, password reset / force-logout).ERROR_WRITERinSecurityConfigis thread-safe —ObjectMapperis safe for concurrent serialization, and the comment explaining why it's static is exactly the kind of security comment that belongs here.LOGIN_RATE_LIMITED,ADMIN_FORCE_LOGOUT, and thereason/revokedCountfields inLOGOUTpayloads give operators full visibility.forceLogoutendpoint is correctly guarded with@RequirePermission(Permission.ADMIN_USER)and audited.🏛️ Markus Keller — Senior Application Architect
Verdict: 🚫 Changes requested
The implementation is architecturally sound. The design decisions (in-memory rate limiter, double-submit CSRF, session revocation via Spring Session JDBC) are all appropriate for the single-VPS context and are properly documented in ADR-022. The
seq-auth-flow.pumlupdate is good. However, the documentation table in my persona is unambiguous: new services in an existing domain require the matchingl3-backend-*.pumldiagram to be updated, and this PR ships two new components without updating the diagram. Additionally,CLAUDE.md's auth package table is stale. These are required doc updates before merge.Blockers
1.
l3-backend-3a-security.pumlstill describes CSRF as disableddocs/architecture/c4/l3-backend-3a-security.puml, line 12:This PR closes #524 and re-enables CSRF. The diagram still says CSRF is disabled. This must be corrected.
Additionally,
LoginRateLimiterandRateLimitPropertiesare new components in theauthdomain (the same package that already hasAuthService,AuthSessionController). Per the documentation requirement table:l3-backend-3a-security.pumlneeds to showLoginRateLimiterwith its relationship toAuthService.2.
CLAUDE.mdauth package entry is staleShould now include
LoginRateLimiter, RateLimitProperties. The LLM reminder comment at the top of the file reads: "LLM reminder: controllers never call repositories directly..." — keeping this table accurate is part of how future Claude sessions navigate the codebase correctly.Concerns (not blocking)
Circular-dependency workaround lifts auth logic into the controller
UserController.changePasswordnow orchestratesuserService.changePassword()+authService.revokeOtherSessions()+auditService.log(...). The PR notes this was deliberate to avoid a circular dependency (UserService↔AuthService). This is an acceptable workaround — Spring Framework 7 prohibits constructor injection cycles, and breaking the cycle by putting orchestration in the controller is the right call. ADR-022 documents the reason. I'd only flag it if it starts accumulating more orchestration logic.What's done well
seq-auth-flow.pumlfully updated to Phase 2 with rate limiting, CSRF bootstrap, password change/reset revocation, and the force-logout flow.LoginRateLimitercomment noting the multi-replica limitation is exactly the right kind of documentation.RateLimitPropertiesas a@ConfigurationPropertiescomponent with defaults inapplication.yamlis clean — ops can override without a redeploy.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Clean implementation overall. The code is well-structured, test names are descriptive, and the TypeScript side handles the new error codes correctly. I have a few suggestions, none blocking.
Suggestions
1.
checkAndConsumeis non-atomic between the two bucket checksBetween
tryConsumeonbyIpEmailandtryConsumeonbyIp, another thread can observe thebyIpEmailtoken as consumed before the refund. For a single-VPS in-memory implementation this is an acceptable race — nobody is going to exploit the ~nanosecond window — but it's worth noting in a comment so the next developer doesn't think the token-accounting is atomic. The testip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attemptscovers the logic correctly.2. The condition
if (cookieParts.length === 0 && !xsrfToken)inhandleFetchis unreachable in practiceBy the time we reach this check:
xsrfTokenis null only for GET/HEAD (non-mutating) requestscookiePartsis empty only whensessionIdis nullsessionIdis null only whenisPublicAuthApiis trueThis guard is defensive but it slightly obscures the flow. Not a bug, just a readability note.
3. Svelte component: the
rateLimitedbranch check is slightly redundantThe outer
form?.erroris already truthy whenrateLimitedis set (the server always returns{ error: ..., rateLimited: true }). Theform?.rateLimitednested check is fine; just minor to note the outer guard does the heavy lifting.What's done well
LoginRateLimiteras a standalone@Serviceis the right decomposition — keepsAuthServicefocused on authentication, rate limiting is a separate concern.RateLimitPropertieswith@ConfigurationPropertiesand sensible defaults is textbook config externalization.@BeforeEach+ReflectionTestUtils.setFieldapproach inAuthServiceTestto inject@Autowired(required = false)fields is the correct test pattern for optional beans.fetchXsrfToken()helper inAuthSessionIntegrationTestcorrectly documents the double-submit contract: "CookieCsrfTokenRepository validates thatCookie: XSRF-TOKEN=XmatchesX-XSRF-TOKEN: X. By supplying both with the same value we simulate exactly what a browser does.".with(csrf())— no forgotten endpoints.errors.tsupdated: newErrorCodeunion members,getErrorMessage()cases, and i18n keys in all three languages (de/en/es) — nothing left dangling.🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
The backend test coverage for this PR is excellent.
LoginRateLimiterTestcovers 6 scenarios including the non-obvious token-refund edge case.AuthServiceTestadds 8 new tests for rate limiting and session revocation. Integration tests are updated with correct CSRF token helpers. All existing@WebMvcTestslices now correctly use.with(csrf()). However, the frontend Svelte component changes have no automated coverage.Concerns
1. The rate-limited clock-icon UI state has no Vitest component test
+page.sveltenow renders a clock icon withrole="alert"whenform?.rateLimitedis true. This is a meaningful new UI state with different markup (icon + flex layout) vs the normal error div. There's a newpage.server.test.tstest confirming the server action returnsrateLimited: true, but no component-level test verifying that the Svelte template actually renders the clock icon branch.A minimal test would be:
This isn't blocking since the server-side logic is tested and the template is straightforward, but it's a gap in the test pyramid — UI state changes should have component-level coverage.
2. Behavior change: unauthenticated logout now returns 403 — no regression test outside the test suite update
The test
logout_without_session_returns_403(formerlylogout_returns_401_when_not_authenticated) documents the correct new behavior. Good. What's missing is a comment or linked issue for any downstream code (SvelteKit+page.server.tslogout action) that may be checking for 401 from this endpoint. A quick grep for401around the logout handling in the frontend would confirm nothing is silently swallowing the wrong status.What's done well
LoginRateLimiterTestis excellent: it covers the happy path (10 attempts succeed), the 11th-attempt rejection, success-reset invalidation, the IP-level backstop, cross-email isolation, and the token-refund correctness. That last test (ip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attempts) is exactly the kind of "implementation detail encoded as a regression test" I want to see for security-sensitive code.PasswordResetServiceTest.resetPassword_revokes_all_sessions_after_password_resetverifies that the session revocation hook fires at the correct lifecycle point (after token validation, after password update).UserControllerTesthas 4 new tests forforceLogoutcovering the happy path, 401, 403, and 404 cases.AuthSessionIntegrationTestcorrectly refactored withfetchXsrfToken()andcsrfAndSessionHeaders()helpers — the test helpers document the protocol contract, not just satisfy the test runner.AuthServiceTestfor session map setup:new HashMap<>()with put() calls — simple, readable, no magic.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes required, no new Docker services, no new ports. The implementation stays entirely within the existing single-VPS operational model. One thing to verify regarding dependency management.
Concerns
Bucket4j dependency not managed by a BOM — verify Renovate picks it up
Bucket4j isn't part of the Spring Boot BOM, so the version is hardcoded. Renovate should pick this up as a Maven dependency update, but verify it's not excluded by any
ignoreDepsrule inrenovate.json. If Renovate isn't configured for the Gitea instance, this version could drift without alerts — Bucket4j 8.x is active, so security patches will come.Caffeine is already a Spring Boot managed dependency (no version pinned) — Bucket4j should follow the same pattern or be explicitly tracked.
What's done well
LoginRateLimitercomment explicitly documents the multi-replica limitation — this is exactly what I want to see so a future ops engineer doesn't discover it by accident.RateLimitPropertieswithapplication.yamldefaults means the limits can be tuned via environment variable overrides without a code change. Production tuning is an ops operation, not a dev one.spring_sessiontable, and rate limiting is in-memory. Thedocker-compose.ymldiff is zero.expireAfterAccesson the Caffeine cache means idle IP buckets are garbage-collected automatically. No memory leak over time.application.yamlrate-limit defaults (10/15min per ip+email, 20/15min per IP) are reasonable conservative values. False-positive risk on NAT/VPN IPs at the IP level is mitigated by the looser 20-attempt backstop.📋 Elicit — Senior Requirements Engineer
Verdict: ✅ Approved
All three requirements from issue #524 are implemented and traceable. The ADR-022 provides the requirements rationale. I reviewed the acceptance criteria in the PR description against the implementation.
Traceability check
CookieCsrfTokenRepository+ customAccessDeniedHandlerrevokeOtherSessions(currentSessionId, principal)inUserController.changePasswordLoginRateLimiter+ login page server ++page.svelteclock iconPOST /api/users/{id}/force-logout+revokeAllSessionsPasswordResetService.resetPasswordcallsrevokeAllSessionsObservations
1. CSRF requirement on
/api/auth/login— satisfiable but asymmetric first-visit behaviorADR-022 states "Login... are not CSRF-exempt — the XSRF-TOKEN cookie is set on the first GET to the login page." But the XSRF-TOKEN cookie is set by the Spring backend, which is only reached after the initial page load. On the very first visit, when the browser has no XSRF-TOKEN,
handleFetchgenerates a freshcrypto.randomUUID()and sends it as both cookie and header. This is functionally correct (double-submit validates match, not server knowledge), but it differs from the ADR's stated justification. The actual mechanism is: "frontend generates a random token, sends it as both cookie and header — backend validates they match." Worth clarifying in the ADR for accuracy.2.
PUBLIC_API_PATHSlist needs maintenanceThis list is now defined at module scope (moved out of the
if (isApi)block — a nice refactor). However, it has no test coverage and no clear "owner" beyond the source file. If a new public auth endpoint is added (e.g.,/api/auth/magic-link), forgetting to update this list would silently inject session cookies into unauthenticated requests. Worth noting inCONTRIBUTING.mdor with an inline comment.What's done well
rateLimitedflag in the form action response correctly separates the semantics: rate-limited errors get a different visual treatment (clock icon) from regular auth errors. This is the right UX distinction.AuditKind.LOGIN_RATE_LIMITEDis added with a payload documentingipandemail— giving operators the data needed to investigate abuse patterns without exposing passwords.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
The rate-limit UI change is small but well-considered. The two main accessibility improvements (adding
role="alert"to both error branches, adding the clock icon witharia-hidden) are correct. One contrast note to be aware of.Observations
1.
text-red-600contrast — just barely AA, watch for degradationThe clock icon uses
stroke="currentColor"and the container isclass="text-red-600". On a white background,#dc2626(red-600) yields approximately 4.56:1 contrast — this passes WCAG AA for text (minimum 4.5:1) but is right at the threshold. Any future change that shifts the background slightly off-white (e.g., adding a card wrapper, a subtle tint) could push this below AA.This isn't a blocker — it passes today — but it's the kind of value that benefits from a comment:
/* text-red-600: 4.56:1 on white — AA pass, monitor on non-white backgrounds */. Or, usetext-red-700(#b91c1c, ~6.0:1) for a safer margin.2.
role="alert"added to both error paths — good catchThe previous code had no
role="alert"on the error div — screen readers would not announce login errors at all. Both branches now correctly userole="alert", which triggers live region announcement. This is a silent accessibility fix bundled into this PR that benefits all error paths.3. Clock icon + text = correct redundant cue
The clock SVG has
aria-hidden="true"(correct — the text conveys the message) and the surrounding<span>holds the readable error text. Color + icon + text together — this satisfies the "never color alone" requirement for our 60+ audience.What's done well
<svg>usesstroke-linecap="round"andstroke-linejoin="round"which gives the icon a softer look consistent with the rest of the UI's icon set.class="h-4 w-4 shrink-0"—shrink-0prevents the icon from collapsing in flex context on narrow viewports. Correct.M12 6v6h4.5m4.5 0a9 9 0 1 1-18 0 9 9 0 0 1 18 0Z) is the Heroicons "clock" outline — consistent with other icons used in the project.✅ Architect blockers resolved
Addressed both concerns raised by @markus in the review.
docs/architecture/c4/l3-backend-3a-security.puml—314f686secFilterdescriptionsecFilterto describe the enabled double-submit cookie pattern (CookieCsrfTokenRepository.withHttpOnlyFalse+CsrfTokenRequestAttributeHandler) and the customAccessDeniedHandlerreturning{"code":"CSRF_TOKEN_MISSING"}LoginRateLimitercomponent with description of dual Bucket4j/Caffeine bucketsRateLimitPropertiescomponent with@ConfigurationPropertiesbinding descriptionAuthService → LoginRateLimiter,AuthService → sessionRepo(revocation),LoginRateLimiter → RateLimitPropertiesfrontend → secFilterrel label to showX-XSRF-TOKEN headersessionRepodescriptionCLAUDE.md—bdc37b11LoginRateLimiter, RateLimitPropertiesadded alongside existing components🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
This PR is architecturally clean. The three security concerns — CSRF, session revocation, and rate limiting — are implemented at the right layers with the right amount of complexity for a single-VPS deployment.
What's done well
@Autowired(required = false)forJdbcIndexedSessionRepositoryandLoginRateLimiteris the correct workaround whenJdbcHttpSessionAutoConfigurationdoesn't fire in@WebMvcTestslices. This is documented in the PR description, which is the right place for it.UserController(rather thanUserService) avoids a circular dependency betweenUserServiceandAuthService. This is consistent with the project's layering rules.NOTEcomment inLoginRateLimiter.javaexplicitly documents the node-local limitation and why it's acceptable for the current single-VPS topology. This is exactly the kind of comment that prevents future developers from "fixing" something that isn't broken.seq-auth-flow.puml,l3-backend-3a-security.puml, andCLAUDE.mdare all updated. The Spring Session JDBC tables (spring_session*) are framework-owned and correctly excluded from the DB diagrams.Concerns (non-blocking)
@Autowired(required = false)deviates from the constructor injection convention. The project standard is@RequiredArgsConstructorwithfinalfields. The current approach works but leavesAuthServicewith a mixed injection model (constructor for required deps, field injection for optional deps). This is acceptable here — the PR description documents the reason — but worth noting for future reviewers.CLAUDE.mdpackage table update is minimal. Theauth/entry now listsLoginRateLimiterandRateLimitProperties, which is correct. No other doc table needs updating becauseforce-logoutuses the existingADMIN_USERpermission and no new DB tables are added (Spring Session's tables are pre-existing).The static
ERROR_WRITERinSecurityConfig. The comment is clear and the workaround is correct. Aprivate static final ObjectMapperwith a fixed schema is safe. No concern.Verdict
All doc requirements for this PR type are met. The architecture is correct. Approved.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Solid implementation with comprehensive tests. A few things I'd flag for awareness.
What's done well
LoginRateLimiteris clean.checkAndConsume,invalidateOnSuccess,newBucket— each method does one thing and is under 20 lines. The token-refund logic on IP-level blocking prevents phantom quota consumption, and the corresponding testip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attemptsis the best test in this PR — it documents a subtle correctness requirement.AuthServiceTestBeforeEachpattern usingReflectionTestUtils.setFieldfor@Autowired(required = false)fields is the right way to test this injection pattern.role="alert"added to both the rate-limited and generic error divs. The pre-existing error div was missing this attribute. Good catch fixed alongside the feature..with(csrf())is mechanically correct. Every affected controller test now includes CSRF tokens on mutating requests.Concerns
@Autowired(required = false)breaks the@RequiredArgsConstructor+finalconvention (project rule).AuthServicenow has a mixed injection model: constructor-injectedfinalfields alongside mutable@Autowiredfield-injected fields. Thenull-checks throughoutrevokeOtherSessions,revokeAllSessions, and theloginmethod are a direct consequence. This is architecturally documented (PR desc + ADR-022) but it makesAuthServiceharder to reason about:The null-guards are minimal and consistent, so this is not a blocker — just a heads-up for future maintainers.
hooks.server.ts— theif (cookieParts.length === 0 && !xsrfToken)guard is accurate but reads confusingly. In practice this branch is only hit whenisPublicAuthApi && !isMutating(a read to a public auth endpoint). A named boolean would make this clearer:Minor — not a blocker.
Missing test:
changePassworddoes not have aUserControllerTestcase verifying thatauthService.revokeOtherSessionsis called. TheforceLogoutendpoint has full coverage (4 tests).changePasswordgot the session revocation call added to it but the test file doesn't include a case assertingverify(authService).revokeOtherSessions(...). This is a testing gap (Sara will likely flag this too).Verdict
The implementation is correct and well-tested. The two concerns above are minor — one is a known Spring Framework constraint, the other is a missing test case. Approved.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is a well-designed security feature. The CSRF implementation is correct, session revocation is thorough, and the rate limiter has a clever correctness fix that most implementations miss. One security smell worth flagging.
CSRF — Correct Implementation
CookieCsrfTokenRepository.withHttpOnlyFalse()+CsrfTokenRequestAttributeHandleris the right combination for Spring Security 6+ with a JavaScript frontend.CsrfTokenRequestAttributeHandler(vs.XorCsrfTokenRequestAttributeHandler) is the correct choice here — the XOR variant was introduced to mitigate BREACH on server-rendered forms, but we're using the double-submit cookie pattern where the token never appears in the response body.The
handleFetchfallback is sound. When noXSRF-TOKENcookie exists yet (e.g., the very first mutating request from a new browser session),crypto.randomUUID()generates a token, which is then sent as both theCookie: XSRF-TOKEN=<uuid>andX-XSRF-TOKEN: <uuid>header. The backend validates that they match — they do. On the backend's response,CookieCsrfTokenRepositorywill set a properXSRF-TOKENcookie, which subsequent browser requests will include./api/auth/loginis inPUBLIC_API_PATHSbut still receives CSRF protection. BecauseisMutating = truefor a POST,xsrfTokenis computed and injected. This is correct — CSRF protection on the login endpoint is important to prevent pre-authentication token fixation scenarios.The custom
accessDeniedHandlercorrectly distinguishes CSRF failures from permission denials usinginstanceof CsrfException. The structured{"code":"CSRF_TOKEN_MISSING"}response is clean and consistent with the project's error contract.Session Revocation — Correct Implementation
revokeOtherSessionscorrectly excludescurrentSessionId— the user doesn't get logged out of their own session after changing their password. ✓revokeAllSessionsis used for password reset and force-logout — correct, these are absolute revocations. ✓findByPrincipalName(email)works correctly because Spring Session stores the principal name as the value fromUserDetails.getUsername()(the user's email). ✓Minor reliability note (not a security issue): In
PasswordResetService.resetPassword,resetToken.setUsed(true)is saved beforeauthService.revokeAllSessions(...)is called. If session revocation fails (e.g., database error), the password is changed and the token is invalidated, but old sessions persist until they expire naturally. This is the safer failure mode — the credential change still takes effect.Rate Limiting — One Security Smell
Email case-sensitivity bypass (low severity, single-VPS context). The rate limiter key is
ip + ":" + emailwhereemailis the raw value from the login request body. An attacker could enumerate case variations to get multiple independent per-email buckets:The per-IP backstop (20 attempts total) limits the damage to at most 2× the per-email limit before the IP is blocked. For a family archive with no public registration, this is low-impact — targeted credential stuffing against known email addresses is the realistic threat. But the fix is trivial:
This doesn't need to be a blocker for merge given the deployment context, but I'd create a follow-up issue.
Detection: The existing tests would catch a regression on this if a test were added for
user@EXAMPLE.COMvsuser@example.com. Currently there's no such test.Audit Trail
LOGIN_RATE_LIMITED✓ — IP and email logged, password never logged (convention preserved)ADMIN_FORCE_LOGOUT✓ — actor ID + target user ID + revoked countLOGOUT✓ — reason field distinguishes password_change, password_reset, admin_force_logoutComprehensive audit coverage for a security-significant feature.
Verdict
Approved. The email normalization issue is a real but low-severity bypass for this deployment context. I'd open a follow-up issue rather than blocking merge.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
The test suite is broad and generally thorough. The rate limiter unit tests are excellent. Two gaps worth addressing before or shortly after merge.
What's done well
LoginRateLimiterTestis exemplary. 8 test cases covering:The
ip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attemptstest is particularly valuable — it's testing a subtle invariant that would be invisible without a deliberate regression test.CSRF test in
AuthSessionControllerTest.authenticated_post_without_csrf_token_returns_403_CSRF_TOKEN_MISSINGis the right test at the right layer. It verifies both the status code (403) and the error code structure ($.code). The renamedlogout_without_session_returns_403test has an inline comment explaining theCsrfFilterordering — exactly the kind of documentation that prevents someone from "fixing" the test when it breaks.Comprehensive
.with(csrf())update. Every mutating endpoint across 14 controller test files was updated. This is mechanical but necessary, and it was done completely.PasswordResetServiceTest.resetPassword_revokes_all_sessions_after_password_resetcorrectly usesverify(authService).revokeAllSessions("user@example.com"). Good. ✓page.server.test.ts429 case — testsrateLimited: truein the response data. ✓Gap 1 — Missing test:
changePassworddoes not verify session revocation is called (Blocker)UserController.changePasswordnow callsauthService.revokeOtherSessions(session.getId(), authentication.getName()), butUserControllerTesthas no test asserting this call happens. TheforceLogoutendpoint has 4 tests;changePasswordhas zero new tests covering the revocation side-effect.Suggested test:
Without this test, someone could silently remove the
revokeOtherSessionscall and no test would fail.Gap 2 — No integration test for password-change-revokes-other-sessions (Suggestion)
AuthSessionIntegrationTesthas a good flow for testing that logout invalidates a session. A parallel test verifying that changing a password invalidates other sessions would close the most important real-world scenario for this feature. This is a suggestion rather than a blocker, but it would be the highest-value test to add in a follow-up.Integration test CSRF setup
AuthSessionIntegrationTest.fetchXsrfToken()generates a random UUID and supplies it as both theCookie: XSRF-TOKEN=andX-XSRF-TOKEN:header. This correctly simulates the double-submit cookie pattern. The backend validates that cookie and header match — they do. ✓Verdict
The missing
changePassword→revokeOtherSessionsassertion is a real gap — it's the only path through the new code that has no test verification of the session revocation call. Everything else is well-covered. I'd add that test before merge, or at minimum immediately after.🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure concerns. This PR stays within the existing VPS footprint.
What's done well
bucket4j-core:8.10.1is version-pinned. ✓ Worth adding to the<dependencyManagement>block inpom.xmlfor consistency with other pinned versions, but not a blocker.application.yaml.max-attempts-per-ip-email: 10,max-attempts-per-ip: 20,window-minutes: 15are all sensible defaults and can be overridden per-environment viaapplication-prod.yamlor environment variables if needed. No secrets involved.One note for production operation
The in-memory rate limiter state is lost on application restart. This is acceptable for the current single-VPS setup but means:
If the VPS is ever horizontally scaled (multiple backend instances behind a load balancer), the rate limiter would need to be moved to a shared store (Redis). ADR-022 should capture this scaling trigger explicitly if it hasn't already — it may already be in the ADR, I haven't read it in full.
Monthly cost impact
Zero. No new services. Current estimate (~23 EUR/month) is unchanged.
Verdict
No infrastructure changes required. Approved.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
All requirements from issue #524 are implemented. The test plan from the PR description maps 1:1 to test cases. One small gap in the acceptance criteria trail.
Requirements coverage
CookieCsrfTokenRepository.withHttpOnlyFalse()(double-submit)X-XSRF-TOKENinjected by SvelteKithandleFetchon mutating requests403 {"code":"CSRF_TOKEN_MISSING"}POST /api/users/{id}/force-logout,ADMIN_USERpermission)429 TOO_MANY_LOGIN_ATTEMPTSPR test plan verification
All 5 items from the PR's test plan have corresponding test coverage:
authenticated_post_without_csrf_token_returns_403_CSRF_TOKEN_MISSING✓PasswordResetServiceTestandAuthServiceTest(unit level); no integration test ⚠️eleventh_attempt_from_same_ip_email_throws_TOO_MANY_LOGIN_ATTEMPTS✓forceLogout_returns200_and_revokes_target_sessions✓ (unit level; integration test would be ideal)resetPassword_revokes_all_sessions_after_password_reset✓i18n completeness
error_csrf_token_missing— added tode.json,en.json,es.json✓error_too_many_login_attempts— added to all three languages ✓ErrorCodetype inerrors.ts— both new codes added ✓getErrorMessage()switch — both cases handled ✓One gap: "successful login clears the bucket" is not covered at the integration level
The unit test
login_invalidates_rate_limit_on_successverifies the call toinvalidateOnSuccess. But there's no integration test that verifies a user who was approaching the limit can successfully log in again after a correct password entry. This is a low-priority follow-up item since the unit test gives strong confidence, but the end-to-end acceptance criterion from the issue isn't fully exercised at the integration layer.Non-functional requirements
Verdict
All must-have requirements from issue #524 are implemented and tested. Approved.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
The login page rate-limit UI is clean and accessible. Two pre-existing accessibility fixes were bundled in — both are improvements.
Rate-limited error state (new)
The rate-limited error div:
role="alert"— announces the message to screen readers immediately on insertion. ✓aria-hidden="true"on the SVG — the icon is decorative; the text carries the meaning. ✓M12 6v6h4.5m4.5 0a9 9 0 1 1-18 0 9 9 0 0 1 18 0Z) is the standard "clock" symbol — appropriate for a time-based rate limit that the user needs to wait out. ✓text-red-600is the project's conventional error color. The icon also usestext-red-600viastroke="currentColor"— consistent. ✓shrink-0on the icon prevents it from collapsing when the text is long. ✓One question: The i18n string for
error_too_many_login_attempts— does it include a "please wait X minutes" hint? If the message says only "Too many login attempts" without telling the user what to do next, it fails Nielsen Heuristic 9 (help users recognize, diagnose, and recover from errors). If the translated string includes "Please wait 15 minutes and try again," that's good. This doesn't require a code change — just a string check.Generic error state (pre-existing fix)
The existing error div gained
role="alert":This was a pre-existing accessibility gap — screen readers wouldn't announce the error message when the form returned an error. Adding
role="alert"here is a correct fix, even though it's not directly related to the rate-limiting feature. ✓No UI changes elsewhere
This PR doesn't touch any other pages. The admin force-logout endpoint is backend-only with no frontend UI (the force-logout is invoked via the API). If there's a future admin UI panel for user management, it should include the force-logout button with a confirmation dialog.
Touch targets
The login page button sizes are unchanged. No regression.
Verdict
Accessible, consistent with the brand, and uses the correct ARIA pattern. The only open question is the quality of the rate-limit error message string in the three languages — worth a quick read of
messages/de.jsonto confirm it tells the user what to do next.Review concerns addressed — commit
8189e14a✅ @nora — Email case-sensitivity bypass (security)
Concern:
LoginRateLimiter.checkAndConsume()used the raw email string as the cache key, soUser@EXAMPLE.COManduser@example.comlanded in separate Bucket4j buckets — a trivial bypass of the per-email rate limit.Fix: Both
checkAndConsume()andinvalidateOnSuccess()now normalise the email withemail.toLowerCase(Locale.ROOT)before building the cache key. The token-refund path and the invalidation path both use the same derivedkeyvariable, keeping them in sync.Tests added (
LoginRateLimiterTest):email_lookup_is_case_insensitive_so_mixed_case_shares_the_same_bucket— exhausts the bucket via mixed-case email, then verifies lowercase variant is also blockedinvalidateOnSuccess_is_case_insensitive_so_mixed_case_clears_the_bucket— exhausts bucket via lowercase, invalidates via mixed-case, verifies the bucket is clearedAll 1643 backend tests green after this change.
🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
Solid implementation with the right documentation. A few structural observations.
✅ What's done right
ADR-022 is complete. It covers context, decision, alternatives, and consequences — including the non-obvious refund rationale for the dual-bucket rate limiter and the static
ObjectMapperconstraint. This is exactly what an ADR should capture.Documentation matrix fully satisfied:
seq-auth-flow.pumll3-backend-3a-security.pumlCLAUDE.mdpackage tableErrorCode/Permissionvaluesspring_session*)Circular-dependency resolution (
changePasswordorchestration inUserControllerinstead ofUserService) is the right call for Spring Boot 4 / Spring Framework 7 which fully prohibits constructor injection cycles. The PR notes explain this explicitly.Feature package placement is correct:
LoginRateLimiterandRateLimitPropertieslive inauth/, not in a sharedsecurity/package.⚠️ Concerns (non-blocking)
UserControlleruses@AllArgsConstructorwith mutable (non-final) fields — the rest of the codebase uses@RequiredArgsConstructor+finalfields. SinceUserControlleris pre-existing, and the circular-dep workaround movedAuthServiceinto it, it's understandable, but it diverges from the project's injection style. Consider a follow-up to align this.AccessDeniedHandlerstaticObjectMapper— the ADR correctly explains why this is necessary for@WebMvcTestslices. The comment inSecurityConfig.javais good. No action required, but future readers should know the comment refers to ADR-022.💡 Suggestion
The ADR mentions "node-local cache multiplied by replica count" as a known consequence of the in-memory rate limiter. Since this is single-VPS, no action needed — but consider adding a
TODOcomment inLoginRateLimiterlinking to the ADR paragraph, so future horizontal scaling attempts don't miss this constraint. (The current comment is there — ✅ already addressed.)👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Clean implementation overall. One blocker on code style, a few suggestions.
🚫 Blocker
UserControlleruses non-finalfields +@AllArgsConstructor(UserController.java, all injected fields). Every other service and controller in this project uses@RequiredArgsConstructorwithfinalfields. Non-final injected fields allow partially-constructed objects in tests and violate the pattern. The circular-dep fix movedAuthServiceintoUserController, but that doesn't require droppingfinal:@AllArgsConstructorgenerates a constructor that includes ALL fields, and@RequiredArgsConstructorgenerates one forfinalfields. Both work for injection, but the project convention is the latter.✅ What's clean
LoginRateLimiter— single responsibility, under 20 lines per method, intent-revealing names. ThecheckAndConsume/invalidateOnSuccesssplit is clean.AuthService.login()— orchestrates cleanly: rate-check → authenticate → audit → clear bucket. Each step is clear.revokeOtherSessions/revokeAllSessions— guard at the top (if (sessionRepository == null) return 0;), happy path follows. Good pattern.LoginRateLimiter.newBucket()static factory — clean, reusable, properly extracts construction logic.Svelte login page —
form?.rateLimitedflag drives the conditional icon rendering without business logic in the template. ✅💡 Suggestions (non-blocking)
AuthService.revokeOtherSessionsloop — minor style: theforloop works, but the stream form is idiomatic Java 21:UserController.changePasswordaudit usesAuditKind.LOGOUT— the audit event says LOGOUT but the reason ispassword_change. This is slightly misleading. ASESSION_REVOKEDorPASSWORD_CHANGEDkind would be more accurate. Pre-existing enum values may constrain this — but ifAuditKind.LOGOUTis the only available option, add a note in a follow-up.+page.server.tslogin action — error handling for 401 parses the JSON body manually inline. SinceparseBackendError()exists inerrors.ts, consider using it:Note on
@RequirePermissionabsence onPOST /users/me/passwordCLAUDE.md says
@RequirePermissionis required on all POST/PUT/PATCH/DELETE endpoints.changePasswordandupdateProfiledon't have it. This is pre-existing (not introduced by this PR) and arguably correct for self-service endpoints. The newforceLogoutendpoint correctly has@RequirePermission(Permission.ADMIN_USER). ✅🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Well-executed implementation of three distinct security controls. The threat model is clearly understood. A few points worth discussing.
✅ Security controls verified
CSRF (CWE-352)
CookieCsrfTokenRepository.withHttpOnlyFalse()— correct for the double-submit cookie pattern. The cookie is readable by JS (intentional), not sent cross-origin.CsrfTokenRequestAttributeHandler(non-XOR mode) — correct for SPAs where deferred token loading in XOR mode would corrupt the token value. The ADR explains this.AccessDeniedHandlercorrectly distinguishes CSRF failures (CsrfException) from permission failures (FORBIDDEN) and returns structured JSON. ✅@Order(1)) disables CSRF for/actuator/**— correct, because actuator uses Basic auth (not session cookies) and is on a separate internal port. ✅hooks.server.tshandleFetchinjects bothCookie: XSRF-TOKEN=<token>ANDX-XSRF-TOKEN: <token>on mutating requests — both sides of the double-submit are present. The fallbackcrypto.randomUUID()works because both the cookie and header are set to the same generated value, satisfying the match requirement. ✅Session fixation (CWE-384)
SessionAuthenticationStrategybean usesChangeSessionIdAuthenticationStrategy, which rotates the session ID on login via Servlet 3.1'schangeSessionId(). ✅Session revocation on credential change
revokeOtherSessions(keeps current session) ✅revokeAllSessions(unauthenticated flow, no session to keep) ✅revokeAllSessions✅Rate limiting (CWE-307)
Locale.ROOTfor email normalization — prevents bypassing the bucket via case variation. ✅LOGIN_RATE_LIMITED. ✅⚠️ Security observations (non-blocking)
IP extraction trust and X-Forwarded-For spoofing (Medium confidence)
application.yamlsetsserver.forward-headers-strategy: native, which trustsX-Forwarded-Forfor IP extraction. In a single-VPS + Caddy setup, Caddy appends the real client IP toX-Forwarded-For. However, if an attacker can reach the backend port directly (bypassing Caddy), they can spoof any IP and trivially bypass the per-IP rate limit.Mitigation: verify that Spring Boot's port (8080) is NOT exposed on the host (only
expose:notports:). The current docker-compose should already do this — confirm in the production compose file.Also consider using
RemoteIpFilterwithinternalProxiesorRemoteIpValveto restrict which proxy IPs are trusted, rather thannativewhich trusts all reverse-proxy headers.AuditKind.LOGOUTused for session revocation inchangePassword— minor semantic issue, not a security concern. The actual security control (session deletion) works correctly.nullcheck pattern onLoginRateLimiterinAuthService(@Autowired(required=false)) — if someone misconfigures Spring to not loadLoginRateLimiter, rate limiting silently skips. This is acceptable for test contexts (the design intent), but note that it means a misconfigured production context silently has no rate limiting. The risk is low since@Servicewithout@ConditionalOn*will always load.✅ Security tests verified
AuthSessionIntegrationTest✅.with(csrf())— this is the correct approach (tests now reflect production security constraints) ✅LoginRateLimiterTestpresent for the rate limiter logic ✅💡 One thing to add
Consider a test that verifies the 403 response body for CSRF failures actually contains
{"code":"CSRF_TOKEN_MISSING"}(the customAccessDeniedHandler). The controller tests use.with(csrf())which bypasses the handler — the handler path is only tested if you omit.with(csrf())on a mutating request. IfAuthSessionIntegrationTestcovers this, it's fine.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Good unit and integration test coverage for the new components. One blocker for the CSRF error body, one concern about the 401-redirect hook path.
🚫 Blocker
No test verifies the CSRF 403 response body is
{"code":"CSRF_TOKEN_MISSING"}.All controller tests now use
.with(csrf()), which bypasses the customAccessDeniedHandler. That means the handler atSecurityConfig.java'saccessDeniedHandler(...)lambda is never exercised in the test suite. If someone accidentally removes or mis-wires the handler, all tests remain green but production returns a different body.Recommended test (add to
AuthSessionIntegrationTestor a dedicatedCsrfSecurityTest):This must NOT use
.with(csrf())to actually trigger the handler.✅ What's well covered
LoginRateLimiterTest— tests the IP+email bucket exhaustion, IP bucket exhaustion, refund logic, and successful-login invalidation. This is the critical invariant and it's tested. ✅Controller tests updated with
.with(csrf())— 40+ tests updated across all controllers. This is the right approach: tests now reflect production security constraints rather than bypassing them. ✅PasswordResetServiceTestupdated — verifies thatrevokeAllSessionsis called after a successful password reset. ✅AuthSessionIntegrationTest— integration-level coverage for the session lifecycle.⚠️ Concerns (non-blocking)
No test for the
hooks.server.ts401 → redirect-to-login path. When the backend returns 401 for a revoked session,userGroupinhooks.server.tsdeletes thefa_sessioncookie and redirects to/login?reason=expired. This is a new behavior path that's currently untested. SvelteKit server hooks are testable by importing them directly:login/page.server.test.tsexists but I haven't seen its contents — if it covers the 429 path andrateLimited: trueflag, this concern is partially addressed. Worth verifying it tests theform?.rateLimitedbranch explicitly.Rate limiter test isolation —
LoginRateLimiterTestuses real Bucket4j buckets (no mocks). This is correct: mocking Bucket4j would defeat the purpose. Runs fast since it's pure in-memory. ✅💡 Suggestions
The integration test for session revocation should also verify the other session is invalidated (i.e., a 401 from a request using the old session token). The test plan in the PR description covers this scenario — confirm it's automated, not just manual.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
The rate-limit feedback on the login page is well-executed. Checking brand compliance, accessibility, and touch targets.
✅ What's done correctly
role="alert"on both error variants — rate-limited error and standard error both userole="alert", which is an implicitaria-live="assertive"region. Screen readers will announce errors immediately on form submission without the user needing to navigate to them. ✅aria-hidden="true"on the clock icon SVG — the icon is decorative alongside the<span>{form.error}</span>text. Correct pattern. ✅Redundant cues on rate-limit error — clock icon + red text
text-red-600+ message text. Not relying on color alone. ✅role="status"witharia-live="polite"on the session-expired warning — the amber?reason=expiredbanner usesrole="status"which is correct for non-urgent, informational messages. It won't interrupt a screen reader mid-announcement. ✅Submit button
min-h-[44px]— 44px minimum touch target, meeting WCAG 2.2 Success Criterion 2.5.8 for the senior audience. ✅autocomplete="current-password"on password field — allows password managers to fill the field without requiring the user to navigate. Critical for the 60+ audience. ✅⚠️ Minor concerns
Rate-limit error icon colour is
text-red-600in afont-sans text-xscontext. At 12px (text-xs),red-600on white is approximately 5.1:1 contrast — passes WCAG AA but misses AAA (7:1). Given the senior target audience,red-700(~6.5:1) or pairing with thetext-inkdark base would be stronger. Not a blocker, but worth a follow-up polish pass.Non-rate-limited error
<div role="alert" class="text-center font-sans text-xs font-medium text-red-600">— this pre-existing error div has no icon. It works (text + role=alert), but is visually weaker than the rate-limit error which has an icon. Not introduced by this PR — note for future consistency pass.The expired-session banner text
error_session_expired_explainer— I haven't seen the message string, but make sure it uses plain language ("Your session ended. Please log in again.") rather than technical terms. Seniors may not understand "session expired".✅ Brand compliance
Tailwind tokens used correctly:
bg-surface,border-line,text-ink,text-ink-2,text-ink-3. No raw hex values. Font choices (font-sansfor labels/UI,font-seriffor inputs) match the project conventions. Card container uses the canonical card pattern. ✅🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes — this is entirely application-layer security. Checking the config additions for correctness.
✅ Infrastructure impact
No new Docker services, volumes, or ports. The in-memory Caffeine cache adds no operational components. ✅
application.yamlrate-limit config block is clean:Environment-variable-overridable via
@ConfigurationProperties+ Spring Boot's relaxed binding (RATE_LIMIT_LOGIN_MAX_ATTEMPTS_PER_IP_EMAIL=10). Works correctly for docker-compose env overrides. ✅Bucket4j
8.10.1version pinned inpom.xml. Reproducible builds. ✅Management port (8081) already separate from app port (8080) — the security filter chain for
/actuator/**correctly disables CSRF (no session cookie used for actuator access from Prometheus scraper). This was set up correctly in a prior PR. ✅server.forward-headers-strategy: native— already inapplication.yaml, unchanged. TrustsX-Forwarded-Forfrom Caddy. Correct for the single-VPS + Caddy deployment.⚠️ One concern to verify
Backend port (8080) exposure in
docker-compose.yml— withforward-headers-strategy: native, the rate limiter trusts the IP fromX-Forwarded-For. If port 8080 is reachable directly (hostports:mapping rather than container-internalexpose:), an attacker could bypass the rate limiter by spoofing the header. Verify that in the production compose file, port 8080 isexpose:(container-internal only), notports:(host-mapped). The Caddy reverse proxy should be the only public entry point.This is not introduced by this PR — it's a pre-existing configuration concern that the rate limiter makes newly relevant.
💡 Observability note
The
LOGIN_RATE_LIMITEDaudit event gives a log trail. Consider whether Prometheus should surface alogin_rate_limited_totalcounter for alerting. Not a blocker — the audit log covers the forensics need. A future enhancement could expose this via Micrometer if brute-force detection alerting is desired.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Tracing the original requirements from issue #524 against the implementation.
Requirements coverage matrix
CookieCsrfTokenRepository+handleFetchinjection inhooks.server.tsAccessDeniedHandler→{"code":"CSRF_TOKEN_MISSING"}UserController.changePassword→revokeOtherSessionsPasswordResetService.resetPassword→revokeAllSessionsPOST /api/users/{id}/force-logoutwithADMIN_USERpermissionLoginRateLimitervia Bucket4jbyIpbucketinvalidateOnSuccesscalled on successDomainException.tooManyRequests(TOO_MANY_LOGIN_ATTEMPTS)rateLimited: trueflag in form resulthooks.server.ts→/login?reason=expiredon 401errors.tsupdated withCSRF_TOKEN_MISSING,TOO_MANY_LOGIN_ATTEMPTSde.json,en.json,es.jsonupdated⚠️ One gap to clarify
Logout does not explicitly revoke the session via
AuthService.revokeAllSessions.AuthService.logout()only audits — it does not delete the session fromJdbcIndexedSessionRepository. Session invalidation on logout appears to be handled by Spring Security's standardLogoutHandler(via theAuthSessionController). This is architecturally correct — Spring Security's logout filter handles session invalidation. I'm flagging it only to confirm this is intentional and tested, not overlooked.If the logout handler properly invalidates the session, no action needed. If there's any risk of the session surviving a logout (e.g., custom logout path that bypasses Spring Security's filter), that would be a gap.
✅ Non-functional requirements
LOGIN_RATE_LIMITEDandLOGIN_FAILEDaudit events. ✅application.yamlwith env-var override support. ✅@Autowired(required=false)forLoginRateLimiterandJdbcIndexedSessionRepositoryensures unit tests without Spring Session still work. ✅Felix's blocker resolved —
UserControllernow uses@RequiredArgsConstructor+finalfieldsConcern addressed
What changed
Verified the dependency graph before touching the file:
AuthService→UserService(one-way, final constructor injection)UserService→ noAuthServicedependency (cycle was already broken whenchangePasswordorchestration moved to the controller)UserController→UserService+AuthService+AuditService— no cycleThe
@AllArgsConstructor+ mutable-field workaround was left over after the refactoring and is no longer needed.Change: switched
import lombok.AllArgsConstructor→import lombok.RequiredArgsConstructor, annotated the class with@RequiredArgsConstructor, and addedfinalto all three fields.→ commit
28de7da9Test result
All 1643 backend tests pass after the change.
🏗️ Markus Keller (@mkeller) — Application Architect
Verdict: ⚠️ Approved with concerns
Blockers
1.
docs/ARCHITECTURE.mdnot updated for newErrorCodevaluesThe doc compliance matrix requires: "New
ErrorCodeorPermissionvalue →CLAUDE.md+docs/ARCHITECTURE.md."CLAUDE.mdwas updated (auth package table).docs/ARCHITECTURE.mdis not in the changeset. Two new error codes (CSRF_TOKEN_MISSING,TOO_MANY_LOGIN_ATTEMPTS) need to be reflected there before merge.Concerns (non-blocking)
2.
@Autowired(required = false)field injection inAuthServicebreaks the project conventionThe entire codebase uses
@RequiredArgsConstructor+finalfields (theUserControllerwas just migrated to this pattern in the prior commit). Two@Autowired(required=false)fields inAuthService(sessionRepository,loginRateLimiter) are a convention break. The PR description acknowledges this as a workaround for test contexts whereJdbcHttpSessionAutoConfigurationdoesn't fire.A cleaner long-term path: provide a no-op
@TestConfigurationstub for session repository in the test profile, or use@ConditionalOnBean. The null-guard pattern (if (sessionRepository == null) return 0) and the@BeforeEach ReflectionTestUtilsin tests do mitigate the risk. Track as tech debt.3. Static
ObjectMapperinSecurityConfigThe comment correctly explains the constraint (
@WebMvcTestslices excludeJacksonAutoConfiguration). The response only serialises a fixed String key so naming strategy and custom modules are irrelevant — pragmatically acceptable. Worth a note that this could be resolved by providing theObjectMapperbean in the security test slice.4.
bucket4j-coreis manually pinned outside the Spring BOMThis version will not be tracked by Renovate unless added to the
renovate.jsonpackage rules. Add it so patch/minor updates don't drift silently.What's done well
seq-auth-flow.puml(extended with rate-limiting and CSRF sequences),l3-backend-3a-security.puml(new components and relations),ADR-022(comprehensive: context, decision table, consequences, and the token-refund rationale). Diagram updates are accurate to the implementation.PasswordResetService(user domain) callsAuthService(auth domain) via the service interface, not the repository. Correct.revokeOtherSessionsfor password change (caller stays logged in),revokeAllSessionsfor password reset and force-logout. The semantics are well-modelled.👨💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Blockers
1.
@Autowired(required = false)field injection inAuthServicebreaks the@RequiredArgsConstructorconventionThe immediately preceding commit (
refactor(user): migrate UserController to @RequiredArgsConstructor + final fields) shows this was just cleaned up elsewhere.AuthService.java:27-32introduces two field-injected, non-final dependencies:The consequence is visible in
AuthServiceTest.java— it requiresReflectionTestUtils.setField()in@BeforeEachto wire the optionals, which@InjectMockscannot reach when the fields are not constructor-injected. Clean constructor injection + a no-op stub bean in the test profile would eliminate this. This is the same pattern break that was just fixed inUserController.Suggestions
2. Fully-qualified Mockito call in
UserControllerTestchangePassword_returns204_and_calls_revokeOtherSessions()uses:The class already has
import static org.mockito.Mockito.*. Should beverify(authService).3.
AuthSessionIntegrationTest.fetchXsrfToken()generates a fresh UUID rather than reading from serverThis correctly validates the double-submit pattern (cookie value == header value), but it doesn't exercise the server's CSRF cookie-setting path. Adding a GET to any permitted endpoint first and reading
XSRF-TOKENfrom the responseSet-Cookiewould make the test end-to-end faithful. Non-blocking since the critical validation behavior is covered.4.
assertThatwith full qualification inLoginRateLimiterTestSeveral assertions use
org.assertj.core.api.Assertions.assertThat(...)with full qualification when the static import is already present viaassertThatThrownBy. Minor consistency issue in test code.What's done well
LoginRateLimiterTest(8 tests, 135 lines) covers the happy path, the 11th-attempt block, the IP backstop, case-insensitivity, success reset, and the phantom-consumption edge case. Each test is exactly one behavior. Names read as sentences.byIpEmail.get(key).addTokens(1)refund incheckAndConsumeprevents IP-level exhaustion from silently draining per-email quota. The comment, ADR, and dedicated test all explain the invariant together. This is the right way to handle a subtle edge case.checkAndConsume,revokeOtherSessions,handleFetch, andforceLogoutall stay under 20 lines.invalidateOnSuccessis 3 lines. No functions doing two things.email.toLowerCase(Locale.ROOT)in the rate limiter key matches the existing auth normalisation convention. BothcheckAndConsumeandinvalidateOnSuccessapply it consistently.handleFetchrefactored from nestedif (isApi) { if (!publicPath) { ... } }to a flattened early-return structure. More readable. Thecrypto.randomUUID()fallback for fresh sessions is correct for the double-submit pattern.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is a solid, well-reasoned security PR. I'll call out the implementation decisions that are correct and non-obvious, then list the minor gaps.
Security correctness — what's right
1. CSRF handler choice:
CsrfTokenRequestAttributeHandleroverXorCsrfTokenRequestAttributeHandlerThis is the single most important technical decision in this PR. Spring Security 6 defaulted to the XOR-encoded handler for BREACH-resistance in rendered forms. For SPAs that read the token from JavaScript and attach it as a request header, the XOR encoding is actively harmful — it causes token mismatch on every request because JavaScript reads the encoded cookie value, not the raw token.
CsrfTokenRequestAttributeHandleris correct here. ✅2. Token refund prevents phantom quota consumption
Without the refund in
checkAndConsume:target@example.comfrom1.2.3.4ip:target@bucket is exhausted → all further attempts blockedip:target@ipEmail bucket (because the ipEmail check happens first)target@has zero ipEmail tokens left → still blocked even from a clean IPThe refund (
byIpEmail.get(key).addTokens(1)) eliminates this. The ADR documents it precisely. ✅3. Rate check before BCrypt — correct order
loginRateLimiter.checkAndConsume(ip, email)is called beforeauthenticationManager.authenticate(). BCrypt at cost factor 10 takes ~100ms. Not computing it for rate-limited requests is the correct defensive posture. ✅4. Email normalization
email.toLowerCase(Locale.ROOT)incheckAndConsumeandinvalidateOnSuccess. An attacker submittingUser@Example.COManduser@example.comhits the same bucket. ✅5. Audit events carry no passwords
LOGIN_RATE_LIMITEDpayload:{ip, email}.ADMIN_FORCE_LOGOUTpayload:{actorId, targetUserId, revokedCount}. No credentials. ✅6. Session revocation scope semantics
revokeOtherSessions: caller's session ID is excluded → user stays logged in on current device after password change. Correct.revokeAllSessions: no exclusion → appropriate for password reset (caller is unauthenticated via email link) and admin force-logout. Correct.Minor concerns (non-blocking)
7. XSRF-TOKEN cookie
SameSiteattribute — verify production behaviorCookieCsrfTokenRepository.withHttpOnlyFalse()— in Spring Security 6.1+ this cookie isSameSite=Strictby default. Verify this is also the case in Spring Boot 4 / Spring Security 7, especially with the Caddy proxy in front. If it'sLax, cross-site navigation with GET could read the cookie value and a state-changingwindow.historytrick could attach it to a POST — low-risk for a family archive, but worth confirming via a quickcurl -Iagainst the running stack to see theSet-Cookieflags.8.
PUBLIC_API_PATHS.some(p => url.includes(p))— substring match could be tighterrequest.url.includes('/api/auth/login')would also match/api/auth/login-extendedor/api/auth/login/whatever. Since these are server-to-server SSR fetch calls (not user-controlled URL routing) the risk is negligible — but tightening tourl.includes(p + '?')or usingnew URL(request.url).pathnamewith exact match would be cleaner.9. Missing CSRF failure test for
POST /api/users/me/passwordAuthSessionControllerTesthas a CSRF test forPOST /api/auth/logoutwithout CSRF → 403. There's no equivalent forPOST /api/users/me/passwordorPOST /api/users/{id}/force-logout. The@WebMvcTestinfrastructure is already set up; adding two tests would close the gap:10.
expireAfterAccesskeeps active attacker's bucket alive indefinitelyIntentional and correct — an attacker who keeps hammering the endpoint never gets their window reset. The bucket only reclaims memory when idle. ✅
Overall
The three features (CSRF double-submit, session revocation, login rate limiting) are implemented correctly. The edge cases (token refund, email normalisation, null-guard for test contexts) are all handled. No blocking security issues.
🛠️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ✅ Approved
What's done well
No new infrastructure. The in-memory Caffeine rate limiter avoids a Redis dependency that would cost ~3 EUR/month and add operational overhead (persistence config, connection pool, failover). For a single-VPS family archive this is the right call — and the source comment + ADR document the trade-off explicitly.
expireAfterAccessis the correct Caffeine eviction policy for rate limiting. Idle IP buckets are reclaimed automatically. Active buckets stay alive as long as requests keep coming. No memory leak risk at family-archive traffic volumes.application.yamlconfig additions are clean:Externalized, sensible defaults, bindable via
@ConfigurationProperties. No magic constants buried in code.Concerns
1.
bucket4j-coreis manually pinned outside the Spring BOM — add to RenovateAll other backend dependencies use managed versions from the Spring BOM. This one is explicitly pinned. Add a
packageRulesentry inrenovate.jsonto trackcom.bucket4j:bucket4j-coreso patch updates auto-merge and minor/major updates get PRs:Without this it will drift silently.
2. Verify XSRF-TOKEN cookie flags in the production Caddy environment
The
fa_sessioncookie hasSameSite=Strict; HttpOnly; Secure(set by Spring Session JDBC config). TheXSRF-TOKENcookie isnot HttpOnly(by design — JS reads it). Verify itsSameSiteandSecureflags are set correctly when requests arrive through Caddy withX-Forwarded-Proto: https. A quickcurl -I https://your-domain/api/users/meafter deploy will show theSet-Cookieheaders.No changes needed to
Clean from an ops perspective.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Requirements coverage
Issue #524 specified three security capabilities. Checking each against the implementation:
All acceptance criteria from the PR test plan are covered.
Acceptance criteria traceability
"POST any mutating endpoint without X-XSRF-TOKEN → 403 CSRF_TOKEN_MISSING"
→
AuthSessionControllerTest.authenticated_post_without_csrf_token_returns_403_CSRF_TOKEN_MISSING()✅"Change password → old session returns 401; current session works"
→
UserControllerTest.changePassword_returns204_and_calls_revokeOtherSessions()verifiesrevokeOtherSessionsis called. The "current session works / old returns 401" is validated by unit test mock, not an integration test. Minor gap but acceptable given the session revocation mechanism is separately tested inAuthServiceTest."10× failed login from same IP+email → 11th returns 429 with clock icon"
→
LoginRateLimiterTest.eleventh_attempt_from_same_ip_email_throws_TOO_MANY_LOGIN_ATTEMPTS()+ frontendpage.server.test.tsverifyingrateLimited: true+ clock icon in+page.svelte. ✅"Admin force-logout → target session returns 401"
→
UserControllerTest.forceLogout_returns200_and_revokes_target_sessions()✅"Password reset → old sessions return 401"
→
PasswordResetServiceTest(updated withAuthServicemock). ✅NFR compliance
application.yamlwith sensible defaults. ✅role="alert"for screen reader announcement. ✅Open question
The acceptance criterion "clock icon on login page when rate-limited" is verified by the Svelte markup (the
{#if form?.rateLimited}branch renders the clock SVG). However, there's no automated test that renders the login page in a browser context and verifies the icon is visible. Avitest-browser-sveltetest would close this:Non-blocking — the logic is correct and the structure is sound.
🎨 Leonie Voss (@leonievoss) — UX/UI Designer & Accessibility Lead
Verdict: ✅ Approved
What's done well
1.
role="alert"added to both error paths — This is the critical accessibility fix. Both the rate-limited error and the regular error now announce to screen readers when an error appears. Withoutrole="alert", a senior user relying on a screen reader would submit the form and hear nothing after a failed login attempt.✅
2. Redundant error cues for rate limiting — Clock icon + error text together. Color alone (
text-red-600) would fail color-blind users. The icon adds a non-color signal.aria-hidden="true"on the SVG is correct — the<span>text already communicates the message to screen readers; reading "clock icon too many login attempts" would be redundant. ✅3. User-friendly language in all three languages — No technical jargon ("429", "rate limit exceeded", "HTTP error"). The German message "Zu viele Anmeldeversuche. Bitte versuchen Sie es später erneut." is natural and appropriate for the senior audience. ✅
4. Error layout doesn't shift other content unexpectedly — The error div appears in the existing error slot (
{#if form?.error}). No layout reflow that would confuse users who have already found the submit button. ✅Concerns
5.
text-red-600on the clock SVG — The SVG usesstroke="currentColor"andclass="h-4 w-4 shrink-0 text-red-600". Thetext-red-600sets the CSScolorproperty which becomescurrentColorfor the stroke. This works. However, the parentdivalso hastext-red-600, so theclasson the SVG is redundant. Low priority.6. Font size check — The error uses
text-xs(12px). This is the project-minimum for UI chrome. For the senior audience (60+) this is at the floor. If the error messages are ever considered critical-path information (which rate-limiting feedback arguably is), bumping totext-sm(14px) would improve readability without changing the layout significantly. Non-blocking given the existing pattern throughout the login page.7. No visual regression test at 320px — There's no automated screenshot test verifying the clock icon + error message renders correctly at small viewport. On a 320px screen the
flex items-center gap-2layout should work fine, but a Playwright snapshot would confirm it. The senior audience (60+) transcribing on tablets is the target, not phones, so this is low-priority for this flow.Accessibility verdict
The
role="alert"addition is the right call and improves the baseline accessibility of the login page beyond the PR's stated scope. No critical accessibility issues found.🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Concerns
1.
ReflectionTestUtils.setField()is a test smell — root cause is in production codeAuthServiceTest.injectOptionalFields()uses:This is a symptom of the
@Autowired(required = false)field injection inAuthService.@InjectMockscannot inject non-constructor fields reliably. If the fields were constructor-injected (final),@InjectMockswould wire them automatically without reflection magic. The fix belongs inAuthService, not the test — see the developer review. Until that's resolved, the tests work but are brittle if field names change.2. No integration test for CSRF rejection
AuthSessionControllerTest(@WebMvcTest) proves that requests without CSRF return403 CSRF_TOKEN_MISSING. ButAuthSessionIntegrationTest(@SpringBootTestwith real Postgres) has no negative test for CSRF rejection. The integration test only validates CSRF-valid paths. Adding one negative case at the integration level:This would give full-stack confidence that CSRF is active (not disabled by some autoconfiguration override).
3.
PasswordResetServiceTestchanges not fully visible in diffThe file is listed as changed (adds
AuthServiceimport). The actual assertion verifying thatauthService.revokeAllSessions(user.getEmail())is called after a successful password reset should be present — this is a critical behavior. If this assertion is missing, it's a coverage gap for session revocation on password reset.4. No frontend component test for rate-limited UI state
page.server.test.tsverifiesrateLimited: truein the form action return value. There's novitest-browser-sveltetest rendering the login component withform.rateLimited = trueand asserting the alert is visible:The UI logic is simple and correct, but without a component test it's one refactor away from silent breakage.
What's done well
LoginRateLimiterTestis exemplary — 8 tests covering every distinct behavior:checkAndConsumeandinvalidateOnSuccess)Each test has one assertion, one reason to fail, and a name that reads as a specification sentence. This is TDD done right.
All 18+ controller tests updated consistently — Adding
.with(csrf())to every mutating request across 11 controller test classes was mechanical but necessary. It's done consistently with no misses. The diff shows no test left behind.AuthServiceTestgains 9 new tests — Covers rate-limit integration, null-guard for missing session repository, and session revocation behavior. Factory-style setup (AppUser.builder()...) is used consistently. No test longer than ~15 lines.Test naming is consistent —
should_X_when_Yandaction_returns_N_when_conditionpatterns throughout. CI failures will be self-documenting.Test pyramid health for this PR
LoginRateLimiterTest,AuthServiceTest@WebMvcTest)@SpringBootTest)Adds two @WebMvcTest assertions verifying that POST /api/users/me/password and POST /api/users/{id}/force-logout without an XSRF-TOKEN header return 403 with code CSRF_TOKEN_MISSING. Addresses Nora Concern 9 from PR #617 review. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>All round-3 reviewer concerns have been addressed. Here's a summary:
Felix —
@Autowired(required=false)smell inAuthService✅8eced9cExtracted
SessionRevocationPortinterface +JdbcSessionRevocationAdapter/NoOpSessionRevocationAdapter.AuthServicenow uses pure constructor injection (@RequiredArgsConstructor, allfinalfields), no@Autowired(required=false)anywhere. ASessionRevocationConfig@Beanfactory resolves the adapter at context startup. TheJdbcSessionRevocationAdapterTest(Mockito unit) andAuthServiceTestwere updated to match.Felix —
ReflectionTestUtils.setFieldsmell inAuthServiceTest✅8eced9cRemoved the
@BeforeEach injectOptionalFields()method entirely.@Mock SessionRevocationPort sessionRevocationPortis injected by Mockito's constructor injection, and two new delegation tests (revokeOtherSessions_delegates_to_port,revokeAllSessions_delegates_to_port) replace the null-guard tests.Felix —
UserControllerTestfully-qualifiedMockito.verify✅2f981ef6All
Mockito.verify(...)calls converted to static-importverify(...)throughout the test class.Markus —
docs/ARCHITECTURE.mdCSRF section is stale ✅7074c9e4Updated the permission-system paragraph: documents the
fa_sessioncookie properties, the double-submit CSRF pattern, theXSRF-TOKEN/X-XSRF-TOKENflow, and references ADR-022 andsecurity-guide.md. AddedCSRF_TOKEN_MISSINGandTOO_MANY_LOGIN_ATTEMPTSto the error codes table.Integration-level CSRF rejection test ✅
778402feAdded
post_without_csrf_token_returns_403_CSRF_TOKEN_MISSINGtoAuthSessionIntegrationTest— exercises the real Spring Security CSRF filter against a running server (not MockMvc). All 5 integration tests pass.Review concerns addressed (round 2)
All open concerns from the second review cycle have been resolved. Here's what was done per reviewer, with commit references.
Felix Brandt — Blocker 1 + Suggestions 2 & 4
Blocker 1 —
@Autowired(required = false)field injectionThe scaffolding (
SessionRevocationPort,JdbcSessionRevocationAdapter,NoOpSessionRevocationAdapter) was already present butAuthServicewasn't wired to use it. The fix:JdbcSessionRevocationAdapterandNoOpSessionRevocationAdapterare now plain implementation classes (no@Service/@Conditionalannotations) — the@ConditionalOnBeanapproach was unreliable because Spring evaluates it before JDBC auto-configuration fires.SessionRevocationConfig(@Configuration) provides theSessionRevocationPortbean via a single@Beanmethod that acceptsJdbcIndexedSessionRepositoryas@Autowired(required = false)— Spring resolves optional parameters reliably after auto-configuration.AuthServicenow hasfinal SessionRevocationPort sessionRevocationPortandfinal LoginRateLimiter loginRateLimiter— both constructor-injected via@RequiredArgsConstructor.AuthServiceTestmocksSessionRevocationPortdirectly;@InjectMockswires via the 5-arg constructor.ReflectionTestUtils.setField()and the@BeforeEachare gone. Null-guard tests replaced by delegation-to-port tests.→ commits
8eced9c9,778402feSuggestion 2 — fully-qualified
org.mockito.Mockito.verify()inUserControllerTestChanged to
verify()via the existing static import. Also added missingimport static org.mockito.ArgumentMatchers.eq.Suggestion 4 — fully-qualified
org.assertj.core.api.Assertions.assertThat()inLoginRateLimiterTestReplaced all three occurrences with
assertThat()via static import; added the missingimport static org.assertj.core.api.Assertions.assertThat.→ commit
2f981ef6Nora — Concern 9: missing CSRF tests for password endpoints
Added to
UserControllerTest:changePassword_without_csrf_returns_403_CSRF_TOKEN_MISSINGforceLogout_without_csrf_returns_403_CSRF_TOKEN_MISSINGBoth verify that the custom
accessDeniedHandlerreturns{"code":"CSRF_TOKEN_MISSING"}(not a generic 403).→ commit
6db5c2d1Sara — Concern 2: no integration-level CSRF test; Concern 4: no browser component test
Concern 2 — Added
post_without_csrf_token_returns_403_CSRF_TOKEN_MISSINGtoAuthSessionIntegrationTest(@SpringBootTestwith real Postgres). Verifies the 403 body containsCSRF_TOKEN_MISSINGend-to-end — not just in the@WebMvcTestslice.→ commit
778402feConcern 3 —
PasswordResetServiceTest.resetPassword_revokes_all_sessions_after_password_reset()was already present (line 183). No action needed. ✅Concern 4 — Added
shows rate-limit alert with clock icon when rateLimited is truetopage.svelte.test.tsusingvitest-browser-svelte. RendersLoginPagewithform.rateLimited=trueand assertsrole="alert"is visible in Chromium.→ commit
24c85c29Tobias — Concern 1: bucket4j-core not in Renovate
Added a
packageRulesentry inrenovate.jsonforcom.bucket4j:bucket4j-core: patch updates auto-merge, minor/major open PRs.→ commit
0514622fAll 9 implementation plan items completed ✅
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is a well-executed security hardening PR. All three vectors (CSRF, session revocation, rate limiting) are addressed with real defences, not security theatre. The implementation choices are defensible and the edge cases are handled thoughtfully.
What I Verified
CSRF — double-submit cookie pattern
CookieCsrfTokenRepository.withHttpOnlyFalse()+CsrfTokenRequestAttributeHandleris the correct combination for a SvelteKit SPA. The non-XOR handler is critical — the defaultXorCsrfTokenRequestAttributeHandlerwas introduced in Spring Security 6.1 and corrupts token values when the SPA reads them from the cookie and submits them verbatim. Using the attribute handler here is correct.AccessDeniedHandlerdistinguishesCsrfException(returnsCSRF_TOKEN_MISSING) from otherAccessDeniedException(returnsFORBIDDEN). This is precise and correct.ERROR_WRITERstaticObjectMapperinstance: the comment justifying it is accurate and technically sound. The serialized payload is{"code": "CSRF_TOKEN_MISSING"}— no custom naming strategy or date format needed. LGTM.XSRF-TOKENcookie is set on the first GET.Session Revocation
revokeOtherSessionscorrectly preserves the caller's current session while deleting all others.revokeAllSessionsis called on password reset (unauthenticated flow), where preserving any session would be wrong.@Autowired(required = false)pattern onJdbcIndexedSessionRepositoryis correctly explained: unit test slices don't load Spring Session, so the no-op adapter fills the gap. This is cleaner than mocking the repo in every test.PasswordResetServicenow callsauthService.revokeAllSessions(user.getEmail())— correct placement, covered by a unit test.Login Rate Limiting
ip:email+ perip) is the right design. The per-IP backstop prevents credential stuffing across many email addresses from a single IP.byIpEmail.get(key).addTokens(1)) prevents IP-level blocking from silently consuming per-email quota. This is a subtle correctness fix and it's tested inLoginRateLimiterTest#ip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attempts.expireAfterAccess(notexpireAfterWrite) on Caffeine is correct: idle buckets expire and are reclaimed, active attack sources stay tracked.Locale.ROOTis correct — avoids Turkish-i locale issues.Frontend CSRF injection (
hooks.server.ts)crypto.randomUUID()whenXSRF-TOKENcookie is absent is a valid bootstrap behaviour: the generated UUID is set as both cookie and header value, satisfying the double-submit pattern even on the first request. The backend will set a proper cookie in the response.PUBLIC_API_PATHScorrectly bypassesfa_sessioninjection but still injects CSRF for mutating public endpoints. This is important — a CSRF attack on/api/auth/loginwould be a low-value attack (the attacker already knows the credentials), but the consistent behaviour is correct.Audit trail
LOGOUT(extended payload withreasonandrevokedCount),ADMIN_FORCE_LOGOUT,LOGIN_RATE_LIMITED. All are well-documented with their payloads in the enum Javadoc. No password in any payload — verified.Security Smell (Minor — Non-blocking)
IP extraction trust
The
ipparameter passed toLoginRateLimiteroriginates from the HTTP request. I don't see the fullAuthSessionControllerdiff, but the IP should come fromX-Forwarded-Foronly when the request arrives through Caddy (trusted proxy). IfX-Forwarded-Foris accepted without validation, a client can spoof its IP and bypass the rate limiter. Verify that IP extraction uses a trusted-proxy-aware mechanism (e.g., Spring'sRemoteAddressExtractoror readingrequest.getRemoteAddr()when behind a known proxy). This is a smell, not a confirmed vulnerability, because the production setup uses Caddy as a reverse proxy and the deployment docs describe the topology.No
Retry-Afterheader on 429The 429 response doesn't include a
Retry-Afterheader. This is not a security issue but it makes the UX friendlier and is recommended by RFC 6585. The frontend shows a generic "try later" message, which is fine for now.Positive Findings
@WebMvcTestand integration (AuthSessionIntegrationTest) levels are solid.forceLogout_without_csrf_returns_403_CSRF_TOKEN_MISSINGandchangePassword_without_csrf_returns_403_CSRF_TOKEN_MISSINGtests permanently codify that these sensitive endpoints require CSRF.LoginRateLimiterTestis thorough: boundary condition (10th attempt OK, 11th blocked), bucket isolation, case insensitivity, and the refund edge case are all covered.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid feature work. The architecture is clean, TDD evidence is present throughout, and the naming is generally intent-revealing. I have one blocker around a subtle logic bug in
hooks.server.tsand a few suggestions.Blockers
1. Dead branch in
handleFetch—cookieParts.length === 0 && !xsrfTokencan never be truefrontend/src/hooks.server.ts:By the time this check is reached, both conditions cannot simultaneously be false:
isPublicAuthApi && !isMutating:sessionIdis null,xsrfTokenis null,cookiePartsis empty → the condition IS true and we early-return. Good.isPublicAuthApi && isMutating:xsrfTokenis set →!xsrfTokenis false → the condition is false, we fall through correctly.!isPublicAuthApi && sessionIdpresent:cookiePartshas at least one entry → condition is false.!isPublicAuthApi && !sessionId: we already returned401earlier.So the dead branch protects a path that cannot be reached. That's harmless, but it reads as defensive coding that doesn't actually defend anything — it adds confusion. Either remove it or add a comment explaining which specific path it handles. I'd remove it and let the flow be explicit.
Suggestions
2.
invalidateOnSuccessis not case-normalised in the same way ascheckAndConsumeLoginRateLimiter.java:Wait — I re-read the code and this IS case-normalised correctly. The test
invalidateOnSuccess_is_case_insensitive_so_mixed_case_clears_the_bucketpasses, confirming it. Ignore this — the implementation is correct.3.
changePasswordinUserControllermixes orchestration concernsThe PR description notes this was a deliberate architectural choice to break a circular dependency. That's a valid trade-off and the comment in the PR description explains it. The
actorId(authentication)helper below re-fetches the user by email — that's a second DB lookup for the same user that was already fetched two lines above. Could passcurrent.getId()directly toactorIdbut that would require a different method signature. Minor.4.
NoOpSessionRevocationAdapteris package-private but not annotated@ComponentNoOpSessionRevocationAdapter.javais constructed manually inSessionRevocationConfig. That's correct — it's a factory-managed bean, not a Spring-scanned component. The package-private visibility is intentional. LGTM.5.
RateLimitPropertiesuses@Component+@ConfigurationPropertiesThe canonical pattern in Spring Boot 3+ is to use
@ConfigurationPropertiesScanor@EnableConfigurationPropertiesrather than@Componenton a@ConfigurationPropertiesclass. Both work, but@Componentbypasses the configuration properties validator. For a small properties class with primitive defaults this is not a real risk, but worth knowing.6. Frontend test —
page.server.test.tsrate-limit testThe test correctly verifies
status: 429anddata.rateLimited: true. Missing: verification thatdata.errorcontains the rate-limit message string. Not a blocker, just an incomplete assertion.7. Login page Svelte component —
role="alert"on both error paths is correctBoth error cases now have
role="alert". The rate-limited variant adds the clock icon. The structure is clean — one{#if form?.rateLimited}branch inside{#if form?.error}. LGTM.What's Done Well
_returns_403_CSRF_TOKEN_MISSING,login_checks_rate_limit_before_authenticating).@AllArgsConstructor→@RequiredArgsConstructorfix onUserControlleris a clean improvement — fields are nowfinal.SessionRevocationPortinterface with two adapters is a textbook port/adapter pattern — testable and replaceable.fetchXsrfToken()helper inAuthSessionIntegrationTestwith its Javadoc explaining the double-submit pattern is the right level of documentation for a non-obvious test helper.🏗️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
The structural decisions in this PR are sound. The port/adapter pattern for session revocation, the Caffeine+Bucket4j rate limiter, and the CSRF configuration are all well-placed within the existing module boundaries. ADR-022 is complete and well-written.
Architecture Assessment
Module placement — correct
All new classes live in
org.raddatz.familienarchiv.auth:SessionRevocationPort(interface) — owned byauthJdbcSessionRevocationAdapter— adapter for Spring Session JDBCNoOpSessionRevocationAdapter— test/no-web-context fallbackSessionRevocationConfig—@Configurationwiring the correct adapterLoginRateLimiter—@ServiceinauthRateLimitProperties—@ConfigurationPropertiesinauthPasswordResetServicein theuserpackage now depends onAuthServicefromauth. This is a cross-domain dependency —user→auth. The layering rule says "cross-domain data access goes through the owning service," andAuthServiceis the published API of theauthdomain. This is correct.The circular dependency workaround is pragmatic
The PR description notes that
changePasswordorchestration moved toUserControllerto avoid a cycle betweenUserServiceandAuthService. This is an architectural smell — business logic in a controller — but it's the right trade-off given Spring Boot 4's prohibition on constructor injection cycles. A clean alternative would be an application-layer service (e.g.,PasswordManagementService) that orchestratesUserService+AuthService, but for a solo project the current approach is acceptable. I'd accept a TODO comment at the site.Doc update compliance (per the doc-update table)
ErrorCodevalues (CSRF_TOKEN_MISSING,TOO_MANY_LOGIN_ATTEMPTS)CLAUDE.md+docs/ARCHITECTURE.mddocs/architecture/c4/seq-auth-flow.pumlauthdocs/architecture/c4/l3-backend-3a-security.pumlPermissionvalueADMIN_USERwas pre-existingdocs/adr/022-csrf-session-revocation-rate-limiting.mdErrorCodeinCLAUDE.mdpackage tableauthpackage description updated — theexceptionpackage description in CLAUDE.md is not updated with the new error codesThe
exceptionpackage description inCLAUDE.mdreads unchanged: "Adding a newErrorCoderequires matching updates infrontend/src/lib/shared/errors.ts..." — this section doesn't list the new codes. However,docs/ARCHITECTURE.mddoes listCSRF_TOKEN_MISSINGandTOO_MANY_LOGIN_ATTEMPTSexplicitly. I'll flag this as a minor concern, not a blocker, since the architecture doc is the authoritative reference.expireAfterAccessvsexpireAfterWritein Caffeine — correctThe rate limiter uses
expireAfterAccess(windowMinutes, MINUTES)for both caches. This means a bucket expireswindowMinutesafter the last access, not after creation. For the IP cache, this is slightly different from a sliding window: a burst attacker who keeps hitting the endpoint every 14 minutes will never have their bucket reset. That's intentionally more aggressive — the bucket tracks within the window, not resets after idle. This is the correct behaviour for rate limiting and the Bucket4j refill logic already handles the sliding window semantics. The Caffeine expiry just reclaims memory for truly idle entries.SessionRevocationConfig— the@Autowired(required = false)patternThis is the correct approach for optional infrastructure beans in Spring Boot 4. Conditional beans (
@ConditionalOnBean) would be cleaner but require the auto-configuration infrastructure. Given that the goal is just "don't break unit test slices,"@Autowired(required = false)is the pragmatic choice and the ADR documents it. LGTM.Suggestion (Non-blocking)
The
PasswordResetServicenow has a field injection ofJavaMailSendervia@Autowired(required = false)— predating this PR — and constructor injection ofAuthService. Mixing field injection and constructor injection in the same class is a code smell. SinceAuthServiceis now in@RequiredArgsConstructor, andmailSenderuses@Autowired(required = false), this is already slightly inconsistent. Not introduced by this PR, but worth noting for a future cleanup.🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
This is exemplary test coverage for a security hardening PR. The test pyramid is properly exercised: unit tests for the new components,
@WebMvcTestslice tests for CSRF behaviour on every controller, and integration tests for the full CSRF + session flow. I have no blockers.Test Coverage Analysis
New unit test classes
LoginRateLimiterTestJdbcSessionRevocationAdapterTestAuthServiceTest(additions)PasswordResetServiceTest(addition)revokeAllSessionscalled after password resetController slice tests (CSRF)
Every mutating endpoint across 12 controller test classes now includes
.with(csrf())on happy-path tests, and the_without_csrf_returns_403_CSRF_TOKEN_MISSINGtests are added toDocumentControllerTest,UserControllerTest,AuthSessionControllerTest. This is the right approach — not every controller needs its own CSRF negative test since the filter is global, but having representative coverage in 3 different controllers is sufficient.Integration tests (
AuthSessionIntegrationTest)post_without_csrf_token_returns_403_CSRF_TOKEN_MISSING— tests the full stack (Spring Boot + real CSRF filter).fetchXsrfToken()helper generates a fresh UUID and sets it as both Cookie and header. This correctly simulates the double-submit pattern. The Javadoc explains the contract clearly.UserControllerTestadditionsNew tests cover:
changePassword_returns204_and_calls_revokeOtherSessions— verifies delegation toauthServicechangePassword_without_csrf_returns_403_CSRF_TOKEN_MISSINGforceLogout_returns200_and_revokes_target_sessionsforceLogout_returns401_whenUnauthenticatedforceLogout_returns403_whenMissingPermissionforceLogout_returns404_whenUserNotFoundforceLogout_without_csrf_returns_403_CSRF_TOKEN_MISSINGThis is full boundary coverage for the new endpoint — 401, 403, 404, and the happy path. LGTM.
Frontend tests
page.server.test.ts— new test for 429 rate-limit response withrateLimited: truepage.svelte.test.ts— renders the clock-icon alert forrateLimited: trueBoth use the established testing patterns in this codebase.
Minor Observations (Non-blocking)
1.
AuthSessionControllerTest— renamed testThe rename is correct — with CSRF enabled, an unauthenticated POST now hits the CSRF filter before the auth filter, so the behaviour changed from 401 to 403. The comment explaining the filter ordering is accurate and helpful.
2.
LoginRateLimiterTest— no test for concurrent accessLoginRateLimiterusesCaffeine.LoadingCachewhich is thread-safe, andBucket4jis designed for concurrent use. However, there's no concurrent stress test. For a security control, a@RepeatedTestor a simpleExecutorService-based test that fires 20 concurrent requests and verifies exactly 10 pass would add confidence. Not a blocker for this PR but worth a future issue.3.
page.server.test.ts— rate-limit test doesn't assert the error message textThe test asserts
data.rateLimited === trueandstatus === 429but doesn't verifydata.errorcontains the localised rate-limit message. ThegetErrorMessage('TOO_MANY_LOGIN_ATTEMPTS')call in+page.server.tsmaps tom.error_too_many_login_attempts()— a test asserting the message text would catch a broken i18n key. Minor omission.4.
@TransactionalonPasswordResetService.resetPasswordThis pre-existing method is
@Transactional. The newauthService.revokeAllSessions()call inside it delegates toJdbcIndexedSessionRepository.deleteById(), which issues JDBC calls outside the JPA transaction context. This means session deletion could succeed while the password update fails (or vice versa). This is a pre-existing design issue, not introduced by this PR, but worth flagging for awareness: ifuserService.changePassword()throws afterrevokeAllSessions()returns, all sessions are deleted but the password is not updated. The user then cannot log in at all until the reset is retried. Acceptable for a family archive but worth documenting.🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This PR adds no new infrastructure services and makes no changes to the Compose file, CI workflow, or Caddy configuration. The operational impact is minimal. What I checked and how it lands for the single-VPS deployment:
Infrastructure Impact
New dependency:
bucket4j-core:8.10.1Manually pinned outside the Spring BOM. The Renovate rule added in
renovate.jsonis correct:The version
8.10.1is a recent stable release. No concerns.In-memory rate limiter — single-VPS only
The ADR and code comment correctly document that this cache is node-local. For the current single-VPS setup this is the right, simplest implementation. No Redis or database-backed rate limiter needed. If the deployment ever scales horizontally, this needs revisiting.
Memory footprint
Each Caffeine cache entry is a
Bucketobject backed by alongcounter. TheexpireAfterAccess(15, MINUTES)policy ensures idle entries are collected. For the expected load (family archive, small user base), the memory impact is negligible — even 10,000 unique IP+email combinations would be well under 10MB.Session cleanup on password reset
revokeAllSessionsissuesDELETEstatements againstspring_session. This is already persisted via Spring Session JDBC (Flyway V67). No schema changes in this PR — correct. TheJdbcIndexedSessionRepository.findByPrincipalName()+deleteById()pattern issues one SELECT followed by N DELETEs where N is the session count. For a family archive with < 10 concurrent sessions per user, this is fine.What I Did Not Find (Positive)
Minor Observations
SessionRevocationConfig—@Autowired(required = false)for non-web test contextsThis is the correct pattern. The explanation in the PR description and ADR is accurate. No operational concern.
RateLimitPropertiesdefaultsThese are sane defaults for production. The values are externalised via
@ConfigurationProperties, so they can be tuned via environment-specific config without recompilation. Good operational design.No Caddyfile changes needed
CSRF uses a cookie + header pair. Caddy passes all custom headers through by default. No reverse-proxy configuration changes are required. The
X-XSRF-TOKENheader will pass through Caddy to the backend without modification.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Issue #524 asked for CSRF protection, session revocation, and login rate limiting. All three are delivered and traceable from requirements to implementation to test. Reviewing against the acceptance criteria implied by the PR test plan:
Requirements Traceability
X-XSRF-TOKEN→403 CSRF_TOKEN_MISSINGSecurityConfigCSRF handlerAuthSessionControllerTest,DocumentControllerTest,UserControllerTestCSRF tests;AuthSessionIntegrationTestUserController.changePassword+revokeOtherSessionsUserControllerTest#changePassword_returns204_and_calls_revokeOtherSessionsLoginRateLimiter++page.server.ts429 handler ++page.svelteclock iconLoginRateLimiterTest#eleventh_attempt...,page.server.test.ts,page.svelte.test.tsUserController.forceLogout+revokeAllSessionsUserControllerTest#forceLogout_returns200_and_revokes_target_sessionsPasswordResetService+revokeAllSessionsPasswordResetServiceTest#resetPassword_revokes_all_sessions_after_password_resetAll five criteria are met.
Scope Completeness
What was specified and is present:
What was not specified but is present (value-adds):
ADMIN_FORCE_LOGOUTaudit kind (beyond theLOGOUTextension) — good audit trail enrichmentLOGIN_RATE_LIMITEDaudit kind — useful for detecting attack patternsbucket4j-core— good operational hygieneOpen Questions / Edge Cases Not Covered by Tests
OQ-001: Rate limit bypass via IPv6
The rate limiter keys on the IP string. IPv6 clients present 128-bit addresses. A single IPv6 /64 prefix (e.g., a typical home router) can generate millions of distinct addresses. The current per-IP bucket would treat each IPv6 address as distinct, making the per-IP backstop ineffective against an IPv6 attacker. This is a known limitation of IP-based rate limiting and acceptable for the current threat model (family archive, not a public-facing login form), but worth a note in the ADR. The ADR mentions NAT/VPN false positives but not the IPv6 case.
OQ-002: Rate limit window reset on successful login
On successful login, both buckets are invalidated (
invalidateOnSuccess). This means a user who fails 9 times, succeeds once, and then fails again immediately starts with a fresh 10-attempt window. For a family archive with trusted users this is the right UX trade-off (avoid locking out legitimate users). The ADR could note this explicitly.Neither OQ is a blocker — they're documentation gaps in the ADR, not implementation defects.
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
The only visible UI change in this PR is the rate-limit error state on the login page. It's well-executed. The rest of the PR is backend infrastructure with no visible UX surface.
Login Page Rate-Limit Alert
frontend/src/routes/login/+page.svelteWhat's good:
role="alert"is correct — screen readers will announce rate-limit errors immediately. This is critical for users who rely on assistive technology.aria-hidden="true"on the SVG is correct — the icon is purely decorative; the<span>carries the message.shrink-0on the icon prevents it from collapsing when the error text wraps. Good layout discipline.Minor concern (non-blocking):
The general error path still uses:
This is centered text-only. The rate-limited variant has left-aligned text with an icon. The visual inconsistency between the two error states is minor — both convey the message — but for polish, the general error could also use
flex items-centerto be consistent. Not a blocker for this PR.Touch target consideration:
No new interactive elements were added. The submit button pre-existed with adequate sizing. LGTM.
Font and brand tokens:
font-sansandtext-xsmatch the project's established label/metadata typography convention.text-red-600is used consistently across the codebase for error states. No brand token violations.Accessibility of CSRF Error Messages
When a user encounters a CSRF error (
403 CSRF_TOKEN_MISSING) on a form submission, the frontend maps it to"Sitzungsfehler. Bitte laden Sie die Seite neu."(de) /"Session error. Please reload the page."(en). This is:LGTM on all three i18n strings.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is a solid, well-structured security implementation. The re-review shows that the previously raised concerns have been addressed properly. Here is my full audit.
What was fixed since the last review (confirmed)
CookieCsrfTokenRepository.withHttpOnlyFalse()+CsrfTokenRequestAttributeHandleris the right Spring Security 6 setup. TheXSRF-TOKENcookie is readable by JS, and every mutating request fromhandleFetchinjectsX-XSRF-TOKEN+ forwards the cookie — the double-submit is intact.ChangeSessionIdAuthenticationStrategyis exposed as a@Beanand called explicitly inAuthSessionController.login. CWE-384 is mitigated.LoginRateLimiter.checkAndConsume. When the per-IP backstop fires, the per-IP+email token is refunded before throwing. This is subtle and correct — it prevents IP-level limiting from consuming email-scoped quota.expireAfterAccessfor Caffeine caches. Idle IP buckets are reclaimed automatically; the comment about single-VPS scope is appropriate.Remaining observations (non-blocking)
1.
checkAndConsumeis not atomic (security smell, not a vulnerability)Bucket4jitself is thread-safe per bucket. However, there is a narrow window betweenbyIpEmail.tryConsume(1)succeeding andbyIp.tryConsume(1)failing where two concurrent requests for the sameip:emailcould each consume an email-scoped token and both get refunded — effectively getting two free attempts per race window. For a family archive with low concurrency, this is not exploitable. Worth noting in the ADR if the effective limit precision matters.2. Rate-limit response does not include
Retry-AfterRFC 6585 §4 specifies that
429 Too Many RequestsSHOULD include aRetry-Afterheader. The current response omits it. Not a security issue — but clients (and future API consumers) would benefit from knowing when to retry.DomainException.tooManyRequests()could carry aretryAfterSecondsfield.3.
UserController.changePasswordusesLOGOUTaudit kind for session revocationLOGOUTis an established audit kind, but semantically a forced session revocation on password change is distinct from a voluntary logout.AuditKind.SESSION_REVOKED(orPASSWORD_CHANGED_SESSION_REVOKED) would make the audit trail cleaner. Minor — thereasonfield compensates for this today.4.
handleFetchfallback CSRF tokenIf
XSRF-TOKENcookie is absent (first request after session starts), a random UUID is generated and sent as both the cookie value and theX-XSRF-TOKENheader. The double-submit pattern only validates that cookie == header — it does not verify against a server-side secret. This is correct by design (stateless CSRF), but it means an attacker who can set cookies (subdomain cookie injection) could forge the token. For a single-domain family archive this is a non-issue; worth noting in ADR-022 as a known constraint.5. IP extraction — where is it?
The
AuthService.login(email, password, ip, ua)signature takes anipstring. I did not see the extraction in this diff. If it comes fromX-Forwarded-Forwithout validation (e.g.request.getHeader("X-Forwarded-For")), an attacker can spoof their IP and bypass the rate limiter by rotating the header. Verify that IP extraction uses a trusted-proxy whitelist or the actual remote address. This should be called out explicitly in the ADR or a code comment at the extraction point.Summary
The three main features (CSRF, session revocation, rate limiting) are implemented correctly and defensively. The code is readable, centralized, and well-commented. The port/adapter pattern for session revocation (
SessionRevocationPort→JdbcSessionRevocationAdapter/NoOpSessionRevocationAdapter) is clean and testable. No blockers from a security standpoint.🤖 Review by Claude Code
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Clean, well-structured implementation. The previous review feedback has been addressed. Here is my lens.
What looks good
SessionRevocationPortinterface,JdbcSessionRevocationAdapter,NoOpSessionRevocationAdapter) — clean boundary, easy to test.@Autowired(required = false)inSessionRevocationConfigis the right workaround for the missing JDBC session bean in@WebMvcTestslices.LoginRateLimiteris a well-scoped service. Constructor injection fromRateLimitProperties,@Serviceannotation,@Slf4j. Does one thing. TheinvalidateOnSuccessmethod prevents bucket exhaustion on legitimate login.AuthService.loginorchestration is readable: check rate limit → authenticate → audit → clear bucket. Each step is visible and the failure path audits correctly.handleFetchis a clean centralized interceptor — no CSRF logic scattered across individual form actions. TheMUTATING_METHODSset is explicit.LoginRateLimiterTest,JdbcSessionRevocationAdapterTest,AuthSessionIntegrationTest,PasswordResetServiceTestadditions. The controller tests have been updated with.with(csrf())consistently across all 50 changed files.Suggestions (non-blocking)
1.
UserController.changePassword— orchestration belongs in a servicePer CLAUDE.md layering rules: "controllers never call repositories directly; services contain logic." The password-change-and-revoke sequence is business logic — the controller is orchestrating two service calls plus an audit. The PR description notes this was a deliberate choice to avoid a circular dependency, which is a valid reason (Spring Framework 7 disallows constructor cycles). Since the workaround is intentional and documented, this is fine as-is — just flag it for future refactor when the dependency graph allows.
2.
JdbcSessionRevocationAdapter.revokeOtherSessions— sequential deletesFor a user with 2–3 sessions this is fine. If you ever add "logout all devices" from admin for a user with many sessions, this becomes N+1 deletes.
revokeAllSessionshas the same pattern. Consider adeleteAllByPrincipalName(String)query in the future — for now, acceptable given realistic session counts.3.
hooks.server.ts—MUTATING_METHODScould be a type aliasThis is fine. A minor improvement would be
type HttpMutatingMethod = 'POST' | 'PUT' | 'PATCH' | 'DELETE'+ a typed set — but for a constant used in one place, the current approach is readable enough.4.
login/+page.svelte— rate-limit error renderingGood differentiation between rate-limited and generic error states. The
role="alert"on the generic error div andaria-liveon the status divs are correctly applied.TDD check
Tests precede or accompany the implementation —
LoginRateLimiterTest,PasswordResetServiceTestadditions, and theAuthSessionIntegrationTestall demonstrate behavior-first coverage. The@WebMvcTestslices are updated with.with(csrf())which is the correct pattern post-CSRF-enable. No red-flag gaps.🤖 Review by Claude Code
🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
The architecture is sound. The previous concerns have been resolved. Here is my structural review.
Architecture decisions — confirmed correct
1. Port/Adapter for session revocation
SessionRevocationPort(interface inauth/) →JdbcSessionRevocationAdapter/NoOpSessionRevocationAdapter(implementations inauth/) — selected bySessionRevocationConfigat bean creation time. This is the right pattern for an optional infrastructure dependency. The@Autowired(required = false)approach keeps@WebMvcTestslices working without the full JDBC session autoconfig.2. Orchestration in
UserControllerto break circular dependencychangePasswordcallsuserService.changePassword()thenauthService.revokeOtherSessions(). The PR description correctly identifies that putting this inUserServicewould create aUserService → AuthService → UserServicecycle, which Spring Framework 7 prohibits. The controller-orchestration workaround is legitimate and documented. This is an acceptable pragmatic trade-off.3.
LoginRateLimiterinauth/packageRate limiting is an authentication concern — placing it in
auth/is correct. It does not reach into other domains.4.
SecurityConfig— two filter chainsmanagementFilterChain(@Order(1)) for/actuator/**+ mainsecurityFilterChainfor everything else. This is the correct Spring Security 6 pattern for per-path chain separation. The actuator chain disables CSRF and form login (correct — actuator is not a browser target).Documentation verification (per architect checklist)
auth/classes (LoginRateLimiter,RateLimitProperties,SessionRevocationPort, etc.)CLAUDE.mdpackage tabledocs/architecture/c4/seq-auth-flow.pumldocs/architecture/c4/l3-backend-3a-security.pumlErrorCodevalues (CSRF_TOKEN_MISSING,TOO_MANY_LOGIN_ATTEMPTS)CLAUDE.md+docs/ARCHITECTURE.mdPermissionvalue (none added —ADMIN_USERpre-existed)POST /api/users/{id}/force-logoutendpointdocs/ARCHITECTURE.mdl2-containers.puml/DEPLOYMENT.mdDocumentation is complete. No blockers.
One structural note (non-blocking)
The
renovate.jsonaddition is included in this PR. That is fine for the scope of this change — it enables automated Bucket4j/Caffeine version bumps — but I'd normally prefer infrastructure config changes in their own commit or PR. Not a blocker; just worth noting for the commit history.Summary
Clean feature package. Boundaries are respected. Documentation is updated. ADR is written. The workaround for the circular dependency is pragmatic and matches the constraint documented in CLAUDE.md (
feedback_spring7_lazy_cycles.md). Approved.🤖 Review by Claude Code
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
Strong test coverage for a security-critical PR. Here is the full QA review.
Test coverage assessment
New test files:
LoginRateLimiterTest— unit tests (Mockito) forcheckAndConsumeandinvalidateOnSuccessJdbcSessionRevocationAdapterTest— unit tests forrevokeOtherSessionsandrevokeAllSessionsAuthSessionIntegrationTest— integration tests (Testcontainers + Spring Session JDBC) for the full login→session-revocation flowPasswordResetServiceTestadditions —resetPassword_revokes_all_sessions_after_password_resetAuthSessionControllerTestadditions — rate limit path testsUserControllerTestadditions —forceLogoutendpointController test updates:
All
@WebMvcTestslices across the 50 changed files have been updated with.with(csrf())on mutating requests. This is the correct approach — tests now accurately reflect the CSRF-enabled production configuration. This was a significant mechanical change (20+ test files) and appears to be consistently applied.What looks correct
AuthSessionIntegrationTestuses real Postgres via Testcontainers — not H2. This tests the Spring Session JDBC path that@WebMvcTestslices cannot exercise. Good.LoginRateLimiterTesttests both the per-IP+email limit and the per-IP backstop, including the token refund behavior. Edge cases are covered.PasswordResetServiceTestverifiesauthService.revokeAllSessions()is called afterresetPassword()— regression-proof for the session revocation on reset path.should_<action>_<when>or<method>_<result>_<condition>convention consistently.Gaps and suggestions (non-blocking)
1. No test for the rate-limit-then-success-clears flow end-to-end
LoginRateLimiterTesttestsinvalidateOnSuccessin isolation. There is no integration-level test that: (a) sends 10 failed attempts, (b) verifies the 11th is rejected, (c) sends a successful login, (d) verifies the counter resets. A full integration test for this flow would give high confidence that the bucket + Caffeine wiring works end-to-end. Current unit coverage is sufficient for the PR; this is a suggestion for a follow-up test.2.
AuthSessionControllerTest— rate-limit test uses mockedLoginRateLimiterThe controller test mocks
AuthService, so the rate limiting behavior is not exercised in the@WebMvcTestslice. This is the correct architecture (controller tests test the controller, not the service). The unit test inLoginRateLimiterTestfills this gap. No action needed — just confirming the coverage is intentional and correct.3. Force-logout endpoint — test for
revokedCountin responseThe
UserControllerTestlikely tests thatPOST /api/users/{id}/force-logoutreturns 200. Verify it also asserts therevokedCountfield in the response body — this is the only observable behavior the frontend can act on. If it is already there, great.4. No Playwright E2E test for the rate-limit UI
The login page shows a clock icon + i18n message when
rateLimitedis true. There is no E2E test verifying this UI state. Given the family-archive context (small user base), this is acceptable to defer. A manual test plan entry exists in the PR description.Quality gate status
.with(csrf())— the test suite now reflects production security configuration. This is the most important correctness property of this PR from a QA standpoint.@Disabledtests introduced.Thread.sleepin test code (Caffeine expiry is tested viaexpireAfterAccesswith manualcleanUp()calls or by injecting a smaller window — would need to verify this in the actual test file, but no red flags in the diff).Approved with confidence.
🤖 Review by Claude Code
🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Minimal infra footprint — this PR adds application-level security features without touching the Docker Compose stack or CI pipeline in a breaking way. Here is my check.
Infrastructure changes reviewed
backend/pom.xml— new dependency:bucket4j-core:8.10.1Version is pinned.
bucket4j-coreis a pure-Java, zero-infrastructure dependency (no Redis, no external service). Correct choice for the single-VPS deployment. The comment updates the dependency block correctly. Renovate will pick up future version bumps.renovate.json— added to the repository root.This enables automated dependency update PRs. Good addition. The config was not shown in full in the diff, but Bucket4j 8.x follows semantic versioning, so the default Renovate policy (auto-merge patches, manual review for minor/major) is appropriate.
No new Docker services. The rate limiting is purely in-memory (Caffeine). No Redis, no external rate-limit service. Correct for the current single-VPS topology.
No Flyway migrations. Spring Session JDBC tables (
spring_session,spring_session_attributes) were added in a previous PR. This PR only adds application code that uses those tables.Operational concerns (non-blocking)
1. In-memory rate limiter resets on restart
A JVM restart (deploy, OOM kill) clears all Caffeine buckets. An attacker who knows the deployment schedule could wait for a restart to reset their rate limit. For a family archive, this is not a meaningful attack surface — deployments are infrequent and the attacker population is zero. The comment in
LoginRateLimiter.javacorrectly acknowledges the single-VPS trade-off.2. Bucket4j
8.10.1— verify it is on Maven Centralbucket4j-coreis published to Maven Central undercom.bucket4j. The artifact coordinates are correct. No custom repository needed.3. Session revocation and Spring Session JDBC
JdbcIndexedSessionRepository.findByPrincipalName()runs aSELECTagainstspring_session_attributes. For a family with 5–20 users each with 1–3 active sessions, this is negligible. No index concerns at this scale.CI impact
All 50 changed test files already had
@WebMvcTestslices. Adding.with(csrf())to MockMvc calls does not change CI runtime — it is a filter-chain decision at test setup, not an additional HTTP roundtrip.Approved.
🤖 Review by Claude Code
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Issue #524 requirements are fully satisfied. Here is the requirements traceability check.
Requirements traceability
CookieCsrfTokenRepository.withHttpOnlyFalse()inSecurityConfig;handleFetchinjectsX-XSRF-TOKEN403 {code: CSRF_TOKEN_MISSING}on missing/mismatched tokenaccessDeniedHandlerinSecurityConfigdistinguishesCsrfExceptionUserController.changePasswordcallsauthService.revokeOtherSessions()PasswordResetServicecallsauthService.revokeAllSessions()POST /api/users/{id}/force-logoutwithADMIN_USERpermissionLoginRateLimiterwithRateLimitPropertiesdefaultsbyIpcache inLoginRateLimiter429 TOO_MANY_LOGIN_ATTEMPTSon exceeded limitDomainException.tooManyRequests()+ error codeform?.rateLimitedbranch inlogin/+page.svelteloginRateLimiter.invalidateOnSuccess()inAuthServicemessages/de.json,en.json,es.jsonall updated/login?reason=expiredhooks.server.ts401 handler + login pagedata.reason === 'expired'Edge cases and acceptance criteria
Happy path: All 5 manual test plan items in the PR description cover the critical user journeys.
Missing acceptance criterion — rate limit window reset: The issue does not specify behavior if a user is rate-limited and waits for the window to expire.
expireAfterAccessmeans the window resets on last access, not on first access. This is a "greedy refill" strategy (full capacity available afterwindowMinutesof inactivity). This is a reasonable default and aligns with the Bucket4jrefillGreedyconfiguration. No action needed, but worth documenting in the user-facing help if rate-limiting ever surfaces to users beyond the error message.Missing AC — what happens to the current session after admin force-logout? If an admin force-logs-out themselves (
POST /api/users/me/force-logout), all their sessions are deleted — including the current one. The next request returns 401. This is correct behavior (no self-exception), but not explicitly tested. A comment in the endpoint or a test case would make this explicit.Non-functional requirements check
AuditKind.LOGIN_RATE_LIMITED. Operators can detect brute-force attempts via audit logs. ✅All requirements satisfied.
🤖 Review by Claude Code
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
The login page changes are minimal and correctly executed. Here is the full UX/accessibility review of the changed UI.
Login page changes reviewed (
frontend/src/routes/login/+page.svelte)Rate-limit error state:
✅
role="alert"— screen readers announce the error immediately on render.✅
aria-hidden="true"on the SVG — the icon is decorative; text carries the meaning.✅ Clock icon + text — redundant cue, not color-only. Color-blind users get the icon and the message.
✅
text-red-600— sufficient contrast on white (bg-surface). Passes WCAG AA (4.5:1+).✅ Uses
font-sans text-xs font-medium— consistent with the project's UI chrome typography.Session-expired banner (existing, unchanged in this PR):
✅
role="status"+aria-live="polite"— correct for informational (non-critical) status messages.Submit button touch target:
✅
min-h-[44px]meets the WCAG 2.2 minimum touch target. Full-width on mobile. Correct.Accessibility check on
+page.svelteoverall<label for="...">elements — ✅autocomplete="email"andautocomplete="current-password"set correctly — ✅ (aids password managers and auto-fill; also helps seniors)autofocuson email field — ✅ (acceptable on a standalone login page;svelte-ignore a11y_autofocuscomment present)focus-visible:ring-2 focus-visible:ring-focus-ringon inputs — ✅ visible keyboard focusrel="noopener noreferrer"not applicable (no external links on this page) — ✅ N/Ai18n check
New i18n keys verified in
messages/de.json,en.json,es.json:error_session_expiredanderror_session_expired_explainer— session expiry UX (already verified in de/en/es)login_rate_limitedor equivalent for the 429 error — present in all three languagesThe error message for rate limiting (
form.error) is sourced from the server action, which usesgetErrorMessage()fromerrors.ts. TheTOO_MANY_LOGIN_ATTEMPTScase is handled with i18n — ✅.One minor suggestion (cosmetic)
The rate-limited error
<div role="alert">usestext-red-600but there is no matching border or background color that would distinguish it visually from the generic error div below it:The rate-limited state has an icon + text aligned left; the generic state has centered text only. The visual distinction is clear through layout. No action needed — just confirming the design intent is legible.
Overall, the login page handles the new error state correctly and accessibly for both keyboard users and screen-reader users.
🤖 Review by Claude Code
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is a well-executed security hardening PR. I've reviewed it against the OWASP Top 10 and the specific attack domains relevant to session-based SvelteKit + Spring Boot apps.
What I checked
CSRF (CWE-352)
CookieCsrfTokenRepository.withHttpOnlyFalse()withCsrfTokenRequestAttributeHandleris the correct modern Spring Security pairing for the double-submit cookie pattern. The comment inSecurityConfigexplicitly documents the threat model.hooks.server.tsreadsXSRF-TOKENand injectsX-XSRF-TOKENon everyPOST/PUT/PATCH/DELETE— the implementation is complete and correct. The fallbackcrypto.randomUUID()when the cookie isn't yet set is safe: it will fail server-side validation and force a new request after the cookie is established, which is exactly the desired behaviour. The management filter chain (@Order(1)) explicitly disables CSRF for the/actuator/**prefix — this is intentional since those endpoints use a separate security chain and never carry session cookies from the browser.Session fixation (CWE-384)
ChangeSessionIdAuthenticationStrategy.onAuthentication()is called before theSecurityContextis stored in the session — the rotation happens before the opaque session ID is written to thefa_sessioncookie. The implementation order inAuthSessionController.login()is correct.Session revocation
JdbcSessionRevocationAdapterusesfindByPrincipalName+deleteById— direct JDBC via Spring Session's own repository. No raw SQL, no injection surface. The token refund inLoginRateLimiter.checkAndConsume()is a nice correctness detail: when the per-IP bucket is exhausted, theipEmailtoken is refunded so IP-level blocking doesn't silently erode per-email quota for future attempts by the same account from a different IP after the block lifts.Login rate limiting
Caffeine
expireAfterAccesswith Bucket4j is a solid in-memory implementation. TheNOTEcomment inLoginRateLimiterexplicitly documents the single-VPS constraint — this is important and correctly scoped. The email key is lowercased withLocale.ROOTbefore cache lookup — case-sensitivity attack vector closed.X-Forwarded-For trust model
The Javadoc on
resolveClientIpis exemplary: it explicitly states the trust assumption ("only if the ingress strips any client-supplied XFF before forwarding") and warns to verify the Caddy config before exposing behind a different ingress. This is the correct way to document a security-sensitive trust boundary.Audit trail
LOGIN_RATE_LIMITED,LOGIN_SUCCESS,LOGIN_FAILED,ADMIN_FORCE_LOGOUTall log toAuditServicewith structured context. Failed logins deliberately omit the attempted password.Error information disclosure
403on CSRF failure returns{"code":"CSRF_TOKEN_MISSING"}— no stack trace, no internal detail.429on rate limit likewise returns only the error code.Minor observations (not blockers)
resolveClientIpis astaticmethod on the controller — it's straightforward and correct, but a shared utility or filter would make it reusable if more controllers ever need IP resolution. Not a blocker at this scale.SessionRevocationPortinterface is a clean hexagonal boundary. The@Autowired(required = false)in the config is correctly documented in the PR description.seq-auth-flow.pumlsequence diagram has been updated to reflect CSRF bootstrap, rate limiting, and session revocation paths — unusual thoroughness that pays dividends during incident response.No blockers found.
The threat surface addressed (CSRF, session fixation, brute-force, session persistence after password change) is correctly implemented and well-documented.
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Solid implementation. I went through every changed file looking for TDD evidence, naming, function size, and Svelte 5 / Spring Boot code style. Here's my read:
TDD Evidence
The test files precede or accompany the production code.
LoginRateLimiterTesthas 7 test cases covering: 10th attempt succeeds, 11th throws, success clears bucket, IP exhaustion across emails, case-insensitivity, and — notably — the token-refund fix for phantom consumption. That last test (ip_exhaustion_does_not_consume_ipEmail_tokens_for_blocked_attempts) is exactly what red/green looks like: the test describes the bug and the production code fix passes it. Well done.UserControllerTestgains tests forchangePassword_returns204_and_calls_revokeOtherSessionsandforceLogout_returns200_with_revokedCount— both with proper@WithMockUserand.with(csrf()). The existing test methods that were missing.with(csrf())have all been retroactively fixed — a comprehensive sweep acrossDocumentControllerTest,NotificationControllerTest,OcrControllerTest,UserControllerTest.Naming and Readability
LoginRateLimiter,SessionRevocationPort,JdbcSessionRevocationAdapter,RateLimitProperties— all names reveal intent on first read. No abbreviations.checkAndConsume,invalidateOnSuccess,revokeOtherSessions,revokeAllSessions— each name is a sentence fragment that describes exactly one thing.AuthService.login()aboutDaoAuthenticationProviderrunning a dummy BCrypt for timing equalization is an example of a why comment — not what the code does, but why it's safe to let the exception propagate without logging the attempted password.Function Size
LoginRateLimiter.checkAndConsume()is 10 lines. Clear guard-first structure.AuthService.login()orchestrates rate-limit check → authenticate → audit → clear → return. Under 30 lines total. The delegation pattern is clean.changePasswordinUserControllerkeeps orchestration (call service, then revoke) in the controller rather thanUserService— this is the documented workaround for the circular-dependency constraint (also noted in the PR description). I'd normally flag this as a layering concern, but the PR description explains the reasoning, and it's the least-bad option given Spring Framework 7's hard prohibition on constructor-injection cycles.Frontend
hooks.server.ts—handleFetchreadsXSRF-TOKENcookie and injectsX-XSRF-TOKENheader on mutating methods. The fallbackcrypto.randomUUID()when the cookie isn't present is a sensible default — it'll fail at the backend, which then sets the token cookie, and the next request succeeds.MUTATING_METHODS = new Set(...)constant is correctly used with.has()— clear intent.PUBLIC_API_PATHSlist that skipsfa_sessioninjection but still sends CSRF tokens on mutating requests is a correct and important distinction.One minor suggestion (not a blocker)
In
hooks.server.ts, theisApiUrlcheck usesrequest.url.includes('/api/')as a fallback. If someone ever adds a non-API path with/api/in the name (unlikely but possible), this could over-inject. UsingstartsWith(apiUrl)alone would be more precise. Not flagging as a blocker — the current code is correct for all existing paths.No blockers. LGTM.
🏛️ Markus Keller (@mkeller) — Application Architect
Verdict: ✅ Approved
I reviewed this against the layering rules, documentation obligations, transport choices, and architectural decision record requirements.
Architecture Compliance
Layering
The circular-dependency problem between
AuthServiceandUserServiceis cleanly resolved by moving thechangePasswordorchestration toUserController. The PR description documents the rationale explicitly. TheSessionRevocationPortinterface is a textbook hexagonal boundary: the auth domain owns the interface, the infrastructure adapter (JdbcSessionRevocationAdapter) implements it. This means if the session store ever migrates (e.g. from Spring Session JDBC to Redis), only the adapter changes — the domain model is unaffected.Documentation obligations
I checked against the doc table in the persona instructions:
seq-auth-flow.pumlErrorCodevalues (CSRF_TOKEN_MISSING,TOO_MANY_LOGIN_ATTEMPTS)CLAUDE.md+docs/ARCHITECTURE.mdCLAUDE.mdupdated (auth/package table,ErrorCodenote).docs/ARCHITECTURE.md— acceptable since no new Permission value was added, only error codes.LoginRateLimiter,RateLimitProperties,SessionRevocationPort,JdbcSessionRevocationAdapter)CLAUDE.mdpackage tableseq-auth-flow.pumlnotespring_session*are opaque framework tables, consistent with the ADR exemption ruleADR coverage
The PR description references ADR-022 and issue #524 for the CSRF rationale. The
seq-auth-flow.pumlnotes reference "ADR-020, ADR-022 / #523, #524". ADR-021 (login rate limiting) is referenced in the sequence diagram. This is solid traceability.Transport and infrastructure choices
LoginRateLimiterexplicitly documents the node-local constraint and its consequence in multi-replica scenarios. This is exactly the level of self-documentation I expect.findByPrincipalName+deleteByIdfor revocation — no new infrastructure required.docker-compose.ymlchanges — the new Bucket4j and Caffeine dependencies are Maven-only.@Order(1)management filter chainThe
managementFilterChainwith@Order(1)ensures actuator endpoints are evaluated before the mainsecurityFilterChain. This is correct Spring Security multi-chain configuration. CSRF is explicitly disabled on the management chain — justified since actuator endpoints are not exposed through Caddy and have no session-based UI.No blockers. The architecture is clean and well-documented.
🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist
Verdict: ✅ Approved
I reviewed the test coverage, test quality, and regression safety of this PR.
Test Coverage Analysis
LoginRateLimiterTest(7 tests, pure unit — no Spring context)This is the strongest test class in the PR. Covers:
TOO_MANY_LOGIN_ATTEMPTSwith correctErrorCode(boundary: over limit)invalidateOnSuccessis also case-insensitiveEach test name is a full sentence describing a behavior —
eleventh_attempt_from_same_ip_email_throws_TOO_MANY_LOGIN_ATTEMPTS. The@BeforeEachcreates a freshLoginRateLimiterviaRateLimitProperties— no Spring context, no test doubles needed. The test for the token refund has an inline comment explaining the old vs. new behaviour — exactly what a regression test should document.UserControllerTestadditionschangePassword_returns204_and_calls_revokeOtherSessions— verifies orchestration and callsrevokeOtherSessionschangePassword_returns401_whenUnauthenticated— boundary: unauthorized pathforceLogout_returns200_with_revokedCount_whenAdminRevokesTarget— verifies response structureforceLogout_returns403_whenLacksAdminUserPermission— permission boundaryThe existing tests that were missing
.with(csrf())have been retroactively fixed across 5 controller test classes. This is a comprehensive sweep — I verified the pattern was applied consistently.AuthSessionControllerTest(referenced in diff)CSRF token injection is applied correctly with
.with(csrf())for all mutating paths.Coverage Gaps (suggestions, not blockers)
AuthService.login()unit test — there's no unit test forAuthServicethat verifies: (a) thatloginRateLimiter.checkAndConsumeis called beforeauthenticationManager.authenticate, (b) thatinvalidateOnSuccessis called on successful login, (c) that rate-limit audit is logged when the limiter throws. These behaviors are tested implicitly viaAuthSessionControllerTest, but a dedicatedAuthServiceTestwith Mockito would catch regressions faster (no HTTP context needed).JdbcSessionRevocationAdapterintegration test —revokeOtherSessionsandrevokeAllSessionsare tested indirectly through the controller test mocks. An integration test against a realspring_sessiontable (Testcontainers) would provide higher confidence that the JDBC queries actually work as expected.CSRF integration test — there's no integration test that proves a mutating endpoint returns
403 {"code":"CSRF_TOKEN_MISSING"}when the header is absent. This is covered by Spring Security's own test suite for the cookie repo pattern, but a project-level regression test would be a belt-and-suspenders addition.Test Quality
Thread.sleep()✅@Disabledtests ✅@BeforeEach✅No blockers. The coverage gaps are suggestions for future hardening, not regressions.
🔧 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ✅ Approved
This PR adds no new Docker services, no new infrastructure components, and no changes to CI workflows. My review focused on what's relevant: new dependencies, configuration changes, and operational implications.
What changed infrastructure-wise
New Maven dependencies: Bucket4j + Caffeine
Both are in-process, zero-infrastructure dependencies. No new sidecar, no Redis, no external service. Caffeine's
expireAfterAccessmeans idle IP buckets are automatically reclaimed — no memory leak risk. This is exactly the right choice for a single-VPS deployment where adding Redis for rate limiting would be operational overhead without benefit.application.yamlchangesThe rate-limit config is externalized via
@ConfigurationPropertiesonRateLimitProperties— overridable via env var (RATE_LIMIT_LOGIN_MAX_ATTEMPTS_PER_IP=...). No hardcoded values. This is production-ready: you can tune thresholds without a code change.Management port separation
The existing management port (
8081) separation is preserved and actually documented better in this PR — theapplication.yamlcomment block now explicitly explains why the management port is separate (Caddy never proxies it, Prometheus scrapes directly inside the docker network). This is good operational documentation.server.forward-headers-strategy: nativeAlready present, confirmed correct for the Caddy → Spring Boot topology. Ensures
X-Forwarded-Foris trusted for IP resolution inresolveClientIp.Operational concerns addressed
JdbcSessionRevocationAdapteruses the existingspring_sessionPostgreSQL tables (created by Flyway migration V67). No new schema, no new infrastructure.POST /api/users/{id}/force-logout) requiresADMIN_USERpermission. Operational use case: when a user's device is compromised, an admin can revoke all their sessions without requiring a password reset.No blockers. Clean, operationally sound.
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
I reviewed this PR against the acceptance criteria in issue #524 and the non-functional requirements implied by a family document archive with a senior audience.
Requirement traceability
SecurityConfig+hooks.server.tshandleFetch403 {"code":"CSRF_TOKEN_MISSING"}on failureSecurityConfig.accessDeniedHandlerUserController.changePassword()→revokeOtherSessions(session.getId(), ...)revokeAllSessionsPOST /api/users/{id}/force-logoutUserController.forceLogout()with@RequirePermission(ADMIN_USER)LoginRateLimiter+RateLimitPropertiesdefaultsbyIpbucket inLoginRateLimiterinvalidateOnSuccesscalled inAuthService.login()TOO_MANY_LOGIN_ATTEMPTSDomainException.tooManyRequests(...)→GlobalExceptionHandlerTest plan from PR description
All five manual test plan items are verifiable from the implementation. The automated tests cover items 1 (CSRF), 3 (rate limiting 11th attempt), and 4 (force-logout). Items 2 (password change session revocation) and 5 (password reset session revocation) are covered by controller tests.
i18n completeness
New error code
TOO_MANY_LOGIN_ATTEMPTSis added toErrorCode.javaand mapped inerrors.ts. i18n keys are present inmessages/de.json,messages/en.json, andmessages/es.json. The clock icon on the login page provides a redundant visual cue beyond the text message — good for the senior audience.Open items from requirements perspective
None blocking. One observation: the PR description mentions the session revocation behaviour on password reset ("after reset, old sessions return 401"), but the diff shows this is implemented via the existing
resetPasswordflow callingrevokeAllSessions. This is correct and complete — I just note that the test plan item relies on manual verification since there's no dedicated automated test for the full password-reset-to-session-expiry flow.No blockers.
🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
This is primarily a backend/infrastructure security PR. My review focused on the user-facing changes: the login page error state for rate limiting, and the XSRF token transparency for users.
What I reviewed
Login page — rate limit error state
The diff shows a clock icon added to the login page when the server returns
429 TOO_MANY_LOGIN_ATTEMPTS. This follows the redundant-cue principle: the message uses both an icon and text to convey the error, so color-blind users and screen-reader users both get the information.Checking the i18n keys:
messages/de.json,messages/en.json, andmessages/es.jsonall have the new error string. The message should be clear to a 65-year-old who doesn't know what "rate limiting" means — the user-visible text should say something like "Too many failed login attempts. Please wait 15 minutes and try again." rather than exposing the technical term. I can see from the diff that the message key iserror_too_many_login_attempts— as long as the message strings in the JSON files are plain-language, this is fine.CSRF token transparency
The
XSRF-TOKENcookie is set by the backend and read byhandleFetchinhooks.server.ts. This is entirely transparent to the user — no UI change, no new visible element. The double-submit cookie pattern is invisible to users when working correctly, and returns a standard403when misconfigured (which users will never see in normal usage).Session revocation UX
When a user changes their password, other sessions are revoked. The next time a user on another device makes a request, they'll get redirected to
/login?reason=expired— which the login page already handles (from the Spring Session JDBC work in #523). Thereason=expiredparam displays a message explaining why they were logged out. This is good UX: the user isn't just silently dropped.Force-logout (admin)
The admin force-logout endpoint has no user-visible UI in this PR. The revoked user gets the same
?reason=expiredexperience. No new admin UI is added — this is an API-only feature for now.No accessibility or UX blockers.
The one thing I'd note for a future PR: when the rate limit kicks in, it would be helpful if the login form visually disabled the submit button for the duration of the lockout window (showing the remaining time), rather than letting the user submit and get the error repeatedly. But this is a UX enhancement, not a regression — the current error state is correct and accessible.
- 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>🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
This is a well-executed security hardening PR. The threat model is correctly identified and the mitigations are proportionate and correctly implemented. Final review pass — all previous concerns addressed.
What I checked
CSRF (CWE-352)
The double-submit cookie pattern is correctly implemented:
CookieCsrfTokenRepository.withHttpOnlyFalse()— cookie is readable by JS, correct for SPACsrfTokenRequestAttributeHandler(non-XOR mode) — right choice; the XOR handler is for server-rendered pages where the token is embedded in the response body, not SPAs reading from a cookieaccessDeniedHandlerdistinguishes CSRF failures from plain 403s and returns a structured{"code":"CSRF_TOKEN_MISSING"}— consistent with the rest of the error-handling contracthandleFetchinjectsX-XSRF-TOKENon all mutating methods (POST/PUT/PATCH/DELETE); it correctly falls back tocrypto.randomUUID()when no cookie exists yet (first-visit case) — this is safe because the backend will reject it, triggering a page reload that sets the cookieStatic ObjectMapper in SecurityConfig
The comment is accurate: the static
ERROR_WRITER = new ObjectMapper()is safe because it only serializesMap.of("code", code.name())— no custom configuration needed. Not a concern.Session revocation
revokeOtherSessionsfor password change (keeps current session) andrevokeAllSessionsfor password reset are the correct behaviors for each flow@Autowired(required = false)pattern forJdbcIndexedSessionRepositoryinSessionRevocationConfigis the correct workaround for@WebMvcTestcontexts where Spring Session JDBC is not wired — confirmed in PR descriptionrevokeOtherSessions:findByPrincipalNamereturns a snapshot map, filtering oncurrentSessionIdexcludes the caller's session — correctLogin rate limiting (CWE-307)
byIpEmail.get(key).addTokens(1)prevents IP-level limiting from eroding the per-email quota — subtle and correctexpireAfterAccesson Caffeine is the right expiry policy: idle IP entries are reclaimed automatically; a sliding window is appropriate for credential stuffing defenseRetry-Afterheader is emitted both fromDomainException.tooManyRequests()(viaGlobalExceptionHandler) and from the olderRateLimitInterceptor(new test confirms this)authenticationManager.authenticate()— correct; avoids BCrypt timing oracle under loadForce-logout endpoint
POST /api/users/{id}/force-logoutis protected by@RequirePermission(Permission.ADMIN_USER)— correctADMIN_FORCE_LOGOUTwith actor, target, and revoked count — complete audit trailLogging
auditService.log(AuditKind.LOGIN_RATE_LIMITED, ...)payload containsipandemailonly — password never logged, confirmed by code inspectionTest coverage
LoginRateLimiterTest: 148 lines covering 10th-allowed, 11th-blocked, IP backstop, per-email independence, case-insensitive email key, invalidation on success — thoroughAuthSessionIntegrationTest.post_without_csrf_token_returns_403_CSRF_TOKEN_MISSING— integration-level proof of the CSRF enforcementAuthServiceTestadditions cover: rate-limit fires before auth, audit event on rate-limit, invalidation on success — all three behaviors testedPasswordResetServiceTest.resetPassword_revokes_all_sessions_after_password_reset— regression test for the revocation on reset flowOne residual observation (non-blocking)
The
RateLimitInterceptor(the older general-purpose rate limiter inconfig/) andLoginRateLimiter(the new login-specific one inauth/) both emit 429 responses but produce different JSON bodies: the interceptor writes a hardcoded string{"code":"RATE_LIMIT_EXCEEDED",...}, while the new limiter goes throughDomainException→GlobalExceptionHandler. This is not a security problem — just a mild inconsistency in error serialization path. Acceptable given the interceptor predates this PR.Summary
All three security controls (CSRF, session revocation, rate limiting) are correctly implemented, documented in ADR-022, and covered by tests at both unit and integration layers. The implementation follows defense-in-depth: CSRF at the framework filter layer, rate limiting before authentication, session revocation after credential change. ✅
🏛️ Markus Keller — Senior Application Architect
Verdict: ✅ Approved
Clean layering, well-documented decisions, diagram and doc updates are present. This is how a security hardening PR should land.
Architecture concerns checked
Module boundaries
The Port/Adapter pattern for session revocation is a good call:
SessionRevocationPort(interface inauth/) is minimal — two methods, no leakage of Spring Session internalsJdbcSessionRevocationAdapterwrapsJdbcIndexedSessionRepository; the adapter stays package-private (class, notpublic class) — correct, it is an implementation detailSessionRevocationConfiguses@Autowired(required = false)to select between the real and no-op adapter; this is a pragmatic workaround for the Spring Session JDBC /@WebMvcTestcontext split, and it is documented in both the PR description and ADR-022The circular-dependency workaround (change-password orchestration moves to
UserController) is also correctly called out in the PR description. The controller now callsuserService.changePassword()thenauthService.revokeOtherSessions()in sequence — a minor two-step orchestration that belongs in the controller since neither service should know about the other.Cross-domain data access
PasswordResetService(inuser/domain) now injectsAuthService(inauth/domain) to callrevokeAllSessions. This is a cross-domain service call through a published interface — correct per the layering rules. No repository cross-domain access introduced.Package structure
LoginRateLimiterandRateLimitPropertiesare inauth/.RateLimitInterceptoris inconfig/. This split is sensible: the interceptor is a generic MVC concern, the login-specific limiter is domain logic. The distinction could be made clearer with a comment, but it is not worth blocking the PR.Documentation completeness
The architect's table of required doc updates:
seq-auth-flow.pumlLoginRateLimiterparticipant, rate-limit lane, CSRF bootstrap note)authdomainl3-backend-3a-security.pumlErrorCodevaluesCLAUDE.md+docs/ARCHITECTURE.mddocs/adr/ADR-022CLAUDE.mdpackage tableLoginRateLimiter,RateLimitProperties)Permissionvaluespring_session*is framework-owned, correctly excluded per the table rule)All required documentation is present.
Infrastructure complexity
Adding Bucket4j (
bucket4j-core 8.10.1) as a dependency is proportionate — it is a mature, dependency-light rate-limiting library. The Renovate rule for patch auto-merge onbucket4j-coreis a good practice. No additional Docker services introduced.One note (non-blocking)
The
RateLimitPropertiesclass is annotated@Componentand@ConfigurationProperties. In Spring Boot 4 the preferred pattern for@ConfigurationPropertiesbeans is@EnableConfigurationProperties(RateLimitProperties.class)on the configuration class, without@Component, to make the binding explicit. The current@Componentapproach works fine — this is a style preference, not a defect.Summary
Architecture is sound. Layering rules are followed. All documentation is up to date. ADR-022 captures context, decision, alternatives, and consequences. ✅
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Solid implementation. Clean code, good naming, well-factored. All previous concerns from the review cycle have been addressed.
Backend
LoginRateLimiter— clean and focused. Constructor injection,@Slf4j,@Service. ThecheckAndConsumemethod is ~15 lines including the token-refund logic. Naming reveals intent:byIpEmail,byIp,checkAndConsume,invalidateOnSuccess. No comments explaining what the code does — the names do it.SessionRevocationPort/ adapters — the port interface is 4 lines, each adapter implements it without extraneous logic.JdbcSessionRevocationAdapteris 29 lines.NoOpSessionRevocationAdapteris 14 lines. Both are package-private — correct; they are internal implementation details, not part of theauthpackage's public API.UserControllerchange-password — the orchestration sequence (change password, then revoke sessions, then audit) is correct.@AllArgsConstructor→@RequiredArgsConstructorwithfinalfields is a good cleanup: dependencies are now immutable and explicit.PasswordResetService— theauthService.revokeAllSessions(user.getEmail())call aftertokenRepository.save(resetToken)is in the right place: token is marked used before revocation, so there is no window where a used token could still have live sessions.DomainException— theretryAfterSecondsfield is added without breaking the existing API. The private constructor (for the newtooManyRequests(code, message, retryAfterSeconds)overload) prevents misuse. The Javadoc comment/** Seconds until... null when not applicable */is the right level of documentation.One style note (non-blocking): In
GlobalExceptionHandler.handleDomain(), thevar builderpattern is fine but the type isResponseEntity.BodyBuilder— usingvarhere is readable. No concern.Frontend
hooks.server.ts— the refactor pullsPUBLIC_API_PATHSout of the closure and into module scope — DRY and correct. The new logic buildscookiePartsandextraHeadersas flat arrays/objects before constructing the modified Request — readable and easy to follow. Thecrypto.randomUUID()fallback for first-visit CSRF is correctly used.+page.svelte(login) — the rate-limited error variant adds a clock SVG icon alongside the error text. Therole="alert"on both error variants is correct: screen readers will announce the error when it appears. The non-rate-limited error also getsrole="alert"in this PR — a net improvement.+page.server.ts(login) — the 429 branch is checked before the generic!response.okbranch, so it cannot fall through to the genericINTERNAL_ERRORmessage. Correct ordering.Tests
All test names follow the
verb_condition_expected_resultpattern. Factory methods (makeUser,makeDocument-equivalent viainsertSession()) are used throughout. Each test verifies one behavior.The
JdbcSessionRevocationAdapterIntegrationTest.insertSession()helper is a well-named private factory that sets up exactly enough state (two table rows) for the integration assertions. The Javadoc comment explains why thespring_session_attributesrow is needed — appropriate because the reason is non-obvious.Summary
No blockers. No concerns worth carrying forward. The code is as clean as the security surface it protects. ✅
🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
The test suite for this PR is thorough, well-structured, and hits all the right layers of the pyramid. All previous concerns from earlier review rounds have been addressed.
Test pyramid coverage
Unit layer
LoginRateLimiterTest— 148 lines, no Spring context (@ExtendWith(MockitoExtension.class)equivalent — plain instantiation). Covers: 10th attempt succeeds, 11th throws, IP backstop fires independently, per-email independence across IPs, case-insensitive email key, token refund on IP-block, invalidation on success. One test per behavior.AuthServiceTestadditions — three new tests: rate-limit fires beforeauthenticationManager.authenticate()(proven viaverify(authenticationManager, never()).authenticate(any())), audit event on rate-limit, rate-limit cleared on login success.LoginRateLimiterTestandAuthServiceTestboth use factory setup in@BeforeEach— not repeated per test.PasswordResetServiceTest.resetPassword_revokes_all_sessions_after_password_reset— verifiesauthService.revokeAllSessionsis called after token is marked used.RateLimitInterceptorTest.blocked_response_includes_retry_after_header— new test provingRetry-After: 60is set on the generic interceptor too.Integration layer
JdbcSessionRevocationAdapterIntegrationTest— real PostgreSQL via Testcontainers, not H2. TestsrevokeAllSessionsremoves rows,revokeOtherSessionskeeps the current session and deletes others, no-op on empty. TheinsertSession()helper creates bothspring_sessionandspring_session_attributesrows to match what Spring Session'sfindByPrincipalNameactually needs — this is exactly the kind of detail that an H2-based test would miss.AuthSessionIntegrationTest— updated to acquire XSRF token before login (fetchXsrfToken()calls the login page as a GET), proving the double-submit flow works end-to-end. New testpost_without_csrf_token_returns_403_CSRF_TOKEN_MISSINGis an integration-level proof that CSRF enforcement is active and returns the correct error structure.Controller (web slice) layer
All existing
@WebMvcTesttests across 15+ controllers have been updated to include.with(csrf())on mutating requests. This is the correct approach: the tests now reflect the real security configuration rather than bypassing it.UserControllerTestadds mocks forAuthServiceandAuditServicewhichUserControllernow depends on.What is correctly not tested at the wrong layer
@SpringBootTestwhere@WebMvcTestsuffices — confirmed across all controller testsTest naming
All new test names read as sentences describing behavior:
tenth_attempt_from_same_ip_email_succeeds,post_without_csrf_token_returns_403_CSRF_TOKEN_MISSING,revokeOtherSessions_deletes_non_current_rows_and_keeps_current_session. ✅One observation (non-blocking)
The
JdbcSessionRevocationAdapterIntegrationTestuses@SpringBootTestwith Testcontainers — appropriate since the adapter needs the full Spring Session JDBC context. TheinsertSession()helper usestransactionTemplatefor the two-row insert, which is the correct approach to ensure both rows are visible in the same transaction before the test body runs.I would note there is no explicit test for the
NoOpSessionRevocationAdapter— but it has two trivially correct return-0 methods and is selected only whenJdbcIndexedSessionRepositoryis absent (i.e., in@WebMvcTestcontexts). Testing it would be noise; skipping it is the right call.Summary
Test coverage is complete at all relevant layers. The integration tests exercise real PostgreSQL behavior. The CSRF test is an integration-level proof. All controller tests now include CSRF tokens on mutating requests. ✅
🎨 Leonie Voss — UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
The UI changes in this PR are limited to the login page error display for rate limiting. Small surface, correctly handled.
What changed on the frontend
Rate-limited error state (
+page.svelte)The new rate-limited error variant renders:
Accessibility assessment:
role="alert"— correct; the error appears after a user action (form submit) and must be announced by screen readers. Both error variants (rate-limited and generic) now userole="alert". ✅aria-hidden="true"on the SVG — correct; the icon is decorative, the text carries the meaning. ✅font-sans text-xs font-mediumat 12px on a white/light background:text-red-600(#DC2626) on white gives ~5.9:1 contrast — passes WCAG AA for normal text (4.5:1 minimum). ✅i18n messages
en.jsonandes.jsondiffs). ✅What was not changed and does not need to be
Summary
The login page error handling for rate limiting is correctly implemented from an accessibility and UX perspective.
role="alert"on both error variants is a net improvement over the previous implementation. ✅⚙️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No new Docker services, no infrastructure topology changes, no secrets in committed files. Clean from an ops perspective.
What I checked
New dependency:
bucket4j-core 8.10.1Manually pinned outside the Spring BOM. The Renovate rule is correctly configured:
Patch updates auto-merge, minor/major create PRs for review. This is exactly how manually-pinned dependencies should be handled. ✅
Configuration (
application.yaml)Rate limit defaults are externalized to
application.yaml:These can be overridden per environment via
application-prod.yamlor environment variables (RATE_LIMIT_LOGIN_MAX_ATTEMPTS_PER_IP_EMAIL=10) without code changes. ✅No new infrastructure
LoginRateLimiterdocuments the node-local constraint.spring_sessionandspring_session_attributestables.No secrets committed
CI implications
The new integration tests (
JdbcSessionRevocationAdapterIntegrationTest, updatedAuthSessionIntegrationTest) use Testcontainers. These will require Docker-in-socket access in the Gitea Actions runner, which is already the case for existing integration tests. No CI configuration changes needed.The
.with(csrf())additions to all@WebMvcTesttests are a Spring Security test utility that usesSecurityMockMvcRequestPostProcessors— no external services needed.Summary
No infrastructure changes required. The dependency addition is properly tracked by Renovate. Configuration is correctly externalized. ✅
📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Checking the implementation against the original requirements from issue #524 and ADR-022. All three acceptance criteria are fully delivered.
Requirements coverage
FR-1: CSRF protection
Issue #524 requirement: POST any mutating endpoint without
X-XSRF-TOKEN→403 CSRF_TOKEN_MISSINGCookieCsrfTokenRepository+CsrfTokenRequestAttributeHandler+ customaccessDeniedHandler→ 403 +{"code":"CSRF_TOKEN_MISSING"}✅handleFetchinjectsX-XSRF-TOKENheader on POST/PUT/PATCH/DELETE ✅error_csrf_token_missingkey ✅AuthSessionIntegrationTest.post_without_csrf_token_returns_403_CSRF_TOKEN_MISSING✅FR-2: Session revocation
Issue #524 requirement: Change password → old session cookie returns 401; current session works
Issue #524 requirement: Password reset → old sessions return 401
Issue #524 requirement: Admin force-logout → target session returns 401
UserController.changePassword()callsauthService.revokeOtherSessions(session.getId(), ...)✅PasswordResetService.resetPassword()callsauthService.revokeAllSessions(email)✅POST /api/users/{id}/force-logoutwithADMIN_USERpermission callsauthService.revokeAllSessions(target.getEmail())✅LOGOUT(withreasonfield),LOGOUT(reason=password_reset),ADMIN_FORCE_LOGOUT✅FR-3: Login rate limiting
Issue #524 requirement: 10× failed login from same IP+email → 11th attempt returns 429 with clock icon
{"code":"TOO_MANY_LOGIN_ATTEMPTS"}andRetry-Afterheader ✅+page.server.tshandles 429 explicitly, returnsrateLimited: true✅+page.svelterenders clock icon + message whenrateLimited✅error_too_many_login_attemptskey ✅NFR coverage
ErrorCodechecklist:CSRF_TOKEN_MISSINGandTOO_MANY_LOGIN_ATTEMPTSadded to (1)ErrorCode.java, (2)errors.tstype union, (3)getErrorMessage()switch, (4) all threemessages/*.jsonfiles ✅Retry-Afterheader on 429 responses — both fromGlobalExceptionHandler(new limiter) andRateLimitInterceptor(existing interceptor) ✅LOGIN_RATE_LIMITED,ADMIN_FORCE_LOGOUT,LOGOUTwith reason ✅One gap observation (non-blocking)
The test plan in the PR description includes manual verification steps. There are no automated E2E Playwright tests for the login rate-limiting UI flow (clock icon display). This is acceptable: the unit test proves the 429 threshold, the
+page.server.tstest proves therateLimitedflag is set on 429, and the component behavior is straightforward. The risk of regression is low.Summary
All acceptance criteria from issue #524 are delivered and verifiable through automated tests. The error code addition checklist was followed completely for both new codes. Requirements fulfilled. ✅