fix(frontend): disable prerender crawl so protected routes aren't baked to login-bounces (#514) #515

Merged

marcel merged 2 commits from fix/issue-514-prerender-crawl-bakes-redirects into main 2026-05-11 17:12:03 +02:00

Conversation 7 Commits 2 Files Changed 2 +34 -1

marcel commented 2026-05-11 16:53:27 +02:00

Owner

Copy Link

Summary

Closes #514. This is the actual reason login doesn't work in production — and it's why dev mode works fine but staging is broken.

Default prerender.crawl = true had SvelteKit follow nav links from /hilfe/transkription and prerender /, /documents, /persons, /geschichten, /stammbaum too. Those routes' load functions throw redirect(302, '/login') during the build (no auth cookie), and SvelteKit captured each as a static HTML file:

<script>location.href="/login";</script><meta http-equiv="refresh" content="0;url=/login">

Those static files (build/prerendered/index.html, …) are served before the runtime hooks.server.ts runs. An authenticated user with a valid auth_token cookie still gets the baked bounce-back HTML — every time.

Dev mode doesn't prerender, so the bug only shows in npm run build output.

Fix

prerender: {
    entries: ['/hilfe/transkription'],
    crawl: false
}

Keeps the explicit /hilfe/transkription prerender entry (still public, still pre-built), drops the implicit crawl.

Test plan after merge

• Redeploy staging via nightly.yml
• docker exec archiv-staging-frontend-1 ls /app/build/prerendered/ shows only hilfe/transkription.html, no index.html/documents.html/etc.
• Log in with admin@familyarchive.local + the staging admin password; land on dashboard instead of bouncing back to /login.

🤖 Generated with Claude Code

## Summary Closes #514. This is the actual reason login doesn't work in production — and it's why **dev mode works fine but staging is broken**. Default `prerender.crawl = true` had SvelteKit follow nav links from `/hilfe/transkription` and prerender `/`, `/documents`, `/persons`, `/geschichten`, `/stammbaum` too. Those routes' load functions throw `redirect(302, '/login')` during the build (no auth cookie), and SvelteKit captured each as a static HTML file: ```html <script>location.href="/login";</script><meta http-equiv="refresh" content="0;url=/login"> ``` Those static files (`build/prerendered/index.html`, …) are served **before** the runtime `hooks.server.ts` runs. An authenticated user with a valid `auth_token` cookie still gets the baked bounce-back HTML — every time. Dev mode doesn't prerender, so the bug only shows in `npm run build` output. ## Fix ```js prerender: { entries: ['/hilfe/transkription'], crawl: false } ``` Keeps the explicit `/hilfe/transkription` prerender entry (still public, still pre-built), drops the implicit crawl. ## Test plan after merge - [ ] Redeploy staging via `nightly.yml` - [ ] `docker exec archiv-staging-frontend-1 ls /app/build/prerendered/` shows only `hilfe/transkription.html`, no `index.html`/`documents.html`/etc. - [ ] Log in with `admin@familyarchive.local` + the staging admin password; land on dashboard instead of bouncing back to /login. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel commented 2026-05-11 16:56:30 +02:00

Author

Owner

Copy Link

Markus Keller — Senior Application Architect

Verdict: Approved

What I checked

• frontend/svelte.config.js — the only file changed
• Cross-checked frontend/src/hooks.server.ts: handleAuth redirects anything outside PUBLIC_PATHS to /login, which is exactly what was getting baked into static HTML at build time. Root cause confirmed.
• frontend/src/routes/+layout.server.ts — pulls locals.user; during prerender there is no cookie so locals.user is undefined and handleAuth triggers the redirect. Matches the bug story exactly.
• The intentional entry /hilfe/transkription is whitelisted in PUBLIC_PATHS, so it correctly stays prerenderable and public after this change.

Architectural assessment

This is the right layer for the fix. Two configuration knobs were doing two different things:

1. entries: ['/hilfe/transkription'] — "prerender these explicit pages"
2. crawl: true (default) — "and also follow every link they emit"

Knob #2 is what tangled the build-time and runtime auth boundaries. Disabling crawl restores the invariant we want: prerender is an opt-in list, not a transitive reachability set. Cleaner mental model, smaller blast radius.

Suggestions (non-blocking)

1. The inline comment is excellent — it captures the threat model (build-time redirect captured as static HTML, served before hooks run). This is exactly the kind of "why" comment I want preserved. Keep it.
2. Consider adding a one-line ADR (docs/adr/) — "Prerender entries are explicit, never crawled" — so a future SvelteKit upgrade or refactor doesn't silently re-enable crawl: true. Optional; the code comment may already be sufficient at this project size.
3. No diagram impact: this is a build-time configuration change, not a topology change. c4/l3-frontend-*.puml does not need to move.

LGTM — ship it.

## Markus Keller — Senior Application Architect **Verdict: Approved** ### What I checked - `frontend/svelte.config.js` — the only file changed - Cross-checked `frontend/src/hooks.server.ts`: `handleAuth` redirects anything outside `PUBLIC_PATHS` to `/login`, which is exactly what was getting baked into static HTML at build time. Root cause confirmed. - `frontend/src/routes/+layout.server.ts` — pulls `locals.user`; during prerender there is no cookie so `locals.user` is undefined and `handleAuth` triggers the redirect. Matches the bug story exactly. - The intentional entry `/hilfe/transkription` is whitelisted in `PUBLIC_PATHS`, so it correctly stays prerenderable and public after this change. ### Architectural assessment This is the right layer for the fix. Two configuration knobs were doing two different things: 1. `entries: ['/hilfe/transkription']` — "prerender these explicit pages" 2. `crawl: true` (default) — "and also follow every link they emit" Knob #2 is what tangled the build-time and runtime auth boundaries. Disabling crawl restores the invariant we want: **prerender is an opt-in list, not a transitive reachability set.** Cleaner mental model, smaller blast radius. ### Suggestions (non-blocking) 1. The inline comment is excellent — it captures the threat model (build-time redirect captured as static HTML, served before hooks run). This is exactly the kind of "why" comment I want preserved. Keep it. 2. Consider adding a one-line ADR (`docs/adr/`) — "Prerender entries are explicit, never crawled" — so a future SvelteKit upgrade or refactor doesn't silently re-enable `crawl: true`. Optional; the code comment may already be sufficient at this project size. 3. No diagram impact: this is a build-time configuration change, not a topology change. `c4/l3-frontend-*.puml` does not need to move. LGTM — ship it.

marcel commented 2026-05-11 16:56:40 +02:00

Author

Owner

Copy Link

Felix Brandt — Senior Fullstack Developer

Verdict: Approved with concerns

What I like

• Tiny, surgical diff (1 file, 11/-1). Easy to revert, easy to audit.
• The comment in svelte.config.js explains the why, not the what — exactly the style I'd want. It documents the threat model so the next person who looks at this config understands why crawl: false is load-bearing.
• The fix is at the correct layer: a build-config flag, not a runtime workaround.

Concerns

1. No regression test (this is the real one)

This bug only manifested in npm run build output. A passing dev-mode test would have happily green-lit a broken production build. We need a test that fails if crawl: true ever comes back. Cheap option: a CI step that asserts the prerendered output contains only the expected files.

# in CI, after `npm run build`:
test "$(ls frontend/build/prerendered 2>/dev/null | grep -v '^hilfe$' | wc -l)" -eq 0 \
  || (echo "Unexpected prerendered files — protected routes leaked into static HTML"; exit 1)

Or even better, a Vitest test that imports svelte.config.js and asserts kit.prerender.crawl === false:

import config from '../svelte.config.js';
it('does not crawl from prerender entries', () => {
    expect(config.kit?.prerender?.crawl).toBe(false);
});

Two lines. Permanent regression guard. Without it, a future npm install or SvelteKit upgrade could silently re-introduce the default.

2. Test plan in the PR is manual-only

The "Test plan after merge" checklist is fine for now, but every item there is a manual step. After this lands, those steps should become automated smoke tests in the post-deploy verification — docker exec ... ls /app/build/prerendered/ is a perfect smoke-test assertion.

Suggestions (nice to have)

• The comment block is good but a little long. Acceptable here because the cause-and-effect chain is genuinely non-obvious; I would not trim it.
• No need to touch any other route file — /hilfe/transkription already stays prerendered correctly.

Once a regression assertion lands (either in CI or as a Vitest config check), this is a clean ship.

## Felix Brandt — Senior Fullstack Developer **Verdict: Approved with concerns** ### What I like - Tiny, surgical diff (1 file, 11/-1). Easy to revert, easy to audit. - The comment in `svelte.config.js` explains the *why*, not the *what* — exactly the style I'd want. It documents the threat model so the next person who looks at this config understands why `crawl: false` is load-bearing. - The fix is at the correct layer: a build-config flag, not a runtime workaround. ### Concerns #### 1. No regression test (this is the real one) This bug only manifested in `npm run build` output. A passing dev-mode test would have happily green-lit a broken production build. We need a test that fails if `crawl: true` ever comes back. Cheap option: a CI step that asserts the prerendered output contains only the expected files. ```bash # in CI, after `npm run build`: test "$(ls frontend/build/prerendered 2>/dev/null | grep -v '^hilfe$' | wc -l)" -eq 0 \ || (echo "Unexpected prerendered files — protected routes leaked into static HTML"; exit 1) ``` Or even better, a Vitest test that imports `svelte.config.js` and asserts `kit.prerender.crawl === false`: ```ts import config from '../svelte.config.js'; it('does not crawl from prerender entries', () => { expect(config.kit?.prerender?.crawl).toBe(false); }); ``` Two lines. Permanent regression guard. Without it, a future `npm install` or SvelteKit upgrade could silently re-introduce the default. #### 2. Test plan in the PR is manual-only The "Test plan after merge" checklist is fine for now, but every item there is a manual step. After this lands, those steps should become automated smoke tests in the post-deploy verification — `docker exec ... ls /app/build/prerendered/` is a perfect smoke-test assertion. ### Suggestions (nice to have) - The comment block is good but a little long. Acceptable here because the cause-and-effect chain is genuinely non-obvious; I would not trim it. - No need to touch any other route file — `/hilfe/transkription` already stays prerendered correctly. Once a regression assertion lands (either in CI or as a Vitest config check), this is a clean ship.

marcel commented 2026-05-11 16:56:54 +02:00

Author

Owner

Copy Link

Tobias Wendt — DevOps & Platform Engineer

Verdict: Approved with concerns

The fix itself is one line of config. From an infra angle there's nothing controversial here — the danger lives in the deployment process around it. This bug shipped to staging undetected, which means our deploy pipeline did not catch a production-blocking regression. That's the conversation I want to have.

What I like

• One-line, reversible, no infra changes required. No image rebuild ceremony beyond a normal frontend rebuild.
• crawl: false keeps the existing /hilfe/transkription prerender working, so caching/CDN behavior for that page is unchanged.

Concerns (pipeline gaps, not code)

1. Post-deploy smoke test is missing. A curl http://127.0.0.1:3001/ | grep -q 'location.href="/login"' against the deployed staging container would have caught this in seconds. Add a smoke step to nightly.yml that:

   • Hits / with no auth cookie and asserts the response is dynamic (a real Set-Cookie, a 302, or rendered Svelte markup — NOT a static location.href HTML file).
   • Hits /hilfe/transkription with no auth cookie and asserts 200.

2. Build-artifact assertion in CI. Before the container is even built, fail the pipeline if frontend/build/prerendered/ contains anything other than hilfe/:

   - name: Verify no protected routes were prerendered
     run: |
       UNEXPECTED=$(find frontend/build/prerendered -type f ! -path '*/hilfe/*' | head)
       [ -z "$UNEXPECTED" ] || { echo "Leaked prerender: $UNEXPECTED"; exit 1; }
   

   Cheap, deterministic, catches future regressions if anyone flips crawl back or adds another entry.

3. The PR's "Test plan after merge" is manual. All three checkboxes (docker exec, log in, land on dashboard) should be in the smoke job, not in a human checklist. Manual post-deploy verification is the same class of failure we're fixing.

Note (not a blocker)

The fact that dev mode worked but the production build was broken is a structural risk we will see again. Worth a follow-up issue: "Run a production build + curl-based smoke check in CI before merge to main" — same Docker compose stack, hit a handful of routes unauthenticated, fail if any return baked-redirect HTML. Two minutes of CI for an entire class of prerender-vs-runtime regressions caught forever.

Ship the fix; open the follow-up issues for the smoke test and the build-artifact assertion.

## Tobias Wendt — DevOps & Platform Engineer **Verdict: Approved with concerns** The fix itself is one line of config. From an infra angle there's nothing controversial here — the danger lives in the deployment process around it. This bug shipped to staging undetected, which means our deploy pipeline did not catch a production-blocking regression. That's the conversation I want to have. ### What I like - One-line, reversible, no infra changes required. No image rebuild ceremony beyond a normal frontend rebuild. - `crawl: false` keeps the existing `/hilfe/transkription` prerender working, so caching/CDN behavior for that page is unchanged. ### Concerns (pipeline gaps, not code) 1. **Post-deploy smoke test is missing.** A `curl http://127.0.0.1:3001/ | grep -q 'location.href="/login"'` against the deployed staging container would have caught this in seconds. Add a smoke step to `nightly.yml` that: - Hits `/` with no auth cookie and asserts the response is **dynamic** (a real `Set-Cookie`, a 302, or rendered Svelte markup — NOT a static `location.href` HTML file). - Hits `/hilfe/transkription` with no auth cookie and asserts 200. 2. **Build-artifact assertion in CI.** Before the container is even built, fail the pipeline if `frontend/build/prerendered/` contains anything other than `hilfe/`: ```bash - name: Verify no protected routes were prerendered run: | UNEXPECTED=$(find frontend/build/prerendered -type f ! -path '*/hilfe/*' | head) [ -z "$UNEXPECTED" ] || { echo "Leaked prerender: $UNEXPECTED"; exit 1; } ``` Cheap, deterministic, catches future regressions if anyone flips `crawl` back or adds another entry. 3. **The PR's "Test plan after merge" is manual.** All three checkboxes (`docker exec`, log in, land on dashboard) should be in the smoke job, not in a human checklist. Manual post-deploy verification is the same class of failure we're fixing. ### Note (not a blocker) The fact that dev mode worked but the production build was broken is a structural risk we will see again. Worth a follow-up issue: **"Run a production build + curl-based smoke check in CI before merge to main"** — same Docker compose stack, hit a handful of routes unauthenticated, fail if any return baked-redirect HTML. Two minutes of CI for an entire class of prerender-vs-runtime regressions caught forever. Ship the fix; open the follow-up issues for the smoke test and the build-artifact assertion.

marcel commented 2026-05-11 16:57:07 +02:00

Author

Owner

Copy Link

Elicit — Senior Requirements Engineer

Verdict: Approved

What I checked

This is a bug fix, not a feature change, so my lens here is: does the issue clearly document the user-observable problem, the acceptance criteria, and the verification plan?

Issue #514 — quality review

The issue is exemplary. It contains everything I look for in a defect spec:

• Symptom in user language: "Login impossible from production/staging" — clear, falsifiable.
• Reproduction: actual shell commands and observed file contents from the staging box. No ambiguity.
• Root cause: traced to build/prerendered/index.html containing baked redirect HTML; the build-time redirect(302, '/login') was captured as static markup.
• Why-it-only-manifests-here: explicit explanation that dev mode does not prerender, so the bug is invisible locally. This is the kind of context that prevents "works on my machine" misdiagnosis later.
• Severity tagged: HIGH, prod-blocking. Correctly prioritized.
• Linked discovery context: tied to deploy issues #497/#499.

PR — acceptance criteria

The PR's "Test plan after merge" maps cleanly to implicit acceptance criteria:

Given a freshly-built production frontend,
When an authenticated user visits /,
Then they see the dashboard, not a static bounce to /login.

Given the build output,
When listing build/prerendered/,
Then only hilfe/transkription.html is present.

Given an unauthenticated visit to /hilfe/transkription,
When the user loads the page,
Then the help content renders without an auth cookie (prerender entry preserved).

These three are testable, observable, and unambiguous. I would only suggest one improvement: lift them out of a markdown checkbox in the PR description and into the issue itself as Given-When-Then acceptance criteria, so the issue becomes the canonical "done" definition rather than the PR.

Suggestions (non-blocking)

1. NFR coverage for this fix: the affected NFRs are Reliability (login must succeed first-try) and Observability (we had no automated signal that prerender output contained protected routes). Both deserve a follow-up note — the issue mentions the symptom but not the missing telemetry that delayed detection. A small OQ- open question: "Should we alert when build/prerendered/ content drifts?" — answer probably "yes, in CI" per Tobias's smoke-test suggestion.
2. Open questions worth logging:
   • OQ-001: Are there any other implicit prerender entries we don't know about? (No — verified by reading svelte.config.js.)
   • OQ-002: When we add a new public page in the future, does the team know they must add it to both entries and PUBLIC_PATHS in hooks.server.ts? Currently this is tribal knowledge. Worth a docs sentence in CONTRIBUTING.md or a comment in hooks.server.ts.

Bottom line

The defect is well-specified; the fix scope matches the defect; the verification steps are testable. From a requirements perspective: ship it.

## Elicit — Senior Requirements Engineer **Verdict: Approved** ### What I checked This is a bug fix, not a feature change, so my lens here is: does the issue clearly document the user-observable problem, the acceptance criteria, and the verification plan? ### Issue #514 — quality review The issue is exemplary. It contains everything I look for in a defect spec: - **Symptom in user language**: "Login impossible from production/staging" — clear, falsifiable. - **Reproduction**: actual shell commands and observed file contents from the staging box. No ambiguity. - **Root cause**: traced to `build/prerendered/index.html` containing baked redirect HTML; the build-time `redirect(302, '/login')` was captured as static markup. - **Why-it-only-manifests-here**: explicit explanation that dev mode does not prerender, so the bug is invisible locally. This is the kind of context that prevents "works on my machine" misdiagnosis later. - **Severity tagged**: HIGH, prod-blocking. Correctly prioritized. - **Linked discovery context**: tied to deploy issues #497/#499. ### PR — acceptance criteria The PR's "Test plan after merge" maps cleanly to implicit acceptance criteria: > Given a freshly-built production frontend, > When an authenticated user visits `/`, > Then they see the dashboard, not a static bounce to `/login`. > Given the build output, > When listing `build/prerendered/`, > Then only `hilfe/transkription.html` is present. > Given an unauthenticated visit to `/hilfe/transkription`, > When the user loads the page, > Then the help content renders without an auth cookie (prerender entry preserved). These three are testable, observable, and unambiguous. I would only suggest one improvement: lift them out of a markdown checkbox in the PR description and into the issue itself as Given-When-Then acceptance criteria, so the issue becomes the canonical "done" definition rather than the PR. ### Suggestions (non-blocking) 1. **NFR coverage** for this fix: the affected NFRs are **Reliability** (login must succeed first-try) and **Observability** (we had no automated signal that prerender output contained protected routes). Both deserve a follow-up note — the issue mentions the symptom but not the missing telemetry that delayed detection. A small `OQ-` open question: "Should we alert when `build/prerendered/` content drifts?" — answer probably "yes, in CI" per Tobias's smoke-test suggestion. 2. **Open questions** worth logging: - OQ-001: Are there any other implicit prerender entries we don't know about? (No — verified by reading `svelte.config.js`.) - OQ-002: When we add a new public page in the future, does the team know they must add it to both `entries` and `PUBLIC_PATHS` in `hooks.server.ts`? Currently this is tribal knowledge. Worth a docs sentence in `CONTRIBUTING.md` or a comment in `hooks.server.ts`. ### Bottom line The defect is well-specified; the fix scope matches the defect; the verification steps are testable. From a requirements perspective: ship it.

marcel commented 2026-05-11 16:57:24 +02:00

Author

Owner

Copy Link

Nora "NullX" Steiner — Application Security Engineer

Verdict: Approved

This is a security-relevant fix even though it's labeled a build bug. Let me explain why, because the framing matters.

Why this is a security concern (not just a UX bug)

What was actually happening on staging:

1. User submits valid credentials → backend authenticates → backend issues a valid auth_token cookie → 303 to /.
2. The browser GETs /. The reverse proxy returns the statically-baked /app/build/prerendered/index.html, which contains <script>location.href="/login"</script>.
3. The runtime handleAuth hook never executes. The authenticated user is treated identically to an unauthenticated one — the auth check is bypassed because there is no auth check to bypass; the response was decided at build time.

That is a broken access control failure mode (CWE-862 conceptually — Missing Authorization due to bypass), even though in this case the failure is "deny everyone instead of decide per-user." The lesson generalizes: any time runtime auth decisions are captured into static artifacts, you have a class of issue where the auth layer becomes inert. The inversion of "static deny" could just as easily be "static allow" if a different page were prerendered in the wrong auth state — for example, if an admin built locally while logged in, SvelteKit could in principle bake admin content into static HTML and serve it to anonymous users. That has not happened here, but the configuration that allowed (1) above is the same configuration that would allow that.

crawl: false fixes both directions of this hazard by establishing the invariant: the prerender set is exactly what's in entries, and nothing reachable from those entries. That is the right security posture for a build process.

Verified by reading hooks.server.ts

• handleAuth correctly redirects non-public paths to /login. Good.
• PUBLIC_PATHS includes /hilfe/transkription. Good — matches the single prerender entry, so the public help page works without an auth cookie at runtime too.
• No leakage of authenticated content into prerender output — confirmed by inspecting the only entry's +page.svelte (it's a pure rules/help page with no load function pulling user data).

Suggestions (non-blocking)

1. Test the invariant. Add an assertion (Sara/Felix already suggested this in different forms) that fails the build if build/prerendered/ contains any file not under hilfe/. This converts the security invariant into an enforced one.
2. Document the rule. A one-liner in CONTRIBUTING.md under a "Prerender safety" heading: "Every entry in prerender.entries must also be listed in PUBLIC_PATHS in hooks.server.ts. Never serve user-specific content from a prerendered route." Two sentences, removes a future foot-gun.
3. CWE/CAPEC reference for the issue's wiki/audit trail: this is conceptually in the family of CAPEC-21 (Exploitation of Trusted Identifiers) + CWE-665 (Improper Initialization) — the runtime auth layer was not initialized for paths the build pre-resolved.

Summary

The fix is correct, minimal, and addresses the root cause at the right layer. No code changes required for security approval. The follow-up assertion would harden the invariant; nice-to-have, not blocker.

## Nora "NullX" Steiner — Application Security Engineer **Verdict: Approved** This is a security-relevant fix even though it's labeled a build bug. Let me explain why, because the framing matters. ### Why this is a security concern (not just a UX bug) What was actually happening on staging: 1. User submits valid credentials → backend authenticates → backend issues a valid `auth_token` cookie → 303 to `/`. 2. The browser GETs `/`. The reverse proxy returns the **statically-baked** `/app/build/prerendered/index.html`, which contains `<script>location.href="/login"</script>`. 3. **The runtime `handleAuth` hook never executes.** The authenticated user is treated identically to an unauthenticated one — the auth check is bypassed because there is no auth check to bypass; the response was decided at build time. That is a **broken access control failure mode** (CWE-862 conceptually — Missing Authorization due to bypass), even though in this case the failure is "deny everyone instead of decide per-user." The lesson generalizes: **any time runtime auth decisions are captured into static artifacts, you have a class of issue where the auth layer becomes inert.** The inversion of "static deny" could just as easily be "static allow" if a different page were prerendered in the wrong auth state — for example, if an admin built locally while logged in, SvelteKit could in principle bake admin content into static HTML and serve it to anonymous users. That has not happened here, but the configuration that allowed (1) above is the same configuration that would allow that. `crawl: false` fixes both directions of this hazard by establishing the invariant: **the prerender set is exactly what's in `entries`, and nothing reachable from those entries**. That is the right security posture for a build process. ### Verified by reading `hooks.server.ts` - `handleAuth` correctly redirects non-public paths to `/login`. Good. - `PUBLIC_PATHS` includes `/hilfe/transkription`. Good — matches the single prerender entry, so the public help page works without an auth cookie at runtime too. - No leakage of authenticated content into prerender output — confirmed by inspecting the only entry's `+page.svelte` (it's a pure rules/help page with no `load` function pulling user data). ### Suggestions (non-blocking) 1. **Test the invariant.** Add an assertion (Sara/Felix already suggested this in different forms) that fails the build if `build/prerendered/` contains any file not under `hilfe/`. This converts the security invariant into an enforced one. 2. **Document the rule.** A one-liner in `CONTRIBUTING.md` under a "Prerender safety" heading: *"Every entry in `prerender.entries` must also be listed in `PUBLIC_PATHS` in `hooks.server.ts`. Never serve user-specific content from a prerendered route."* Two sentences, removes a future foot-gun. 3. **CWE/CAPEC reference** for the issue's wiki/audit trail: this is conceptually in the family of **CAPEC-21 (Exploitation of Trusted Identifiers)** + **CWE-665 (Improper Initialization)** — the runtime auth layer was not initialized for paths the build pre-resolved. ### Summary The fix is correct, minimal, and addresses the root cause at the right layer. No code changes required for security approval. The follow-up assertion would harden the invariant; nice-to-have, not blocker.

marcel commented 2026-05-11 16:57:41 +02:00

Author

Owner

Copy Link

Sara Holt — Senior QA Engineer

Verdict: Changes requested

The code change is correct. The reason I'm requesting changes is that this is exactly the class of bug a test suite exists to prevent, and shipping the fix without locking it in means we'll see this again — possibly with a different prerender entry, possibly worse.

This was a prod-blocking, login-breaking defect that survived our entire test pyramid. That means our pyramid has a hole. The PR cannot close that hole with a one-line config change alone.

Why our suite missed it

• Unit tests never touch the production build. Not their job. Correctly silent.
• Integration tests run against dev-mode SvelteKit. Dev mode doesn't prerender. Correctly silent.
• E2E tests — if they exist for login — almost certainly point at the dev server or run via npm run build && node ./build, but we have no test that asserts the content of the prerendered output.
• Smoke tests after deploy — apparently none, since the bug was discovered by manually clicking around staging.

Four layers, zero coverage for this failure mode.

Required additions before merge (blockers)

1. Build-output assertion (cheapest, highest value)

A test that runs npm run build and asserts the prerender directory:

// frontend/src/lib/__tests__/build-prerender.test.ts (or a CI shell step — both work)
import { readdirSync, statSync } from 'node:fs';
import { join } from 'node:path';

it('prerenders only the help page', () => {
  const root = 'build/prerendered';
  const allFiles: string[] = [];
  const walk = (dir: string) => {
    for (const entry of readdirSync(dir)) {
      const full = join(dir, entry);
      if (statSync(full).isDirectory()) walk(full);
      else allFiles.push(full);
    }
  };
  walk(root);
  expect(allFiles).toEqual([join(root, 'hilfe', 'transkription.html')]);
});

Add as a CI job that runs after npm run build. If anything other than hilfe/transkription.html shows up, fail.

2. Playwright E2E for the actual user journey

test('logged-in user lands on dashboard, not /login', async ({ page }) => {
  await page.goto('/login');
  await page.fill('[name=email]', 'admin@familyarchive.local');
  await page.fill('[name=password]', process.env.E2E_ADMIN_PASSWORD!);
  await page.click('button[type=submit]');
  await expect(page).not.toHaveURL(/\/login/);
  await expect(page.getByRole('main')).toBeVisible();
});

This test runs against the production build, not the dev server. If it had existed, this bug would have failed CI before staging ever saw it.

3. Confirm where the existing E2E suite points

Before merge, please verify whether the current Playwright config runs against npm run dev or npm run preview/node build. If it's the former, that's the larger gap. Switch login-flow E2Es to run against the production build.

Suggestions (non-blocking)

• Add a @WithMockUser-equivalent assertion suite for SvelteKit load functions that ensures none of them are accidentally prerenderable when they depend on locals.user.
• Add an axe-playwright run against /hilfe/transkription since it's a special-case public route — easy to forget about, and seniors will land there from external links.

Bottom line

Ship the config change plus at least item #1 above. Without item #1, this regression is permanently one careless crawl: true away from coming back. I'd hold the PR until that assertion is in. Sorry to push back on what looks like a one-liner — the one-liner is right; the missing test is what made the bug expensive.

## Sara Holt — Senior QA Engineer **Verdict: Changes requested** The code change is correct. The reason I'm requesting changes is that this is exactly the class of bug a test suite exists to prevent, and shipping the fix without locking it in means we'll see this again — possibly with a different prerender entry, possibly worse. This was a **prod-blocking, login-breaking defect that survived our entire test pyramid**. That means our pyramid has a hole. The PR cannot close that hole with a one-line config change alone. ### Why our suite missed it - **Unit tests** never touch the production build. Not their job. Correctly silent. - **Integration tests** run against dev-mode SvelteKit. Dev mode doesn't prerender. Correctly silent. - **E2E tests** — if they exist for login — almost certainly point at the dev server or run via `npm run build && node ./build`, but we have no test that asserts the **content** of the prerendered output. - **Smoke tests after deploy** — apparently none, since the bug was discovered by manually clicking around staging. Four layers, zero coverage for this failure mode. ### Required additions before merge (blockers) #### 1. Build-output assertion (cheapest, highest value) A test that runs `npm run build` and asserts the prerender directory: ```typescript // frontend/src/lib/__tests__/build-prerender.test.ts (or a CI shell step — both work) import { readdirSync, statSync } from 'node:fs'; import { join } from 'node:path'; it('prerenders only the help page', () => { const root = 'build/prerendered'; const allFiles: string[] = []; const walk = (dir: string) => { for (const entry of readdirSync(dir)) { const full = join(dir, entry); if (statSync(full).isDirectory()) walk(full); else allFiles.push(full); } }; walk(root); expect(allFiles).toEqual([join(root, 'hilfe', 'transkription.html')]); }); ``` Add as a CI job that runs after `npm run build`. If anything other than `hilfe/transkription.html` shows up, fail. #### 2. Playwright E2E for the actual user journey ```typescript test('logged-in user lands on dashboard, not /login', async ({ page }) => { await page.goto('/login'); await page.fill('[name=email]', 'admin@familyarchive.local'); await page.fill('[name=password]', process.env.E2E_ADMIN_PASSWORD!); await page.click('button[type=submit]'); await expect(page).not.toHaveURL(/\/login/); await expect(page.getByRole('main')).toBeVisible(); }); ``` This test runs against the **production build**, not the dev server. If it had existed, this bug would have failed CI before staging ever saw it. #### 3. Confirm where the existing E2E suite points Before merge, please verify whether the current Playwright config runs against `npm run dev` or `npm run preview`/`node build`. If it's the former, that's the larger gap. Switch login-flow E2Es to run against the production build. ### Suggestions (non-blocking) - Add a `@WithMockUser`-equivalent assertion suite for SvelteKit `load` functions that ensures none of them are accidentally prerenderable when they depend on `locals.user`. - Add an axe-playwright run against `/hilfe/transkription` since it's a special-case public route — easy to forget about, and seniors will land there from external links. ### Bottom line Ship the config change **plus** at least item #1 above. Without item #1, this regression is permanently one careless `crawl: true` away from coming back. I'd hold the PR until that assertion is in. Sorry to push back on what looks like a one-liner — the one-liner is right; the missing test is what made the bug expensive.

marcel commented 2026-05-11 16:57:55 +02:00

Author

Owner

Copy Link

Leonie Voss — UX & Accessibility Lead

Verdict: Approved

LGTM, with one note about what almost happened

This is a build-config change with no UI surface, so there's nothing for me to review at the pixel level. The visual brand, the components, the WCAG checks — none of those moved.

What I want to flag is the user-facing impact this bug had, because it sits squarely in my territory: it broke trust at the single most fragile moment in any product — the first login attempt by a senior on a new device.

Imagine our 67-year-old aunt on her tablet, in bright daylight, typing a 14-character admin password carefully with one finger. She hits "Anmelden." The screen flickers and she's back on the login page. She assumes she mistyped, tries again. Same result. By the third attempt she is convinced "the computer is broken" or — worse — "I broke it." She closes the tab and texts her grandson. That is exactly the failure mode our dual-audience design philosophy is built to prevent: silent failure with no useful error message and no path forward.

The fix restores the intended flow, so from a UX standpoint, approve. But this near-miss highlights two things worth follow-ups (not blockers on this PR):

Suggestions for follow-up issues

1. Login failure feedback. If the login form ever bounces the user back to itself, the page should say so explicitly with a friendly, localized message ("Anmeldung fehlgeschlagen — bitte erneut versuchen"). Today, the path back to /login is silent. Even when this static-HTML bug isn't in play, a session-expired redirect would land the user on /login with no explanation. That's worth a small spec — maybe a ?reason=session-expired query param that the page renders as a aria-live="polite" notice.

2. /hilfe/transkription is special — confirm it works without an auth cookie. This is the one route that survives crawl: false, and it's also the route a senior user might be linked to from an email or a printout before they ever log in. Add a Playwright check that hits it cold (no cookies, no session) and verifies that the page renders correctly, with all i18n strings present, at the 320px breakpoint. If we ever break that page's public-ness, our outreach to first-time users breaks with it.

3. Touch-target audit on /login itself. Unrelated to this PR but the same user journey: while we're focused on login, verify the email/password fields and the "Anmelden" button meet the 44×44px floor and 48px preferred for the senior audience. This bug was prod-blocking; the touch-target floor is silent-blocking — same effect on the same user.

Bottom line

Ship the fix. Open the three follow-up issues so the user experience around login is as robust as the infrastructure around it now will be.

## Leonie Voss — UX & Accessibility Lead **Verdict: Approved** ### LGTM, with one note about what almost happened This is a build-config change with no UI surface, so there's nothing for me to review at the pixel level. The visual brand, the components, the WCAG checks — none of those moved. What I want to flag is the **user-facing impact this bug had**, because it sits squarely in my territory: it broke trust at the single most fragile moment in any product — the first login attempt by a senior on a new device. Imagine our 67-year-old aunt on her tablet, in bright daylight, typing a 14-character admin password carefully with one finger. She hits "Anmelden." The screen flickers and she's back on the login page. She assumes she mistyped, tries again. Same result. By the third attempt she is convinced "the computer is broken" or — worse — "I broke it." She closes the tab and texts her grandson. That is exactly the failure mode our dual-audience design philosophy is built to prevent: **silent failure with no useful error message and no path forward**. The fix restores the intended flow, so from a UX standpoint, approve. But this near-miss highlights two things worth follow-ups (not blockers on this PR): ### Suggestions for follow-up issues 1. **Login failure feedback.** If the login form ever bounces the user back to itself, the page should say so explicitly with a friendly, localized message ("Anmeldung fehlgeschlagen — bitte erneut versuchen"). Today, the path back to `/login` is silent. Even when this static-HTML bug isn't in play, a session-expired redirect would land the user on `/login` with no explanation. That's worth a small spec — maybe a `?reason=session-expired` query param that the page renders as a `aria-live="polite"` notice. 2. **`/hilfe/transkription` is special — confirm it works without an auth cookie.** This is the one route that survives `crawl: false`, and it's also the route a senior user might be linked to from an email or a printout *before* they ever log in. Add a Playwright check that hits it cold (no cookies, no session) and verifies that the page renders correctly, with all i18n strings present, at the 320px breakpoint. If we ever break that page's public-ness, our outreach to first-time users breaks with it. 3. **Touch-target audit on `/login` itself.** Unrelated to this PR but the same user journey: while we're focused on login, verify the email/password fields and the "Anmelden" button meet the 44×44px floor and 48px preferred for the senior audience. This bug was prod-blocking; the touch-target floor is silent-blocking — same effect on the same user. ### Bottom line Ship the fix. Open the three follow-up issues so the *user experience* around login is as robust as the *infrastructure* around it now will be.

marcel referenced this issue from a commit 2026-05-11 17:01:31 +02:00

test(ci): assert prerender output is only /hilfe/transkription

marcel added 2 commits 2026-05-11 17:01:31 +02:00

fix(frontend): disable prerender crawl so /, /documents, /persons aren't baked ... b2955fb695

Closes #514.

The build was prerendering protected routes via crawl from
/hilfe/transkription. Their load functions throw redirect('/login')
during the build (no auth cookie), so SvelteKit captured the redirect
as static HTML and shipped /app/build/prerendered/{index,documents,
persons,geschichten,stammbaum}.html with a `location.href=/login`
script. In production these files are served BEFORE hooks.server.ts
runs, so an authenticated user with a valid cookie is still served
the baked bounce-back page.

Setting `crawl: false` keeps the explicit /hilfe/transkription entry
prerendered (needed for the public help page) without dragging the
nav targets along with it.

Verified locally: build now emits only `hilfe/transkription.html`
under build/prerendered/, no index.html or documents.html etc.

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

test(ci): assert prerender output is only /hilfe/transkription ... 6ba7254344

Addresses Sara's review request on #515.

Without this gate, a future regression that turns prerender.crawl
back on (or adds a new prerender entry whose nav links into
protected routes) would silently bake /, /documents, /persons etc.
to "redirect-to-login" HTML and re-introduce #514.

Verified the script catches the current broken build state:
  $ find build/prerendered ... -not -path 'hilfe/*' ...
  build/prerendered/{index,documents,persons,geschichten,stammbaum}.html

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

marcel force-pushed fix/issue-514-prerender-crawl-bakes-redirects from 0da768f3a4 to 6ba7254344 2026-05-11 17:01:31 +02:00 Compare

marcel merged commit 6ba7254344 into main 2026-05-11 17:12:03 +02:00

marcel referenced this pull request 2026-05-11 17:25:37 +02:00

bug(user): UserDataInitializer blind-INSERTs Administrators group; fails on retry (HIGH, prod-blocking) #518

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

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

P1-high

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

P2-medium

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

P3-later

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

audit

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

bug

Something isn't working

cleanup

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

collaboration

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

conversation

descoped

We will do that later

devops

documentation

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

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

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

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#515