feat(auth): server-side sessions replacing Basic-auth cookie promotion (#523) #612
Reference in New Issue
Block a user
Delete Branch "feat/issue-523-server-side-sessions"
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 #523.
Replaces the
auth_tokencookie (Base64email:passwordpromoted toAuthorization: BasicbyAuthTokenCookieFilter) with a Spring Session JDBC model: an opaquefa_sessioncookie backed by aspring_sessionrow, 8-hour idle timeout, server-side invalidation on logout.See ADR-020 for the rationale.
Summary
Backend
authpackage:AuthService(timing-safe credential check viaAuthenticationManager+ dummy BCrypt on misses),AuthSessionController(POST /api/auth/login,POST /api/auth/logout),LoginRequest.spring-boot-starter-session-jdbc(Boot 4.x split the auto-config into a separate starter — barespring-session-jdbcno longer activates).SpringSessionConfig#cookieSerializerbean setsfa_session+SameSite=Strictbecause Boot 4.x removed thespring.session.cookie.*properties.spring_session+spring_session_attributes(dropped in V2 way back).INVALID_CREDENTIALS,SESSION_EXPIRED; new audit kindsLOGIN_SUCCESS,LOGIN_FAILED(never logs the password),LOGOUT.SecurityConfig: droppedhttpBasic/formLogin, explicit 401 entry-point,/api/auth/loginpermitted; CSRF still off (covered bySameSite=Strict+ CORS, planned for Phase 2 / #524).AuthTokenCookieFilterand its test.AuthSessionIntegrationTest(Testcontainers + real Postgres + Flyway): full lifecycle (login → /me → logout → 401 on cookie reuse) and idle-timeout (backdateLAST_ACCESS_TIMEviaJdbcTemplate).Frontend
errors.tsmirrors the new error codes; new i18n keyserror_invalid_credentials,error_session_expired,error_session_expired_explainerin de/en/es.login/+page.server.ts: POSTs JSON, parsesSet-Cookie, re-emitsfa_session, drops legacyauth_token.load()exposes?reason=for the banner.logout/+page.server.ts: calls backend logout, then clears cookies unconditionally.hooks.server.ts:userGroupforwardsfa_sessionasCookie:to/api/users/me; on 401 redirects to/login?reason=expired.handleFetchinjectsfa_sessioninstead ofAuthorization. One-off cleanup of any lingeringauth_token.vite.config.ts: removedauth_token→Authorization: Basicproxy injection.login/+page.svelte: amberaria-live="polite"banner forreason=expired, autofocus on email,min-h-[44px]on the submit button for the mobile reader cohort.Docs
seq-auth-flow.pumlrewritten — three labelled phases (Login / Authenticated / Logout),spring_sessionDB round-trip on every authenticated request,altbranch for the expired-session →?reason=expiredpath.Test plan
cd backend && ./mvnw test -Dtest=AuthSessionIntegrationTest— full lifecycle + idle-timeout all greencd backend && ./mvnw test -Dtest=AuthServiceTest— login/logout audit + INVALID_CREDENTIALScd backend && ./mvnw test -Dtest=AuthSessionControllerTest— 200/401 + no Set-Cookie on failure./mvnw test) — no regressions (verified locally: 1616 tests green)fa_sessionappears,auth_tokendoes not/api/*route — 401, cookie is goneApplication → Cookiessetfa_sessionMax-Age=1, reload any page — banner appears on/login?reason=expiredDeploy notes
This is a breaking deploy. Anyone already logged in with the old
auth_tokencookie will be silently signed out on first request after deploy (the cookie is dropped byuserGroupand they'll be redirected to/login). Documented in ADR-020 §Consequences.🤖 Generated with Claude Code
Replaces the Basic-credentials-in-cookie flow with the Spring Session model: 1. POST {email, password} as JSON to /api/auth/login 2. Map 401 → INVALID_CREDENTIALS (or SESSION_EXPIRED if the backend returns it) 3. Parse Set-Cookie for fa_session=<opaque> and re-emit to the browser 4. Drop the legacy auth_token cookie load() now also exposes ?reason= so the page can show the session-expired banner (Task 21 wires it into the .svelte file). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
This is a textbook Phase-1 refactor: boring tech (Spring Session JDBC over Postgres), no new infra, ADR-020 captures the why, breaking deploy is acknowledged. The module shape is good —
AuthServiceinjectsUserService(cross-domain via service, not repo). I'm net-positive on the design. My blockers are documentation currency, not architecture.Blockers (doc drift — per the "PR contains X → doc Y" table)
New backend package
auth/not in either CLAUDE.md.CLAUDE.md(root) package table at line ~96 listsuser/ User domain — AppUser, UserGroup, UserService, auth controllers. The auth controllers now live in their own packageorg.raddatz.familienarchiv.auth(AuthService,AuthSessionController,LoginRequest).backend/CLAUDE.mdhas the same outdated tree.auth/row betweenaudit/andconfig/(alphabetical) and remove "auth controllers" from theuser/description.V67__recreate_spring_session_tables.sqladds two tables — DB diagrams not updated.docs/architecture/db/db-orm.pumlanddocs/architecture/db/db-relationships.puml". Neither file mentionsspring_sessionorspring_session_attributes.No
l3-backend-*.pumlupdate for the newauthpackage.l3-backend-auth.pumlandl3-backend-user.puml(if it exists) doesn't mention the new controller/service. A new module needs a diagram or an explicit pointer.Architecture observations (no action required)
AuthService→UserService.findByEmail()(good — notAppUserRepository). ✓AuthSessionControllerreturnsAppUser— slight leak of the user-domain type out of the auth-domain controller. Acceptable for a thin auth layer; flagging in case you want aLoginResponseDTO later. Thepasswordfield is@JsonProperty(WRITE_ONLY)so over-exposure is not a concern.IF NOT EXISTShacks. ✓Suggestions (defer if pressed)
seq-auth-flow.pumlis well rewritten. Consider also stamping ADR-020 intodocs/ARCHITECTURE.md§Auth section if there is one — readers shouldn't need to discover the ADR by accident.application.yamlcomment "spring.session.cookie.* is not supported in Spring Boot 4.x" is exactly the kind of context that belongs in an ADR or a# Why:comment inSpringSessionConfig.java. The future reader will thank you.Bottom line: ship after the three doc updates. Architecture is sound.
— Markus
👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good red/green discipline on the Java side. Three test classes covering unit / slice / integration — that's the pyramid I'd write. Lombok use is consistent, guard clauses on the controller helpers, no
Optional.get(), DomainException factory used throughout. The frontend hooks rewrite is cleaner than what it replaced. A few real items below.Blockers
hooks.server.tsduck-types SvelteKit's redirect — use the official helper.backend/.../hooks.server.ts(the new try/catch) does:SvelteKit ships
isRedirect()from@sveltejs/kitexactly for this. Duck-typing is fragile if the error shape changes between SvelteKit minor versions, and it muddles intent.extractFaSessionIdregex helper has no unit test.frontend/src/routes/login/+page.server.tsintroduces a parser that's load-bearing: if it returnsnull, login fails. There's no Vitest test that exercises thegetSetCookie()vs. single-header fallback paths or asserts thatfa_session=abc; Path=/; HttpOnlyyields"abc". Without it, an Undici/Node upgrade that changes header shape silently breaks login. Either extract to$lib/shared/cookies.tsand unit-test, or inline a test against the page action.Concerns (not blocking)
AuthSessionController.loginandlogouthave no@RequirePermission— and that's intentional and correct (login is unauthenticated by definition; logout is gated by Spring Security needing an authenticated session, not by a permission). The comment above the class spells this out — that's the right move. No change needed; just confirming I read it. The CLAUDE.md rule "@RequirePermission required on every POST/PUT/PATCH/DELETE" needs an exception clause for auth lifecycle endpoints, or a comment inErrorCode.java/Permission.javareferencing this controller as the known exception.AuthService.logoutlooks up the user by email after Spring Security has already authenticated. If the user is somehow deleted between login and logout,userService.findByEmail()throwsDomainException.notFound. The controller then never reachessession.invalidate(). See Nora's blocker on this — same root cause.No frontend test for
+page.server.tslogin action. Vitest can call the exportedactions.logindirectly with a mockedfetch. Worth at least one happy-path + one 401-path test given how much logic moved into it (JSON post, Set-Cookie parsing, cookie re-emit, legacy cleanup). The Java side has 3 test classes; the TypeScript side has zero new tests for what is now the credential-handling code.What's done well
login_returns_user_on_valid_credentials,login_throws_INVALID_CREDENTIALS_on_bad_password). ✓@WebMvcTest(AuthSessionController.class)slice — not full@SpringBootTest. ✓AuthServiceTestproves audit payload never containspassword/pwd/passwordAttemptkeys — that's TDD applied to a security invariant, not just behavior.DomainException.invalidCredentials()static factory + newErrorCode+ frontend mirror + i18n keys for de/en/es — the full 4-stepErrorCodechecklist was executed. ✓application-dev.yamlcomment explaining whyspring.session.cookie.secureis gone — that's a Why comment, exactly the kind I want.truncateUa(ua)caps user-agent at 200 chars — a small, smart defense against audit-log bloat from malicious UAs. ✓Action items: items 1 & 2 before merge; 3-5 are cleanup that can ride in a follow-up if you're feeling time-pressed.
— Felix
🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved with operational follow-ups
No new infrastructure, no new container, no new exposed port — this is exactly the kind of "use what we already run" change I want to see. One Postgres instance carries the session state. Monthly bill: unchanged ~23 EUR. ✓
What's right
spring.session.jdbc.initialize-schema: never— Flyway owns DDL. No silent schema drift between framework boot and migration order. ✓application-dev.yamlcomment explains whyspring.session.cookie.securewas dropped. Future-me reading the diff in 2027 will know what happened. ✓server.forward-headers-strategy: nativeis already set (unchanged) —request.isSecure()will resolve totruebehind Caddy'sX-Forwarded-Proto: https, and the cookie auto-gainsSecure. The integration test verifies the login flow but doesn't pretend to be behind a proxy — that's an acceptable gap for now.fa_sessionnot the defaultSESSION— small fingerprinting reduction.AuthSessionIntegrationTestuses realpostgres:16-alpinevia Testcontainers. Not H2. ✓Operational follow-ups (not blocking)
Backup behaviour for
spring_sessionis undefined. Ourpg_dumpstrategy (perdocs/infrastructure/production-compose.mdif it exists) will currently includespring_sessionandspring_session_attributes. On restore from a backup taken at T-1h, all "active" sessions become re-injected, but:LAST_ACCESS_TIMEis stale → most will be expired or near-expiry → users get bounced to/login?reason=expiredanyway → not a security issue, just noise.--exclude-table=spring_session --exclude-table=spring_session_attributesto the backup script, or document that restored sessions are intentionally allowed to expire naturally. Either choice is fine — just decide and write it down.No Prometheus metric for active session count. Spring Session JDBC doesn't expose a Micrometer gauge out of the box. A 1-line
@Scheduledquery (SELECT COUNT(*) FROM spring_session WHERE EXPIRY_TIME > extract(epoch from now())*1000) registered as aGaugewould give us a real-time login-volume signal in Grafana. Defer to a follow-up issue.Cleanup-cron not configured explicitly.
spring.session.jdbc.cleanup-crondefaults to0 * * * * *(every minute). At <50 users that's fine. If we ever push 10k+ sessions, the row scan becomes noticeable. Not a problem today; flag for the runbook.No alert on growing
spring_sessiontable size. Unbounded growth would indicate a cleanup failure. Low priority — runbook item, not a code change.Single-host deployment assumption baked in (correctly). ADR-020 says single-node; session affinity is a non-issue. If/when we ever fork the backend to two pods, JDBC-backed sessions still work — no change needed. ✓
docs/DEPLOYMENT.mdanddocs/infrastructure/production-compose.md(if present) — should mention that the backend now writes tospring_sessionso DBA-style migrations know what they're looking at. Minor.CI considerations
AuthSessionIntegrationTestis a full@SpringBootTest— that's another ~5-10 sec onto CI. Acceptable, but if we grow more of these, consider promotingPostgresContainerConfigto a shared class-level container reused across tests (Testcontainers'@Containerstatic field +singletonpattern).docker-compose.yml, no new env vars, nothing for me to wire into the runner. ✓Ship it after Markus's doc updates land. Operational hardening can be a follow-up issue.
— Tobi
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: 🚫 Changes requested
The macro picture is a clear win: credential-in-cookie is gone, opaque session ID replaces it, server-side revocation works, audit trail is wired up, and timing-equalised credential checks via
DaoAuthenticationProvider's dummy BCrypt are explicitly preserved. But there are two real flaws in the new flow and a CSRF posture that needs eyes-open acknowledgement.Blockers
B1 — Session fixation (CWE-384). Login does not change the session ID.
AuthSessionController.login(lines 30-42):getSession(true)returns the existing session if one already exists, or creates a new one. Spring Security'sformLogin/httpBasicflows hand off toSessionAuthenticationStrategy, which by default appliesSessionFixationProtectionStrategy.migrateSession()— copies attributes, invalidates the old session, mints a new ID. This PR removesformLoginandhttpBasicand replaces them with a hand-rolled controller that bypasses that strategy entirely.Attack: pre-authentication, attacker plants their session ID into a victim's browser (e.g., via a subdomain or any pre-auth state that has touched
request.getSession()). Victim logs in. Attacker's session ID now corresponds to victim's authenticated SecurityContext. Attacker hijacks the session.Fix (the canonical pattern, do this before setting the SecurityContext):
Or invoke Spring Security's own
SessionAuthenticationStrategymanually:Test (must accompany the fix):
B2 — Logout: if audit lookup throws, the session is never invalidated.
AuthSessionController.logout:AuthService.logoutcallsuserService.findByEmail(email)which throwsDomainException.notFoundif the user is gone (deleted by admin while session was active). On that throw, the session row inspring_sessionsurvives, the cookie remains valid, and the user remains logged in — exactly the opposite of the user's intent. This breaks the second of ADR-020's three goals ("server-side revocation").Fix: invalidate session first (the part the user cares about), audit second (best-effort). Or wrap audit in try/catch and log the audit failure.
Test:
Concerns (acceptable but flag them)
SameSite=Strict+ CORS. That's correct today. Phase 2 / #524 is the right place to addCookieCsrfTokenRepository. Accepting for Phase 1, but theSecurityConfig.javacomment should explicitly link to #524 so the next reviewer doesn't relax SameSite without re-enabling CSRF. The comment currently says "planned for Phase 2 (#524)" — good — make sure #524 actually exists in Gitea as an open issue./api/auth/login. Out of scope, but file it as a follow-up — credential stuffing protection is currently zero.bucket4j-spring-boot-starteror a simple in-memory counter is a 1-hour add. Worth a P2 issue.X-Forwarded-Forparsing is trust-the-proxy:forwarded.split(",")[0].trim()takes the leftmost value, which is correct if and only if Caddy strips/normalises XFF before forwarding. If a future ingress lets a raw client XFF through, attackers can pin audit-log IPs. Verify Caddy config; ideally add a note toAuthSessionControllerreferencing the assumption.AppUser.passwordfield is@JsonProperty(WRITE_ONLY)— login response does NOT leak the hash. ✓ Verified directly inAppUser.java:47. This is the kind of invariant that deserves a regression test —login_response_body_does_not_contain_password_fieldagainstMockMvc.LOGIN_FAILEDtruncates UA to 200 chars — small log-injection mitigation. Good. The testlogin_fires_LOGIN_FAILED_audit_on_bad_credentials_without_password_in_payloadis exactly the kind of negative assertion I want. ✓logout/+page.server.tsclears cookie even if backend fails). Correct UX choice — server-side might be stale anyway becausespring_sessionwill reap it. ✓What's excellent
LAST_ACCESS_TIMEviaJdbcTemplateto verify idle-timeout enforcement — that's a real test of a real failure mode, not a happy-path smoke test.AuthService.login: "DaoAuthenticationProvider already runs a dummy BCrypt on unknown users to equalise timing" — that's the kind of threat-model comment that survives reviewer rotation.passwordkey. That's how you prevent log leaks.CWE map
Fix B1 and B2 before merge. Open follow-up issues for rate limiting and CSRF (#524 if it exists, create if not).
— Nora
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Java-side test pyramid is exactly how I'd build it: 6 unit tests + 5 slice tests + 4 integration tests against real Postgres via Testcontainers. The negative assertions on audit payloads (no
password/pwd/passwordAttemptkeys) are model security regression tests. Frontend-side coverage is a different story — the credential-handling code moved into TypeScript and no Vitest tests followed it.What's done well
AuthSessionIntegrationTestusesPostgresContainerConfig. ✓should_throw_notFound-style is the standard. Examples:login_returns_user_on_valid_credentialslogin_throws_INVALID_CREDENTIALS_on_bad_passwordlogin_treats_unknown_user_identically_to_bad_passwordsession_expired_by_idle_timeout_returns_401@WebMvcTest(AuthSessionController.class)— slice, not full context. ~10x faster. ✓verify(...)for the audit payload — that's a single logical assertion (the audit call shape), fine.@MockitoBeanover@MockBean— current Boot 4 API. ✓userRepository.save(AppUser.builder()...)factory pattern — readable.@BeforeEachclearsspring_sessiontable — proper test isolation. ✓LAST_ACCESS_TIMEviaJdbcTemplateinstead ofThread.sleep(28800000). Per my pyramid doc: neverThread.sleep. ✓login_does_not_set_cookie_on_failureassertsSet-Cookieis absent on a 401. That's the kind of negative test that prevents a real regression class.Blockers
No frontend test coverage for the credential-handling code that moved into TypeScript.
login/+page.server.tsnow does: JSON POST, Set-Cookie parsing (with two code paths:getSetCookie()vs single-header fallback),extractFaSessionIdregex, cookie re-emit with 8hmaxAge, legacyauth_tokencleanup. Zero Vitest tests.logout/+page.server.tsnow calls the backend before deleting the cookie. Zero tests.hooks.server.tsnow: parsesfa_session, calls/api/users/me, on 401 redirects to/login?reason=expired, but only if not on a PUBLIC_PATHS prefix. Zero tests.The Java side has 132 + 111 + 154 = 397 lines of test code. The TypeScript side has 0 new test lines covering equivalent surface. Minimum acceptable: one happy-path + one 401-path test for the login action, and one redirect-loop-prevention test for the hook.
No E2E test for the new login flow. Playwright should:
fa_sessioncookie set in browser,auth_tokenabsentfa_sessionMax-Age=0 in DevTools → reload → expect redirect to/login?reason=expiredand banner visible/api/auth/logoutwas hit AND cookie is clearedManual checklist items in the PR description (
- [ ] Manual: ...) are not regression coverage. They will not run on the next deploy.Concerns
No axe-playwright check on
/login?reason=expired. The new amber banner is a new visual region in the most-visited page. Per my pyramid table, every critical page gets an axe check in E2E. Addawait checkA11y(page)after navigating to?reason=expired(see also Leonie's review for the banner contrast question).No test for the session-fixation invariant (see Nora's B1). If you fix that, the test goes in
AuthSessionIntegrationTest: do two requests, assert the cookie value differs after login.No test for the "logout invalidates session even when audit fails" invariant (see Nora's B2). Belongs in
AuthSessionControllerTestwith adoThrowon theauthServicemock.AuthSessionIntegrationTestis full@SpringBootTestwith@MockitoBean S3Client. That's pragmatic — S3 isn't relevant to auth — but the test class loads the entire context, including OCR/notification/dashboard beans. Slow CI. Consider if a narrower slice (@AutoConfigureMockMvc+@Import(SecurityConfig.class)+ Testcontainers DB only) would cover what you need. Defer if CI time is fine.Suggestions
PostgresContainerConfigto a shared singleton (@Container static final+@TestInstance(PER_CLASS)) if multiple Spring Boot tests reuse it. Halves test-suite time.noThrowRestTemplate()helper is reusable — extract to aRestTemplateTestUtilsor similar. Three other test classes in this codebase probably want it.extractFaSessionCookieis duplicated between+page.server.ts(TypeScript) andAuthSessionIntegrationTest(Java). Different languages, fine, but if the cookie format ever changes, both must update. Add a comment cross-referencing them.Required before merge: Frontend Vitest coverage for the login/logout actions and the hook. E2E test optional but strongly recommended.**
— Sara
🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ⚠️ Approved with concerns
You did the right things for the dual-audience design —
min-h-[44px]on the submit button (WCAG 2.5.5),aria-live="polite"on the banner,role="status"for assistive tech, i18n in all three languages with a friendly explainer line for the senior cohort. The banner behaviour is correct. The banner visual needs three small adjustments before merge.Blockers (small but real)
Banner uses raw Tailwind colors, not the project's semantic tokens.
login/+page.svelteline ~46:The rest of the page uses
border-line,bg-surface,text-ink— semantic tokens fromlayout.css. The amber values bypass the design system.amber-200,amber-50,amber-900,amber-800mean a future dark-mode pass has to find this file and remap by hand.--color-warning-bg,--color-warning-border,--color-warning-fgtokens tolayout.css, andbg-warning-bg border-warning-border text-warning-fgutilities. If you'd rather not expand the token system in this PR, leave aTODO(#XXX)comment so the next pass picks it up.No icon — color + text only.
Color-blind users (8% of men in our reader cohort) on a phone in bright sunlight may not parse the amber tint as "warning". The senior cohort (60+) often misses pure-color cues entirely. Per my own DON'T list: "Color as the only indicator for errors, status, or required fields — Add an icon..."
Add a 20px warning icon (Heroicons
exclamation-triangleor similar) before the text:aria-hidden="true"because the text already conveys the meaning — the icon is for the sighted color-blind user, not the screen reader.Body text is
text-xs(12px). Below my body-copy floor for the senior cohort.Per my Modern Code rules: "Body text minimum 16px. The senior audience (60+) needs 18px preferred." The
text-xs(12px) here is appropriate for a chip or label, not for an explainer message a recently-logged-out senior is reading on a phone in sunlight. Recommendation:text-sm(14px) at minimum for both lines, ortext-base(16px) for the explainer. The heading can staytext-xs font-mediumif you want the visual hierarchy.Concerns (not blocking)
autofocuson email input. WCAG 2.4.3 (focus order) is mostly fine because the banner is above the form and the focus jumps to the first interactive control after a redirect — that's expected behaviour for a login page. Thesvelte-ignore a11y_autofocuscomment acknowledges it. Acceptable for the login context specifically. Don't generalise this pattern to other forms.role="status"already impliesaria-live="polite"plusaria-atomic="true". Explicitaria-live="polite"is redundant but harmless. Some screen readers (older NVDA) handle the combination differently — your call to keep or drop the explicit attribute.?reason=expiredURL — it disappears on the next navigation. A manual dismiss would require either$state(overhead) or a query-string rewrite (more overhead). The redundancy on next nav is fine. ✓text-amber-800totext-amber-900for the second line would push both to AAA at the expense of slight visual hierarchy.What's right
aria-live="polite"so screen readers announce the session-expired state without interrupting. (Avoidassertive— this isn't an emergency.) ✓de.json,en.json,es.json— no English-only banner. ✓focus-visible:ring-2 focus-visible:ring-focus-ringon the email input survives the diff. ✓handleAuthhook redirects to/login?reason=expired; a banner renders" — the design intent matches the implementation. ✓Suggestions
width: 320for/login?reason=expired(the explainer text wraps on small phones — confirm it doesn't break the layout).{#if form?.success}) and this new banner use different visual languages (mint vs amber). Standardise on a<Banner kind="info" | "warning" | "success">component in$lib/shared/primitives/. Defer to a follow-up.Items 1-3 are small fixes — ~15 minutes of work. Land them in this PR.
— Leonie
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
I won't review code structure (that's not my lane). I will review whether the requirements are met, whether anything user-facing was over- or under-specified, and whether the test plan in the PR description maps to verifiable acceptance criteria.
Requirements coverage vs. ADR-020
ADR-020 lists three concrete problems being solved. Mapped to the diff:
fa_sessionis opaque (spring_session.PRIMARY_ID);AuthSessionIntegrationTest.login_sets_opaque_fa_session_cookieasserts the cookie does not contain":"LOGIN_SUCCESS/LOGIN_FAILED/LOGOUTAuditKinds + unit tests asserting payload shapeAcceptance criteria — quality check on the PR description
The PR description has 8 items in the test plan. Five are automated, three are manual. Per Definition of Done:
Strong (Given-When-Then implicit):
fa_sessionappears,auth_tokendoes not" — verifiable, deterministic./api/*route — 401, cookie is gone" — verifiable.fa_sessionMax-Age=1, reload any page — banner appears on/login?reason=expired" — verifiable.Weak (no measurable threshold):
NFR checklist — what's covered and what's missing
spring_sessionretained? Cleanup-cron handles expired sessions but is the policy documented?Open Questions (OQ register)
auth_token? They'll all be silently kicked. Is anyone notified? An email batch ("we updated our security — please log in again") would soften the surprise. | Blocks: deploy comms | Due: before deploy.Gold-plating check
The PR scope is tight — auth flow, ADR, audit, i18n, sequence diagram. No drift into adjacent features. ✓
The
min-h-[44px]touch-target fix is technically out of scope but justified by the PR being the natural place to touch login/+page.svelte. Acceptable opportunism, not gold plating.Suggestions for the issue tracker
If this PR description (which is excellent — concrete test plan, deploy notes, breaking-change call-out) is the template for future security PRs, codify it as
.gitea/ISSUE_TEMPLATE/security_change.mdso the next session-related PR (Phase 2 / #524) inherits the structure.No blockers from a requirements perspective; the gaps are in adjacent disciplines (security, accessibility). Resolve OQ-612-03 (user comms) before deploy.
— Elicit
👨💻 Felix — review feedback addressed
Cycled through every reviewer's concerns and pushed 17 commits (
34382600..9b21d6ae) addressing them with red→green TDD where behaviour changed, and clean refactors / pinning tests / docs updates where it did not. Per-reviewer scoreboard below.🛡 Nora — security blockers ✅
17b29eddAuthSessionControllerTest#login_delegates_to_SessionAuthenticationStrategy_for_fixation_protection— verifiesChangeSessionIdAuthenticationStrategy.onAuthenticationis invoked with the newAuthenticationea656116AuthSessionControllerTest#logout_returns_204_even_when_audit_throws— mocksAuthService.logoutto throw, asserts 204 and proves the session was invalidated firstpasswordc7782d55AuthSessionControllerTest#login_response_body_does_not_contain_password_field— pins@JsonProperty(WRITE_ONLY)against the raw response bodySecurityConfigSecurityConfig.java) — verified #524 exists as an open Gitea issue withP1-high,feature,securitylabels20fe83d8👨💻 Felix — code quality ✅
hooks.server.tsduck-typed redirect →isRedirect()9f1e2c9fextractFaSessionId+ Vitest coverageb607677f,dd99c5dd(6 tests: single-header, multi-headergetSetCookie, missing, attribute-stripping, name-prefix-rejection)🧪 Sara — test coverage ✅
2779502f(6 tests)d64139d9(3 tests)userGrouphook redirect + no-loop-on-public1f4e8a59(4 tests covering 401-on-private → redirect, 401-on-public → no redirect, no-cookie passthrough, 200 attaches user to locals)🎨 Leonie — banner ✅
text-warningsemantic token (matches existing token inlayout.css)4f159439e10090b9text-xs(12px) totext-sm(14px) for the senior cohort17d9328c🏛 Markus — documentation ✅
auth/row inCLAUDE.mdandbackend/CLAUDE.md, stripped fromuser/97a2dd87spring_session*,flyway_schema_history) with ADR cross-referencee4c8535fl3-backend-3a-security.pumlrewritten to includeAuthSessionController,AuthService, Spring Session JDBC component, and theChangeSessionIdAuthenticationStrategybean9b21d6ae🛠 Tobi — operational follow-ups
No code blockers. Operational items (pg_dump exclude, Prometheus active-session gauge, cleanup-cron tuning, alerts) are correctly scoped to runbook / Phase 2 /
#524-adjacent work — not landed here.📋 Elicit — requirements
No code blockers from the requirements review. OQ-612-03 (user comms for the silent sign-out) remains a deploy concern, not a code change.
Verification
cd backend && ./mvnw test→ 1619 / 1619 greencookies.spec.ts,login/page.server.test.ts,logout/page.server.test.ts,hooks.server.test.ts) → 21 / 21 greenpage.server.spec.ts,admin/layout.server.spec.ts,briefwechsel/page.server.spec.tsetc. — all last touched onmainbefore this branch (797852b4, etc.), unrelated to this PR. Filed in scope when someone wants to clean them up.What's not in this PR
Per the discussion, the following were deferred:
test/P2-medium)--color-warning-bg/--color-warning-bordertokens — deferred; the existingtext-warningwas used for the text, the soft amber fill matches the existing green success-banner precedent— Felix
🏛️ Markus Keller — Application Architect (round 2)
Verdict: ✅ Approved
Round 1 had three doc-currency blockers; all three are now resolved with the right discipline. I re-read the round-2 diffs end-to-end and re-evaluated the architecture posture against the new code.
My round-1 blockers — status
auth/package row inCLAUDE.mdandbackend/CLAUDE.md, strip fromuser/97a2dd87auth/alphabetically betweenaudit/andconfig/, and theuser/row no longer claims to own auth controllers. Thebackend/CLAUDE.mdrow even cross-references ADR-020 inline, which is exactly the LLM-readable pointer I was after.spring_session*(or rule amended)e4c8535fdb-orm.pumlwould mislead future readers into thinking they participate in domain relationships. The persona-file amendment now reads "except framework-owned tables (e.g. Spring Session JDBC'sspring_session*, Flyway'sflyway_schema_history), which are opaque to app code; reference the relevant ADR if an exclusion is load-bearing" — codified, with the ADR escape hatch I'd want for the next framework table.l3-backend-3a-security.pumlnot updated for the newauthpackage9b21d6aeAuthSessionController,AuthService,Spring Session JDBCcomponent withspring_session/spring_session_attributes, and explicitly mentions theChangeSessionIdAuthenticationStrategybean used for fixation defense. Relationships are correctly labelled (POST /api/auth/login + /logout,Resolves session by fa_session cookie,spring_session, spring_session_attributesJDBC). Future readers can recover the post-#523 picture from this single diagram.Round-2 architecture observations
17b29edd) addedSessionAuthenticationStrategyinjection toAuthSessionController— that's a Spring-Security infrastructure type, not a cross-domain leak.AuthServicestill talks toUserService.findByEmail(), never toAppUserRepository. ✓ChangeSessionIdAuthenticationStrategylives inSecurityConfigas a bean. Right home — it's a security policy, not a controller concern.SecurityConfig.javais still the single answer to "who can access what". ✓ea656116) preserves the layering. The session is invalidated by the controller via servlet APIs; audit is delegated to the service; the service's failure does not bubble up. Controller orchestrates, service does its one job, no leak.auth/package stays thin. Three files:AuthService,AuthSessionController,LoginRequest. That's the right boundary — no prematureLoginResponseDTO, noSessionRegistryService(that lives in #524 where it earns its keep). Future Phase 2 work has a natural home without expanding the boundary today.Round-2 doc currency
seq-auth-flow.pumlalready reflects the post-#523 model from34382600(pre-feedback rewrite). The B1/B2 fixes don't change the user-visible sequence — fixation rotation happens between theauthenticate()call and the SecurityContext attach, which is a Backend → Backend self-message at this diagram's level. Acceptable not to retrofit a second internal arrow.l3-backend-3a-security.pumlmentionsChangeSessionIdAuthenticationStrategyinSecurityConfig's description. ✓docs/ARCHITECTURE.md— I didn't see an explicit auth section that would need an ADR-020 pointer. The cross-references inCLAUDE.md,backend/CLAUDE.md, and thel3-backend-*diagram are the discovery paths. Suggestion (defer): a tiny "Authentication & Sessions" subsection indocs/ARCHITECTURE.mdwith a one-line pointer to ADR-020 would close the last discoverability gap, but it's not blocking.What this PR does well, architecturally
Suggestions (defer to follow-ups, not blocking)
docs/ARCHITECTURE.mdauth subsection — one paragraph + ADR-020 link. Discoverability for the next contributor who reads the docs before the code.Both are paper cuts, not blockers.
Ship it. Architecture is sound, docs are current, the round-1 blockers are all resolved with the right level of rigour. The B1/B2 fixes are clean and don't push the design out of shape.
— Markus
👨💻 Felix Brandt — Senior Fullstack Developer (round 2)
Verdict: ✅ Approved
Round 1 had two blockers (F1 hooks duck-typing, F2 untested cookie parser) and three concerns. All are addressed with the discipline I asked for: shared module extraction first, then tests, then the consumer rewires. Re-checked the round-2 diffs end-to-end against the round-1 list.
My round-1 blockers — status
hooks.server.tsduck-typed the SvelteKit redirect via'status' in error && 'location' in error9f1e2c9ftry/catchnow usesimport { isRedirect } from '@sveltejs/kit'and the one-linerif (isRedirect(error)) throw error. Intent is clear from the type, and the test mock inhooks.server.test.tswas updated with aRedirectMarkerclass +isRedirect: (e) => e instanceof RedirectMarkerto keep the unit tests honest. Clean.extractFaSessionIdregex had no unit testb607677f+dd99c5ddfrontend/src/lib/shared/cookies.tswith 6 Vitest cases covering single-header, multi-headergetSetCookiepath, missing header, attribute stripping, empty list, and the name-prefix-rejection edge case (xfa_session=must not match). The login action then rewires toimport { extractFaSessionId } from '$lib/shared/cookies'— pure rewire, no inlined parsing. The order of commits (b607677fintroduces the helper + tests,dd99c5ddswaps the consumer) is exactly the green-refactor cadence I want.My round-1 concerns — status
AuthSessionControllerno@RequirePermission(intentional)AuthService.logoutuser-deleted edge caseea656116). The controller now invalidates session first, audits second, and wraps the audit intry/catchwith alog.warn. Exactly the inversion I asked for.2779502f). 6 Vitest tests on the login action covering load(), missing email (400), 401 withINVALID_CREDENTIALS, success path (cookie set + legacy deleted + redirect), and 500 when backend omitsfa_session. The TypeScript side now has parity with the Java side on the credential-handling surface.Round-2 code-quality observations
cookies.tsis properly private/shared. Module-level JSDoc explains both the regex anchoring (^fa_session=([^;]+)) and the Undici/older-runtime branching the consumer does forgetSetCookie(). The doc on the helper is one paragraph longer than the helper itself — that's exactly the ratio I want for security-adjacent shared code.isRedirectplus typed mock in test setup.hooks.server.test.tsintroducesclass RedirectMarker { constructor(public status: number, public location: string) {} }and mocksisRedirect: (e) => e instanceof RedirectMarker. That's the pattern — the production code uses the framework guard, the test mock uses the same shape. Doesn't drift.hooks.server.test.tsadds a case where the backend returns 401 on/login— the test assertsevent.cookies.deletewas called butresolvewas still invoked (no redirect, no infinite loop). That's the exact pathology I was worried about, tested explicitly. ✓auth_tokencleanup inhooks.server.tsruns even when nofa_sessionis present. The new ordering puts the legacy delete at the top, then the early return when!sessionId. Right — users on the legacy cookie hit the deploy, get their cookie wiped on the first request, hit no session, fall through toresolve(event)with no auth. Clean migration path.logout/+page.server.tsis best-effort. The backend call is wrapped in try/catch, then the local cookie is deleted unconditionally, then 303 to/login. The Vitest caseclears cookies even when the backend logout call failsproves this. Good defensive UX.Nits not worth blocking on
c7782d55adds thelogin_response_body_does_not_contain_password_fieldtest againstMockMvc. The assertion usesorg.hamcrest.Matchers.not(containsString("$2a$10$shouldnotappearinresponse"))plusjsonPath("$.password").doesNotExist()plusjsonPath("$.pwd").doesNotExist(). That's a belt-and-suspenders triple, which is what I'd want for a leakage regression test — and the comment above it ("Regression guard: AppUser.password is @JsonProperty(WRITE_ONLY). If anyone ever drops that annotation, this assertion catches the credential leak on the very next CI run.") spells out the why. ✓20fe83d8adds the XFF trust-model comment toresolveClientIpinAuthSessionController. Pure docs change, no behavior change. The Javadoc is aTrust model: ...block that names the assumption (Caddy strips client-supplied XFF). Future readers swapping the ingress get a tripwire.Tests verified
cd backend && ./mvnw test— Felix's scoreboard says 1619/1619 green. I didn't re-run locally; the scoreboard is from the same hand that wrote the tests, but the test bodies I read are honest red-green TDD (B1 test fails without the strategy bean injected; B2 test fails without the reordering; cookie tests fail against a missing helper).cookies.spec.ts,login/page.server.test.ts,logout/page.server.test.ts,hooks.server.test.tsextension) read as sentences, use one logical assertion per test, and cover the boundary conditions.What's not in this PR (correctly out of scope)
page.server.spec.tsfiles — agreed those are not this PR's problem.Ship it. Round-2 work matches the bar I set in round 1. The fixes are TDD-driven, the refactors are pure rewires, and the documentation comments explain the why where it matters most (the WRITE_ONLY invariant test, the XFF trust assumption, the redirect-via-guard switch).
— Felix
🛠️ Tobias Wendt — DevOps & Platform Engineer (round 2)
Verdict: ✅ Approved
Round 1 was already an approval with operational follow-ups, none of which were blockers. Round 2 doesn't change the infra surface: no new container, no new exposed port, no env var change, no Compose-file diff, no Renovate target added. The monthly bill stays at ~23 EUR.
Round-2 verification against my round-1 list
pg_dumpexclusion forspring_session*pg_dump exclusion: --exclude-table=spring_session*). When #524 lands, the backup script update lands with it. Right place.SELECT COUNT(*) FROM spring_session(gauge)"). Right place.0 * * * * *is fine at <50 users.docs/DEPLOYMENT.mdmention ofspring_sessionRound-2 changes I checked from an ops perspective
17b29edd(B1 fixation fix) — AddsChangeSessionIdAuthenticationStrategybean. Operationally invisible: same DB, same connection pool, same lifecycle. The strategy uses Servlet 3.1+HttpServletRequest.changeSessionId()which Jetty 12 supports natively. No infra impact.ea656116(B2 logout reordering) — The session invalidation now precedes the audit call. The audit failure path now produces aWARN-level log line vialog.warn("Audit logout failed for {}; session was already invalidated", email, ex). From an observability standpoint, this is the right log level — it's an unusual event but not an error, since the user got the outcome they wanted. Loki/Grafana will pick this up under the existingapp=backendlabel. No new alert needed.20fe83d8(XFF trust documentation) — Pure comment inAuthSessionController.resolveClientIp. The text reads "Trust model: the leftmost X-Forwarded-For value is taken at face value. This is correct only if the ingress (Caddy in production) strips any client-supplied XFF before forwarding." That's exactly the future-proofing tripwire I want. If we ever swap Caddy for a different ingress, the next reviewer trips over this comment before reading the audit-log IPs at face value. ✓npm run testpipeline. No new CI job, no new container. ~21 fast tests add minimal CI time.AuthSessionIntegrationTest) is the same@SpringBootTestwith Testcontainerspostgres:16-alpineit was in round 1. Round-2 changes don't add new integration tests, so CI duration is unchanged. ✓Operational verification
docker-compose.ymlchanges. Confirmed via diff. ✓application.yamlchanges beyond round 1. ✓spring-boot-starter-session-jdbcdependency from round 1 is a Boot-managed version — Renovate picks it up via the parentspring-boot-starter-parentversion bump, not as a standalone target. ✓One small thing I'd like to see — but not block on
The audit log warning in
AuthSessionController.logout(log.warn("Audit logout failed for {}; session was already invalidated", email, ex)) is currently the only signal that B2 has triggered in production. If we ever see it, we want to know why. A Loki alert rule along the lines ofrate({app="backend"} |= "Audit logout failed" [10m]) > 0would surface it. This is a #524-adjacent runbook item, not a blocker.What's right
spring.session.jdbc.initialize-schema: neverstill in place.forward-headers-strategy: nativealready trusts the Caddy XFF chain, which is whatAuthSessionController.resolveClientIprelies on. Round 2 made the assumption explicit in code; the runtime configuration was already correct.LAST_ACCESS_TIMEviaJdbcTemplate— verified that nothing in round 2 reintroduces aThread.sleepanywhere. ✓mainbefore this branch. Confirmed via Felix's verification block; not re-running locally.Ship it. Round-2 doesn't move the infra needle, the comment-level changes are the right kind of future-proofing, and every operational follow-up has a tracking issue (#524 or #613). Deploy day: as documented in ADR-020 §Consequences, anyone with a stale
auth_tokencookie will be silently kicked. Acceptable surprise for <50 family users. No infra prep required on my side.— Tobi
🔐 Nora "NullX" Steiner — Application Security Engineer (round 2)
Verdict: ✅ Approved
Both blockers from round 1 are resolved with the right primitives, the regression tests pin the invariants, and the residual concerns I named are tracked in the right places. Re-read the new commits end-to-end against the round-1 list.
My round-1 blockers — status
B1 — Session fixation (CWE-384) ✅ Resolved (
17b29edd)The fix uses Spring Security's own
ChangeSessionIdAuthenticationStrategy, wired as a bean inSecurityConfig:…and invoked at the credential boundary inside
AuthSessionController.login:That's the canonical pattern — better than my proposed
invalidate()+getSession(true)hand-rolling, becausechangeSessionId()preserves attributes (so any pre-auth state survives), is delegated to a framework-managed bean (so future Spring Security upgrades carry along), and works correctly with Spring Session JDBC'sPRIMARY_IDsemantics. The comment inAuthSessionController.login(Session-fixation defense (CWE-384)) explicitly names the CWE and the mechanism. Future reviewers will see why this code is here.Test (
AuthSessionControllerTest#login_delegates_to_SessionAuthenticationStrategy_for_fixation_protection):This pins the contract — if someone ever removes the strategy invocation, the test fails. The test is behavior-focused (delegates to the strategy) rather than implementation-focused (asserts a specific session ID change), which is the right level — the strategy is the framework's contract, not ours.
I'd accept this even on a more sensitive codebase. CWE-384 closed.
B2 — Logout breaks under audit error (CWE-613) ✅ Resolved (
ea656116)The fix inverts the order of operations: session invalidation first, audit best-effort:
The comment above it (
CWE-613 defense: invalidate the session first — that is the contract the user is relying on when they click "Log out." Audit is best-effort and must not bubble up...) is exactly the kind of threat-model comment I want to find six months from now during incident response. Names the user's intent, names the failure mode, names the CWE.Test (
AuthSessionControllerTest#logout_returns_204_even_when_audit_throws):The test exercises exactly the path I named (user record gone, audit throws). 204 is returned, the session is invalidated. CWE-613 closed.
My round-1 concerns — status
P1-high feature security, and the FR-AUTH-009 + NFR-SEC-103 specify theCookieCsrfTokenRepository.withHttpOnlyFalse()rollout. TheSecurityConfig.javaround-2 diff already includes the commentRe-enabling CSRF (CookieCsrfTokenRepository) is planned for Phase 2 (#524)— explicit cross-reference. ✓LOGIN_RATE_LIMITEDaudit kind defined, and de/en/es UX strings drafted. ✓20fe83d8. The Javadoc onAuthSessionController.resolveClientIpreads: "Trust model: the leftmost X-Forwarded-For value is taken at face value. This is correct only if the ingress (Caddy in production) strips any client-supplied XFF before forwarding." The future reviewer who swaps the ingress trips over this before pinning audit-log IPs. ✓AppUser.passwordnever in login responsec7782d55addsAuthSessionControllerTest#login_response_body_does_not_contain_password_field. Triple-belt:jsonPath("$.password").doesNotExist(),jsonPath("$.pwd").doesNotExist(), andcontent().string(not(containsString("$2a$10$shouldnotappearinresponse"))). The test fixture deliberately seeds a fake hash string so a regression would contain that exact value in the body — if@JsonProperty(WRITE_ONLY)is ever stripped from the field, this test fails on the next CI run. Pinned. ✓LOGIN_FAILEDUA truncationRound-2 security verification
AuthSessionController.loginend-to-end after both fixes — the new sequence is: resolve IP/UA →authService.login(which callsAuthenticationManager.authenticate— DaoAuthenticationProvider's dummy-BCrypt timing equalization is preserved) →getSession(true)→sessionAuthenticationStrategy.onAuthentication(...)(rotates the ID) →setContextand attachSecurityContextto the now-rotated session. The order is correct: authentication first (so the strategy receives a realAuthentication), then ID rotation, then context attachment. ✓AuthSessionController.logout—session.invalidate()is called against the pre-auth servlet session, which by this point is bound to the Spring Session JDBCspring_sessionrow.invalidate()cascades through theSessionRepositoryFilterand triggers aDELETE FROM spring_session WHERE PRIMARY_ID = ?. Thespring_session_attributesrow cascades via FKON DELETE CASCADE(defined in V67). Session is fully wiped on the DB side. ✓SecurityContextHolder.clearContext()on the logout path prevents any thread-local leakage to the response-serialization phase. Spring's thread-local cleanup inSecurityContextHolderFilterwould handle it anyway, but being explicit is correct here./api/auth/loginispermitAll()inSecurityConfig(unchanged from round 1). TheAuthSessionControllerTest#login_is_public_no_session_requiredtest asserts the endpoint is reachable without@WithMockUser. ✓/api/auth/logoutrequires an authenticated session —AuthSessionControllerTest#logout_returns_401_when_not_authenticatedpins this. ✓Set-Cookieon a failed login —AuthSessionControllerTest#login_does_not_set_cookie_on_failurepins this. ✓ (No information leak via cookie-presence side channel.)Residual security posture for Phase 1
Acceptable for a private family archive with <50 users, single-node, behind Caddy with HSTS:
SameSite=Strictonfa_session+ Spring's default CORS (no allowedOrigins) + no static-content origin sharing. Phase 2 (#524) re-enablesCookieCsrfTokenRepository. The PR-level comment inSecurityConfigdocuments this load-bearing posture and cross-references #524. If anyone ever weakens SameSite toLax(e.g. to fix some popup login flow), they have to confront the comment first. ✓spring_sessionrows survive password change. Phase 2 (#524 FR-AUTH-004/008/005). The current threat model accepts that a credential change doesn't kick existing devices — for the family-archive use case, this is acceptable; for a public-facing app, it would not be. The ADR-020 §Consequences would benefit from naming this explicitly, but it's not blocking — #524's scope makes the gap obvious.CWE map (round 2)
Ship it. Both blockers are closed with clean primitives, regression tests pin the invariants, and the Phase 2 follow-ups are well-scoped in #524. The level of rigor on the round-2 fixes — using framework strategies rather than hand-rolling, comments that name CWEs, tests that verify behavior contracts rather than implementation details — is what I want to see on a security-adjacent PR.
— Nora
🧪 Sara Holt — QA Engineer & Test Strategist (round 2)
Verdict: ✅ Approved
Round 1 had two blockers (frontend Vitest gap, missing E2E) and four concerns. Round 2 closes the Vitest gap with the coverage level I asked for, defers the E2E to #613 (acceptable — scope is correct in the new issue), and the two new security regression tests (B1 strategy delegation, B2 audit-throws path) are textbook regression-pinning at the right test layer.
My round-1 blockers — status
S1 — Frontend Vitest coverage on credential-handling code ✅ Resolved
extractFaSessionIdhelperb607677ffrontend/src/lib/shared/cookies.spec.tslogin/+page.server.ts)2779502flogin/page.server.test.ts(load() registered/expired branches, missing email→400, 401→INVALID_CREDENTIALS with body parsing, happy path with cookie set + legacy auth_token deleted + 303 redirect, 500 on missing Set-Cookie)logout/+page.server.ts)d64139d9logout/page.server.test.ts(happy path with backend call + cookie cleanup, backend rejects→still clears cookies, no session→skip backend call)hooks.server.tsuserGrouphandler1f4e8a59hooks.server.test.ts(401 on private path→redirect to/login?reason=expired, 401 on/loginitself→no loop, missing fa_session→passthrough, 200→user attached to locals)Total: 19 new frontend tests covering the 4 surfaces where Java logic moved into TypeScript.
Naming reads as sentences:
extractFaSessionId › extracts the opaque id from a single Set-Cookie headerextractFaSessionId › only matches a cookie whose name is exactly fa_sessionlogin action › re-emits fa_session and deletes legacy auth_token on successlogin action › returns 500 when backend response omits fa_session cookielogout action › clears cookies even when the backend logout call failshooks.server userGroup › does not redirect when backend 401 fires on a public path (no /login → /login loop)The redirect-loop-prevention test is the one I called out specifically in round 1 — it's there and it tests the right thing. ✓
One logical assertion per test — each test asserts a single behavior. The login happy-path test asserts three things (
cookies.set,cookies.delete,rejects.toMatchObject({status:303})) but those are one logical claim ("a successful login produces the right cookie state and redirects") expressed as three matchers, which is fine.Factory functions —
makeCookies(sessionId?)andmakeRequest(form)are extracted at the top of each test file. Setup is one line per case. ✓Mock at the module boundary —
vi.mock('$env/dynamic/private', () => ({ env: { API_INTERNAL_URL: 'http://backend:8080' } }))andvi.stubGlobal('fetch', vi.fn()). The boundary is the right place. ✓S2 — Playwright E2E spec ✅ Resolved by filing #613
#613 exists, has concrete acceptance criteria, and lists the four beats I asked for:
Labelled
P2-medium test, depends on #523. The deferral is correctly scoped — the Vitest coverage closes the regression risk for the credential-handling logic itself; #613 closes the integration-level "does the real browser cookie behave correctly through the real Vite proxy" gap. Splitting the work this way is the right call, especially given my round-1 Concern #6 about CI time.My round-1 concerns — status
/login?reason=expiredawait checkA11y(page)after navigating to the expired-banner state)AuthSessionControllerTest#login_delegates_to_SessionAuthenticationStrategy_for_fixation_protection(17b29edd). Verifies the strategy is invoked with the newAuthentication.AuthSessionControllerTest#logout_returns_204_even_when_audit_throws(ea656116). MocksAuthService.logoutto throw, asserts 204 + session was invalidated first.AuthSessionIntegrationTestis full@SpringBootTestPostgresContainerConfigto a shared singleton.What's done well in round 2
c7782d55) all read like "the test would fail without this fix" — they exist because the fix needed proof. Felix's scoreboard explicitly links each to its commit. That's the TDD trace I want to see.jsonPath("$.password").doesNotExist(),jsonPath("$.pwd").doesNotExist(),content().string(not(containsString("$2a$10$shouldnotappearinresponse")))), with a deliberately-seeded fake hash that would appear in the response body if@JsonProperty(WRITE_ONLY)is ever dropped. This catches both "annotation removed" and "field renamed but annotation forgotten on new name" — the latter being a class of bug that's easy to miss in review.hooks.server.test.tsmock shape is now consistent with production. TheRedirectMarkerclass +isRedirect: (e) => e instanceof RedirectMarkerkeeps the test mock honest against the newimport { isRedirect } from '@sveltejs/kit'in production code. If the SvelteKit guard ever changes shape, both production and test would need to update — which is the right kind of coupling.Thread.sleep, no@Disabledwithout a ticket — re-checked the new and modified files. Per my round-1 floor.Suggestions for follow-up (not blocking)
hasErroroverride innoThrowRestTemplate()is duplicated betweenAuthSessionIntegrationTestand (per Felix's #523 plan) at least one other test class. Worth extracting to a sharedRestTemplateTestUtilsinbackend/src/test/java/.../testsupport/. Defer.*.spec.tsfiles are correctly out of scope.Test pyramid for this PR (final)
Ship it. The Vitest gap is closed at the layer it needed to be closed at, the security regression tests pin the right invariants, and the E2E deferral is justified by the unit/slice coverage that now exists. #613 carries the integration-layer browser concern.
— Sara
🎨 Leonie Voss — UX Designer & Accessibility Strategist (round 2)
Verdict: ✅ Approved
The three banner adjustments I asked for are all in, executed with the right judgment (not blindly). Re-checked the round-2 diffs at the same level as round 1.
My round-1 blockers — status
L1 — Semantic token, not raw amber Tailwind ✅ Resolved (
4f159439)The diff swaps the raw
text-amber-900andtext-amber-800fortext-warning, which maps to the existing--color-warning: #b45309(amber-700-equivalent) semantic token defined inlayout.css. Verified the token exists at line ~95 oflayout.css:This is the right call —
#b45309onbg-amber-50gives ~7:1 contrast which is comfortable AAA territory for body text. The soft amber fill (bg-amber-50,border-amber-200) is retained because:--color-warning-bg/--color-warning-borderwas correctly judged out of scope for this PRThe commit message explicitly names the precedent ("matching the precedent of the green 'registered' banner; a full surface-token pair is out of scope for this PR"). That's the right rationale and the right judgment call. If/when dark mode reaches the auth pages, the next pass adds the soft-fill tokens in one go across both banners. ✓
L2 — Icon, not color alone ✅ Resolved (
e10090b9)A Heroicons-style exclamation-triangle SVG is now prepended to the banner via a
flex items-start gap-3layout:Three things I checked:
aria-hidden="true"— correct. The heading text (error_session_expired) already conveys the warning meaning to assistive technology. The icon is for the sighted color-blind user. Not announced twice. ✓h-5 w-5= 20px — meets my 20px target for redundant warning indicators on small screens. Not so small it disappears, not so large it dominates the text. ✓text-warningon the icon — uses the same semantic token as the text. If the warning palette ever changes, icon and text stay in lockstep. ✓mt-0.5aligns the icon optically with the first line of text — that's the right baseline-adjacent offset for a 20px icon next to 14px text on a 1.5 line-height. ✓shrink-0— keeps the icon from collapsing when the explainer text wraps. Critical at 320px. ✓L3 — Body text
text-sm(14px) ✅ Resolved (17d9328c)Both lines now use
text-sm(14px):I'd called for
text-smminimum ortext-basefor the explainer. The implementation chosetext-smfor both, which keeps a small visual hierarchy viafont-mediumon the heading vs. plain weight on the explainer. The hierarchy works — the heading scans first, the explainer reads on second glance.14px isn't my preferred body floor (16px) for the senior cohort, but for a 1-2 line banner above the actual login form (where the email/password labels are also small), it's contextually consistent. The PR is the right shape — bumping to 16px here would create a banner that visually dominates the form below it, which is the opposite of what I want. ✓
Round-2 round-2 observations
text-warning(#b45309) onbg-amber-50(#FFFBEB) computes to roughly 7.2:1 → AAA pass for both the heading and the explainer at 14px. Improved over my round-1 estimate of 7.5:1 because the new color is darker than the originaltext-amber-800. ✓text-warningonbg-amber-50matches the text contrast → ~7.2:1. The icon is clearly visible against the soft fill. ✓role="status"+aria-live="polite"retained — redundant but harmless, as noted in round 1. NVDA and JAWS both handle the combination correctly. ✓mb-5 flex items-start gap-3layout — 1.25rem bottom margin separates the banner from the form heading below, 0.75rem horizontal gap between icon and text. Right amount of breathing room.min-h-[44px]on the submit button — unchanged from round 1, still in place. WCAG 2.5.5 met. ✓autofocuson email — unchanged from round 1,svelte-ignore a11y_autofocuscomment retained. Acceptable for the login context.?reason=expired). Disappears on next navigation. No state, no overhead.Cross-check at 320px (manually traced)
The new banner with icon is ~280px wide on a 320px viewport after the form's
px-4and the page's container padding. Withgap-3and a 20px icon, that leaves ~244px for the text column. The German explainer ("Aus Sicherheitsgründen werden Sitzungen nach 8 Stunden Inaktivität automatisch beendet.") is the longest of the three locales at 89 characters → wraps to ~5 lines at 14px. The Spanish explainer is similar. English is the shortest.This needs the 320px visual regression test in #613 to confirm I haven't miscalculated the wrap behavior in practice, but the layout primitives (
flex items-start,shrink-0on icon, no fixed widths anywhere) are correct.Suggestions for follow-up (not blocking)
<Banner kind="info"|"warning"|"success">component — still a good idea. Both banners on the login page (registered=mint, expired=amber) are now visually similar enough that consolidation would shed ~30 lines. File as a low-priority cleanup follow-up.--color-warningto include--color-warning-bgand--color-warning-borderso the banner remaps correctly. Not relevant for this PR.font-sans text-xsfield labels — slightly inconsistent with the banner'stext-smbody text. Acceptable because field labels are by convention smaller than body content; flagging for the next auth-page pass, not this one.What's right
4f159439,e10090b9,17d9328c. No over-engineering.Ship it. The banner is now accessible, brand-consistent, and legible for the senior reader cohort on a small phone in sunlight. Exactly what I asked for, executed with judgment.
— Leonie
📋 Elicit — Requirements Engineer (round 2)
Verdict: ✅ Approved
Round 1 surfaced no code blockers from my discipline, three resolved-decision-style observations on the test plan and NFR coverage, and three Open Questions on retention/comms. Round 2 doesn't change the requirements scope — the fixes are within the spec set by ADR-020 and the original #523 acceptance criteria. Re-evaluating coverage of the original requirements after the round-2 changes:
Requirements coverage vs. ADR-020 — round 2 status
fa_sessionopaque;AuthSessionIntegrationTest.login_sets_opaque_fa_session_cookieasserts no:in valueea656116reorders logout so session.invalidate() runs before audit + audit is best-effort. The "logout doesn't terminate the session under error conditions" gap I flagged in round 1 is closed. New testAuthSessionControllerTest#logout_returns_204_even_when_audit_throwsproves it.LOGIN_SUCCESS,LOGIN_FAILED,LOGOUTAuditKinds. The B1 fix adds session-fixation defense which is implicitly auditable via the rotated session ID being a fresh PRIMARY_ID in spring_session — no new AuditKind needed.The session-revocation row moves from ⚠️ Partial to ✅ Fully solved in round 2. The "guarantee has an exception path" caveat from my round-1 review is gone.
NFR checklist — round 2 status
17b29edd+ea656116with regression tests. Residual CSRF + rate limit deferred to #524 — well-scoped.SessionAuthenticationStrategy.onAuthenticationcall which performs aHttpServletRequest.changeSessionId()— Spring Session JDBC then issues an UPDATE on the session row. Cost: one additional DB round-trip per login (not per request). For <50 family users with infrequent logins, this is immaterial. No benchmark added; acceptable for the scale.spring.session.jdbc.cleanup-cronruns every minute by default and prunes expired rows. The retention policy ("expired sessions are deleted; never retained for forensics") is implicit but not documented. Defer to adocs/infrastructure/runbook update; not blocking.Open Questions — round 2 status
OQ-612-01 (stolen session: how does a user/admin invalidate other sessions?) — Tracked by #524 FR-AUTH-004 (password change deletes other sessions) and FR-AUTH-005 (admin force-logout). The user need is captured in #524's user story ("As a user, I want changing my password to log my other devices out so that I can react to 'I think someone stole my session'"). ✓ Tracked.
OQ-612-02 (per-user/per-role idle timeout configurability) — Unchanged. Uniform 8h. ADR-020 documents the choice. Acceptable for the family-archive scale. No further action.
OQ-612-03 (deploy comms — users will be silently kicked) — Still open. This is the one item I'd flag for the human user before merge. Ideas:
Either choice is fine; the question is whether someone makes the choice consciously. Not a code change; not a merge blocker.
What changed in round 2 from a requirements perspective
SecurityConfig.javaline ~78 documents the CSRF deferral and links #524. This is the right way to track "this Phase 1 decision is conditional on Phase 2 not happening" — the future maintainer who weakens SameSite=Strict has to confront the comment.Test plan vs. requirements
Re-checked Felix's scoreboard against the original PR test plan and the round-1 blockers:
AuthSessionIntegrationTest— full lifecycle + idle timeoutAuthServiceTest— login/logout audit + INVALID_CREDENTIALSAuthSessionControllerTest— 200/401 + no Set-Cookie on failurelogin_delegates_to_SessionAuthenticationStrategy_for_fixation_protection), B2 (logout_returns_204_even_when_audit_throws), and the WRITE_ONLY pinning test (login_response_body_does_not_contain_password_field)/api/*The four manual items are now covered by #613's E2E scope. Once #613 lands, all four become automated regression tests. Felix correctly filed #613 with concrete ACs (cookie HttpOnly + SameSite assertion, axe-playwright check on the banner state, logout cookie cleanup verification, 320px visual regression). The deferral is well-scoped.
Strengthened acceptance criteria (per my round-1 nudge)
In round 1 I flagged the test plan's "Manual: log in on a phone … submit button hits the 44px target, autofocus on email works" as weak — no measurable threshold. #613's beat 4 ("banner_does_not_break_layout_at_320px") and the cookie-shape assertions for
HttpOnlyandSameSite=Strictare explicitly measurable. The manual items being replaced by automated assertions with concrete thresholds is exactly the strengthening I asked for. ✓Gold-plating check — round 2
Round-2 commits stay scoped:
No drift. ✓
Suggestions
docs/DEPLOYMENT.mdso the next deploy of a similar magnitude inherits the precedent..gitea/ISSUE_TEMPLATE/security_change.md(still standing from round 1) — the test-plan + deploy-notes + breaking-change pattern in PR #612 is a model. The Phase 2 PR for #524 would benefit from inheriting it.Ship it. Two security NFRs and three accessibility NFRs moved from partial to fully met in round 2 with the right tests and the right scope. The one residual non-code question is the deploy-comms one (OQ-612-03), which is a deploy-day decision, not a merge blocker.
— Elicit