refactor(frontend): replace raw fetch with event.fetch in admin/enrich routes (handleFetch bypass) #465

New Issue

marcel · 2026-05-07T17:25:18+02:00

marcel commented

2026-05-07 17:25:18 +02:00

Context

The audit found four +page.server.ts files that use the global fetch instead of destructuring fetch from the load/action event. This bypasses the handleFetch hook in frontend/src/hooks.server.ts:63-106, which is responsible for injecting the Authorization: <auth_token> header on outbound API calls.

frontend/src/routes/enrich/[id]/+page.server.ts:75   raw fetch
frontend/src/routes/enrich/[id]/+page.server.ts:101  raw fetch
frontend/src/routes/admin/invites/+page.server.ts:23 raw fetch
frontend/src/routes/admin/invites/+page.server.ts:50 raw fetch
frontend/src/routes/admin/invites/+page.server.ts:77 raw fetch
frontend/src/routes/admin/users/[id]/+page.server.ts:48 raw fetch

Two failure modes:

Silent un-authentication: the call goes out without the auth_token cookie's bearer credential, the backend returns 401, and the user sees a confusing error.
Privilege confusion: if the call hardcodes env.API_INTERNAL_URL and works (because the backend is on the same network), then it's effectively running as a server-level identity, not the user's identity. That's a privilege-escalation surface — depending on the endpoint, it could leak admin-only data to a non-admin user.

The login action's raw fetch is intentional and stays (it constructs the Basic header from the form, by design). All others should use event.fetch.

Approach

Each affected file already destructures cookies and request from the action event. Add fetch:

// before
export const actions = {
    default: async ({ request, cookies }) => {
        const baseUrl = env.API_INTERNAL_URL || 'http://localhost:8080';
        const response = await fetch(`${baseUrl}/api/...`, { headers: { Authorization: cookies.get('auth_token')! } });
    }
};

// after
export const actions = {
    default: async ({ request, fetch }) => {
        const api = createApiClient(fetch);
        const result = await api.GET('/api/...', { ... });
        if (!result.response.ok) return fail(result.response.status, { error: getErrorMessage(...) });
    }
};

handleFetch injects the auth header automatically; createApiClient(fetch) gives a typed wrapper from $lib/shared/api.server. This also makes the call testable in vitest-browser.

Critical files

frontend/src/routes/enrich/[id]/+page.server.ts (lines 75, 101)
frontend/src/routes/admin/invites/+page.server.ts (lines 23, 50, 77)
frontend/src/routes/admin/users/[id]/+page.server.ts (line 48)

The login flow at frontend/src/routes/login/+page.server.ts is explicitly out of scope — its raw fetch is intentional and documented.

Verification

grep -rE "fetch\(\?\${.*API|fetch\(env\." frontend/src/routes` — should show only the login action.
Manual smoke: log in as admin, exercise enrich workflow, confirm calls succeed (no 401s in DevTools network tab).
Manual smoke: invite flow, user-edit flow — same.
New test: vitest-browser test for the enrich action's happy path mocking the typed API client (proves the call shape is now consistent with the rest of the codebase).

Acceptance criteria

No fetch(\...${env.API_*}` ...)patterns in+page.server.tsfiles exceptroutes/login/+page.server.ts`.
All affected actions use event.fetch and createApiClient.
Existing enrich/admin happy paths still work (manual smoke + existing E2E if any).
No TypeScript regressions (npm run check).

Effort

S — 1 day, mostly retesting the affected user flows.

Risk if not addressed

Privilege confusion on admin endpoints; silent 401s for users on enrich endpoints.

Tracked in audit doc as F-15 (High).

## Context The audit found four `+page.server.ts` files that use the global `fetch` instead of destructuring `fetch` from the load/action event. This bypasses the `handleFetch` hook in `frontend/src/hooks.server.ts:63-106`, which is responsible for injecting the `Authorization: <auth_token>` header on outbound API calls. ``` frontend/src/routes/enrich/[id]/+page.server.ts:75 raw fetch frontend/src/routes/enrich/[id]/+page.server.ts:101 raw fetch frontend/src/routes/admin/invites/+page.server.ts:23 raw fetch frontend/src/routes/admin/invites/+page.server.ts:50 raw fetch frontend/src/routes/admin/invites/+page.server.ts:77 raw fetch frontend/src/routes/admin/users/[id]/+page.server.ts:48 raw fetch ``` Two failure modes: 1. **Silent un-authentication**: the call goes out without the `auth_token` cookie's bearer credential, the backend returns 401, and the user sees a confusing error. 2. **Privilege confusion**: if the call hardcodes `env.API_INTERNAL_URL` and works (because the backend is on the same network), then it's effectively running as a server-level identity, not the user's identity. That's a privilege-escalation surface — depending on the endpoint, it could leak admin-only data to a non-admin user. The login action's raw `fetch` is **intentional** and stays (it constructs the Basic header from the form, by design). All others should use `event.fetch`. ## Approach Each affected file already destructures `cookies` and `request` from the action event. Add `fetch`: ```typescript // before export const actions = { default: async ({ request, cookies }) => { const baseUrl = env.API_INTERNAL_URL || 'http://localhost:8080'; const response = await fetch(`${baseUrl}/api/...`, { headers: { Authorization: cookies.get('auth_token')! } }); } }; // after export const actions = { default: async ({ request, fetch }) => { const api = createApiClient(fetch); const result = await api.GET('/api/...', { ... }); if (!result.response.ok) return fail(result.response.status, { error: getErrorMessage(...) }); } }; ``` `handleFetch` injects the auth header automatically; `createApiClient(fetch)` gives a typed wrapper from `$lib/shared/api.server`. This also makes the call testable in vitest-browser. ## Critical files - `frontend/src/routes/enrich/[id]/+page.server.ts` (lines 75, 101) - `frontend/src/routes/admin/invites/+page.server.ts` (lines 23, 50, 77) - `frontend/src/routes/admin/users/[id]/+page.server.ts` (line 48) The login flow at `frontend/src/routes/login/+page.server.ts` is **explicitly out of scope** — its raw `fetch` is intentional and documented. ## Verification 1. `grep -rE "fetch\(\`?\\\${.*API|fetch\\(env\\." frontend/src/routes` — should show only the login action. 2. Manual smoke: log in as admin, exercise enrich workflow, confirm calls succeed (no 401s in DevTools network tab). 3. Manual smoke: invite flow, user-edit flow — same. 4. New test: vitest-browser test for the enrich action's happy path mocking the typed API client (proves the call shape is now consistent with the rest of the codebase). ## Acceptance criteria - [ ] No `fetch(\`...${env.API_*}\` ...)` patterns in `+page.server.ts` files except `routes/login/+page.server.ts`. - [ ] All affected actions use `event.fetch` and `createApiClient`. - [ ] Existing enrich/admin happy paths still work (manual smoke + existing E2E if any). - [ ] No TypeScript regressions (`npm run check`). ## Effort S — 1 day, mostly retesting the affected user flows. ## Risk if not addressed Privilege confusion on admin endpoints; silent 401s for users on enrich endpoints. Tracked in audit doc as **F-15** (High).

marcel added the P1-high refactor labels 2026-05-07 17:27:00 +02:00

marcel commented

2026-05-07 17:36:56 +02:00

🏛️ Markus Keller — Application Architect

Observations

The issue is well-scoped and the root cause is correctly diagnosed. The handleFetch hook in hooks.server.ts is the canonical auth injection point for SvelteKit server-side API calls — it exists precisely so that individual +page.server.ts files do not have to manage auth headers manually. The three affected files bypass this layer by calling global fetch directly.

After reading the code, the files fall into two distinct cases:

Case A — full bypass with env.API_INTERNAL_URL construction (admin/invites/+page.server.ts, and partially enrich/[id]/+page.server.ts): These build the URL from the env var and call global fetch, so handleFetch never fires. The auth header is absent entirely — the backend sees an unauthenticated request.

Case B — relative-path call (admin/users/[id]/+page.server.ts line 48): fetch('/api/users/${id}', ...) uses a relative URL. In SvelteKit's server context, this does route through handleFetch, but the hook's isApi check (request.url.includes('/api/')) depends on the fully-resolved URL. Verify whether relative-URL calls in server actions actually trigger handleFetch — if not, this is the same class A bug with different surface.

Multipart note: The enrich save actions use raw fetch with formData as body. This is the documented exception pattern for multipart uploads (the typed client does not support multipart natively), but the fix is to still use event.fetch — just not createApiClient. The distinction between "use event.fetch with a raw Request" and "use createApiClient(fetch)" matters here.

The issue's proposed fix is architecturally correct: destructure fetch from the event, pass it to createApiClient where the endpoint is typed, or pass it directly for multipart calls. Either way, the auth injection happens in one place.

Recommendations

Adopt the fix as specified — no architectural concern with the approach. The createApiClient(fetch) pattern already used in the load functions and delete action of the same files is the right model.
For the enrich multipart actions, use event.fetch directly (not createApiClient) since the typed client does not support multipart bodies. The invocation becomes fetch(\${baseUrl}/api/documents/${params.id}`, { method: 'PUT', body: formData })wherefetch` is destructured from the action event — not the global.
Verify the admin/users/[id] relative-path case before closing: test whether handleFetch fires on relative-URL calls in action context; if it does, the bug is less severe than stated but the refactor is still worthwhile for consistency.
The userGroup hook in hooks.server.ts line 48 also calls global fetch for the /api/users/me bootstrap call. That call correctly sets Authorization manually, so it is not broken — but it is worth noting as a pattern that should not be replicated.
No ADR is needed for this fix — it restores the existing intended pattern rather than introducing a new one.

## 🏛️ Markus Keller — Application Architect ### Observations The issue is well-scoped and the root cause is correctly diagnosed. The `handleFetch` hook in `hooks.server.ts` is the canonical auth injection point for SvelteKit server-side API calls — it exists precisely so that individual `+page.server.ts` files do not have to manage auth headers manually. The three affected files bypass this layer by calling global `fetch` directly. After reading the code, the files fall into two distinct cases: **Case A — full bypass with `env.API_INTERNAL_URL` construction** (`admin/invites/+page.server.ts`, and partially `enrich/[id]/+page.server.ts`): These build the URL from the env var and call global `fetch`, so `handleFetch` never fires. The auth header is absent entirely — the backend sees an unauthenticated request. **Case B — relative-path call** (`admin/users/[id]/+page.server.ts` line 48): `fetch('/api/users/${id}', ...)` uses a relative URL. In SvelteKit's server context, this does route through `handleFetch`, but the hook's `isApi` check (`request.url.includes('/api/')`) depends on the fully-resolved URL. Verify whether relative-URL calls in server actions actually trigger `handleFetch` — if not, this is the same class A bug with different surface. **Multipart note**: The `enrich` save actions use raw `fetch` with `formData` as body. This is the documented exception pattern for multipart uploads (the typed client does not support multipart natively), but the fix is to still use `event.fetch` — just not `createApiClient`. The distinction between "use `event.fetch` with a raw `Request`" and "use `createApiClient(fetch)`" matters here. The issue's proposed fix is architecturally correct: destructure `fetch` from the event, pass it to `createApiClient` where the endpoint is typed, or pass it directly for multipart calls. Either way, the auth injection happens in one place. ### Recommendations - **Adopt the fix as specified** — no architectural concern with the approach. The `createApiClient(fetch)` pattern already used in the `load` functions and `delete` action of the same files is the right model. - **For the enrich multipart actions**, use `event.fetch` directly (not `createApiClient`) since the typed client does not support multipart bodies. The invocation becomes `fetch(\`${baseUrl}/api/documents/${params.id}\`, { method: 'PUT', body: formData })` where `fetch` is destructured from the action event — not the global. - **Verify the `admin/users/[id]` relative-path case** before closing: test whether `handleFetch` fires on relative-URL calls in action context; if it does, the bug is less severe than stated but the refactor is still worthwhile for consistency. - **The `userGroup` hook in `hooks.server.ts` line 48** also calls global `fetch` for the `/api/users/me` bootstrap call. That call correctly sets `Authorization` manually, so it is not broken — but it is worth noting as a pattern that should not be replicated. - No ADR is needed for this fix — it restores the existing intended pattern rather than introducing a new one.

marcel commented

2026-05-07 17:37:11 +02:00

👨‍💻 Felix Brandt — Fullstack Developer

Observations

Having read all three affected files, the pattern inconsistency is clear. The same files are already partially migrated: in enrich/[id]/+page.server.ts, the load function and skip action correctly use createApiClient(fetch) from the event — but save and saveAndReview at lines 72–85 and 97–111 drop back to global fetch with a manually constructed URL. This is the most confusing case because the two styles coexist in a single file.

In admin/invites/+page.server.ts, all three callsites (load, create, revoke) use raw fetch with env.API_INTERNAL_URL. event.fetch is already destructured — it just isn't used for the actual API calls.

In admin/users/[id]/+page.server.ts, the update action at line 48 uses a relative-URL raw fetch, while the delete action immediately below uses createApiClient. Again, two styles in one file.

Specific clean code concern: the update action in admin/users/[id]/+page.server.ts does its own JSON error parsing inline (lines 55–62) rather than using the project's parseBackendError() / getErrorMessage() pattern. This should be fixed in the same PR since it is in the same function being touched.

Multipart caveat: the CONTRIBUTING.md note says "For multipart/form-data, bypass the typed client and use raw fetch" — this was likely written meaning event.fetch, not global fetch. The fix for the enrich save actions is event.fetch with the raw request, not createApiClient.

Recommendations

enrich/[id]/+page.server.ts save and saveAndReview: replace global fetch(${baseUrl}/api/...) with event.fetch(${baseUrl}/api/...) — keep raw fetch for the multipart body, just change the fetch reference. baseUrl comes from env.API_INTERNAL_URL (already imported).
admin/invites/+page.server.ts load, create, revoke: all three can switch to createApiClient(fetch) — the endpoints are JSON, no multipart needed. Remove the apiUrl variable and the manual URL construction entirely.
admin/users/[id]/+page.server.ts update: switch to createApiClient(fetch).PUT(...) and replace the inline error JSON parsing with parseBackendError + getErrorMessage to match the project pattern.
In the same PR: update the CONTRIBUTING.md multipart note to clarify "bypass createApiClient but always use event.fetch, never global fetch." One sentence is enough.
Do not touch login/+page.server.ts — its raw fetch is intentional and already has an explanatory comment.

The fix is mechanical and low-risk. All three files already import the correct utilities; this is wiring, not logic.

## 👨‍💻 Felix Brandt — Fullstack Developer ### Observations Having read all three affected files, the pattern inconsistency is clear. The same files are already partially migrated: in `enrich/[id]/+page.server.ts`, the `load` function and `skip` action correctly use `createApiClient(fetch)` from the event — but `save` and `saveAndReview` at lines 72–85 and 97–111 drop back to global `fetch` with a manually constructed URL. This is the most confusing case because the two styles coexist in a single file. In `admin/invites/+page.server.ts`, all three callsites (load, create, revoke) use raw fetch with `env.API_INTERNAL_URL`. `event.fetch` is already destructured — it just isn't used for the actual API calls. In `admin/users/[id]/+page.server.ts`, the `update` action at line 48 uses a relative-URL raw fetch, while the `delete` action immediately below uses `createApiClient`. Again, two styles in one file. **Specific clean code concern**: the `update` action in `admin/users/[id]/+page.server.ts` does its own JSON error parsing inline (lines 55–62) rather than using the project's `parseBackendError()` / `getErrorMessage()` pattern. This should be fixed in the same PR since it is in the same function being touched. **Multipart caveat**: the `CONTRIBUTING.md` note says "For multipart/form-data, bypass the typed client and use raw `fetch`" — this was likely written meaning `event.fetch`, not global `fetch`. The fix for the enrich save actions is `event.fetch` with the raw request, not `createApiClient`. ### Recommendations - **`enrich/[id]/+page.server.ts` `save` and `saveAndReview`**: replace global `fetch(${baseUrl}/api/...)` with `event.fetch(`${baseUrl}/api/...`)` — keep raw fetch for the multipart body, just change the `fetch` reference. `baseUrl` comes from `env.API_INTERNAL_URL` (already imported). - **`admin/invites/+page.server.ts` load, create, revoke**: all three can switch to `createApiClient(fetch)` — the endpoints are JSON, no multipart needed. Remove the `apiUrl` variable and the manual URL construction entirely. - **`admin/users/[id]/+page.server.ts` update**: switch to `createApiClient(fetch).PUT(...)` and replace the inline error JSON parsing with `parseBackendError` + `getErrorMessage` to match the project pattern. - **In the same PR**: update the `CONTRIBUTING.md` multipart note to clarify "bypass `createApiClient` but always use `event.fetch`, never global `fetch`." One sentence is enough. - **Do not touch `login/+page.server.ts`** — its raw fetch is intentional and already has an explanatory comment. The fix is mechanical and low-risk. All three files already import the correct utilities; this is wiring, not logic.

marcel commented

2026-05-07 17:37:32 +02:00

🔒 Nora "NullX" Steiner — Security Engineer

Observations

This is a real vulnerability, not a smell. I reviewed the three files and can confirm two distinct failure modes depending on which endpoint is called:

Failure Mode 1 — Missing authentication (enrich and invites)
enrich/[id]/+page.server.ts save/saveAndReview and admin/invites/+page.server.ts all send requests to ${env.API_INTERNAL_URL}/api/... using global fetch. The handleFetch hook never fires for global fetch calls — it only intercepts calls made through the SvelteKit-provided event.fetch. The backend receives a request with no Authorization header. Whether this results in a 401 or a pass depends entirely on how the backend's SecurityConfig is configured for those endpoints.

If the backend's @RequirePermission AOP check runs (which it should on PUT /api/documents/{id} and POST/DELETE /api/invites), it will 401 or 403. But the frontend action then catches that error and returns it as a user-facing form error, not a redirect to login — so the user sees a confusing error message rather than being told they're unauthenticated. This is the "silent un-authentication" mode described in the issue.

Failure Mode 2 — Potential privilege escalation (admin/users update)
admin/users/[id]/+page.server.ts line 48: fetch('/api/users/${params.id}', ...) uses a relative URL. In a SvelteKit server context, whether this triggers handleFetch depends on the SvelteKit version and whether the URL is resolved to an absolute form before handleFetch intercepts it. If handleFetch does NOT fire here, the request goes to the backend without the user's auth cookie — but since it's running server-side on the same network as the backend, it might succeed if the backend accepts unauthenticated PUT requests to /api/users/{id}. That would be a privilege escalation: any authenticated frontend user could modify any other user's record if the backend misconfigures @RequirePermission.

CWE: CWE-306 (Missing Authentication for Critical Function) — partial, because the backend has its own auth layer, but the defense-in-depth at the frontend layer is broken.

Important nuance: The backend @RequirePermission annotations are the true security boundary. This frontend bug reduces defense-in-depth and creates a confusing UX, but it does not bypass backend enforcement. The risk level stated in the issue (High / F-15) is appropriate — this is a defense-in-depth failure with a UX impact, and the user-edit endpoint could be an escalation surface if backend auth is not correctly configured on that specific endpoint.

Recommendations

Fix as specified in the issue — use event.fetch throughout. This is not optional; it is a security fix.
Verify the backend @RequirePermission on PUT /api/users/{id}: confirm the endpoint has @RequirePermission(Permission.ADMIN_USER) or equivalent. The frontend fix removes the bypass possibility; the backend annotation closes the door permanently.
Write a regression test (see Sara's comment for specifics) that proves the unauthenticated path to these actions returns 401, not a success response. This test stays in the suite forever.
Add a comment in the affected actions parallel to the login action's comment: // event.fetch is required here — global fetch bypasses handleFetch auth injection. This makes the correct pattern explicit for the next developer.
Do not rely on the handleFetch hook as the sole auth layer — it is defense-in-depth. Backend @RequirePermission remains the primary control. The issue is that removing a defense layer is always a regression, regardless of other layers.

## 🔒 Nora "NullX" Steiner — Security Engineer ### Observations This is a real vulnerability, not a smell. I reviewed the three files and can confirm two distinct failure modes depending on which endpoint is called: **Failure Mode 1 — Missing authentication (enrich and invites)** `enrich/[id]/+page.server.ts` `save`/`saveAndReview` and `admin/invites/+page.server.ts` all send requests to `${env.API_INTERNAL_URL}/api/...` using global `fetch`. The `handleFetch` hook never fires for global `fetch` calls — it only intercepts calls made through the SvelteKit-provided `event.fetch`. The backend receives a request with no `Authorization` header. Whether this results in a 401 or a pass depends entirely on how the backend's `SecurityConfig` is configured for those endpoints. If the backend's `@RequirePermission` AOP check runs (which it should on PUT `/api/documents/{id}` and POST/DELETE `/api/invites`), it will 401 or 403. But the frontend action then catches that error and returns it as a user-facing form error, not a redirect to login — so the user sees a confusing error message rather than being told they're unauthenticated. This is the "silent un-authentication" mode described in the issue. **Failure Mode 2 — Potential privilege escalation (admin/users update)** `admin/users/[id]/+page.server.ts` line 48: `fetch('/api/users/${params.id}', ...)` uses a relative URL. In a SvelteKit server context, whether this triggers `handleFetch` depends on the SvelteKit version and whether the URL is resolved to an absolute form before `handleFetch` intercepts it. If `handleFetch` does NOT fire here, the request goes to the backend without the user's auth cookie — but since it's running server-side on the same network as the backend, it might succeed if the backend accepts unauthenticated PUT requests to `/api/users/{id}`. That would be a privilege escalation: any authenticated frontend user could modify any other user's record if the backend misconfigures `@RequirePermission`. **CWE**: CWE-306 (Missing Authentication for Critical Function) — partial, because the backend has its own auth layer, but the defense-in-depth at the frontend layer is broken. **Important nuance**: The backend `@RequirePermission` annotations are the true security boundary. This frontend bug reduces defense-in-depth and creates a confusing UX, but it does not bypass backend enforcement. The risk level stated in the issue (High / F-15) is appropriate — this is a defense-in-depth failure with a UX impact, and the user-edit endpoint could be an escalation surface if backend auth is not correctly configured on that specific endpoint. ### Recommendations - **Fix as specified in the issue** — use `event.fetch` throughout. This is not optional; it is a security fix. - **Verify the backend `@RequirePermission` on PUT `/api/users/{id}`**: confirm the endpoint has `@RequirePermission(Permission.ADMIN_USER)` or equivalent. The frontend fix removes the bypass possibility; the backend annotation closes the door permanently. - **Write a regression test** (see Sara's comment for specifics) that proves the unauthenticated path to these actions returns 401, not a success response. This test stays in the suite forever. - **Add a comment in the affected actions** parallel to the login action's comment: `// event.fetch is required here — global fetch bypasses handleFetch auth injection`. This makes the correct pattern explicit for the next developer. - **Do not rely on the `handleFetch` hook as the sole auth layer** — it is defense-in-depth. Backend `@RequirePermission` remains the primary control. The issue is that removing a defense layer is always a regression, regardless of other layers.

marcel commented

2026-05-07 17:37:48 +02:00

🧪 Sara Holt — QA Engineer

Observations

The issue's verification section gives a grep command and three manual smoke tests, but no automated test specification. Given that this is a security-adjacent regression fix, manual smoke is insufficient — the next time someone edits these files, there is no automated check that prevents the same pattern from creeping back.

After reading the code, I can see the test approach needs to account for two layers:

SvelteKit action behavior — can the action be tested by importing the action function directly and mocking event.fetch? Yes, because +page.server.ts action functions are plain TypeScript that accept the event object.
Integration test — can we verify that the actual request reaching the backend carries the auth header?

The issue also mentions "New test: vitest-browser test for the enrich action's happy path mocking the typed API client." A vitest-browser test is for component rendering — it is the wrong layer for server action testing. Server actions are tested with Vitest in Node environment by importing and calling the action function directly.

The verification grep (grep -rE "fetch\(\?${.*API|fetch(env."`) is a good sanity check but will not catch future regressions automatically. This should be a CI lint rule or a test assertion.

Recommendations

Write a Vitest Node test for each affected action that calls the action with a mock fetch and verifies the mock was called (proving event.fetch was used, not global fetch). Example pattern:

// enrich/[id]/+page.server.test.ts
it('save action uses event.fetch, not global fetch', async () => {
  const mockFetch = vi.fn().mockResolvedValue(new Response('{}', { status: 200 }));
  await actions.save({ params: { id: 'test-id' }, request: makeFormDataRequest(), fetch: mockFetch });
  expect(mockFetch).toHaveBeenCalled();
  expect(mockFetch.mock.calls[0][0]).toContain('/api/documents/test-id');
});

Add a Playwright E2E test for the enrich happy path: log in, go to /enrich/[id], submit the form, verify no 401 appears in the network tab or in the page response. This catches the auth header being missing at the integration level.
Convert the grep command to a CI step (or Vitest test using glob + readFileSync) that scans frontend/src/routes for the raw fetch(${env.API pattern excluding login — this is the regression sentinel the grep currently provides manually.
Do not use vitest-browser for this particular test — server actions run in Node environment, not the browser. Use the standard npm run test (Node/Vitest) suite.
Test both the happy path and the 401 path for each migrated action: confirm that when event.fetch returns a 401 response, the action returns the appropriate fail() rather than throwing or swallowing the error.

## 🧪 Sara Holt — QA Engineer ### Observations The issue's verification section gives a grep command and three manual smoke tests, but no automated test specification. Given that this is a security-adjacent regression fix, manual smoke is insufficient — the next time someone edits these files, there is no automated check that prevents the same pattern from creeping back. After reading the code, I can see the test approach needs to account for two layers: 1. **SvelteKit action behavior** — can the action be tested by importing the action function directly and mocking `event.fetch`? Yes, because `+page.server.ts` action functions are plain TypeScript that accept the event object. 2. **Integration test** — can we verify that the actual request reaching the backend carries the auth header? The issue also mentions "New test: vitest-browser test for the enrich action's happy path mocking the typed API client." A vitest-browser test is for component rendering — it is the wrong layer for server action testing. Server actions are tested with Vitest in Node environment by importing and calling the action function directly. The verification grep (`grep -rE "fetch\(\`?\${.*API|fetch\(env\."`) is a good sanity check but will not catch future regressions automatically. This should be a CI lint rule or a test assertion. ### Recommendations - **Write a Vitest Node test for each affected action** that calls the action with a mock `fetch` and verifies the mock was called (proving `event.fetch` was used, not global `fetch`). Example pattern: ```typescript // enrich/[id]/+page.server.test.ts it('save action uses event.fetch, not global fetch', async () => { const mockFetch = vi.fn().mockResolvedValue(new Response('{}', { status: 200 })); await actions.save({ params: { id: 'test-id' }, request: makeFormDataRequest(), fetch: mockFetch }); expect(mockFetch).toHaveBeenCalled(); expect(mockFetch.mock.calls[0][0]).toContain('/api/documents/test-id'); }); ``` - **Add a Playwright E2E test** for the enrich happy path: log in, go to `/enrich/[id]`, submit the form, verify no 401 appears in the network tab or in the page response. This catches the auth header being missing at the integration level. - **Convert the grep command to a CI step** (or Vitest test using `glob` + `readFileSync`) that scans `frontend/src/routes` for the raw `fetch(${env.API` pattern excluding `login` — this is the regression sentinel the grep currently provides manually. - **Do not use vitest-browser** for this particular test — server actions run in Node environment, not the browser. Use the standard `npm run test` (Node/Vitest) suite. - **Test both the happy path and the 401 path** for each migrated action: confirm that when `event.fetch` returns a 401 response, the action returns the appropriate `fail()` rather than throwing or swallowing the error.

marcel commented

2026-05-07 17:38:07 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue is well-written by this codebase's standards: it names the exact affected lines, describes two concrete failure modes, gives a before/after code sample, and defines acceptance criteria. It passes the Definition of Ready check.

One gap: the acceptance criteria are written from the code perspective, not from the user's perspective. This matters for the smoke-test step and for knowing when this is truly done.

Specific gaps I found after reading the code:

The issue says "six raw fetches" in the header listing but the actual count across the three files is: enrich (2) + invites (3) + admin/users (1) = 6. Confirmed correct.
The acceptance criterion "No fetch(\...${env.API_}` ...)patterns" would NOT catch theadmin/users/[id]/+page.server.tsline 48 pattern, which uses a relative URLfetch('/api/users/${params.id}', ...)— not anenv.API_` interpolation. The grep in the verification section would also miss this case.
"Existing enrich/admin happy paths still work (manual smoke + existing E2E if any)" — there are no existing E2E tests for these specific flows based on the directory structure. The criterion should explicitly acknowledge this and require a new Playwright smoke test rather than relying on "existing E2E if any."
The issue states effort as "S — 1 day, mostly retesting." Given that this is a security-adjacent fix with no existing automated test coverage for the affected flows, the retesting burden is real. The estimate is reasonable if the PR author plans to add the regression tests during the same session.

Recommendations

Add a user-facing acceptance criterion: "A non-admin user cannot trigger the admin/invites or admin/users endpoints by submitting the form — the backend returns 403 and the user sees a meaningful error message." This tests the actual failure mode, not just the code pattern.
Tighten the grep pattern in the AC: the current pattern misses relative-URL raw fetch calls. Use a two-part check: (1) the env.API interpolation grep, and (2) separately verify that admin/users/[id]/+page.server.ts update action no longer contains a raw fetch( call.
Update AC item 3 from "existing E2E if any" to "a new Playwright smoke test covering the enrich submit and admin invite create flows" — set the expectation explicitly.
The CONTRIBUTING.md multipart note ambiguity (does "bypass typed client" mean "bypass event.fetch" too?) should be resolved in this PR to prevent the same misreading from recurring. Add an AC item: "CONTRIBUTING.md multipart fetch note clarified to specify event.fetch must still be used."

## 📋 Elicit — Requirements Engineer ### Observations The issue is well-written by this codebase's standards: it names the exact affected lines, describes two concrete failure modes, gives a before/after code sample, and defines acceptance criteria. It passes the Definition of Ready check. One gap: the acceptance criteria are written from the code perspective, not from the user's perspective. This matters for the smoke-test step and for knowing when this is truly done. **Specific gaps I found after reading the code:** 1. The issue says "six raw fetches" in the header listing but the actual count across the three files is: enrich (2) + invites (3) + admin/users (1) = 6. Confirmed correct. 2. The acceptance criterion "No `fetch(\`...${env.API_*}\` ...)` patterns" would NOT catch the `admin/users/[id]/+page.server.ts` line 48 pattern, which uses a relative URL `fetch('/api/users/${params.id}', ...)` — not an `env.API_*` interpolation. The grep in the verification section would also miss this case. 3. "Existing enrich/admin happy paths still work (manual smoke + existing E2E if any)" — there are no existing E2E tests for these specific flows based on the directory structure. The criterion should explicitly acknowledge this and require a new Playwright smoke test rather than relying on "existing E2E if any." 4. The issue states effort as "S — 1 day, mostly retesting." Given that this is a security-adjacent fix with no existing automated test coverage for the affected flows, the retesting burden is real. The estimate is reasonable if the PR author plans to add the regression tests during the same session. ### Recommendations - **Add a user-facing acceptance criterion**: "A non-admin user cannot trigger the admin/invites or admin/users endpoints by submitting the form — the backend returns 403 and the user sees a meaningful error message." This tests the actual failure mode, not just the code pattern. - **Tighten the grep pattern** in the AC: the current pattern misses relative-URL raw fetch calls. Use a two-part check: (1) the `env.API` interpolation grep, and (2) separately verify that `admin/users/[id]/+page.server.ts` update action no longer contains a raw `fetch(` call. - **Update AC item 3** from "existing E2E if any" to "a new Playwright smoke test covering the enrich submit and admin invite create flows" — set the expectation explicitly. - The `CONTRIBUTING.md` multipart note ambiguity (does "bypass typed client" mean "bypass event.fetch" too?) should be resolved in this PR to prevent the same misreading from recurring. Add an AC item: "CONTRIBUTING.md multipart fetch note clarified to specify `event.fetch` must still be used."

marcel commented

2026-05-07 17:38:20 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Observations

This is a frontend code fix, so the infrastructure surface is narrow. A few things worth noting from my angle:

The env.API_INTERNAL_URL pattern: The raw-fetch calls use env.API_INTERNAL_URL || 'http://localhost:8080' as the base URL. This is the same pattern used in hooks.server.ts and api.server.ts. The env var is set in the Docker Compose environment for the frontend service. After the fix, this env var is still needed in api.server.ts (which createApiClient uses) — so no environment change is required.

Internal network auth concern: The issue flags "privilege confusion" where a call to env.API_INTERNAL_URL from the server side might succeed without auth because the backend and frontend container are on the same Docker network. In the current docker-compose.yml, the backend does not have --spring.security.require-ssl=true or similar network-level isolation. Backend @RequirePermission is the only control. This is correct for the current setup, but worth noting.

The hooks.server.ts global fetch in userGroup: The hook at line 47 calls global fetch for /api/users/me with an explicit Authorization header. This is a slightly different case — the hook runs before handleFetch is in scope for that particular call. The pattern is correct (manual auth header), just flagged for awareness that it is the same-looking code for a different reason.

No Compose or CI changes needed for this fix — it is pure TypeScript.

Recommendations

Verify API_INTERNAL_URL is correctly set in all environments (dev Compose, CI Compose overlay, production) before merging. The fix routes all calls through createApiClient which reads this env var — a missing value falls back to localhost:8080, which is fine in dev but would fail in CI if the backend runs on a different hostname. This is already true for the load functions that use createApiClient, so no new risk is introduced, but it is worth double-checking the CI compose overlay has this var.
After merging, run the full smoke suite against the Docker Compose stack (not just npm run dev) to confirm the auth header injection works correctly when the frontend and backend are on separate containers and the API_INTERNAL_URL is the container hostname.
No new environment variables, no Compose changes, no CI pipeline changes required. This is a clean fix.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer ### Observations This is a frontend code fix, so the infrastructure surface is narrow. A few things worth noting from my angle: **The `env.API_INTERNAL_URL` pattern**: The raw-fetch calls use `env.API_INTERNAL_URL || 'http://localhost:8080'` as the base URL. This is the same pattern used in `hooks.server.ts` and `api.server.ts`. The env var is set in the Docker Compose environment for the frontend service. After the fix, this env var is still needed in `api.server.ts` (which `createApiClient` uses) — so no environment change is required. **Internal network auth concern**: The issue flags "privilege confusion" where a call to `env.API_INTERNAL_URL` from the server side might succeed without auth because the backend and frontend container are on the same Docker network. In the current `docker-compose.yml`, the backend does not have `--spring.security.require-ssl=true` or similar network-level isolation. Backend `@RequirePermission` is the only control. This is correct for the current setup, but worth noting. **The `hooks.server.ts` global fetch in `userGroup`**: The hook at line 47 calls global `fetch` for `/api/users/me` with an explicit `Authorization` header. This is a slightly different case — the hook runs before `handleFetch` is in scope for that particular call. The pattern is correct (manual auth header), just flagged for awareness that it is the same-looking code for a different reason. **No Compose or CI changes needed** for this fix — it is pure TypeScript. ### Recommendations - **Verify `API_INTERNAL_URL` is correctly set in all environments** (dev Compose, CI Compose overlay, production) before merging. The fix routes all calls through `createApiClient` which reads this env var — a missing value falls back to `localhost:8080`, which is fine in dev but would fail in CI if the backend runs on a different hostname. This is already true for the `load` functions that use `createApiClient`, so no new risk is introduced, but it is worth double-checking the CI compose overlay has this var. - **After merging, run the full smoke suite against the Docker Compose stack** (not just `npm run dev`) to confirm the auth header injection works correctly when the frontend and backend are on separate containers and the `API_INTERNAL_URL` is the container hostname. - **No new environment variables, no Compose changes, no CI pipeline changes required.** This is a clean fix.

marcel commented

2026-05-07 17:38:34 +02:00

🎨 Leonie Voss — UI/UX & Accessibility

Observations

This fix is infrastructure — no visual changes, no component changes. But there is a UX consequence worth naming explicitly.

Current UX failure mode: When the enrich save or saveAndReview action fails silently due to missing auth, the user sees a form error. The error message comes from getErrorMessage(backendError?.code) — which is correct — but the upstream cause (the backend returning 401 because the auth header was absent) maps to a generic or confusing error code. The user who just filled out the enrichment form sees an opaque error and does not know whether to try again or log in again.

Post-fix UX: After switching to event.fetch, a missing auth token will cause handleFetch to return new Response('Unauthorized', { status: 401 }) before the backend is even called. This is still an error, but at least it is the correct, expected error flow. The parseBackendError call will fail to parse this plain-text response (no JSON body), returning null, and getErrorMessage(undefined) will show the generic internal error message. This is still not a great user experience — the user should see "session expired, please log in again."

The 401 from handleFetch: The hook returns new Response('Unauthorized', { status: 401 }) with a plain text body, not the JSON { code: 'UNAUTHORIZED' } structure that parseBackendError expects. So after the fix, a session-expired user submitting the enrich form will see the generic error, not a targeted "please log in" message.

Recommendations

This issue's fix is correct and complete from a UX standpoint — the 401 path already exists (backend can return 401) and parseBackendError already handles null gracefully. The improvement in reliability is the right priority.
As a follow-up (separate issue): make handleFetch's 401 response return a JSON body { code: 'UNAUTHORIZED' } so that parseBackendError can map it to the i18n string for "session expired." This would give users a clear message to log in again rather than a generic error. This is a Minor UX improvement, not a blocker for this fix.
No component changes are needed for this issue. The enrich and admin pages' error display already uses getErrorMessage correctly.

## 🎨 Leonie Voss — UI/UX & Accessibility ### Observations This fix is infrastructure — no visual changes, no component changes. But there is a UX consequence worth naming explicitly. **Current UX failure mode**: When the enrich `save` or `saveAndReview` action fails silently due to missing auth, the user sees a form error. The error message comes from `getErrorMessage(backendError?.code)` — which is correct — but the upstream cause (the backend returning 401 because the auth header was absent) maps to a generic or confusing error code. The user who just filled out the enrichment form sees an opaque error and does not know whether to try again or log in again. **Post-fix UX**: After switching to `event.fetch`, a missing auth token will cause `handleFetch` to return `new Response('Unauthorized', { status: 401 })` before the backend is even called. This is still an error, but at least it is the correct, expected error flow. The `parseBackendError` call will fail to parse this plain-text response (no JSON body), returning `null`, and `getErrorMessage(undefined)` will show the generic internal error message. This is still not a great user experience — the user should see "session expired, please log in again." **The 401 from `handleFetch`**: The hook returns `new Response('Unauthorized', { status: 401 })` with a plain text body, not the JSON `{ code: 'UNAUTHORIZED' }` structure that `parseBackendError` expects. So after the fix, a session-expired user submitting the enrich form will see the generic error, not a targeted "please log in" message. ### Recommendations - **This issue's fix is correct and complete from a UX standpoint** — the 401 path already exists (backend can return 401) and `parseBackendError` already handles null gracefully. The improvement in reliability is the right priority. - **As a follow-up (separate issue)**: make `handleFetch`'s 401 response return a JSON body `{ code: 'UNAUTHORIZED' }` so that `parseBackendError` can map it to the i18n string for "session expired." This would give users a clear message to log in again rather than a generic error. This is a Minor UX improvement, not a blocker for this fix. - **No component changes are needed for this issue.** The enrich and admin pages' error display already uses `getErrorMessage` correctly.

marcel commented

2026-05-07 17:39:00 +02:00

Decision Queue

Consolidated open decisions raised by the review. No verdicts — these need a call before or during implementation.

Theme 1 — Multipart + event.fetch scope

Raised by: Markus, Felix

The enrich save/saveAndReview actions must use raw fetch (not createApiClient) because the typed client does not support multipart bodies. The question is how explicit this exception should be documented:

Option A: Fix the code to use event.fetch for multipart calls, and add a one-sentence clarification to CONTRIBUTING.md ("for multipart, bypass createApiClient but never bypass event.fetch").
Option B: Same code fix, but also add an inline comment to each multipart callsite explaining why createApiClient is not used.

Both options fix the bug. Option B adds noise in the code; Option A keeps the explanation in the doc. The project style ("intentional choices need no doc comment") suggests Option A — but this is a subtle distinction that has already confused at least one developer once, so the tradeoff is real.

Theme 2 — SvelteKit relative-URL fetch and handleFetch

Raised by: Markus, Nora

admin/users/[id]/+page.server.ts line 48 uses fetch('/api/users/${params.id}', ...) with a relative URL. The question is: does SvelteKit route relative-URL server-action fetches through handleFetch?

The answer determines how severe this specific callsite is:

If yes, handleFetch fires and the auth header IS injected — the bug is less severe (UX annoyance, not auth bypass), but the refactor is still worth doing for consistency.
If no, this is the same severity as the other callsites — a missing auth header on an admin-level mutation endpoint.

This can be determined empirically: add a console.log to handleFetch and submit the user-edit form in dev. This must be verified before merging, not assumed.

Theme 3 — handleFetch 401 response format (follow-up scope)

Raised by: Leonie

When handleFetch returns a 401 (no auth token in cookie), it returns new Response('Unauthorized', { status: 401 }) with a plain-text body. parseBackendError cannot parse this and falls back to the generic error message — the user sees "Something went wrong" instead of "Session expired, please log in."

Decision: should fixing handleFetch to return { code: 'UNAUTHORIZED' } JSON be in scope for this issue, or tracked as a separate follow-up?

Recommendation: separate issue — it is a UX improvement, not a security fix, and adding it here risks scope creep on a P1 security item. But it should be filed before this PR closes so it is not forgotten.

## Decision Queue Consolidated open decisions raised by the review. No verdicts — these need a call before or during implementation. --- ### Theme 1 — Multipart + event.fetch scope **Raised by**: Markus, Felix The enrich `save`/`saveAndReview` actions must use raw fetch (not `createApiClient`) because the typed client does not support multipart bodies. The question is how explicit this exception should be documented: - **Option A**: Fix the code to use `event.fetch` for multipart calls, and add a one-sentence clarification to `CONTRIBUTING.md` ("for multipart, bypass `createApiClient` but never bypass `event.fetch`"). - **Option B**: Same code fix, but also add an inline comment to each multipart callsite explaining why `createApiClient` is not used. Both options fix the bug. Option B adds noise in the code; Option A keeps the explanation in the doc. The project style ("intentional choices need no doc comment") suggests Option A — but this is a subtle distinction that has already confused at least one developer once, so the tradeoff is real. --- ### Theme 2 — SvelteKit relative-URL fetch and handleFetch **Raised by**: Markus, Nora `admin/users/[id]/+page.server.ts` line 48 uses `fetch('/api/users/${params.id}', ...)` with a relative URL. The question is: **does SvelteKit route relative-URL server-action fetches through `handleFetch`?** The answer determines how severe this specific callsite is: - If yes, `handleFetch` fires and the auth header IS injected — the bug is less severe (UX annoyance, not auth bypass), but the refactor is still worth doing for consistency. - If no, this is the same severity as the other callsites — a missing auth header on an admin-level mutation endpoint. This can be determined empirically: add a `console.log` to `handleFetch` and submit the user-edit form in dev. **This must be verified before merging, not assumed.** --- ### Theme 3 — handleFetch 401 response format (follow-up scope) **Raised by**: Leonie When `handleFetch` returns a 401 (no auth token in cookie), it returns `new Response('Unauthorized', { status: 401 })` with a plain-text body. `parseBackendError` cannot parse this and falls back to the generic error message — the user sees "Something went wrong" instead of "Session expired, please log in." **Decision**: should fixing `handleFetch` to return `{ code: 'UNAUTHORIZED' }` JSON be in scope for this issue, or tracked as a separate follow-up? Recommendation: **separate issue** — it is a UX improvement, not a security fix, and adding it here risks scope creep on a P1 security item. But it should be filed before this PR closes so it is not forgotten.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#465