Implement auth helpers, code generation, and request hooks #3
Reference in New Issue
Block a user
Delete Branch "%!s()"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Task 3 — Plan reference:
docs/superpowers/plans/2026-05-05-erbstuecke-wannsee.mdUser story: As a developer, I need auth helper functions and SvelteKit hooks so that every request has
locals.familyCodeandlocals.adminpopulated from cookies, and cookie set/clear helpers are available to all routes.Acceptance criteria
src/lib/auth.tsexports:verifyAdminPassword(name, password)— async, reads bcrypt hash fromADMIN_{NAME}_PASSWORD_HASHenv var, returnsfalsefor unknown names (no user enumeration)validateFamilyCode(code)— synchronous DB lookup, returns{ id, displayName }ornullgenerateCode()— returns 8-character string from[A-Z2-9]usingcrypto.randomBytes(NFR-SEC-001)setFamilyCodeCookie,setAdminCookie,clearFamilyCodeCookie,clearAdminCookie— allhttpOnly,sameSite: 'lax'src/hooks.server.tspopulateslocals.familyCodeandlocals.adminon every requestGET /→/galerieiflocals.familyCodeis validGET /admin→/admin/inventariflocals.adminis setFiles to create
src/lib/auth.tssrc/lib/auth.test.tssrc/hooks.server.tsNFR
generateCode()usescrypto.randomBytes— notMath.random()Depends on: #2 | Size: S | Spec: system-design §4
👤 Markus Keller — Application Architect
Observations
admin_sessionvalidation is missing. The plan'shooks.server.tssetsevent.locals.admin = event.cookies.get('admin_session') ?? nulldirectly. This means any string a user manufactures in that cookie becomeslocals.admin. There is no whitelist check againstADMIN_NAMES. The system design spec (§4) explicitly calls out: "Unbekannter Benutzername gibt dieselbe Fehlermeldung wie falsches Passwort" — the same discipline must apply in the hook.verifyAdminPasswordearly-return creates a timing oracle. The plan's implementation returnsfalseimmediately when the name is not inADMIN_NAMES, before ever callingbcrypt.compare. An attacker can distinguish unknown names from wrong passwords purely by response latency. The plan's ownauth.tsfixes this for the DB path, but the guard clause for unknown names reintroduces it.sameSite: 'lax'is used in the plan, but the security persona spec says'strict'. The system design doc §4 specifieshttpOnly Same-Site-Cookie. The plan implementssameSite: 'lax'for both cookies. For a family app served on a subdomain without cross-site link-click requirements,'strict'is the safer default and matches the security spec.302instead of303. The hook redirects withredirect(302, ...)for theGET /andGET /admincases. The pattern throughout the rest of the plan (Form Actions) uses303 See Other, which is the correct SvelteKit idiom for post-redirect patterns. GET-to-GET redirects can stay as302, but using303consistently avoids accidental method-preservation bugs if the route is ever reached via a form submission.locals.adminaccepts raw cookie value — not validated against known admin names. If the admin cookie is present but contains an arbitrary string (e.g. an old/corrupted cookie value),locals.adminwill be truthy and the admin layout guard will pass. The hook must checkADMIN_NAMES.includes(adminSessionValue)before settinglocals.admin.familyCodefieldcodeinApp.Localsis populated from the raw cookie, which is correct. The plan correctly stores the raw code string alongsideidanddisplayNameso the hook can reconstruct the full struct. This is clean.Recommendations
ADMIN_NAMESwhitelist check inhooks.server.tsbefore settinglocals.admin:verifyAdminPassword, perform a dummybcrypt.compareeven for unknown names to eliminate the timing channel:sameSite: 'lax'tosameSite: 'strict'in bothsetFamilyCodeCookieandsetAdminCookieto match the security spec.ADMIN_NAMESfromauth.tsand import it inhooks.server.tsto avoid duplication.Open Decisions (omit if none)
sameSite: 'lax'vs'strict'— The plan uses'lax'; the security persona spec and Nora's guidelines say'strict'.'strict'breaks the?code=link-click flow if the link is shared from an external app (WhatsApp, email) — the cookie won't be sent on the first navigation, so the redirect from gate to/galeriewould fail on the first tap. If family members share invite links via messaging apps,'lax'may be required for the gate-screen?code=auto-login to work. (Raised by: Markus Keller)👤 Felix Brandt — Fullstack Developer
Observations
validateFamilyCodetest relies ongetDb()directly. The test callsgetDb().prepare(...).run()to seed data. This is correct — it uses the same:memory:singleton thatvalidateFamilyCodewill call internally viagetDb(). The_resetForTesting()inbeforeEachensures a fresh DB per test.generateCodetest regex is too broad. The plan's test asserts/^[A-Z0-9]{8}$/but the implementation's charsetCODE_CHARS = 'ABCDEFGHJKLMNPQRSTUVWXYZ23456789'deliberately excludesI,O,0, and1to avoid visual confusion. The test should reflect the actual intended charset, not a superset. A code containingI,O,0, or1would pass the test but violate the NFR-SEC-001 intent (see security spec comment in persona files).generateCodebeyond collision avoidance. The plan tests that 20 calls produce >15 unique values. This is weak — it could pass even with a bad PRNG. The security spec calls for 32^8 ≈ 10^12 combinations; a charset-length test would be more meaningful.setFamilyCodeCookie,setAdminCookie,clearFamilyCodeCookie,clearAdminCookiehave no unit tests in the plan. The acceptance criteria list them as required exports; they should have at least a smoke test verifying they callcookies.set/cookies.deletewith the correct options.hooks.server.tshas no tests in the plan. The hook's redirect logic (GET /→/galerie,GET /admin→/admin/inventar) and thelocalspopulation are untested. These are critical behaviors. SvelteKit hooks can be tested by importinghandleand invoking it with a mock event.locals.familyCodewhen cookie exists but code is invalid. If an old/expired code cookie is present,validateFamilyCodereturnsnullandlocals.familyCodemust benull. The plan's implementation handles this correctly, but there is no test asserting it.display_name→displayNamemapping is done inline invalidateFamilyCode. This is clean and correct — the snake_case DB column is camelCased at the boundary.Recommendations
generateCodetest regex to match the actual charset (noI,O,0,1):Cookiesobject:ADMIN_NAMESfromauth.tssohooks.server.tscan import rather than redeclare it.👤 Nora "NullX" Steiner — Application Security Engineer
Observations
CWE-203 Timing Oracle —
verifyAdminPasswordleaks username existence via early return. The plan's implementation guards withif (!(ADMIN_NAMES as readonly string[]).includes(name)) return falsebefore callingbcrypt.compare. An attacker sending login attempts for unknown usernames vs. known-but-wrong-password usernames will observe a measurable latency difference (~0 ms vs. ~250 ms for WF12). This is a textbook user-enumeration timing channel. The system design spec (§4) explicitly prohibits user enumeration.admin_sessioncookie value is trusted without server-side validation in hooks. The plan'shooks.server.tssetsevent.locals.admin = event.cookies.get('admin_session') ?? null. Any value in that cookie — including attacker-forged strings — becomeslocals.admin. The hook must validate the value against the known admin name whitelist before trusting it. This is the same mistake flagged in Nora's persona spec as the "Wrong: trusting the cookie value directly" antipattern.sameSite: 'lax'instead of'strict'on both cookies. The plan's cookie helpers usesameSite: 'lax'. The security persona spec requires'strict'for both cookies.'lax'permits cookies to be sent on top-level navigations from external origins (e.g., clicking a link from WhatsApp). While this has a use-case for the?code=gate flow, it also means the admin session cookie is sent on external-link navigation — a small but unnecessary exposure for the admin session specifically. At minimum, the admin cookie must be'strict'.No
secure: !devflag on cookies. The plan's cookie helpers set neithersecure: truenor the SvelteKitdev-conditional. In production (HTTPS behind Caddy), cookies withoutSecurecan be sent over HTTP if a downgrade occurs. SvelteKit'sdevimport resolves this cleanly:generateCodecharset excludes confusable characters — correct.CODE_CHARS = 'ABCDEFGHJKLMNPQRSTUVWXYZ23456789'omitsI,O,0,1. This is the right implementation per NFR-SEC-001. The 32-character alphabet over 8 positions gives 32^8 ≈ 1.1 × 10^12 combinations — brute-force infeasible at normal web request rates without rate limiting.No rate limiting on the gate-screen code submission. The issue spec and plan don't mention rate limiting on the
?code=validation or the gate-screen form action. With 10^12 combinations and no rate limiting, the attack window is wide enough that a determined attacker could make meaningful progress over months. For a short-lived family app this is acceptable, but it should be a documented decision, not an oversight.No
SESSION_SECRETstartup guard. The system design and Markus's architecture persona both require a startup crash ifSESSION_SECRETis missing. This is not implemented in Task 3 (it may belong indb.tsorhooks.server.ts). It should be placed inhooks.server.tssince that is the first server-side module loaded per request.Recommendations
secure: !devto both cookie helpers. Importdevfrom'$app/environment'.admin_sessioncookie tosameSite: 'strict'. Family code cookie may stay'lax'if the?code=link-click flow requires it (see Open Decisions).hooks.server.ts:SESSION_SECRETstartup guard inhooks.server.ts:verifyAdminPasswordresponse time for unknown names is not significantly lower than for wrong-password cases (or document the dummy-hash approach in a code comment explaining the threat model).Open Decisions (omit if none)
?code=gate. 10^12 combinations with no throttle is acceptable for a short-lived family app, but it is an undocumented assumption. Options: (a) accept the risk and document it; (b) add SvelteKit middleware that counts failed attempts per IP; (c) rely on Caddy rate-limiting at the proxy level. (Raised by: Nora Steiner)👤 Sara Holt — QA Engineer & Test Strategist
Observations
7 tests are specified in the acceptance criteria; the plan delivers exactly 7. Count:
verifyAdminPassword(4 tests) +validateFamilyCode(2 tests) +generateCode(2 tests from plan, but one is a uniqueness check not in the original 7-count). The count checks out against the AC, but several critical behaviors are untested at the unit level.No test for the stale-cookie path in hooks. The hook must set
locals.familyCode = nullwhen the cookie value exists but is not in the database. This is a correctness requirement (expired/deleted codes must not grant access) with no test coverage.No tests for the redirect behaviors specified in the AC. The AC states: "Hook redirects
GET /→/galerieiflocals.familyCodeis valid" and "Hook redirectsGET /admin→/admin/inventariflocals.adminis set." These behaviors have zero test coverage in the plan.The
generateCodeuniqueness test is statistically weak. Asserting that 20 calls produce >15 unique values proves almost nothing about entropy — a broken PRNG producing sequential outputs would still pass. A charset-membership assertion per character is more meaningful.beforeEachcleanup is incomplete. The test cleansdelete process.env.ADMIN_MARCEL_PASSWORD_HASHbut does not cleanADMIN_RENATE_PASSWORD_HASHorADMIN_BERIT_PASSWORD_HASH. If a test sets those env vars and they bleed into the next test, false positives are possible.No test for
validateFamilyCodewith a code that was deleted. The DB is fresh per test, but there is no test that inserts a code, deletes it, then verifiesvalidateFamilyCodereturnsnull. This edge case is important for the "code invalidation" workflow in the admin.Cookie helper tests are absent.
setFamilyCodeCookie,setAdminCookie,clearFamilyCodeCookie,clearAdminCookieare listed in the AC as required exports but have no tests. At minimum, each should verify the correct cookie name and options are passed.Test names are acceptable but could be more specific. "returns true for correct password" is readable. "returns 8-character string of A-Z and 0-9" is slightly wrong now (the charset excludes some digits and letters) — the name should reflect the actual charset.
Recommendations
beforeEachto delete all three admin env vars:src/hooks.server.test.ts) covering at minimum:locals.familyCode === null,locals.admin === nulllocals.familyCodepopulated withidanddisplayNamelocals.familyCode === nullGET /with valid family code → redirects to/galerieGET /adminwith valid admin cookie → redirects to/admin/inventarvi.fn()mock for theCookiesinterface to verify correct options (httpOnly, sameSite, path, maxAge) per cookie type.Open Decisions (omit if none)
hooks.server.tslogic (redirect behaviors,localspopulation) is currently untested by any file in the plan. Options: (a) addsrc/hooks.server.test.tsas a Vitest unit test that mocks the SvelteKit event; (b) cover these behaviors only via Playwright E2E in a later task. Unit-level is faster and catches regressions earlier, but requires a mocking strategy forevent.cookies. (Raised by: Sara Holt)👤 Tobias Wendt — DevOps & Platform Engineer
Observations
SESSION_SECRETis referenced in the system design but not validated at startup. Task 3 is the first task that loadshooks.server.ts, which is the natural place to guard against a missingSESSION_SECRET. If this guard is omitted, the app will start and serve requests without the secret configured — a silent security misconfiguration. Tobias's persona spec explicitly calls for a startup crash on missingSESSION_SECRET.The three admin password hash env vars (
ADMIN_MARCEL_PASSWORD_HASH,ADMIN_RENATE_PASSWORD_HASH,ADMIN_BERIT_PASSWORD_HASH) are consumed inauth.tsviaprocess.env. This is correct — they come from environment, not from code. The.env.examplementioned in the DevOps persona spec should be created or updated alongside this task so the deployment documentation stays in sync with the code.No
.env.exampleupdate is planned in this task. Task 3 introduces the env var consumption pattern for all auth variables. The.env.examplefile should documentADMIN_MARCEL_PASSWORD_HASH,ADMIN_RENATE_PASSWORD_HASH,ADMIN_BERIT_PASSWORD_HASH, andSESSION_SECRETwith generation instructions. This is a documentation gap that will cause friction during the first deployment.DATABASE_PATHenv var is also consumed bydb.ts(Task 2), not Task 3. The auth module importsgetDb()which readsDATABASE_PATH. The env var chain is:hooks.server.ts→auth.ts→getDb()→process.env.DATABASE_PATH. All of these need to be documented together in.env.example.UPLOAD_DIRis not yet in scope for Task 3 (that's Task 4), but the complete.env.exampleshould be deferred until at least Task 4 lands.Recommendations
SESSION_SECRETstartup guard at the top ofhooks.server.ts(runs once at module-init time, before any request):.env.examplefile alongside Task 3 commit with at minimum:.envis in.gitignorebefore committing this task — the auth credentials env vars make it critical.👤 Leonie Voss — UX Design Lead & Accessibility Strategist
Observations
This task is backend-only — no UI components, no templates, no rendered output.
auth.tsandhooks.server.tsare pure server modules. There are no accessibility or visual concerns to flag in the implementation itself.The redirects defined in the hook directly affect UX flows. Two redirect behaviors are specified:
GET /→/galeriewhenlocals.familyCodeis valid (prevents authenticated family members from seeing the gate screen again)GET /admin→/admin/inventarwhenlocals.adminis set (prevents admins from landing on a dead route)Both are correct from a UX standpoint — users should not be asked to re-authenticate if already authenticated.
Cookie
maxAgevalues affect the re-authentication UX. The plan setsmaxAge: 60 * 60 * 24 * 365(1 year) for family codes and60 * 60 * 24 * 7(7 days) for admin sessions. From a UX perspective, a 1-year family cookie is appropriate — family members should not have to re-enter their code repeatedly across sessions. The 7-day admin session is reasonable for the usage pattern (infrequent admin visits during a short-lived event). Nora's persona spec suggests 8 hours for admin; this is a security vs. UX tradeoff.No feedback mechanism is specified in this task for when a cookie exists but the code has been deleted. If an admin deletes a family code while the member has an active session, the hook will clear
locals.familyCodesilently. The member will be redirected to the gate screen without explanation. A brief error message on the gate screen ("Dein Zugangscode ist nicht mehr gültig") would improve the experience, but this is a concern for the gate-screen task, not Task 3.The
displayNameis carried inlocals.familyCodeand will be available to all downstream route load functions. This enables personalized greetings in the gallery UI ("Hallo, Markus!") without additional DB queries per request. Good architecture for UX.Recommendations
maxAgeshould be 8 hours (Nora's recommendation) or 7 days (plan's value). 8 hours matches the design spec's intent for a shorter-lived admin session; 7 days is more convenient if the admin works across multiple days. This is a human-context decision.Open Decisions (omit if none)
maxAge: 8 hours vs. 7 days — The security spec suggests 8 hours for admin sessions; the plan implements 7 days. If admins (Marcel, Renate, Berit) will work on the inventory across multiple calendar days without re-logging in, 7 days is more convenient. If the session should expire overnight for security, 8 hours is correct. (Raised by: Leonie Voss)👤 Elicit — Requirements Engineer
Observations
All 5 acceptance criteria are implementable as specified. The issue is well-written: user story is clear, file list is explicit, NFRs are referenced with IDs, and the dependency on #2 is noted. Definition of Ready score: high.
AC gap: the "no user enumeration" requirement appears in the spec (system design §4) and in Nora's domain expertise table, but is not listed as an explicit acceptance criterion. The issue says
verifyAdminPassword"returnsfalsefor unknown names (no user enumeration)" but does not state the timing requirement. A timing oracle is still user enumeration — the AC should be strengthened:NFR-SEC-001 references
crypto.randomBytesand[A-Z2-9]charset. The issue body says[A-Z2-9]but both the security persona spec and the plan implementation use'ABCDEFGHJKLMNPQRSTUVWXYZ23456789'— which excludesI,O,0,1(not just limiting toA-Z2-9). The AC charset description is imprecise. The correct description is: "uppercase letters excluding I and O, digits 2–9 (excluding 0 and 1)."The
locals.adminvalidation requirement is implied but not stated. The system design §4 says "hooks.server.ts →locals.admin" but does not explicitly state that the cookie value must be validated against the known admin whitelist. A developer reading only the issue could implement the insecure version (trust cookie directly) and still claim all AC are met.Dependency on #2 is correct.
validateFamilyCodecallsgetDb()which is implemented in Task 2 (db.ts). The dependency is real and correctly noted.Size estimate "S" is reasonable for 3 files with 7 tests, given that the implementation is well-specified in the plan. 1–3 hours of focused work.
Recommendations
[A-Z2-9]to "uppercase A–Z excluding I and O, digits 2–9":hooks.server.tsadmin cookie validation:🗳️ Decision Queue — Action Required
4 decisions need your input before implementation starts.
Cookie SameSite Policy
sameSite: 'lax'vs'strict'for family code cookie — The plan uses'lax'; the security spec says'strict'.'strict'will break the?code=invite-link flow when family members tap a link from WhatsApp or email — the browser won't send the cookie on that first cross-site navigation, causing the auto-login to fail on the first tap (member lands on gate, sees the code pre-filled but not yet authenticated).'lax'fixes the UX but is a small security concession. The admin cookie should be'strict'regardless. (Raised by: Markus Keller, Nora Steiner)Rate Limiting on Code Submission
?code=gate — 32^8 ≈ 10^12 combinations with no throttle. For a short-lived family app this risk is low, but it is currently undocumented. Options: (a) explicitly accept and document the risk; (b) add application-level IP throttling inhooks.server.ts; (c) add a Caddyrate_limitdirective on the host. Option (a) costs nothing and is likely appropriate; options (b)/(c) add complexity. (Raised by: Nora Steiner)Hook Test Strategy
hooks.server.tsvs. defer to E2E — The hook's redirect behaviors andlocalspopulation are currently untested at the unit level. Options: (a) addsrc/hooks.server.test.tsas a Vitest unit test that mocks the SvelteKiteventobject — fast, runs in CI without a browser, requires a small mock harness; (b) cover these behaviors only via Playwright E2E in a later task — less upfront work but slower feedback loop and deferred coverage. (Raised by: Sara Holt)Admin Session Cookie Lifetime
maxAge: 8 hours vs. 7 days for theadmin_sessioncookie — The security persona spec recommends 8 hours (sessions expire overnight); the plan implements 7 days (admins don't re-login across days). If Marcel, Renate, or Berit will work on the inventory across multiple calendar days, 7 days is more convenient. If overnight expiry is acceptable or desired for security hygiene, 8 hours is correct. (Raised by: Leonie Voss)