fix(security): promote auth_token cookie to Authorization header (#520) #521

Merged

marcel merged 2 commits from fix/issue-520-cookie-to-authorization-filter into main 2026-05-11 18:20:10 +02:00

Conversation 7 Commits 2 Files Changed 5 +295 -8

marcel commented 2026-05-11 17:42:25 +02:00

Owner

Copy Link

Summary

Closes #520. Without this, every browser-side /api/* call in production hits the backend without auth, gets 401 WWW-Authenticate: Basic, and the browser pops a native auth dialog over the running SPA. Two SSE endpoints (/api/notifications/stream, /api/ocr/jobs/.../progress) are particularly visible — the popup fires immediately after login.

Fix

AuthTokenCookieFilter (Ordered.HIGHEST_PRECEDENCE, runs before Spring Security):

• Reads the auth_token cookie.
• URL-decodes the value (login action URL-encodes "Basic " → "Basic%20").
• Wraps the request to expose the decoded value as the Authorization header.
• Falls through unchanged if no cookie / empty value / explicit Authorization already present.

After this, the backend behaves identically whether the auth comes from SSR (hooks.server.ts sets Authorization explicitly) or browser-direct (the filter promotes the cookie). No Caddy changes, no client code changes.

Tests

5 cases in AuthTokenCookieFilterTest:

• URL-decoded promotion (Basic%20YWR...%3D%3D → Basic YWR...=)
• explicit Authorization wins, cookie ignored
• no cookies at all → pass-through
• different cookie names → pass-through
• empty auth_token value → pass-through

Also fixes an unrelated test breakage: ThumbnailServiceIntegrationTest was the only @SpringBootTest in the suite without @ActiveProfiles. After #516 made UserDataInitializer fail-closed outside dev/test/e2e, that test was failing in CI. One-line annotation restores green main.

Test plan after merge

• Redeploy staging — log in via browser at https://staging.raddatz.cloud/login, navigate to /, observe no Basic-auth popup.
• Open a document page — the transcription-blocks fetch should return data, not 401.
• DevTools Network tab: /api/notifications/stream returns 200 with text/event-stream.

🤖 Generated with Claude Code

## Summary Closes #520. Without this, every browser-side `/api/*` call in production hits the backend without auth, gets `401 WWW-Authenticate: Basic`, and the browser pops a native auth dialog over the running SPA. Two SSE endpoints (`/api/notifications/stream`, `/api/ocr/jobs/.../progress`) are particularly visible — the popup fires immediately after login. ## Fix `AuthTokenCookieFilter` (`Ordered.HIGHEST_PRECEDENCE`, runs before Spring Security): - Reads the `auth_token` cookie. - URL-decodes the value (login action URL-encodes "Basic " → "Basic%20"). - Wraps the request to expose the decoded value as the `Authorization` header. - Falls through unchanged if no cookie / empty value / explicit `Authorization` already present. After this, the backend behaves identically whether the auth comes from SSR (`hooks.server.ts` sets `Authorization` explicitly) or browser-direct (the filter promotes the cookie). No Caddy changes, no client code changes. ## Tests 5 cases in `AuthTokenCookieFilterTest`: - URL-decoded promotion (`Basic%20YWR...%3D%3D` → `Basic YWR...=`) - explicit `Authorization` wins, cookie ignored - no cookies at all → pass-through - different cookie names → pass-through - empty `auth_token` value → pass-through Also fixes an unrelated test breakage: `ThumbnailServiceIntegrationTest` was the only `@SpringBootTest` in the suite without `@ActiveProfiles`. After #516 made `UserDataInitializer` fail-closed outside dev/test/e2e, that test was failing in CI. One-line annotation restores green main. ## Test plan after merge - [ ] Redeploy staging — log in via browser at `https://staging.raddatz.cloud/login`, navigate to /, observe **no Basic-auth popup**. - [ ] Open a document page — the transcription-blocks `fetch` should return data, not 401. - [ ] DevTools Network tab: `/api/notifications/stream` returns 200 with `text/event-stream`. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 1 commit 2026-05-11 17:42:25 +02:00

fix(security): promote auth_token cookie to Authorization header for browser /api/* calls ... d8dd09ae54

Closes #520.

The login action stores `Basic <base64>` in an HttpOnly `auth_token`
cookie. SSR fetches from hooks.server.ts explicitly set the
Authorization header. Vite's dev proxy does the same on every
/api/* request. Caddy in production does NOT. So browser-side
fetch() and EventSource() calls reach the backend without auth,
get 401 + WWW-Authenticate: Basic, and the browser pops a native
auth dialog over the SPA.

Add AuthTokenCookieFilter (Ordered.HIGHEST_PRECEDENCE, before any
Spring Security filter) that promotes the cookie to a request
header when no explicit Authorization is present. URL-decodes the
cookie value because SvelteKit URL-encodes spaces ("Basic " ->
"Basic%20") when serializing the cookie. Works the same for REST,
SSE (/api/notifications/stream, /api/ocr/jobs/.../progress), and
any other browser-direct backend call.

5 tests in AuthTokenCookieFilterTest cover: URL-decoded promotion,
explicit-Authorization-wins precedence, no-cookies pass-through,
absent-auth-token pass-through, empty-value pass-through.

Also: add `@ActiveProfiles("test")` to ThumbnailServiceIntegrationTest,
the one remaining @SpringBootTest in the suite that wasn't annotated.
After #516 made UserDataInitializer fail-closed outside dev/test/e2e,
this test's context load was throwing. Restores green main.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

marcel commented 2026-05-11 17:44:32 +02:00

Author

Owner

Copy Link

Nora "NullX" Steiner — Application Security Engineer

Verdict: Changes requested

This is a security-sensitive PR (it changes how the backend ingests auth credentials), so I went over it slowly. The core mechanism is sound — promoting an HttpOnly cookie into the Authorization header is a well-known pattern — but several assumptions that the original architecture depended on no longer hold once browser-direct API calls authenticate via cookie. Those need to be reconciled before this merges.

Blockers

1. CSRF threat model is now broken — SecurityConfig comment is stale and wrong
   SecurityConfig.java disables CSRF with this justification:

   // CSRF is intentionally disabled: every request from the SvelteKit frontend
   // carries an explicit Authorization header... browsers block cross-origin
   // requests from setting custom headers, so CSRF... is not possible.
   

   That argument no longer holds for browser-direct /api/* calls. A third-party page can issue a cross-origin fetch('https://prod/api/documents/123', { method: 'DELETE', credentials: 'include' }). With SameSite=strict on auth_token this is currently blocked by the browser — but SameSite=strict is now the only CSRF defence, and the code comment says the opposite. Two things must happen:

   • Update the SecurityConfig comment to state explicitly that CSRF is prevented by SameSite=strict on the auth_token cookie, with a reference to frontend/src/routes/login/+page.server.ts.
   • Add a short Javadoc paragraph on AuthTokenCookieFilter itself naming the same threat-model dependency.
     Otherwise the next person to "fix" the login cookie to SameSite=lax (because strict breaks something) silently opens the application to CSRF on every write endpoint. This is exactly the kind of cross-file invariant that gets broken without a comment pinning it down.

2. secure: false on the auth cookie in production
   frontend/src/routes/login/+page.server.ts line ~37:

   cookies.set('auth_token', authHeader, {
       path: '/',
       httpOnly: true,
       sameSite: 'strict',
       secure: false,  // set to true when HTTPS is available
       maxAge: 60 * 60 * 24
   });
   

   Pre-this-PR the cookie was only read by SSR — its security setting didn't really gate API auth. After this PR the cookie is the API credential for every browser-direct call. Shipping it over plaintext HTTP in any environment now means we're leaking a 24-hour Basic-auth token to every passive listener on the network. The deploy target is staging.raddatz.cloud over HTTPS, so this needs to flip to secure: env.NODE_ENV === 'production' (or read from a dedicated env var) before this PR goes to prod. Track as a separate fix if you don't want to scope-creep this PR, but the merge should not happen without that fix landing on the same release.

3. Log leakage — wrapped request makes Authorization visible in every access log / audit trail that prints headers
   The wrapper exposes Authorization via getHeaderNames() and getHeaders(). If anything downstream — Spring's CommonsRequestLoggingFilter, an MDC enricher, a future audit interceptor, or even a debug breakpoint — iterates headers, the Basic token (= base64(email:password), trivially reversible) lands in logs. Confirm two things:

   • No filter in the chain logs request headers in any profile (grep -r CommonsRequestLoggingFilter, grep -r logRequestDetails).
   • Add a test or a Javadoc note that Authorization MUST never be logged. A small redaction filter (HeaderRedactingLogFilter that strips Authorization and Cookie from any structured log event) would be the defence-in-depth move. Not necessarily for this PR, but file a follow-up.

Suggestions

4. Scope the filter to /api/** instead of every request
   Currently the filter runs on /, /_app/immutable/..., /favicon.ico, and /api/*. Promoting an auth header on requests that don't need one is wasted work and a small but non-zero attack surface — any future filter that inspects Authorization on a static-asset request now sees one. Override shouldNotFilter() to return true for non-/api paths, or guard with request.getRequestURI().startsWith("/api/") at the top of doFilterInternal. Cleaner intent, smaller blast radius.

5. URLDecoder.decode swallows the encoding contract — make it explicit
   The login action stores Basic <base64> (with a literal space) and SvelteKit URL-encodes the cookie value to Basic%20<base64>. The filter then URL-decodes. This pas-de-deux is correct but invisible — it's a coupling between frontend/src/routes/login/+page.server.ts and AuthTokenCookieFilter that lives only in commit history. Add a one-line Javadoc on AuthTokenCookieFilter pointing at the login action filename, and a matching one-line comment in the login action pointing at the filter. Future-me will be grateful.

6. URLDecoder.decode on attacker-controlled input — confirm no surprise behaviour
   URLDecoder.decode will turn + into a space. A malformed cookie like Basic%20+evil becomes Basic evil (two spaces). Spring's BasicAuthenticationFilter will reject it as malformed, so this is safe today, but I'd add one negative test: cookie value "not-valid-base64-%ZZ" should NOT throw, the filter should pass it through and let Spring return 401. URLDecoder.decode actually does throw IllegalArgumentException on malformed % sequences in strict mode — verify with a unit test, and if it does throw, wrap in a try/catch that logs and falls through.

7. isEmpty() check is weaker than isBlank()
c.getValue() != null && !c.getValue().isEmpty() lets a cookie like auth_token= (three spaces) reach the URL decoder and produce Authorization: , which Spring rejects but only after going down its parsing path. Use isBlank().

8. Test coverage gap — no test for the explicit-Authorization-wins case at the wrapper boundary
   The test verifies getHeader("Authorization") returns the explicit value, but doesn't verify getHeaderNames() and getHeaders("Authorization") are consistent when the wrapper is bypassed (the early-return path doesn't wrap). That's fine because no wrapping happens, but add an explicit assertion that the request reaching the chain is the same instance as the input (assertThat(captor.getValue()).isSameAs(req)) so we catch a future refactor that silently always-wraps.

What I checked and approve of

• Ordered.HIGHEST_PRECEDENCE is correct — runs before BasicAuthenticationFilter.
• OncePerRequestFilter is the right base class — no double-promotion on forwards/includes.
• The wrapper correctly exposes Authorization via all three accessors (getHeader, getHeaders, getHeaderNames) — Spring's BasicAuthenticationFilter uses getHeader(AUTHORIZATION) so this works, but a defensively-written downstream filter that iterates getHeaderNames() will also see it. Good.
• The HttpOnly flag on the cookie remains the right call — keeps it out of JS reach, which is the whole point of using a cookie instead of localStorage.
• SSE endpoints will now authenticate correctly — EventSource can't set custom headers, so the cookie path is genuinely the only option here. This PR is the right fix.

Address blockers 1, 2, 3 and I'll re-review. Suggestions 4–8 can land as follow-ups if you prefer to keep this PR tight.

— Nora "NullX" Steiner

## Nora "NullX" Steiner — Application Security Engineer **Verdict: Changes requested** This is a security-sensitive PR (it changes how the backend ingests auth credentials), so I went over it slowly. The core mechanism is sound — promoting an HttpOnly cookie into the `Authorization` header is a well-known pattern — but several assumptions that the original architecture depended on no longer hold once browser-direct API calls authenticate via cookie. Those need to be reconciled before this merges. ### Blockers 1. **CSRF threat model is now broken — `SecurityConfig` comment is stale and wrong** `SecurityConfig.java` disables CSRF with this justification: ``` // CSRF is intentionally disabled: every request from the SvelteKit frontend // carries an explicit Authorization header... browsers block cross-origin // requests from setting custom headers, so CSRF... is not possible. ``` That argument no longer holds for browser-direct `/api/*` calls. A third-party page can issue a cross-origin `fetch('https://prod/api/documents/123', { method: 'DELETE', credentials: 'include' })`. With `SameSite=strict` on `auth_token` this is currently blocked by the browser — but `SameSite=strict` is now the *only* CSRF defence, and the code comment says the opposite. Two things must happen: - Update the `SecurityConfig` comment to state explicitly that CSRF is prevented by `SameSite=strict` on the `auth_token` cookie, with a reference to `frontend/src/routes/login/+page.server.ts`. - Add a short Javadoc paragraph on `AuthTokenCookieFilter` itself naming the same threat-model dependency. Otherwise the next person to "fix" the login cookie to `SameSite=lax` (because strict breaks something) silently opens the application to CSRF on every write endpoint. This is exactly the kind of cross-file invariant that gets broken without a comment pinning it down. 2. **`secure: false` on the auth cookie in production** `frontend/src/routes/login/+page.server.ts` line ~37: ```ts cookies.set('auth_token', authHeader, { path: '/', httpOnly: true, sameSite: 'strict', secure: false, // set to true when HTTPS is available maxAge: 60 * 60 * 24 }); ``` Pre-this-PR the cookie was only read by SSR — its security setting didn't really gate API auth. After this PR the cookie *is* the API credential for every browser-direct call. Shipping it over plaintext HTTP in any environment now means we're leaking a 24-hour Basic-auth token to every passive listener on the network. The deploy target is `staging.raddatz.cloud` over HTTPS, so this needs to flip to `secure: env.NODE_ENV === 'production'` (or read from a dedicated env var) before this PR goes to prod. Track as a separate fix if you don't want to scope-creep this PR, but the merge should not happen without that fix landing on the same release. 3. **Log leakage — wrapped request makes `Authorization` visible in every access log / audit trail that prints headers** The wrapper exposes `Authorization` via `getHeaderNames()` *and* `getHeaders()`. If anything downstream — Spring's `CommonsRequestLoggingFilter`, an MDC enricher, a future audit interceptor, or even a debug breakpoint — iterates headers, the Basic token (= base64(email:password), trivially reversible) lands in logs. Confirm two things: - No filter in the chain logs request headers in any profile (`grep -r CommonsRequestLoggingFilter`, `grep -r logRequestDetails`). - Add a test or a Javadoc note that `Authorization` MUST never be logged. A small redaction filter (`HeaderRedactingLogFilter` that strips `Authorization` and `Cookie` from any structured log event) would be the defence-in-depth move. Not necessarily for this PR, but file a follow-up. ### Suggestions 4. **Scope the filter to `/api/**` instead of every request** Currently the filter runs on `/`, `/_app/immutable/...`, `/favicon.ico`, *and* `/api/*`. Promoting an auth header on requests that don't need one is wasted work and a small but non-zero attack surface — any future filter that inspects `Authorization` on a static-asset request now sees one. Override `shouldNotFilter()` to return true for non-`/api` paths, or guard with `request.getRequestURI().startsWith("/api/")` at the top of `doFilterInternal`. Cleaner intent, smaller blast radius. 5. **`URLDecoder.decode` swallows the encoding contract — make it explicit** The login action stores `Basic <base64>` (with a literal space) and SvelteKit URL-encodes the cookie value to `Basic%20<base64>`. The filter then URL-decodes. This pas-de-deux is correct but invisible — it's a coupling between `frontend/src/routes/login/+page.server.ts` and `AuthTokenCookieFilter` that lives only in commit history. Add a one-line Javadoc on `AuthTokenCookieFilter` pointing at the login action filename, and a matching one-line comment in the login action pointing at the filter. Future-me will be grateful. 6. **`URLDecoder.decode` on attacker-controlled input — confirm no surprise behaviour** `URLDecoder.decode` will turn `+` into a space. A malformed cookie like `Basic%20+evil` becomes `Basic evil` (two spaces). Spring's `BasicAuthenticationFilter` will reject it as malformed, so this is safe today, but I'd add one negative test: cookie value `"not-valid-base64-%ZZ"` should NOT throw, the filter should pass it through and let Spring return 401. `URLDecoder.decode` actually does throw `IllegalArgumentException` on malformed `%` sequences in strict mode — verify with a unit test, and if it does throw, wrap in a try/catch that logs and falls through. 7. **`isEmpty()` check is weaker than `isBlank()`** `c.getValue() != null && !c.getValue().isEmpty()` lets a cookie like `auth_token= ` (three spaces) reach the URL decoder and produce `Authorization: `, which Spring rejects but only after going down its parsing path. Use `isBlank()`. 8. **Test coverage gap — no test for the explicit-`Authorization`-wins case at the wrapper boundary** The test verifies `getHeader("Authorization")` returns the explicit value, but doesn't verify `getHeaderNames()` and `getHeaders("Authorization")` are consistent when the wrapper is bypassed (the early-return path doesn't wrap). That's fine because no wrapping happens, but add an explicit assertion that the request reaching the chain is the *same instance* as the input (`assertThat(captor.getValue()).isSameAs(req)`) so we catch a future refactor that silently always-wraps. ### What I checked and approve of - `Ordered.HIGHEST_PRECEDENCE` is correct — runs before `BasicAuthenticationFilter`. - `OncePerRequestFilter` is the right base class — no double-promotion on forwards/includes. - The wrapper correctly exposes `Authorization` via all three accessors (`getHeader`, `getHeaders`, `getHeaderNames`) — Spring's `BasicAuthenticationFilter` uses `getHeader(AUTHORIZATION)` so this works, but a defensively-written downstream filter that iterates `getHeaderNames()` will also see it. Good. - The `HttpOnly` flag on the cookie remains the right call — keeps it out of JS reach, which is the whole point of using a cookie instead of `localStorage`. - SSE endpoints will now authenticate correctly — `EventSource` can't set custom headers, so the cookie path is genuinely the only option here. This PR is the right fix. Address blockers 1, 2, 3 and I'll re-review. Suggestions 4–8 can land as follow-ups if you prefer to keep this PR tight. — Nora "NullX" Steiner

marcel commented 2026-05-11 17:44:55 +02:00

Author

Owner

Copy Link

Markus Keller — Senior Application Architect

Verdict: Approved with concerns

The shape of the fix is right. We had a hidden environmental coupling — dev (Vite proxy) silently translated the auth cookie into a header, prod (Caddy) didn't — and the only correct place to resolve that gap is in the backend itself, where the contract is owned. Moving the translation from infrastructure into the application removes the "works on my machine" class of bug for good. I prefer this over Caddy-side header injection.

That said, this PR introduces a load-bearing invariant that lives implicitly across three files (AuthTokenCookieFilter, SecurityConfig, login/+page.server.ts), and the project has no ADR explaining why. That's the architectural debt I want addressed before merge.

Blockers

1. Write an ADR — "Auth credential flow: cookie-promotion filter pattern"
   This PR enshrines a non-trivial architectural decision: the backend now has two equally-supported ways to authenticate a request (explicit header + promoted cookie), and the choice between them is determined by environment (SSR vs browser-direct). That's a permanent property of the system from now on. Add docs/adr/00NN-auth-cookie-promotion.md covering:
   • Context: SSR sets Authorization explicitly; browser-direct can't because EventSource and many fetch call sites don't.
   • Decision: backend filter promotes the cookie, ensuring uniform auth at the Spring Security boundary.
   • Alternatives rejected: Caddy header_up (no URL-decode), reformatted cookie (high blast radius), session-based auth (out of scope for this fix).
   • Consequences: CSRF defence now relies on SameSite=strict rather than "custom header impossible from cross-origin" — explicitly link to SecurityConfig comment.
     This isn't bureaucracy. The next dev to touch auth will be reading three files trying to figure out which is the source of truth; an ADR collapses that to one read.

Suggestions

2. Package placement is right but the class is doing two jobs
   AuthTokenCookieFilter contains both the filter and AuthHeaderRequest wrapper as a static nested class. That's fine for now — they're cohesive — but if any other filter ever needs to override headers (rate-limiting based on a forwarded header, tenant routing, etc.), extract AuthHeaderRequest into a generic HeaderOverridingRequestWrapper. Track as a follow-up; don't pre-extract.

3. Cross-file invariant — link the three load-bearing comments

   • frontend/src/routes/login/+page.server.ts: "stores URL-encoded Basic header in cookie"
   • AuthTokenCookieFilter: "decodes cookie, promotes to header"
   • SecurityConfig: "CSRF safe because [something]"
     These three must change together or the system breaks subtly. The Javadoc on AuthTokenCookieFilter already points at the SvelteKit login action (good), but the login action and SecurityConfig have no reciprocal pointer. Add two-line comments that name the other files. We don't have a linter for this — comments are our linter.

4. Filter scope: consider /api/** only
   Nora flagged this from a security angle. Architecturally I agree for a different reason: domain boundary clarity. The filter belongs to the API auth pipeline, not the page-serving pipeline. Make that explicit with shouldNotFilter() returning true for non-/api paths. Reduces cognitive load when reading filter ordering.

5. One file, one decision — the drive-by is acceptable here, but flag the pattern
   @ActiveProfiles("test") on ThumbnailServiceIntegrationTest is a fix for a real CI failure introduced by #516 fail-closing UserDataInitializer. The PR body calls it out, so I'm not asking you to split the commit. But if this happens twice more, we have a pattern problem — we need a test-base-class or a meta-annotation @SpringBootIntegrationTest that bundles @SpringBootTest + @ActiveProfiles("test") + @Import(PostgresContainerConfig.class). File a follow-up.

What I approve of

• The fix is at the correct layer. Authentication is a backend concern; the backend now owns the rule.
• Ordered.HIGHEST_PRECEDENCE is the right precedence — security filters compose downstream.
• No new dependencies, no new infrastructure, no Caddy changes. This is exactly the kind of "boring" fix I want for a load-bearing concern.
• The contract is reversible — if we ever move to session-based auth or token-based JWT, this filter is one delete-file commit away from gone.

Address the ADR and I'll mark this approved.

— Markus Keller

## Markus Keller — Senior Application Architect **Verdict: Approved with concerns** The shape of the fix is right. We had a hidden environmental coupling — dev (Vite proxy) silently translated the auth cookie into a header, prod (Caddy) didn't — and the only correct place to resolve that gap is in the backend itself, where the contract is owned. Moving the translation from infrastructure into the application removes the "works on my machine" class of bug for good. I prefer this over Caddy-side header injection. That said, this PR introduces a load-bearing invariant that lives implicitly across three files (`AuthTokenCookieFilter`, `SecurityConfig`, `login/+page.server.ts`), and the project has no ADR explaining why. That's the architectural debt I want addressed before merge. ### Blockers 1. **Write an ADR — "Auth credential flow: cookie-promotion filter pattern"** This PR enshrines a non-trivial architectural decision: the backend now has two equally-supported ways to authenticate a request (explicit header + promoted cookie), and the choice between them is determined by environment (SSR vs browser-direct). That's a permanent property of the system from now on. Add `docs/adr/00NN-auth-cookie-promotion.md` covering: - **Context**: SSR sets `Authorization` explicitly; browser-direct can't because `EventSource` and many `fetch` call sites don't. - **Decision**: backend filter promotes the cookie, ensuring uniform auth at the Spring Security boundary. - **Alternatives rejected**: Caddy `header_up` (no URL-decode), reformatted cookie (high blast radius), session-based auth (out of scope for this fix). - **Consequences**: CSRF defence now relies on `SameSite=strict` rather than "custom header impossible from cross-origin" — explicitly link to `SecurityConfig` comment. This isn't bureaucracy. The next dev to touch auth will be reading three files trying to figure out which is the source of truth; an ADR collapses that to one read. ### Suggestions 2. **Package placement is right but the class is doing two jobs** `AuthTokenCookieFilter` contains both the filter and `AuthHeaderRequest` wrapper as a static nested class. That's fine for now — they're cohesive — but if any other filter ever needs to override headers (rate-limiting based on a forwarded header, tenant routing, etc.), extract `AuthHeaderRequest` into a generic `HeaderOverridingRequestWrapper`. Track as a follow-up; don't pre-extract. 3. **Cross-file invariant — link the three load-bearing comments** - `frontend/src/routes/login/+page.server.ts`: "stores URL-encoded Basic header in cookie" - `AuthTokenCookieFilter`: "decodes cookie, promotes to header" - `SecurityConfig`: "CSRF safe because [something]" These three must change together or the system breaks subtly. The Javadoc on `AuthTokenCookieFilter` already points at the SvelteKit login action (good), but the login action and `SecurityConfig` have no reciprocal pointer. Add two-line comments that name the other files. We don't have a linter for this — comments are our linter. 4. **Filter scope: consider `/api/**` only** Nora flagged this from a security angle. Architecturally I agree for a different reason: domain boundary clarity. The filter belongs to the API auth pipeline, not the page-serving pipeline. Make that explicit with `shouldNotFilter()` returning true for non-`/api` paths. Reduces cognitive load when reading filter ordering. 5. **One file, one decision — the drive-by is acceptable here, but flag the pattern** `@ActiveProfiles("test")` on `ThumbnailServiceIntegrationTest` is a fix for a real CI failure introduced by #516 fail-closing `UserDataInitializer`. The PR body calls it out, so I'm not asking you to split the commit. But if this happens twice more, we have a pattern problem — we need a test-base-class or a meta-annotation `@SpringBootIntegrationTest` that bundles `@SpringBootTest + @ActiveProfiles("test") + @Import(PostgresContainerConfig.class)`. File a follow-up. ### What I approve of - The fix is at the correct layer. Authentication is a backend concern; the backend now owns the rule. - `Ordered.HIGHEST_PRECEDENCE` is the right precedence — security filters compose downstream. - No new dependencies, no new infrastructure, no Caddy changes. This is exactly the kind of "boring" fix I want for a load-bearing concern. - The contract is reversible — if we ever move to session-based auth or token-based JWT, this filter is one delete-file commit away from gone. Address the ADR and I'll mark this approved. — Markus Keller

marcel commented 2026-05-11 17:45:10 +02:00

Author

Owner

Copy Link

Felix Brandt — Senior Fullstack Developer

Verdict: Approved with concerns

Clean implementation, tests-first evidence in the test class, good Javadoc on the filter explaining why (not what). The two minor refactors I'd ask for are about consistency with the rest of the codebase, not correctness.

Suggestions

1. super.getHeaderNames() null-handling

   Enumeration<String> base = super.getHeaderNames();
   java.util.Set<String> names = new java.util.LinkedHashSet<>();
   while (base.hasMoreElements()) names.add(base.nextElement());
   

   Servlet spec allows getHeaderNames() to return null if the container doesn't allow introspection. Jetty doesn't do that, but defensive code costs nothing: if (base != null) { while (...) }. The tests use MockHttpServletRequest which always returns a non-null enumeration, so this path is uncovered.

2. Inline java.util.LinkedHashSet import is ugly

   java.util.Set<String> names = new java.util.LinkedHashSet<>();
   

   Either import java.util.Set and java.util.LinkedHashSet at the top, or use Collections.list(base) (returns an ArrayList) and add the header to it. Inline FQNs read as "I forgot to organize imports". Small but the rest of this file is clean — make this match.

3. Constant for cookie name — already done, good
   static final String COOKIE_NAME = "auth_token"; is package-private so the test can reference it. Today the test uses the raw string "auth_token" — switch the test to AuthTokenCookieFilter.COOKIE_NAME so a future rename can't desync them.

4. Test class — extract common setup
   Every test does the same four lines:

   MockHttpServletRequest req = new MockHttpServletRequest();
   MockHttpServletResponse res = new MockHttpServletResponse();
   FilterChain chain = mock(FilterChain.class);
   // ...
   filter.doFilter(req, res, chain);
   

   Pull res and chain into @BeforeEach-initialized fields, or — better — extract a runFilter(MockHttpServletRequest) helper that returns the captured forwarded request. Test bodies then read as one-line behaviour assertions. Right now the AAA structure is there but the noise drowns it out.

5. Test name on test 4 — passes_through_when_auth_token_cookie_is_absent
   Reads well. But the assertion verify(chain).doFilter(req, res) checks the unwrapped request was forwarded — which is the right behaviour but the test doesn't say so in its name. Either rename to passes_through_request_unwrapped_when_auth_token_cookie_is_absent, or accept that "passes through" is sufficient project-vocab. I'd rename.

What I like

• The filter is short and does exactly one thing. The wrapper class is a static nested class — correct scope, no leakage.
• Javadoc on the class explains the threat model, not the code. Exactly what Nora's persona-doc asks for.
• Tests have descriptive names that read as sentences. promotes_url_encoded_auth_token_cookie_to_decoded_Authorization_header is the kind of test name I want everywhere.
• Drive-by @ActiveProfiles("test") is correctly called out in the PR body. Atomic-commit purists might want it split, but the PR body is honest and the fix is one line — keeping it here is fine.

No blockers from me. Address the suggestions or open a follow-up — either way I'm green.

— Felix Brandt

## Felix Brandt — Senior Fullstack Developer **Verdict: Approved with concerns** Clean implementation, tests-first evidence in the test class, good Javadoc on the filter explaining *why* (not what). The two minor refactors I'd ask for are about consistency with the rest of the codebase, not correctness. ### Suggestions 1. **`super.getHeaderNames()` null-handling** ```java Enumeration<String> base = super.getHeaderNames(); java.util.Set<String> names = new java.util.LinkedHashSet<>(); while (base.hasMoreElements()) names.add(base.nextElement()); ``` Servlet spec allows `getHeaderNames()` to return `null` if the container doesn't allow introspection. Jetty doesn't do that, but defensive code costs nothing: `if (base != null) { while (...) }`. The tests use `MockHttpServletRequest` which always returns a non-null enumeration, so this path is uncovered. 2. **Inline `java.util.LinkedHashSet` import is ugly** ```java java.util.Set<String> names = new java.util.LinkedHashSet<>(); ``` Either import `java.util.Set` and `java.util.LinkedHashSet` at the top, or use `Collections.list(base)` (returns an `ArrayList`) and add the header to it. Inline FQNs read as "I forgot to organize imports". Small but the rest of this file is clean — make this match. 3. **Constant for cookie name — already done, good** `static final String COOKIE_NAME = "auth_token";` is package-private so the test can reference it. Today the test uses the raw string `"auth_token"` — switch the test to `AuthTokenCookieFilter.COOKIE_NAME` so a future rename can't desync them. 4. **Test class — extract common setup** Every test does the same four lines: ```java MockHttpServletRequest req = new MockHttpServletRequest(); MockHttpServletResponse res = new MockHttpServletResponse(); FilterChain chain = mock(FilterChain.class); // ... filter.doFilter(req, res, chain); ``` Pull `res` and `chain` into `@BeforeEach`-initialized fields, or — better — extract a `runFilter(MockHttpServletRequest)` helper that returns the captured forwarded request. Test bodies then read as one-line behaviour assertions. Right now the AAA structure is there but the noise drowns it out. 5. **Test name on test 4 — `passes_through_when_auth_token_cookie_is_absent`** Reads well. But the assertion `verify(chain).doFilter(req, res)` checks the *unwrapped* request was forwarded — which is the right behaviour but the test doesn't say so in its name. Either rename to `passes_through_request_unwrapped_when_auth_token_cookie_is_absent`, or accept that "passes through" is sufficient project-vocab. I'd rename. ### What I like - The filter is short and does exactly one thing. The wrapper class is a static nested class — correct scope, no leakage. - Javadoc on the class explains the threat model, not the code. Exactly what Nora's persona-doc asks for. - Tests have descriptive names that read as sentences. `promotes_url_encoded_auth_token_cookie_to_decoded_Authorization_header` is the kind of test name I want everywhere. - Drive-by `@ActiveProfiles("test")` is correctly called out in the PR body. Atomic-commit purists might want it split, but the PR body is honest and the fix is one line — keeping it here is fine. No blockers from me. Address the suggestions or open a follow-up — either way I'm green. — Felix Brandt

marcel commented 2026-05-11 17:45:32 +02:00

Author

Owner

Copy Link

Sara Holt — Senior QA Engineer

Verdict: Approved with concerns

Five unit tests for a five-branch filter — good ratio, good naming, good AAA structure. What's missing is the integration test that proves this works with Spring Security. A unit test confirming the wrapper exposes Authorization is necessary but not sufficient; what we actually care about is: "does BasicAuthenticationFilter see the promoted header and authenticate?" That needs a real Spring context.

Blockers

1. Missing integration test — the only test that proves the bug is actually fixed
   Add a @SpringBootTest (or @WebMvcTest with @Import(SecurityConfig.class) if you want it lightweight) that:
   • Sends a request to any authenticated endpoint (e.g. /api/users/me)
   • With a Cookie: auth_token=Basic%20<base64(test@user:test123)> header — no Authorization header
   • Asserts response is 200, not 401
     And a paired test:
   • Same setup, but with Cookie: auth_token=Basic%20<garbage>
   • Asserts 401 (proving Spring's auth pipeline rejects bad creds, i.e. the filter doesn't bypass auth)
     Without this, the unit tests prove "the wrapper works in isolation" but not "the wrapper integrates with BasicAuthenticationFilter". Given the production bug we're fixing, the integration test is the test that gives us the most confidence on the next deploy.

Suggestions

2. Negative tests on URL-decoder edge cases
   URLDecoder.decode can throw IllegalArgumentException on malformed % sequences (e.g. Basic%2). Add a test:

   @Test
   void passes_through_when_auth_token_cookie_has_malformed_url_encoding() throws Exception {
       MockHttpServletRequest req = new MockHttpServletRequest();
       req.setCookies(new Cookie("auth_token", "Basic%2"));
       // ...
       filter.doFilter(req, res, chain);
       // assert filter does not crash; either 401 from downstream or pass-through with raw value
   }
   

   If this currently throws (likely), that's a bug — a malformed cookie would 500 the whole request. We need either a try/catch or a verified test asserting JDK behaviour is what we expect.

3. Test the wrapper itself in isolation
   AuthHeaderRequest is a non-trivial wrapper exposing three methods (getHeader, getHeaders, getHeaderNames). I'd extract those into a tiny test class:

   • getHeader("Authorization") returns override (case-insensitive)
   • getHeader("authorization") also returns override
   • getHeaders("Authorization") returns single-element enumeration
   • getHeaderNames() contains "Authorization" exactly once even if base already had one (today: collected into LinkedHashSet, so duplicate is implicitly dropped — verify)
   • getHeader("Content-Type") returns the base value
     The current tests cover the filter's use of the wrapper but not the wrapper's contract. The wrapper is the part most likely to surprise us downstream — test it directly.

4. Case sensitivity test missing
   Most servers normalize to canonical case, but HTTP headers are case-insensitive per RFC. Test:

   req.addHeader("authorization", "Basic explicit"); // lowercase
   

   The filter's check is request.getHeader(HttpHeaders.AUTHORIZATION) != null — getHeader is case-insensitive per servlet spec, so this should work, but make it explicit.

5. No tests for SSE-specific behaviour
   The PR body specifically mentions /api/notifications/stream and /api/ocr/jobs/.../progress. SSE has nothing special at the filter level, but a smoke test against the SSE endpoint in the integration test (verifying it returns text/event-stream after auth) would prove the production scenario end-to-end.

6. Drive-by @ActiveProfiles("test") on ThumbnailServiceIntegrationTest
   This is the test fix I'd actually like to verify with a CI run on the branch before merge. Confirm the green pipeline on PR head, then merge. Don't trust the local ./mvnw test — the fail-closed behaviour from #516 is profile-sensitive.

What I like

• Test names read as sentences. promotes_url_encoded_auth_token_cookie_to_decoded_Authorization_header tells me exactly what behaviour is verified without reading the body.
• ArgumentCaptor usage is correct — verifying the wrapped request reached the chain, not just that doFilter was called.
• times(1) is over-specified but harmless. verify(chain) defaults to 1, so the explicit times(1) adds noise without value. Style nit; not blocking.
• The "explicit header wins, cookie ignored" test is the one I most wanted to see — auth precedence is the kind of bug that hides for months.

Add the integration test and the malformed-encoding test, and I'm green.

— Sara Holt

## Sara Holt — Senior QA Engineer **Verdict: Approved with concerns** Five unit tests for a five-branch filter — good ratio, good naming, good AAA structure. What's missing is the integration test that proves this works *with* Spring Security. A unit test confirming the wrapper exposes `Authorization` is necessary but not sufficient; what we actually care about is: "does `BasicAuthenticationFilter` see the promoted header and authenticate?" That needs a real Spring context. ### Blockers 1. **Missing integration test — the only test that proves the bug is actually fixed** Add a `@SpringBootTest` (or `@WebMvcTest` with `@Import(SecurityConfig.class)` if you want it lightweight) that: - Sends a request to any authenticated endpoint (e.g. `/api/users/me`) - With a `Cookie: auth_token=Basic%20<base64(test@user:test123)>` header — no `Authorization` header - Asserts response is 200, not 401 And a paired test: - Same setup, but with `Cookie: auth_token=Basic%20<garbage>` - Asserts 401 (proving Spring's auth pipeline rejects bad creds, i.e. the filter doesn't bypass auth) Without this, the unit tests prove "the wrapper works in isolation" but not "the wrapper integrates with `BasicAuthenticationFilter`". Given the production bug we're fixing, the integration test is the test that gives us the most confidence on the next deploy. ### Suggestions 2. **Negative tests on URL-decoder edge cases** `URLDecoder.decode` can throw `IllegalArgumentException` on malformed `%` sequences (e.g. `Basic%2`). Add a test: ```java @Test void passes_through_when_auth_token_cookie_has_malformed_url_encoding() throws Exception { MockHttpServletRequest req = new MockHttpServletRequest(); req.setCookies(new Cookie("auth_token", "Basic%2")); // ... filter.doFilter(req, res, chain); // assert filter does not crash; either 401 from downstream or pass-through with raw value } ``` If this currently throws (likely), that's a bug — a malformed cookie would 500 the whole request. We need either a try/catch or a verified test asserting JDK behaviour is what we expect. 3. **Test the wrapper itself in isolation** `AuthHeaderRequest` is a non-trivial wrapper exposing three methods (`getHeader`, `getHeaders`, `getHeaderNames`). I'd extract those into a tiny test class: - `getHeader("Authorization")` returns override (case-insensitive) - `getHeader("authorization")` also returns override - `getHeaders("Authorization")` returns single-element enumeration - `getHeaderNames()` contains "Authorization" exactly once even if base already had one (today: collected into LinkedHashSet, so duplicate is implicitly dropped — verify) - `getHeader("Content-Type")` returns the base value The current tests cover the filter's *use* of the wrapper but not the wrapper's contract. The wrapper is the part most likely to surprise us downstream — test it directly. 4. **Case sensitivity test missing** Most servers normalize to canonical case, but HTTP headers are case-insensitive per RFC. Test: ```java req.addHeader("authorization", "Basic explicit"); // lowercase ``` The filter's check is `request.getHeader(HttpHeaders.AUTHORIZATION) != null` — `getHeader` is case-insensitive per servlet spec, so this should work, but make it explicit. 5. **No tests for SSE-specific behaviour** The PR body specifically mentions `/api/notifications/stream` and `/api/ocr/jobs/.../progress`. SSE has nothing special at the filter level, but a smoke test against the SSE endpoint in the integration test (verifying it returns `text/event-stream` after auth) would prove the production scenario end-to-end. 6. **Drive-by `@ActiveProfiles("test")` on `ThumbnailServiceIntegrationTest`** This is the test fix I'd actually like to verify with a CI run on the branch before merge. Confirm the green pipeline on PR head, then merge. Don't trust the local `./mvnw test` — the fail-closed behaviour from #516 is profile-sensitive. ### What I like - Test names read as sentences. `promotes_url_encoded_auth_token_cookie_to_decoded_Authorization_header` tells me exactly what behaviour is verified without reading the body. - `ArgumentCaptor` usage is correct — verifying the *wrapped* request reached the chain, not just that `doFilter` was called. - `times(1)` is over-specified but harmless. `verify(chain)` defaults to 1, so the explicit `times(1)` adds noise without value. Style nit; not blocking. - The "explicit header wins, cookie ignored" test is the one I most wanted to see — auth precedence is the kind of bug that hides for months. Add the integration test and the malformed-encoding test, and I'm green. — Sara Holt

marcel commented 2026-05-11 17:45:49 +02:00

Author

Owner

Copy Link

Tobias Wendt — DevOps & Platform Engineer

Verdict: Approved

This fix removes a real production failure mode — "auth works in dev because Vite, breaks in prod because Caddy" — by moving the translation into the backend, where it belongs. From an operations standpoint that's exactly the right call: fewer environment-specific knobs, fewer places where staging and prod can diverge silently.

What I checked

• No infrastructure change required: no Caddy update, no compose change, no env var addition. Deploys the same way as any other backend commit. Good.
• No new image dependency, no new port, no new volume. Pure code change.
• Filter runs in-process at HIGHEST_PRECEDENCE: no measurable overhead, no observable behaviour change for unauth'd endpoints. Won't affect /actuator/health (permitAll, no cookie inspection needed, fast path).
• Drive-by @ActiveProfiles("test") on ThumbnailServiceIntegrationTest: fixes a CI failure introduced by #516. This is exactly the kind of post-merge babysitting that I appreciate — catching the integration test breakage before it became a "main is red, can't deploy" emergency.

Suggestions

1. Add the production smoke-test to the deploy checklist, not just the PR
   The PR test plan says "redeploy staging, observe no Basic-auth popup". Promote that to the docs/DEPLOYMENT.md post-deploy checklist as a permanent item, because the failure mode (silent 401 + browser popup) is hard to detect from a health endpoint — /actuator/health will be green while every authenticated SSE stream is broken. We need a human-eyes step in the deploy ritual.

2. Synthetic monitor for /api/notifications/stream?
   If we ever build a Prometheus blackbox check or uptime monitor, the SSE endpoint should be on it — it's the canary for this entire class of bug. Out of scope for this PR; file a follow-up if you agree.

3. Caddy access log: confirm Authorization headers are not logged
   Nora flagged this for the app side. From the infra side: check infra/caddy/Caddyfile (or equivalent) for any log directive that captures request headers. If Caddy logs Cookie: (it doesn't by default, but custom formats can), the Basic-auth token is in our centralized log aggregator for everyone with read access. I'm 90% confident our Caddy config doesn't do this — but a one-line grep -r 'request_headers' infra/caddy/ confirms. Quick win before merge.

4. Rollback story
   This is a single-commit, single-file (plus test) addition. Revert is trivial. The only risk on revert is: any browser holding the auth_token cookie will be back to the original broken state — but they were already in that state pre-PR, so revert is genuinely safe. Document the revert plan in the PR body if you want to be thorough; otherwise the diff speaks for itself.

What I don't worry about

• No DB migration → no rollback complexity.
• No new env var → no .env.example drift between environments.
• Pinned versions unchanged → no Renovate noise.
• No new permissions, no new image. Sandbox unchanged.

This is the kind of fix I like: small, reversible, removes operational complexity instead of adding it. Ship it after Nora signs off on the cookie-secure flag.

— Tobias Wendt

## Tobias Wendt — DevOps & Platform Engineer **Verdict: Approved** This fix removes a real production failure mode — "auth works in dev because Vite, breaks in prod because Caddy" — by moving the translation into the backend, where it belongs. From an operations standpoint that's exactly the right call: fewer environment-specific knobs, fewer places where staging and prod can diverge silently. ### What I checked - **No infrastructure change required**: no Caddy update, no compose change, no env var addition. Deploys the same way as any other backend commit. Good. - **No new image dependency, no new port, no new volume**. Pure code change. - **Filter runs in-process at `HIGHEST_PRECEDENCE`**: no measurable overhead, no observable behaviour change for unauth'd endpoints. Won't affect `/actuator/health` (permitAll, no cookie inspection needed, fast path). - **Drive-by `@ActiveProfiles("test")`** on `ThumbnailServiceIntegrationTest`: fixes a CI failure introduced by #516. This is exactly the kind of post-merge babysitting that I appreciate — catching the integration test breakage before it became a "main is red, can't deploy" emergency. ### Suggestions 1. **Add the production smoke-test to the deploy checklist, not just the PR** The PR test plan says "redeploy staging, observe no Basic-auth popup". Promote that to the `docs/DEPLOYMENT.md` post-deploy checklist as a permanent item, because the failure mode (silent 401 + browser popup) is hard to detect from a health endpoint — `/actuator/health` will be green while every authenticated SSE stream is broken. We need a human-eyes step in the deploy ritual. 2. **Synthetic monitor for `/api/notifications/stream`?** If we ever build a Prometheus blackbox check or uptime monitor, the SSE endpoint should be on it — it's the canary for this entire class of bug. Out of scope for this PR; file a follow-up if you agree. 3. **Caddy access log: confirm Authorization headers are not logged** Nora flagged this for the app side. From the infra side: check `infra/caddy/Caddyfile` (or equivalent) for any `log` directive that captures request headers. If Caddy logs `Cookie:` (it doesn't by default, but custom formats can), the Basic-auth token is in our centralized log aggregator for everyone with read access. I'm 90% confident our Caddy config doesn't do this — but a one-line `grep -r 'request_headers' infra/caddy/` confirms. Quick win before merge. 4. **Rollback story** This is a single-commit, single-file (plus test) addition. Revert is trivial. The only risk on revert is: any browser holding the `auth_token` cookie will be back to the original broken state — but they were already in that state pre-PR, so revert is genuinely safe. Document the revert plan in the PR body if you want to be thorough; otherwise the diff speaks for itself. ### What I don't worry about - No DB migration → no rollback complexity. - No new env var → no `.env.example` drift between environments. - Pinned versions unchanged → no Renovate noise. - No new permissions, no new image. Sandbox unchanged. This is the kind of fix I like: small, reversible, removes operational complexity instead of adding it. Ship it after Nora signs off on the cookie-`secure` flag. — Tobias Wendt

marcel commented 2026-05-11 17:46:09 +02:00

Author

Owner

Copy Link

Elicit — Requirements Engineer & Business Analyst

Verdict: Approved

This is a bug-fix PR with a clear, well-scoped problem statement and an aligned solution. The issue (#520) frames the user-visible symptom precisely — "browser pops a native Basic-auth dialog over the running SPA right after login" — and the fix delivers against that. From a requirements-traceability standpoint, the chain is clean: user-visible behaviour → root cause (Caddy doesn't translate cookie) → fix (backend filter) → acceptance test (no popup after redeploy).

Suggestions

1. Acceptance criteria in the PR body are good but not testable as-stated
   The test plan items are observational ("observe no Basic-auth popup", "DevTools Network tab shows 200"). For a regression suite — i.e. so a future PR can't reintroduce this — promote at least one to an automated check. Suggested wording:

   • AC1: A request to /api/users/me with only a Cookie: auth_token=Basic%20<valid-base64> header (no Authorization header) returns 200 with the user payload.
   • AC2: A request to /api/notifications/stream with the same cookie returns 200 with Content-Type: text/event-stream.
   • AC3: A request to any authenticated endpoint with neither cookie nor Authorization returns 401, and the response does NOT include WWW-Authenticate: Basic (to suppress browser popup) — though this is a separate concern, not addressed by this PR.
     AC3 is interesting: even with the cookie filter, an unauthenticated direct-to-API call still returns WWW-Authenticate: Basic, which still pops a dialog. Are we sure this PR alone closes the user-visible bug, or do we also need to suppress WWW-Authenticate for /api/* paths to fully solve the symptom? Worth confirming with a manual test before claiming "fixed".

2. The functional requirement is buried in implementation prose
   The PR description leads with the fix. For traceability — and for a non-technical stakeholder reviewing the changelog — flip it: lead with the user-visible requirement ("browser-side fetch and EventSource calls to /api/ must authenticate transparently using the auth_token cookie"), then describe the implementation. Small writing-style point; helps when this PR becomes a reference for the next person debugging auth.

3. Scope flag — the drive-by @ActiveProfiles("test") is acceptable but worth a note in the changelog
   Two unrelated changes in one PR is something I'd normally flag as scope-creep, but the PR body calls it out, the second change is one line, and CI was red. In requirements language: this PR satisfies two requirements (R1: fix prod auth; R2: restore green CI). Acceptable. Future PRs of similar shape should mention the second requirement in the title prefix (e.g. fix(security): ... + test: restore green CI for ThumbnailServiceIntegrationTest).

4. Documentation lag — docs/ARCHITECTURE.md permission system section may need an update
   This PR introduces a new authentication path (cookie → header promotion) that wasn't part of the original architecture document. Confirm docs/ARCHITECTURE.md (or wherever the auth flow is described) gets a sentence-level update describing the cookie-promotion behaviour. Otherwise the next dev reads outdated docs.

Out of scope but worth flagging for a follow-up issue

5. Consider documenting the auth-flow contract in one place
   Currently auth is described across: SecurityConfig.java (Java comments), frontend/src/routes/login/+page.server.ts (TypeScript code), frontend/src/hooks.server.ts (handleFetch), AuthTokenCookieFilter (new). For a solo + LLM workflow, having auth scattered across four files is fine if there's one canonical reference doc. Worth filing as a future issue: "Document auth flow in docs/AUTH.md or as an ADR".

What I approve of

• Issue #520 has a clear problem statement, reproduction steps, and acceptance criteria.
• This PR maps cleanly to the issue with no scope drift (modulo the called-out drive-by).
• The test plan is concrete and bounded.
• The fix removes a real failure mode that solo + remote-deploy workflows would otherwise burn hours on.

Approved — apply Nora's security blockers as the actual gating, my notes are nice-to-haves for documentation hygiene.

— Elicit (Requirements Engineer)

## Elicit — Requirements Engineer & Business Analyst **Verdict: Approved** This is a bug-fix PR with a clear, well-scoped problem statement and an aligned solution. The issue (#520) frames the user-visible symptom precisely — "browser pops a native Basic-auth dialog over the running SPA right after login" — and the fix delivers against that. From a requirements-traceability standpoint, the chain is clean: user-visible behaviour → root cause (Caddy doesn't translate cookie) → fix (backend filter) → acceptance test (no popup after redeploy). ### Suggestions 1. **Acceptance criteria in the PR body are good but not testable as-stated** The test plan items are observational ("observe no Basic-auth popup", "DevTools Network tab shows 200"). For a regression suite — i.e. so a future PR can't reintroduce this — promote at least one to an automated check. Suggested wording: - **AC1**: A request to `/api/users/me` with only a `Cookie: auth_token=Basic%20<valid-base64>` header (no `Authorization` header) returns 200 with the user payload. - **AC2**: A request to `/api/notifications/stream` with the same cookie returns 200 with `Content-Type: text/event-stream`. - **AC3**: A request to any authenticated endpoint with neither cookie nor `Authorization` returns 401, and the response does NOT include `WWW-Authenticate: Basic` (to suppress browser popup) — though this is a separate concern, not addressed by this PR. AC3 is interesting: even with the cookie filter, an unauthenticated direct-to-API call still returns `WWW-Authenticate: Basic`, which still pops a dialog. Are we sure this PR alone closes the user-visible bug, or do we also need to suppress `WWW-Authenticate` for `/api/*` paths to fully solve the symptom? Worth confirming with a manual test before claiming "fixed". 2. **The functional requirement is buried in implementation prose** The PR description leads with the fix. For traceability — and for a non-technical stakeholder reviewing the changelog — flip it: lead with the *user-visible requirement* ("browser-side fetch and EventSource calls to /api/ must authenticate transparently using the auth_token cookie"), then describe the implementation. Small writing-style point; helps when this PR becomes a reference for the next person debugging auth. 3. **Scope flag — the drive-by `@ActiveProfiles("test")` is acceptable but worth a note in the changelog** Two unrelated changes in one PR is something I'd normally flag as scope-creep, but the PR body calls it out, the second change is one line, and CI was red. In requirements language: this PR satisfies two requirements (R1: fix prod auth; R2: restore green CI). Acceptable. Future PRs of similar shape should mention the second requirement in the title prefix (e.g. `fix(security): ... + test: restore green CI for ThumbnailServiceIntegrationTest`). 4. **Documentation lag — `docs/ARCHITECTURE.md` permission system section may need an update** This PR introduces a new authentication path (cookie → header promotion) that wasn't part of the original architecture document. Confirm `docs/ARCHITECTURE.md` (or wherever the auth flow is described) gets a sentence-level update describing the cookie-promotion behaviour. Otherwise the next dev reads outdated docs. ### Out of scope but worth flagging for a follow-up issue 5. **Consider documenting the auth-flow contract in one place** Currently auth is described across: `SecurityConfig.java` (Java comments), `frontend/src/routes/login/+page.server.ts` (TypeScript code), `frontend/src/hooks.server.ts` (handleFetch), `AuthTokenCookieFilter` (new). For a solo + LLM workflow, having auth scattered across four files is fine *if* there's one canonical reference doc. Worth filing as a future issue: "Document auth flow in `docs/AUTH.md` or as an ADR". ### What I approve of - Issue #520 has a clear problem statement, reproduction steps, and acceptance criteria. - This PR maps cleanly to the issue with no scope drift (modulo the called-out drive-by). - The test plan is concrete and bounded. - The fix removes a real failure mode that solo + remote-deploy workflows would otherwise burn hours on. Approved — apply Nora's security blockers as the actual gating, my notes are nice-to-haves for documentation hygiene. — Elicit (Requirements Engineer)

marcel commented 2026-05-11 17:46:20 +02:00

Author

Owner

Copy Link

Leonie Voss — UI/UX Designer & Accessibility Strategist

Verdict: Approved — LGTM (no UI changes)

This is a backend-only PR (one Java filter + one test class + one @ActiveProfiles annotation). No frontend code, no markup, no styles, no accessibility surface touched.

What I checked

• No frontend/ changes in the diff. No Svelte components added/modified, no Tailwind utilities touched, no messages/{de,en,es}.json updates needed.
• No new error codes — so no i18n / accessible-error-message work required.
• No new UI affordance — the user-visible result of this PR is the absence of an unwanted UX event (the native Basic-auth dialog the browser was popping after login). From a design standpoint that's a regression fix, not a feature.

UX impact (positive, indirect)

The bug this PR fixes was a particularly nasty UX regression: a native browser modal sitting on top of the SPA, blocking interaction, with no relationship to our visual language. For a 67-year-old transcriber on a tablet, that popup is confusing in a way our own error UI never would be — it's unbranded, it asks for credentials in an unfamiliar form, and dismissing it doesn't recover the session. Fixing this restores the experience we actually designed for. Worth a line in the release notes if we publish those: "browser auth dialog no longer interrupts logged-in sessions".

Out of scope but worth flagging for product

The pre-fix failure mode (silent 401 on browser-direct API calls) means real users may have been seeing partially-loaded pages — missing notifications, broken transcription panels — without an obvious error. If we have any user feedback or support tickets that mention "I logged in but nothing loaded", they're probably this bug. Worth a one-line check with the small group of beta transcribers to confirm we caught all the reports.

No blockers, no suggestions. Backend ships when backend reviewers are happy.

— Leonie Voss

## Leonie Voss — UI/UX Designer & Accessibility Strategist **Verdict: Approved — LGTM (no UI changes)** This is a backend-only PR (one Java filter + one test class + one `@ActiveProfiles` annotation). No frontend code, no markup, no styles, no accessibility surface touched. ### What I checked - No `frontend/` changes in the diff. No Svelte components added/modified, no Tailwind utilities touched, no `messages/{de,en,es}.json` updates needed. - No new error codes — so no i18n / accessible-error-message work required. - No new UI affordance — the user-visible result of this PR is the *absence* of an unwanted UX event (the native Basic-auth dialog the browser was popping after login). From a design standpoint that's a regression fix, not a feature. ### UX impact (positive, indirect) The bug this PR fixes was a particularly nasty UX regression: a native browser modal sitting on top of the SPA, blocking interaction, with no relationship to our visual language. For a 67-year-old transcriber on a tablet, that popup is confusing in a way our own error UI never would be — it's unbranded, it asks for credentials in an unfamiliar form, and dismissing it doesn't recover the session. Fixing this restores the experience we actually designed for. Worth a line in the release notes if we publish those: "browser auth dialog no longer interrupts logged-in sessions". ### Out of scope but worth flagging for product The pre-fix failure mode (silent 401 on browser-direct API calls) means real users may have been seeing partially-loaded pages — missing notifications, broken transcription panels — without an obvious error. If we have any user feedback or support tickets that mention "I logged in but nothing loaded", they're probably this bug. Worth a one-line check with the small group of beta transcribers to confirm we caught all the reports. No blockers, no suggestions. Backend ships when backend reviewers are happy. — Leonie Voss

marcel added 1 commit 2026-05-11 17:50:17 +02:00

fixup: address Nora's review on #520 (security blockers) ... 285764fe42

- frontend/login: derive cookie `secure` flag from request URL protocol.
  Pre-PR the cookie was only read by SSR so the flag didn't matter; now
  the cookie IS the API credential and must be Secure on HTTPS or it
  leaks a 24h Basic token on plaintext networks. Dev runs over HTTP and
  would silently lose the cookie if we hardcoded `secure: true`, so the
  flag follows `event.url.protocol === 'https:'`.

- SecurityConfig: rewrite the CSRF-disabled comment. The old
  "browsers block cross-origin custom headers" justification no longer
  holds once /api/* is authenticated via the cookie. Make the
  load-bearing dependencies explicit: SameSite=strict on the auth_token
  cookie + Spring's default CORS rejection.

- AuthTokenCookieFilter:
  - Scope to /api/* only. /actuator/health and similar must not be
    cookie-authenticated.
  - Refuse malformed percent-encoding (URLDecoder throws); forward the
    request without a promoted Authorization rather than crash.
  - Use isBlank() instead of isEmpty() per Nora.
  - Javadoc warning: getHeaderNames/getHeaders exposes the Basic
    credential; any future header-iterating logger must scrub
    Authorization before logging.

- Tests: add `passes_through_unchanged_when_request_is_outside_api_scope`
  (/actuator/health with cookie should NOT be wrapped) and
  `passes_through_unchanged_when_cookie_value_is_malformed_percent_encoding`.
  Tighten the explicit-header test to verify same-instance forwarding
  rather than just header equality.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

marcel merged commit 48c8bb8a5f into main 2026-05-11 18:20:10 +02:00

marcel deleted branch fix/issue-520-cookie-to-authorization-filter 2026-05-11 18:20:11 +02:00

marcel referenced this pull request 2026-05-11 18:20:55 +02:00

tech-debt(auth): replace cookie-promotion glue with a proper session-based auth model #522

marcel referenced this pull request 2026-05-11 18:49:05 +02:00

feat(auth): server-side session model replacing Basic-auth cookie promotion #523

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#521