fix(security): add csrfFetch wrapper, apply to all client-side mutating requests #695

marcel · 2026-05-30T10:51:41+02:00

marcel commented

2026-05-30 10:51:41 +02:00

Summary

Introduces csrfFetch = makeCsrfFetch(fetch) in cookies.ts — a drop-in replacement for fetch that auto-injects X-XSRF-TOKEN on POST/PUT/PATCH/DELETE, so call sites never need to remember withCsrf() manually.
Fixes 8 call sites that were sending mutating requests without the CSRF header: annotation resize, comment POST/PATCH/DELETE, Geschichte CRUD (create/edit/delete), Stammbaum relationship creation, bulk-edit PATCH, and file upload.
Migrates all 14 existing withCsrf call sites to csrfFetch for consistency. withCsrf and makeCsrfFetch are kept in the API for injectable-fetch use cases (e.g. useTranscriptionBlocks in tests).

Root cause

PdfViewer.svelte's updateAnnotation used bare fetch without withCsrf(), triggering CSRF_TOKEN_MISSING → "Sitzungsfehler. Bitte laden Sie die Seite neu." An audit of all client-side fetch calls found 7 more call sites with the same gap.

Test plan

Resize an annotation on a document — should no longer show Sitzungsfehler
Post, edit, and delete a comment — all should succeed
Create, edit, delete a Geschichte
Add a relationship in the Stammbaum side panel
Upload a file on the document edit page
Trigger import / thumbnail backfill from admin/system

## Summary - Introduces `csrfFetch = makeCsrfFetch(fetch)` in `cookies.ts` — a drop-in replacement for `fetch` that auto-injects `X-XSRF-TOKEN` on POST/PUT/PATCH/DELETE, so call sites never need to remember `withCsrf()` manually. - Fixes 8 call sites that were sending mutating requests **without** the CSRF header: annotation resize, comment POST/PATCH/DELETE, Geschichte CRUD (create/edit/delete), Stammbaum relationship creation, bulk-edit PATCH, and file upload. - Migrates all 14 existing `withCsrf` call sites to `csrfFetch` for consistency. `withCsrf` and `makeCsrfFetch` are kept in the API for injectable-fetch use cases (e.g. `useTranscriptionBlocks` in tests). ## Root cause `PdfViewer.svelte`'s `updateAnnotation` used bare `fetch` without `withCsrf()`, triggering `CSRF_TOKEN_MISSING` → "Sitzungsfehler. Bitte laden Sie die Seite neu." An audit of all client-side fetch calls found 7 more call sites with the same gap. ## Test plan - [ ] Resize an annotation on a document — should no longer show Sitzungsfehler - [ ] Post, edit, and delete a comment — all should succeed - [ ] Create, edit, delete a Geschichte - [ ] Add a relationship in the Stammbaum side panel - [ ] Upload a file on the document edit page - [ ] Trigger import / thumbnail backfill from admin/system

marcel added 1 commit 2026-05-30 10:51:41 +02:00

fix(security): add csrfFetch wrapper and apply to all client-side mutating requests

CI / Unit & Component Tests (pull_request) Failing after 2m52s

Details

CI / OCR Service Tests (pull_request) Successful in 21s

Details

CI / Backend Unit Tests (pull_request) Successful in 3m48s

Details

CI / fail2ban Regex (pull_request) Successful in 44s

Details

CI / Semgrep Security Scan (pull_request) Successful in 20s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 1m4s

Details

58254b492b

Introduces `csrfFetch` (= `makeCsrfFetch(fetch)`) in cookies.ts as a
drop-in fetch replacement that auto-injects X-XSRF-TOKEN on POST/PUT/PATCH/DELETE.

Previously 8 call sites sent mutating requests without the CSRF header —
annotation resize, comment POST/PATCH/DELETE, Geschichte CRUD, Stammbaum
relationship creation, bulk-edit PATCH, and file upload — all would fail
with CSRF_TOKEN_MISSING if the backend's cookie-based protection triggered.

All 14 client-side mutating fetches now use csrfFetch; withCsrf/makeCsrfFetch
remain in the API for injectable-fetch use cases (e.g. useTranscriptionBlocks).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel added the bug security labels 2026-05-30 10:51:46 +02:00

marcel commented

2026-05-30 10:53:06 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

The migration is clean, mechanically correct, and internally consistent. The csrfFetch naming is intent-revealing. Keeping makeCsrfFetch for the injectable-fetch path (useTranscriptionBlocks) is the right call. Good DRY improvement.

Blockers

No failing test was written first.

This PR fixes 8 bugs and introduces a new API (csrfFetch). Per TDD, each fix requires a failing test that proves the bug existed before the fix lands. Right now, the regression test suite has no proof that these 8 call sites were ever missing the CSRF header — and no proof that csrfFetch actually injects one.

A minimal cookies.ts unit test that covers the contract:

// cookies.test.ts
import { describe, it, expect, vi, beforeEach } from 'vitest';

// mock document.cookie before importing the module
beforeEach(() => {
  Object.defineProperty(document, 'cookie', {
    writable: true,
    value: 'XSRF-TOKEN=test-token-123'
  });
});

describe('csrfFetch', () => {
  it('injects X-XSRF-TOKEN on PATCH', async () => {
    const inner = vi.fn().mockResolvedValue(new Response());
    const csrf = makeCsrfFetch(inner);
    await csrf('/api/test', { method: 'PATCH' });
    expect(inner.mock.calls[0][1]?.headers?.get('X-XSRF-TOKEN')).toBe('test-token-123');
  });

  it('does NOT inject X-XSRF-TOKEN on GET', async () => {
    const inner = vi.fn().mockResolvedValue(new Response());
    const csrf = makeCsrfFetch(inner);
    await csrf('/api/test', { method: 'GET' });
    expect(inner.mock.calls[0][1]?.headers?.get?.('X-XSRF-TOKEN')).toBeFalsy();
  });
});

Without this, the test suite gives no regression guarantee. If someone replaces csrfFetch with bare fetch in three months, CI won't catch it.

Suggestions

csrfFetch is initialized at module load time with the global fetch. This is intentional and fine for browser contexts, but worth a one-line doc comment noting it's client-side only (the getCsrfToken() no-ops server-side anyway).
The useBlockAutoSave.svelte.ts change dropped withCsrf for csrfFetch but the void fire-and-forget pattern is unchanged — correct, since the result is intentionally not awaited (keepalive beacon on page unload).

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** The migration is clean, mechanically correct, and internally consistent. The `csrfFetch` naming is intent-revealing. Keeping `makeCsrfFetch` for the injectable-fetch path (`useTranscriptionBlocks`) is the right call. Good DRY improvement. --- ### Blockers **No failing test was written first.** This PR fixes 8 bugs and introduces a new API (`csrfFetch`). Per TDD, each fix requires a failing test that proves the bug existed before the fix lands. Right now, the regression test suite has no proof that these 8 call sites were ever missing the CSRF header — and no proof that `csrfFetch` actually injects one. A minimal `cookies.ts` unit test that covers the contract: ```typescript // cookies.test.ts import { describe, it, expect, vi, beforeEach } from 'vitest'; // mock document.cookie before importing the module beforeEach(() => { Object.defineProperty(document, 'cookie', { writable: true, value: 'XSRF-TOKEN=test-token-123' }); }); describe('csrfFetch', () => { it('injects X-XSRF-TOKEN on PATCH', async () => { const inner = vi.fn().mockResolvedValue(new Response()); const csrf = makeCsrfFetch(inner); await csrf('/api/test', { method: 'PATCH' }); expect(inner.mock.calls[0][1]?.headers?.get('X-XSRF-TOKEN')).toBe('test-token-123'); }); it('does NOT inject X-XSRF-TOKEN on GET', async () => { const inner = vi.fn().mockResolvedValue(new Response()); const csrf = makeCsrfFetch(inner); await csrf('/api/test', { method: 'GET' }); expect(inner.mock.calls[0][1]?.headers?.get?.('X-XSRF-TOKEN')).toBeFalsy(); }); }); ``` Without this, the test suite gives no regression guarantee. If someone replaces `csrfFetch` with bare `fetch` in three months, CI won't catch it. --- ### Suggestions - `csrfFetch` is initialized at module load time with the global `fetch`. This is intentional and fine for browser contexts, but worth a one-line doc comment noting it's client-side only (the `getCsrfToken()` no-ops server-side anyway). - The `useBlockAutoSave.svelte.ts` change dropped `withCsrf` for `csrfFetch` but the `void` fire-and-forget pattern is unchanged — correct, since the result is intentionally not awaited (keepalive beacon on page unload).

marcel commented

2026-05-30 10:53:20 +02:00

🏗️ Markus Keller — Application Architect

Verdict: ✅ Approved

Structurally sound. This is exactly the right place to put this abstraction: a shared utility in $lib/shared/cookies.ts that centralizes a cross-cutting concern. The domain boundary is respected — no component needs to know about CSRF mechanics anymore.

What I checked

Documentation table (architect's checklist):

Trigger	Required update	Status
New SvelteKit route	`CLAUDE.md` route table	N/A — no new routes
New backend package/domain	`CLAUDE.md` + C4 diagram	N/A
New Flyway migration	DB diagrams	N/A
Auth or upload flow change	`seq-auth-flow.puml` / `seq-document-upload.puml`	Worth considering — see note
Architectural decision	New ADR	Worth considering — see note

No blockers here — the change is a bug fix with no schema, route, or domain changes.

Notes (non-blocking)

Sequence diagrams: seq-auth-flow.puml and seq-document-upload.puml — if either of these diagrams currently shows a mutating client fetch, they should show csrfFetch in the arrow label going forward. Quick check: if they don't model client-side fetch headers at all (likely), no update needed.

ADR consideration: The pattern "all client-side mutating fetches MUST use csrfFetch" is now a project convention. Whether it warrants a lightweight ADR entry (context: CSRF gap found; decision: csrfFetch wrapper; consequences: all new client-side mutations must use it) is a judgment call. Given the project's existing ADR discipline, I'd lean toward adding a one-paragraph entry under the security ADR rather than a full new ADR. Not a blocker.

csrfFetch at module scope: The module-level export const csrfFetch = makeCsrfFetch(fetch) captures globalThis.fetch at import time. In browser contexts this is the right behavior. The existing makeCsrfFetch injection pattern for useTranscriptionBlocks correctly handles the test case (injectable fetch). The two patterns coexist without tension. ✓

## 🏗️ Markus Keller — Application Architect **Verdict: ✅ Approved** Structurally sound. This is exactly the right place to put this abstraction: a shared utility in `$lib/shared/cookies.ts` that centralizes a cross-cutting concern. The domain boundary is respected — no component needs to know about CSRF mechanics anymore. --- ### What I checked **Documentation table (architect's checklist):** | Trigger | Required update | Status | |---|---|---| | New SvelteKit route | `CLAUDE.md` route table | N/A — no new routes | | New backend package/domain | `CLAUDE.md` + C4 diagram | N/A | | New Flyway migration | DB diagrams | N/A | | Auth or upload flow change | `seq-auth-flow.puml` / `seq-document-upload.puml` | Worth considering — see note | | Architectural decision | New ADR | Worth considering — see note | No blockers here — the change is a bug fix with no schema, route, or domain changes. --- ### Notes (non-blocking) **Sequence diagrams:** `seq-auth-flow.puml` and `seq-document-upload.puml` — if either of these diagrams currently shows a mutating client fetch, they should show `csrfFetch` in the arrow label going forward. Quick check: if they don't model client-side fetch headers at all (likely), no update needed. **ADR consideration:** The pattern "all client-side mutating fetches MUST use `csrfFetch`" is now a project convention. Whether it warrants a lightweight ADR entry (context: CSRF gap found; decision: `csrfFetch` wrapper; consequences: all new client-side mutations must use it) is a judgment call. Given the project's existing ADR discipline, I'd lean toward adding a one-paragraph entry under the security ADR rather than a full new ADR. Not a blocker. **`csrfFetch` at module scope:** The module-level `export const csrfFetch = makeCsrfFetch(fetch)` captures `globalThis.fetch` at import time. In browser contexts this is the right behavior. The existing `makeCsrfFetch` injection pattern for `useTranscriptionBlocks` correctly handles the test case (injectable fetch). The two patterns coexist without tension. ✓

marcel commented

2026-05-30 10:53:38 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ⚠️ Approved with concerns

This PR closes a real security gap. Eight client-side mutations were going out without X-XSRF-TOKEN, making them vulnerable to Cross-Site Request Forgery from any page that could trick a logged-in user's browser into firing the request. The root cause (annotation resize) is fixed, and the scope audit was thorough.

The architectural move to csrfFetch as the blessed wrapper is good defense-in-depth engineering — it makes the safe path the easy path.

What this actually fixed

The CSRF double-submit cookie pattern Spring Security uses here:

Server sets XSRF-TOKEN=abc123 cookie (not HttpOnly — readable by JS)
Client must send X-XSRF-TOKEN: abc123 header on mutating requests
Server validates cookie == header

Previously, the SvelteKit server-side handleFetch hook could serve as a fallback (generating a fresh UUID for both cookie and header when the browser cookie was absent), but this created a dependency on two independent defense layers both working correctly. That's not ideal.

Now the client sends the token explicitly, the proxy forwards Content-Type only (still fine — the server-side hook adds auth), and Spring gets consistent evidence. ✓

Concern (non-blocking, but file a follow-up)

No regression test proves the token is actually sent.

The security rule is: "Before this PR, calling updateAnnotation() without CSRF sent a request that returned CSRF_TOKEN_MISSING." That rule should live as a permanent automated test, not just in the PR description.

The expected test structure (using makeCsrfFetch since it accepts an injectable fetch):

// In a new cookies.security.test.ts or similar
it('csrfFetch includes X-XSRF-TOKEN header on mutating requests', async () => {
  // Given: XSRF-TOKEN cookie is present
  vi.stubGlobal('document', { cookie: 'XSRF-TOKEN=csrf-test-value' });
  const captured: RequestInit[] = [];
  const mockFetch = vi.fn((_url, init) => {
    captured.push(init);
    return Promise.resolve(new Response('{}'));
  });
  const csrf = makeCsrfFetch(mockFetch);

  // When: a PATCH request is made
  await csrf('/api/test', { method: 'PATCH', headers: { 'Content-Type': 'application/json' } });

  // Then: X-XSRF-TOKEN is present
  const headers = new Headers(captured[0]?.headers);
  expect(headers.get('X-XSRF-TOKEN')).toBe('csrf-test-value');
});

Without this, someone could replace csrfFetch with fetch in a future PR and CI would stay green.

What's still correctly untouched

useTranscriptionBlocks.svelte.ts keeps makeCsrfFetch(options.fetchImpl ?? fetch) — correctly injectable for tests. ✓
Server-side load functions and form actions go through the SvelteKit handleFetch hook which handles CSRF server-side — these don't need csrfFetch. ✓
withCsrf and makeCsrfFetch remain exported — no breaking API change for any test code that imports them. ✓

Recommended follow-up issue: "Add regression test coverage for csrfFetch CSRF token injection"

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ⚠️ Approved with concerns** This PR closes a real security gap. Eight client-side mutations were going out without `X-XSRF-TOKEN`, making them vulnerable to Cross-Site Request Forgery from any page that could trick a logged-in user's browser into firing the request. The root cause (annotation resize) is fixed, and the scope audit was thorough. The architectural move to `csrfFetch` as the blessed wrapper is good defense-in-depth engineering — it makes the safe path the easy path. --- ### What this actually fixed The CSRF double-submit cookie pattern Spring Security uses here: - Server sets `XSRF-TOKEN=abc123` cookie (not HttpOnly — readable by JS) - Client must send `X-XSRF-TOKEN: abc123` header on mutating requests - Server validates cookie == header Previously, the SvelteKit server-side `handleFetch` hook could serve as a fallback (generating a fresh UUID for both cookie and header when the browser cookie was absent), but this created a dependency on two independent defense layers both working correctly. That's not ideal. Now the client sends the token explicitly, the proxy forwards `Content-Type` only (still fine — the server-side hook adds auth), and Spring gets consistent evidence. ✓ --- ### Concern (non-blocking, but file a follow-up) **No regression test proves the token is actually sent.** The security rule is: "Before this PR, calling `updateAnnotation()` without CSRF sent a request that returned `CSRF_TOKEN_MISSING`." That rule should live as a permanent automated test, not just in the PR description. The expected test structure (using `makeCsrfFetch` since it accepts an injectable fetch): ```typescript // In a new cookies.security.test.ts or similar it('csrfFetch includes X-XSRF-TOKEN header on mutating requests', async () => { // Given: XSRF-TOKEN cookie is present vi.stubGlobal('document', { cookie: 'XSRF-TOKEN=csrf-test-value' }); const captured: RequestInit[] = []; const mockFetch = vi.fn((_url, init) => { captured.push(init); return Promise.resolve(new Response('{}')); }); const csrf = makeCsrfFetch(mockFetch); // When: a PATCH request is made await csrf('/api/test', { method: 'PATCH', headers: { 'Content-Type': 'application/json' } }); // Then: X-XSRF-TOKEN is present const headers = new Headers(captured[0]?.headers); expect(headers.get('X-XSRF-TOKEN')).toBe('csrf-test-value'); }); ``` Without this, someone could replace `csrfFetch` with `fetch` in a future PR and CI would stay green. --- ### What's still correctly untouched - `useTranscriptionBlocks.svelte.ts` keeps `makeCsrfFetch(options.fetchImpl ?? fetch)` — correctly injectable for tests. ✓ - Server-side load functions and form actions go through the SvelteKit `handleFetch` hook which handles CSRF server-side — these don't need `csrfFetch`. ✓ - `withCsrf` and `makeCsrfFetch` remain exported — no breaking API change for any test code that imports them. ✓ **Recommended follow-up issue:** "Add regression test coverage for `csrfFetch` CSRF token injection"

marcel commented

2026-05-30 10:53:53 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: 🚫 Changes requested

Eight bugs fixed, zero tests added. I can't approve a security bug fix without regression coverage — the whole point of regression tests is that the next person who touches this code can't accidentally reintroduce the bug without CI screaming.

Blockers

1. No unit test for csrfFetch in cookies.ts

The new csrfFetch export has no test. The existing makeCsrfFetch may be exercised indirectly through useTranscriptionBlocks tests, but csrfFetch (the module-level singleton) has no direct coverage.

Minimum required test (one file, three cases):

// frontend/src/lib/shared/cookies.test.ts
describe('makeCsrfFetch', () => {
  it('adds X-XSRF-TOKEN header on POST when cookie present', async () => { ... });
  it('adds X-XSRF-TOKEN header on PATCH when cookie present', async () => { ... });
  it('does NOT add X-XSRF-TOKEN header on GET', async () => { ... });
  it('leaves init unchanged when XSRF-TOKEN cookie is absent', async () => { ... });
});

2. No regression test for the original bug

The PR description says annotation resize caused a Sitzungsfehler. There is no test that:

Proves the updateAnnotation path sends X-XSRF-TOKEN
Would catch a regression if csrfFetch were accidentally removed from PdfViewer.svelte

Even a simple Vitest browser test on AnnotationEditOverlay that asserts the injected updateAnnotation context function calls csrfFetch (not bare fetch) would close this gap.

Suggestions (non-blocking)

Test plan is entirely manual. The PR checklist has six manual steps. These should become the acceptance criteria for follow-up automated tests:

Manual check	Automation target
Resize annotation → no Sitzungsfehler	Vitest: `updateAnnotation` invokes `csrfFetch` with PATCH
Post/edit/delete comment	Vitest: each `CommentThread` mutation path uses `csrfFetch`
Create/edit/delete Geschichte	Playwright: Geschichte CRUD E2E journey
Add relationship in Stammbaum	Vitest: `handleAddRelationship` uses `csrfFetch`

Existing makeCsrfFetch coverage: If useTranscriptionBlocks.svelte.ts tests inject a mock fetch and verify PUT/PATCH headers, that's partial coverage of the mechanism. Check whether those tests explicitly assert X-XSRF-TOKEN is present — if they don't, extend them.

I'll approve if a cookies.test.ts covering makeCsrfFetch is added and the PR description links a follow-up issue for the remaining per-component regression tests.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: 🚫 Changes requested** Eight bugs fixed, zero tests added. I can't approve a security bug fix without regression coverage — the whole point of regression tests is that the next person who touches this code can't accidentally reintroduce the bug without CI screaming. --- ### Blockers **1. No unit test for `csrfFetch` in `cookies.ts`** The new `csrfFetch` export has no test. The existing `makeCsrfFetch` may be exercised indirectly through `useTranscriptionBlocks` tests, but `csrfFetch` (the module-level singleton) has no direct coverage. Minimum required test (one file, three cases): ```typescript // frontend/src/lib/shared/cookies.test.ts describe('makeCsrfFetch', () => { it('adds X-XSRF-TOKEN header on POST when cookie present', async () => { ... }); it('adds X-XSRF-TOKEN header on PATCH when cookie present', async () => { ... }); it('does NOT add X-XSRF-TOKEN header on GET', async () => { ... }); it('leaves init unchanged when XSRF-TOKEN cookie is absent', async () => { ... }); }); ``` **2. No regression test for the original bug** The PR description says annotation resize caused a Sitzungsfehler. There is no test that: - Proves the `updateAnnotation` path sends `X-XSRF-TOKEN` - Would catch a regression if `csrfFetch` were accidentally removed from `PdfViewer.svelte` Even a simple Vitest browser test on `AnnotationEditOverlay` that asserts the injected `updateAnnotation` context function calls `csrfFetch` (not bare `fetch`) would close this gap. --- ### Suggestions (non-blocking) **Test plan is entirely manual.** The PR checklist has six manual steps. These should become the acceptance criteria for follow-up automated tests: | Manual check | Automation target | |---|---| | Resize annotation → no Sitzungsfehler | Vitest: `updateAnnotation` invokes `csrfFetch` with PATCH | | Post/edit/delete comment | Vitest: each `CommentThread` mutation path uses `csrfFetch` | | Create/edit/delete Geschichte | Playwright: Geschichte CRUD E2E journey | | Add relationship in Stammbaum | Vitest: `handleAddRelationship` uses `csrfFetch` | **Existing `makeCsrfFetch` coverage:** If `useTranscriptionBlocks.svelte.ts` tests inject a mock fetch and verify PUT/PATCH headers, that's partial coverage of the mechanism. Check whether those tests explicitly assert `X-XSRF-TOKEN` is present — if they don't, extend them. --- I'll approve if a `cookies.test.ts` covering `makeCsrfFetch` is added and the PR description links a follow-up issue for the remaining per-component regression tests.

marcel commented

2026-05-30 10:54:00 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

Pure TypeScript/Svelte change — nothing touches CI workflows, Docker Compose, image tags, environment variables, or infrastructure config. No infrastructure review items.

What I checked

No changes to .gitea/workflows/ or CI configuration ✓
No changes to docker-compose*.yml ✓
No new environment variables introduced ✓
No new services or dependencies added ✓
The change is fully contained within frontend/src/ ✓

One observation (informational, not a blocker)

The existing CI pipeline will run the frontend test suite (npm run test) and lint checks against the changed files. Since no new tests were added (Sara flagged this), CI won't catch a CSRF regression on future PRs. That's a gap in the quality gate, not an infrastructure issue — but if a cookies.test.ts is added later, it'll slot into the existing Vitest run without any CI changes needed.

Ship it.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** Pure TypeScript/Svelte change — nothing touches CI workflows, Docker Compose, image tags, environment variables, or infrastructure config. No infrastructure review items. --- ### What I checked - No changes to `.gitea/workflows/` or CI configuration ✓ - No changes to `docker-compose*.yml` ✓ - No new environment variables introduced ✓ - No new services or dependencies added ✓ - The change is fully contained within `frontend/src/` ✓ --- ### One observation (informational, not a blocker) The existing CI pipeline will run the frontend test suite (`npm run test`) and lint checks against the changed files. Since no new tests were added (Sara flagged this), CI won't catch a CSRF regression on future PRs. That's a gap in the quality gate, not an infrastructure issue — but if a `cookies.test.ts` is added later, it'll slot into the existing Vitest run without any CI changes needed. Ship it.

marcel commented

2026-05-30 10:54:09 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

No visual, interaction, or accessibility changes in this PR — it's a purely functional security fix. But I want to name the UX impact explicitly, because it's real.

UX impact of this fix (positive)

The "Sitzungsfehler. Bitte laden Sie die Seite neu." error was a Critical UX failure by my severity scale:

FINDING: Resizing an annotation triggered an opaque error dialog forcing a full page reload, discarding any unsaved context. For the 60+ transcriber persona working on a document, this appears as an unexplained system failure — no indication that the action could succeed, no recovery path other than "reload and try again."

This PR eliminates that failure silently and correctly. Eight similar silent failure paths (comment posting, Geschichte editing, relationship creation) were also fixed.

Nothing to flag from accessibility

No markup changes, no ARIA roles modified
No color or contrast changes
No touch target changes
No focus management changes

The void csrfFetch(...) keepalive pattern in useBlockAutoSave.svelte.ts is a background fire-and-forget — correct that there's no UX feedback for it (the auto-save pattern already handles status display separately).

No action required from my side.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** No visual, interaction, or accessibility changes in this PR — it's a purely functional security fix. But I want to name the UX impact explicitly, because it's real. --- ### UX impact of this fix (positive) The "Sitzungsfehler. Bitte laden Sie die Seite neu." error was a **Critical** UX failure by my severity scale: > **FINDING:** Resizing an annotation triggered an opaque error dialog forcing a full page reload, discarding any unsaved context. For the 60+ transcriber persona working on a document, this appears as an unexplained system failure — no indication that the action *could* succeed, no recovery path other than "reload and try again." This PR eliminates that failure silently and correctly. Eight similar silent failure paths (comment posting, Geschichte editing, relationship creation) were also fixed. --- ### Nothing to flag from accessibility - No markup changes, no ARIA roles modified - No color or contrast changes - No touch target changes - No focus management changes The `void csrfFetch(...)` keepalive pattern in `useBlockAutoSave.svelte.ts` is a background fire-and-forget — correct that there's no UX feedback for it (the auto-save pattern already handles status display separately). **No action required from my side.**

marcel commented

2026-05-30 10:54:22 +02:00

📋 Elicit — Requirements Engineer

Verdict: ⚠️ Approved with concerns

This PR closes an implicit, never-formally-documented security requirement. The fix is correct. My concern is process: this gap should have been caught at requirements stage — and the PR lacks automated acceptance criteria to prove the requirement is continuously met.

The missing requirement (now implicit)

The following NFR was never written, but was always implied:

NFR-SEC-XXX: When a client-side component performs any state-mutating HTTP request (POST, PUT, PATCH, DELETE), the system SHALL include the X-XSRF-TOKEN header matching the XSRF-TOKEN session cookie.

Eight components violated this NFR. The PR fixes the violations, but the NFR itself still isn't documented anywhere that would prevent future violations.

Recommendation: Add this to the security NFR register (or the CONTRIBUTING.md API section) as a documented rule: "All client-side fetch calls to mutating endpoints MUST use csrfFetch from $lib/shared/cookies. Do not use bare fetch for POST/PUT/PATCH/DELETE."

Test plan gap

The PR's test plan consists of six manual steps. In requirements terms, these are acceptance criteria — but without automated verification, they can silently regress:

Given	When	Then	Automated?
User is logged in and viewing a document	User resizes an annotation	No error is shown; annotation position is saved	❌ Manual
User is logged in	User posts a comment	Comment appears in thread	❌ Manual
User is logged in	User creates a Geschichte	Geschichte is created and user is redirected	❌ Manual

Each of these should have a corresponding automated test (Vitest or Playwright). The acceptance criteria exist in the PR description but aren't encoded in the test suite.

What's done well

The csrfFetch wrapper converts a scattered, opt-in pattern into a convention — the safe choice is now the default. This is correct requirements engineering: encoding the NFR into the API surface so developers cannot easily violate it by accident. The PR description includes a clear root cause and a clear test plan. Labels (bug, security) are correctly applied. ✓

Suggested follow-up issue title: "Add automated regression tests for CSRF token injection across all mutating client-side fetches (NFR-SEC-XXX)"

## 📋 Elicit — Requirements Engineer **Verdict: ⚠️ Approved with concerns** This PR closes an implicit, never-formally-documented security requirement. The fix is correct. My concern is process: this gap should have been caught at requirements stage — and the PR lacks automated acceptance criteria to prove the requirement is continuously met. --- ### The missing requirement (now implicit) The following NFR was never written, but was always implied: > **NFR-SEC-XXX:** When a client-side component performs any state-mutating HTTP request (POST, PUT, PATCH, DELETE), the system SHALL include the `X-XSRF-TOKEN` header matching the `XSRF-TOKEN` session cookie. Eight components violated this NFR. The PR fixes the violations, but the NFR itself still isn't documented anywhere that would prevent future violations. **Recommendation:** Add this to the security NFR register (or the `CONTRIBUTING.md` API section) as a documented rule: *"All client-side fetch calls to mutating endpoints MUST use `csrfFetch` from `$lib/shared/cookies`. Do not use bare `fetch` for POST/PUT/PATCH/DELETE."* --- ### Test plan gap The PR's test plan consists of six manual steps. In requirements terms, these are acceptance criteria — but without automated verification, they can silently regress: | Given | When | Then | Automated? | |---|---|---|---| | User is logged in and viewing a document | User resizes an annotation | No error is shown; annotation position is saved | ❌ Manual | | User is logged in | User posts a comment | Comment appears in thread | ❌ Manual | | User is logged in | User creates a Geschichte | Geschichte is created and user is redirected | ❌ Manual | Each of these should have a corresponding automated test (Vitest or Playwright). The acceptance criteria exist in the PR description but aren't encoded in the test suite. --- ### What's done well The `csrfFetch` wrapper converts a scattered, opt-in pattern into a convention — the safe choice is now the default. This is correct requirements engineering: encoding the NFR into the API surface so developers cannot easily violate it by accident. The PR description includes a clear root cause and a clear test plan. Labels (`bug`, `security`) are correctly applied. ✓ **Suggested follow-up issue title:** "Add automated regression tests for CSRF token injection across all mutating client-side fetches (NFR-SEC-XXX)"

marcel added 1 commit 2026-05-30 11:41:03 +02:00

fix(security): make csrfFetch a function to respect vi.stubGlobal mocks

CI / Unit & Component Tests (pull_request) Successful in 3m34s

Details

CI / OCR Service Tests (pull_request) Successful in 19s

Details

CI / Backend Unit Tests (pull_request) Successful in 3m45s

Details

CI / fail2ban Regex (pull_request) Successful in 43s

Details

CI / Semgrep Security Scan (pull_request) Successful in 20s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 1m5s

Details

5d8d85057d

The previous `export const csrfFetch = makeCsrfFetch(fetch)` captured the
global fetch at module evaluation time. Tests that mock fetch via
`vi.stubGlobal('fetch', mockFetch)` set up their stub *after* module import,
so all calls through csrfFetch bypassed the mock — 21 browser tests saw 0
fetch calls.

Changing csrfFetch to a plain function means `fetch` is resolved from the
global scope at each call site, picking up whatever stub is in place at
call time. Production behaviour is identical; test isolation is restored.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-05-30 11:47:01 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

The csrfFetch implementation is clean — single responsibility, well-named, and the JSDoc comment explains the testability constraint (the real why). The 15-file migration is purely mechanical: no creative interpretation, no reformatting, no bundled cleanup. Good discipline.

Observations (non-blocking)

useBlockAutoSave.svelte.ts asymmetry — the flushOnUnload path uses module-imported csrfFetch directly while the main debounced save goes through the injectable saveFn. The asymmetry is correct — keepalive fetch can't go through the API client pipeline, and saveFn wouldn't accept keepalive: true — but it will confuse future readers. A one-line comment on flushOnUnload like // keepalive flush bypasses saveFn; csrfFetch adds CSRF header for this raw PUT would save the next person 10 minutes.

bulk-edit/+page.svelte — batch-metadata POST — this is semantically a read (body carries a list of IDs, POST used for URL-length reasons). Adding CSRF is harmless and strictly correct. No action needed, just confirming it's intentional — and it is.

No dead code, no unused imports — every import change is consumed. ✅

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** The `csrfFetch` implementation is clean — single responsibility, well-named, and the JSDoc comment explains the testability constraint (the real *why*). The 15-file migration is purely mechanical: no creative interpretation, no reformatting, no bundled cleanup. Good discipline. ### Observations (non-blocking) **`useBlockAutoSave.svelte.ts` asymmetry** — the `flushOnUnload` path uses module-imported `csrfFetch` directly while the main debounced save goes through the injectable `saveFn`. The asymmetry is correct — keepalive fetch can't go through the API client pipeline, and `saveFn` wouldn't accept `keepalive: true` — but it will confuse future readers. A one-line comment on `flushOnUnload` like `// keepalive flush bypasses saveFn; csrfFetch adds CSRF header for this raw PUT` would save the next person 10 minutes. **`bulk-edit/+page.svelte` — `batch-metadata` POST** — this is semantically a read (body carries a list of IDs, POST used for URL-length reasons). Adding CSRF is harmless and strictly correct. No action needed, just confirming it's intentional — and it is. **No dead code, no unused imports** — every import change is consumed. ✅

marcel commented

2026-05-30 11:47:09 +02:00

🏗️ Markus Keller (@mkeller) — Application Architect

Verdict: ✅ Approved

Architecturally sound. csrfFetch belongs in $lib/shared/cookies.ts — it's cross-cutting infrastructure, and centralizing it creates one place to audit CSRF behavior across the entire frontend.

API surface assessment

The three-level export hierarchy covers the complete injection spectrum:

Export	Use case
`withCsrf(init?)`	Kept for injectable-fetch use cases (SvelteKit load functions where `fetch` is provided by the runtime)
`makeCsrfFetch(inner)`	Kept for hook composition and tests that inject a mock fetch (e.g. `createBlockAutoSave`)
`csrfFetch(input, init?)`	New — drop-in for call sites that own their fetch call

No over-abstraction. No under-abstraction. The old API is preserved; the new API is additive.

Documentation update audit

Per the required table:

No Flyway migrations → no DB diagrams to update ✅
No new backend packages or routes → no CLAUDE.md route/package updates needed ✅
seq-document-upload.puml: DocumentEditLayout and PdfViewer are now CSRF-protected, but CSRF token injection is an HTTP header detail, not an architectural flow step. The diagram correctly operates at the transport level without header detail — no update required ✅

One structural note (non-blocking)

The bulk-edit/+page.svelte onMount now uses csrfFetch for batch-metadata. This is a POST with read semantics (IDs in body). If this endpoint is ever promoted to a typed API client call in a future refactor, csrfFetch will be replaced naturally. The current approach is correct for a raw fetch call site.

## 🏗️ Markus Keller (@mkeller) — Application Architect **Verdict: ✅ Approved** Architecturally sound. `csrfFetch` belongs in `$lib/shared/cookies.ts` — it's cross-cutting infrastructure, and centralizing it creates one place to audit CSRF behavior across the entire frontend. ### API surface assessment The three-level export hierarchy covers the complete injection spectrum: | Export | Use case | |---|---| | `withCsrf(init?)` | Kept for injectable-fetch use cases (SvelteKit load functions where `fetch` is provided by the runtime) | | `makeCsrfFetch(inner)` | Kept for hook composition and tests that inject a mock fetch (e.g. `createBlockAutoSave`) | | `csrfFetch(input, init?)` | New — drop-in for call sites that own their fetch call | No over-abstraction. No under-abstraction. The old API is preserved; the new API is additive. ### Documentation update audit Per the required table: - No Flyway migrations → no DB diagrams to update ✅ - No new backend packages or routes → no CLAUDE.md route/package updates needed ✅ - `seq-document-upload.puml`: `DocumentEditLayout` and `PdfViewer` are now CSRF-protected, but CSRF token injection is an HTTP header detail, not an architectural flow step. The diagram correctly operates at the transport level without header detail — no update required ✅ ### One structural note (non-blocking) The `bulk-edit/+page.svelte` `onMount` now uses `csrfFetch` for `batch-metadata`. This is a POST with read semantics (IDs in body). If this endpoint is ever promoted to a typed API client call in a future refactor, `csrfFetch` will be replaced naturally. The current approach is correct for a raw fetch call site.

marcel commented

2026-05-30 11:47:15 +02:00

🔧 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

Verdict: ✅ Approved

No infrastructure changes. No Compose modifications, no CI workflow changes, no new services, no pinned versions touched. Nothing for me to flag.

Positive CI impact: The 21 failing browser tests on this branch were blocking CI. The root cause fix (making csrfFetch a function so vi.stubGlobal stubs are picked up at call time) will turn those green on next run.

Operational note: In production, the backend's CookieCsrfTokenRepository already sets the XSRF-TOKEN cookie on authenticated responses. The 8 previously unprotected call sites (annotation resize, comment CRUD, Geschichte CRUD, Stammbaum relationship creation, file upload) were silently receiving CSRF_TOKEN_MISSING rejections. Now they'll correctly send X-XSRF-TOKEN from the cookie without any config change. No deployment coordination needed — this is purely a client-side fix.

## 🔧 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer **Verdict: ✅ Approved** No infrastructure changes. No Compose modifications, no CI workflow changes, no new services, no pinned versions touched. Nothing for me to flag. **Positive CI impact:** The 21 failing browser tests on this branch were blocking CI. The root cause fix (making `csrfFetch` a function so `vi.stubGlobal` stubs are picked up at call time) will turn those green on next run. **Operational note:** In production, the backend's `CookieCsrfTokenRepository` already sets the `XSRF-TOKEN` cookie on authenticated responses. The 8 previously unprotected call sites (annotation resize, comment CRUD, Geschichte CRUD, Stammbaum relationship creation, file upload) were silently receiving `CSRF_TOKEN_MISSING` rejections. Now they'll correctly send `X-XSRF-TOKEN` from the cookie without any config change. No deployment coordination needed — this is purely a client-side fix.

marcel commented

2026-05-30 11:47:24 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

The PR is well-scoped: it closes the gap between the stated requirement (all client-side mutating requests carry CSRF tokens) and the implementation (8 call sites were missing the header). The root cause is clearly documented, and the fix is traceable to a concrete user-visible symptom ("Sitzungsfehler. Bitte laden Sie die Seite neu." on annotation resize).

Test plan coverage

The 6-item manual test plan covers all 8 newly protected call sites:

Annotation resize (PdfViewer) ✅
Comment POST/PATCH/DELETE (CommentThread) ✅
Geschichte create/edit/delete ✅
Stammbaum relationship creation (StammbaumSidePanel) ✅
File upload (DocumentEditLayout) ✅
Admin trigger/backfill (admin/system page) ✅

One open question (non-blocking)

batch-metadata POST in bulk-edit/+page.svelte — this endpoint is called in onMount and is semantically a read (it fetches metadata for a list of document IDs; POST is used because the IDs list can exceed URL length limits). The CSRF token will now be sent on this call. Since Spring Security's CSRF filter applies to all state-changing HTTP methods by default, and POST is in that list, this is correct behavior. If the endpoint is classified as idempotent/read-only on the backend and exempt from CSRF filtering, it would still work — the extra header is ignored. Confirming this is a non-issue.

The requirements for issue #524 (CSRF + rate limiting) are now fully satisfied on the frontend side.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** The PR is well-scoped: it closes the gap between the stated requirement (all client-side mutating requests carry CSRF tokens) and the implementation (8 call sites were missing the header). The root cause is clearly documented, and the fix is traceable to a concrete user-visible symptom ("Sitzungsfehler. Bitte laden Sie die Seite neu." on annotation resize). ### Test plan coverage The 6-item manual test plan covers all 8 newly protected call sites: - Annotation resize (PdfViewer) ✅ - Comment POST/PATCH/DELETE (CommentThread) ✅ - Geschichte create/edit/delete ✅ - Stammbaum relationship creation (StammbaumSidePanel) ✅ - File upload (DocumentEditLayout) ✅ - Admin trigger/backfill (admin/system page) ✅ ### One open question (non-blocking) **`batch-metadata` POST in `bulk-edit/+page.svelte`** — this endpoint is called in `onMount` and is semantically a read (it fetches metadata for a list of document IDs; POST is used because the IDs list can exceed URL length limits). The CSRF token will now be sent on this call. Since Spring Security's CSRF filter applies to all state-changing HTTP methods by default, and POST is in that list, this is correct behavior. If the endpoint is classified as idempotent/read-only on the backend and exempt from CSRF filtering, it would still work — the extra header is ignored. Confirming this is a non-issue. The requirements for issue #524 (CSRF + rate limiting) are now fully satisfied on the frontend side.

marcel commented

2026-05-30 11:47:37 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ⚠️ Approved with concerns

The coverage is comprehensive and the implementation is correct. My one blocker is about test coverage of security-critical code.

Coverage audit ✅

All 14 client-side mutating fetch calls are now protected. Method filtering is correct: POST/PUT/PATCH/DELETE get the header; GET/HEAD pass through unchanged. The withCsrf() no-op when the cookie is absent prevents errors in SSR and test contexts.

SvelteKit server-side form actions are correctly left untouched — those use _csrf form fields, not the X-XSRF-TOKEN cookie mechanism. The separation is intentional and correct.

Blockers

1. csrfFetch has no unit test (security-critical code, untested)

cookies.ts is entirely untested. If someone changes the method list — drops DELETE by accident, adds a typo, or reintroduces a module-level const — no test catches the regression. The fix is a small cookies.test.ts in a Node environment:

// cookies.test.ts
describe('csrfFetch', () => {
  beforeEach(() => {
    Object.defineProperty(document, 'cookie', {
      value: 'XSRF-TOKEN=test-token-123', configurable: true
    });
  });

  it('injects X-XSRF-TOKEN on POST', async () => {
    const mockFetch = vi.fn().mockResolvedValue(new Response());
    vi.stubGlobal('fetch', mockFetch);
    await csrfFetch('/api/test', { method: 'POST' });
    const headers = new Headers(mockFetch.mock.calls[0][1].headers);
    expect(headers.get('X-XSRF-TOKEN')).toBe('test-token-123');
  });

  it('does NOT inject X-XSRF-TOKEN on GET', async () => { ... });
  it('is a no-op when XSRF-TOKEN cookie is absent', async () => { ... });
  it.each(['PUT', 'PATCH', 'DELETE'])('injects on %s', async (method) => { ... });
});

Without this, csrfFetch is security-critical untested code. The fix this PR makes (correcting 8 unprotected call sites) is the same class of regression it's trying to prevent — and it would be just as invisible without automated coverage.

Suggestions (non-blocking)

2. No header-level assertion in component tests — the existing component tests verify that fetch is called the right number of times but none check mockFetch.mock.calls[0][1].headers for X-XSRF-TOKEN. For the highest-risk call sites (annotation PATCH, comment DELETE), one assertion per call site would permanently document and enforce the security invariant. Not a blocker given cookies.test.ts above, but defense in depth is worth it.

3. withCsrf and getCsrfToken are also untested — these are the primitives csrfFetch builds on. They belong in the same cookies.test.ts.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ⚠️ Approved with concerns** The coverage is comprehensive and the implementation is correct. My one blocker is about test coverage of security-critical code. ### Coverage audit ✅ All 14 client-side mutating fetch calls are now protected. Method filtering is correct: POST/PUT/PATCH/DELETE get the header; GET/HEAD pass through unchanged. The `withCsrf()` no-op when the cookie is absent prevents errors in SSR and test contexts. SvelteKit server-side form actions are correctly left untouched — those use `_csrf` form fields, not the `X-XSRF-TOKEN` cookie mechanism. The separation is intentional and correct. ### Blockers **1. `csrfFetch` has no unit test (security-critical code, untested)** `cookies.ts` is entirely untested. If someone changes the method list — drops `DELETE` by accident, adds a typo, or reintroduces a module-level const — no test catches the regression. The fix is a small `cookies.test.ts` in a Node environment: ```typescript // cookies.test.ts describe('csrfFetch', () => { beforeEach(() => { Object.defineProperty(document, 'cookie', { value: 'XSRF-TOKEN=test-token-123', configurable: true }); }); it('injects X-XSRF-TOKEN on POST', async () => { const mockFetch = vi.fn().mockResolvedValue(new Response()); vi.stubGlobal('fetch', mockFetch); await csrfFetch('/api/test', { method: 'POST' }); const headers = new Headers(mockFetch.mock.calls[0][1].headers); expect(headers.get('X-XSRF-TOKEN')).toBe('test-token-123'); }); it('does NOT inject X-XSRF-TOKEN on GET', async () => { ... }); it('is a no-op when XSRF-TOKEN cookie is absent', async () => { ... }); it.each(['PUT', 'PATCH', 'DELETE'])('injects on %s', async (method) => { ... }); }); ``` Without this, `csrfFetch` is security-critical untested code. The fix this PR makes (correcting 8 unprotected call sites) is the same class of regression it's trying to prevent — and it would be just as invisible without automated coverage. ### Suggestions (non-blocking) **2. No header-level assertion in component tests** — the existing component tests verify that `fetch` is called the right number of times but none check `mockFetch.mock.calls[0][1].headers` for `X-XSRF-TOKEN`. For the highest-risk call sites (annotation PATCH, comment DELETE), one assertion per call site would permanently document and enforce the security invariant. Not a blocker given `cookies.test.ts` above, but defense in depth is worth it. **3. `withCsrf` and `getCsrfToken` are also untested** — these are the primitives `csrfFetch` builds on. They belong in the same `cookies.test.ts`.

marcel commented

2026-05-30 11:47:48 +02:00

🧪 Sara Holt (@saraholt) — QA Engineer

Verdict: ⚠️ Approved with concerns

Root cause fix: correct ✅

The fix to csrfFetch (function instead of module-level const) correctly addresses the 21 failing browser tests. The vi.stubGlobal('fetch', mockFetch) pattern in the existing tests is right: stubs are set before the component is rendered, csrfFetch evaluates fetch from globalThis at call time, and the stub is found. The afterEach(() => { cleanup(); vi.unstubAllGlobals(); vi.clearAllMocks(); }) pattern correctly isolates stubs between tests — no isolation issues to fix.

Concerns

1. cookies.ts has zero test coverage

csrfFetch, withCsrf, getCsrfToken, and makeCsrfFetch are all security-critical utilities with no tests. A cookies.test.ts (Node environment, no browser needed) should cover:

Test	Expected outcome
POST with `XSRF-TOKEN` cookie set	`X-XSRF-TOKEN` header present with correct value
GET with `XSRF-TOKEN` cookie set	No `X-XSRF-TOKEN` header
POST without `XSRF-TOKEN` cookie	No `X-XSRF-TOKEN` header, call succeeds
DELETE, PATCH, PUT	`X-XSRF-TOKEN` header present

This is Vitest unit territory — no browser, no component, fast. The layer is missing entirely.

2. Fixed call sites don't assert the CSRF header

The component tests that were failing (BulkDocumentEditLayout, admin/system, OcrTrainingCard, useBlockAutoSave) verify that fetch is called the right number of times. None of them assert that X-XSRF-TOKEN is in the request headers. This means the tests prove a fetch happened but not that it was correctly protected. Given point 1 above, this isn't an immediate blocker — but it means a future refactor could silently drop the CSRF header without any test catching it.

3. What's good

The 21 failing tests will pass after this fix without any test modifications needed ✅
The browser test infrastructure (vitest-browser-svelte, vi.stubGlobal, vi.waitFor) is being used correctly ✅
Test isolation with vi.unstubAllGlobals() in afterEach is correct ✅

## 🧪 Sara Holt (@saraholt) — QA Engineer **Verdict: ⚠️ Approved with concerns** ### Root cause fix: correct ✅ The fix to `csrfFetch` (function instead of module-level const) correctly addresses the 21 failing browser tests. The `vi.stubGlobal('fetch', mockFetch)` pattern in the existing tests is right: stubs are set before the component is rendered, `csrfFetch` evaluates `fetch` from `globalThis` at call time, and the stub is found. The `afterEach(() => { cleanup(); vi.unstubAllGlobals(); vi.clearAllMocks(); })` pattern correctly isolates stubs between tests — no isolation issues to fix. ### Concerns **1. `cookies.ts` has zero test coverage** `csrfFetch`, `withCsrf`, `getCsrfToken`, and `makeCsrfFetch` are all security-critical utilities with no tests. A `cookies.test.ts` (Node environment, no browser needed) should cover: | Test | Expected outcome | |---|---| | POST with `XSRF-TOKEN` cookie set | `X-XSRF-TOKEN` header present with correct value | | GET with `XSRF-TOKEN` cookie set | No `X-XSRF-TOKEN` header | | POST without `XSRF-TOKEN` cookie | No `X-XSRF-TOKEN` header, call succeeds | | DELETE, PATCH, PUT | `X-XSRF-TOKEN` header present | This is Vitest unit territory — no browser, no component, fast. The layer is missing entirely. **2. Fixed call sites don't assert the CSRF header** The component tests that were failing (BulkDocumentEditLayout, admin/system, OcrTrainingCard, useBlockAutoSave) verify that `fetch` is called the right number of times. None of them assert that `X-XSRF-TOKEN` is in the request headers. This means the tests prove *a* fetch happened but not that it was *correctly protected*. Given point 1 above, this isn't an immediate blocker — but it means a future refactor could silently drop the CSRF header without any test catching it. **3. What's good** - The 21 failing tests will pass after this fix without any test modifications needed ✅ - The browser test infrastructure (`vitest-browser-svelte`, `vi.stubGlobal`, `vi.waitFor`) is being used correctly ✅ - Test isolation with `vi.unstubAllGlobals()` in `afterEach` is correct ✅

marcel commented

2026-05-30 11:47:54 +02:00

🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead

Verdict: ✅ Approved

This is a pure security infrastructure fix — no template changes, no style changes, no layout changes. Nothing visual to review.

From a UX perspective, it's a quiet win: the 8 previously unprotected call sites were producing CSRF_TOKEN_MISSING errors that surfaced as "Sitzungsfehler. Bitte laden Sie die Seite neu." — an opaque, alarming message for any user, and especially confusing for the senior audience (60+) who may interpret a "session error" as having lost their work or needing to call IT.

After this fix:

Annotation resize in PdfViewer → no more session error after dragging a box
Comment post/edit/delete → actions complete silently as expected
Geschichte create/edit/delete → no error on save
Stammbaum relationship creation → no error on submit
File upload on the document edit form → upload succeeds

No design changes needed. These were silent failures masking as UI bugs.

## 🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead **Verdict: ✅ Approved** This is a pure security infrastructure fix — no template changes, no style changes, no layout changes. Nothing visual to review. From a UX perspective, it's a quiet win: the 8 previously unprotected call sites were producing `CSRF_TOKEN_MISSING` errors that surfaced as "Sitzungsfehler. Bitte laden Sie die Seite neu." — an opaque, alarming message for any user, and especially confusing for the senior audience (60+) who may interpret a "session error" as having lost their work or needing to call IT. After this fix: - Annotation resize in PdfViewer → no more session error after dragging a box - Comment post/edit/delete → actions complete silently as expected - Geschichte create/edit/delete → no error on save - Stammbaum relationship creation → no error on submit - File upload on the document edit form → upload succeeds No design changes needed. These were silent failures masking as UI bugs.

marcel added 1 commit 2026-05-30 11:56:19 +02:00

test(security): add unit tests for cookies.ts CSRF utilities

CI / Unit & Component Tests (pull_request) Successful in 3m40s

Details

CI / OCR Service Tests (pull_request) Successful in 22s

Details

CI / Backend Unit Tests (pull_request) Successful in 3m45s

Details

CI / fail2ban Regex (pull_request) Successful in 44s

Details

CI / Semgrep Security Scan (pull_request) Successful in 19s

Details

CI / Compose Bucket Idempotency (pull_request) Successful in 1m5s

Details

CI / Unit & Component Tests (push) Successful in 3m24s

Details

CI / OCR Service Tests (push) Successful in 22s

Details

CI / Backend Unit Tests (push) Successful in 3m35s

Details

CI / fail2ban Regex (push) Successful in 43s

Details

CI / Semgrep Security Scan (push) Successful in 20s

Details

CI / Compose Bucket Idempotency (push) Successful in 1m4s

Details

nightly / deploy-staging (push) Successful in 2m10s

Details

397fc3c7e4

Covers getCsrfToken (cookie parsing, URL-decoding, server-side null),
withCsrf (header injection, immutability, no-op when absent),
makeCsrfFetch (method filtering, case-insensitivity, inner-vs-global),
and csrfFetch (regression guard: vi.stubGlobal is honoured at call time,
not bypassed by a module-level captured reference).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel merged commit 397fc3c7e4 into main

2026-05-30 14:39:14 +02:00

marcel deleted branch fix/csrf-missing-client-fetches

2026-05-30 14:39:14 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#695