feat(auth): hooks.server.ts admin role gate (closes #15) #20

Merged

forgejo_admin merged 1 commit from 15-hooks-server-ts into main 2026-05-03 18:49:48 +00:00

Conversation 2 Commits 1 Files changed 2 +299 -1

forgejo_admin commented 2026-05-03 16:18:03 +00:00

Owner

Copy link

Summary

Implements the per-request auth gate for westside-admin. This is sub-task 2 of 4 from #2 (Keycloak cookie SSR auth + admin role gate) and the consumer of #14's frozen keycloak.ts lib.

src/hooks.server.ts exports a single handle that authenticates every non-excluded request via the encrypted westside_admin_session cookie, populates event.locals.user from the verified JWT, lazy-refreshes tokens, and enforces realm role admin — rewriting (not redirecting) authenticated-but-not-admin users to a (unauthorized) route group path so they stay at the URL they hit and see a 403.

Closes #15.

Changes

• src/hooks.server.ts (new) — handle export only (handleError is out of scope per #15).

  • Reads westside_admin_session via event.cookies.get.
  • Decrypts via decryptCookiePayload (#14 — returns null on tamper/wrong-key/malformed, never throws).
  • Validates structural shape into KeycloakTokens before passing to lib.
  • Lazy refresh via refreshTokensIfNeeded (#14 — only fires when exp - now < 30s).
  • On refresh, re-encrypts and event.cookies.set(...) with HttpOnly; Secure; SameSite=Lax; Path=/; Max-Age=<exp-now>.
  • Verifies the (possibly refreshed) access token via verifyKeycloakJwt.
  • Materializes event.locals.user from sub, email, name (falls back to preferred_username), and realm_access.roles.
  • Enforces roles.includes('admin'). Missing-admin → internal rewrite via event.url.pathname = UNAUTHORIZED_PATH.
  • All failure modes (no cookie, tampered, malformed payload, TokenRefreshError, JWT verify-fail) treat the request as anonymous and throw redirect(302, '/auth/login?redirect=<original>'). TokenRefreshError additionally calls event.cookies.delete(SESSION_COOKIE, { path: '/' }). Never 5xx.
  • No console.* statements anywhere in the file — verified by grep. Zero risk of leaking the JWT, refresh token, cookie ciphertext, or COOKIE_SIGNING_KEY.

• src/app.d.ts (updated) — extends App.Locals with user: { sub: string; email: string; name: string; roles: string[] }. Strict shape, no any casts. Documents the claim mapping back to Keycloak realm-token convention.

Hard scope respected: did not touch src/lib/server/keycloak.ts, src/lib/server/keycloak.test.ts, src/routes/auth/*, src/routes/(unauthorized)/*, or src/routes/health/*.

Test Plan

Automated (run in this PR):

• npm run check — 0 errors, 0 warnings, 337 files (TypeScript strict + svelte-check).
• npm test — 25/25 passing (existing keycloak.test.ts — no regressions, no new tests added because hooks are an integration boundary; #15 explicitly defers e2e to when sub-tasks #16 + #17 land).
• eslint src/hooks.server.ts src/app.d.ts — clean.
• prettier --check src/hooks.server.ts src/app.d.ts — clean.
• npm run build — adapter-node build succeeds; SvelteKit wires hooks.server.js chunk (10.53 kB).

Manual integration (deferred until #16 + #17 land — required by #15's test expectations):

• Hit / with no cookie → expect 302 to /auth/login?redirect=%2F.
• Hit / with a tampered cookie → expect 302 to /auth/login (treated as anonymous, cookie cleared).
• Hit /health with no cookie → expect 200 from existing health handler (probe pass-through).
• Hit /auth/login with no cookie → expect pass-through to #16's handler (no 302 loop).
• With valid cookie + admin role → expect 200, page renders, locals.user populated.
• With valid cookie + non-admin → expect 200 but body is the (unauthorized) page; URL stays at the original path.
• Refresh path: forge a cookie with exp = now + 10s (within 30s threshold) and a real refresh token → expect new Set-Cookie header on response, request still served.
• TokenRefreshError path: forge a cookie whose refresh_token Keycloak rejects → expect 302 to /auth/login with Set-Cookie: westside_admin_session=; Max-Age=0.

Review Checklist

• PR opened against main.
• No console.* in src/hooks.server.ts (grep verified — zero log statements emit token material).
• event.locals.user typed end-to-end (no any casts in public types).
• Cookie name is exactly westside_admin_session (matches #14's expectation and #2's spec).
• Imports use $lib/server/keycloak style (matches #14).
• keycloak.ts not modified (frozen at #14 merge).
• No new keycloak.ts exports requested — only consumes decryptCookiePayload, encryptCookiePayload, refreshTokensIfNeeded, verifyKeycloakJwt, TokenRefreshError, KeycloakTokens.
• TokenRefreshError handled per AC: clear cookie + 302, never 5xx.
• Missing-admin uses event.url.pathname = ... rewrite (NOT a 302 — matches #15 constraint).
• /health exclusion is in place (k8s probe pass-through preserved per pal-e-deployments/overlays/westside-admin/prod/).
• PR body includes funnel-auth review (below) per feedback_funnel_requires_auth.

Funnel-Auth Review (per feedback_funnel_requires_auth)

westside-admin is fronted by a Tailscale Funnel ingress (verified in pal-e-deployments/overlays/westside-admin/prod/), which exposes the service to the public internet. Per the 2026-04-10 PII-leak post-mortem, every funnel-exposed ingress requires verified, documented auth. This PR is the funnel-auth enforcement point.

How the gate works. The handle hook runs once per request inside the SvelteKit server. By default it requires:

1. A valid AES-256-GCM westside_admin_session cookie that decrypts to a KeycloakTokens shape, AND
2. The embedded access token verifies against Keycloak's JWKS (signature, issuer, audience, expiry — all validated by verifyKeycloakJwt in #14), AND
3. The verified payload's realm_access.roles includes admin.

Any failure on (1) or (2) results in throw redirect(302, '/auth/login?redirect=<original>') — not a 5xx, not a passthrough. Failure on (3) results in an internal rewrite to UNAUTHORIZED_PATH (no 302) so the user sees a 403 page at the URL they hit. The gate is default-deny: every route is protected unless explicitly excluded.

Excluded paths and why each exclusion is bounded.

• /auth/* — sub-task #16's OIDC endpoints (/auth/login, /auth/callback, /auth/logout). These cannot themselves require an existing session because they establish the session. The exclusion is bounded by URL prefix (pathname.startsWith('/auth/') plus exact '/auth') and these endpoints will be reviewed against their own funnel-auth contract in #16's PR (state validation, code exchange, SLO). They expose only the OIDC dance; no application data.

• /health — k8s liveness/readiness probes. Probes cannot authenticate; the probe pod has no Keycloak credentials. Per pal-e-deployments/overlays/westside-admin/prod/deployment-patch.yaml, the probes hit /health directly. Blocking this would break the deployment's probe gating and crash-loop the pod. The exclusion is bounded by exact match on /health plus prefix on /health/ for any future sub-paths; the existing handler at src/routes/health/+server.ts returns only {"status":"ok"} — zero PII, zero state, zero auth bypass.

• The (unauthorized) SvelteKit route group (#17) is not in the URL-pattern exclusion list. SvelteKit route groups produce no URL prefix, so URL-level exclusion is dead code. The route is reached only via internal rewrite from this hook (after authentication succeeds and the admin check fails). An anonymous user can never observe the unauthorized page because they 302 to login first. This means there is no path by which an unauthenticated request can reach a non-/auth/* non-/health rendered page.

No data egress on the unauthenticated path. The redirect to /auth/login carries only the original path + search string in a URL-encoded redirect query param. No cookies, no headers, no JWTs are echoed in the response. The 302 response itself sets no cookies (we use cookies.delete only when clearing a known-bad session, which strips the Set-Cookie value).

No tokens in logs. Verified by grep -n 'console\.' src/hooks.server.ts returning empty. All error paths surface typed exceptions (caught silently for the JWT-verify case, propagated for unexpected refresh errors so SvelteKit's handleError sees them — but not the token material).

Coordination Notes

The (unauthorized) rewrite target is /__unauthorized (double-leading-underscore convention, signals "hook-managed internal route"). Sub-task #17 owns src/routes/(unauthorized)/+page.svelte per its issue body. Forward-compat note: because SvelteKit route groups produce no URL prefix, src/routes/(unauthorized)/+page.svelte would resolve to /, colliding with the existing root page. #17 will need to land its 403 page at a nested path inside the group (e.g., src/routes/(unauthorized)/__unauthorized/+page.svelte) so the URL matches UNAUTHORIZED_PATH in this hook. If #17 settles on a different path, change UNAUTHORIZED_PATH here in lockstep — it is a single-line constant at the top of the hook.

Related Notes

• Closes forgejo_admin/westside-admin#15
• Parent: forgejo_admin/westside-admin#2
• Sibling sub-task (merged): forgejo_admin/westside-admin#14 — provides the keycloak.ts lib this PR consumes (frozen, not modified).
• Sibling sub-tasks not yet shipped (runtime dependencies for the redirect/rewrite targets):
  • forgejo_admin/westside-admin#16 — /auth/login, /auth/callback, /auth/logout endpoints.
  • forgejo_admin/westside-admin#17 — (unauthorized) 403 page.
• arch-dataflow-westside-admin Flow 1
• feedback_funnel_requires_auth

## Summary Implements the per-request auth gate for `westside-admin`. This is sub-task 2 of 4 from `#2` (Keycloak cookie SSR auth + admin role gate) and the consumer of `#14`'s frozen `keycloak.ts` lib. `src/hooks.server.ts` exports a single `handle` that authenticates every non-excluded request via the encrypted `westside_admin_session` cookie, populates `event.locals.user` from the verified JWT, lazy-refreshes tokens, and enforces realm role `admin` — rewriting (not redirecting) authenticated-but-not-admin users to a `(unauthorized)` route group path so they stay at the URL they hit and see a 403. `Closes #15`. ## Changes - **`src/hooks.server.ts`** (new) — `handle` export only (`handleError` is out of scope per #15). - Reads `westside_admin_session` via `event.cookies.get`. - Decrypts via `decryptCookiePayload` (#14 — returns `null` on tamper/wrong-key/malformed, never throws). - Validates structural shape into `KeycloakTokens` before passing to lib. - Lazy refresh via `refreshTokensIfNeeded` (#14 — only fires when `exp - now < 30s`). - On refresh, re-encrypts and `event.cookies.set(...)` with `HttpOnly; Secure; SameSite=Lax; Path=/; Max-Age=<exp-now>`. - Verifies the (possibly refreshed) access token via `verifyKeycloakJwt`. - Materializes `event.locals.user` from `sub`, `email`, `name` (falls back to `preferred_username`), and `realm_access.roles`. - Enforces `roles.includes('admin')`. Missing-admin → internal rewrite via `event.url.pathname = UNAUTHORIZED_PATH`. - All failure modes (no cookie, tampered, malformed payload, `TokenRefreshError`, JWT verify-fail) treat the request as anonymous and `throw redirect(302, '/auth/login?redirect=<original>')`. `TokenRefreshError` additionally calls `event.cookies.delete(SESSION_COOKIE, { path: '/' })`. **Never 5xx.** - **No `console.*` statements** anywhere in the file — verified by grep. Zero risk of leaking the JWT, refresh token, cookie ciphertext, or `COOKIE_SIGNING_KEY`. - **`src/app.d.ts`** (updated) — extends `App.Locals` with `user: { sub: string; email: string; name: string; roles: string[] }`. Strict shape, no `any` casts. Documents the claim mapping back to Keycloak realm-token convention. Hard scope respected: did **not** touch `src/lib/server/keycloak.ts`, `src/lib/server/keycloak.test.ts`, `src/routes/auth/*`, `src/routes/(unauthorized)/*`, or `src/routes/health/*`. ## Test Plan Automated (run in this PR): - `npm run check` — **0 errors, 0 warnings, 337 files** (TypeScript strict + svelte-check). - `npm test` — **25/25 passing** (existing `keycloak.test.ts` — no regressions, no new tests added because hooks are an integration boundary; #15 explicitly defers e2e to when sub-tasks #16 + #17 land). - `eslint src/hooks.server.ts src/app.d.ts` — clean. - `prettier --check src/hooks.server.ts src/app.d.ts` — clean. - `npm run build` — adapter-node build succeeds; SvelteKit wires `hooks.server.js` chunk (10.53 kB). Manual integration (deferred until #16 + #17 land — required by #15's test expectations): - [ ] Hit `/` with no cookie → expect 302 to `/auth/login?redirect=%2F`. - [ ] Hit `/` with a tampered cookie → expect 302 to `/auth/login` (treated as anonymous, cookie cleared). - [ ] Hit `/health` with no cookie → expect 200 from existing health handler (probe pass-through). - [ ] Hit `/auth/login` with no cookie → expect pass-through to #16's handler (no 302 loop). - [ ] With valid cookie + `admin` role → expect 200, page renders, `locals.user` populated. - [ ] With valid cookie + non-admin → expect 200 but body is the `(unauthorized)` page; URL stays at the original path. - [ ] Refresh path: forge a cookie with `exp = now + 10s` (within 30s threshold) and a real refresh token → expect new Set-Cookie header on response, request still served. - [ ] `TokenRefreshError` path: forge a cookie whose refresh_token Keycloak rejects → expect 302 to `/auth/login` with `Set-Cookie: westside_admin_session=; Max-Age=0`. ## Review Checklist - [x] PR opened against `main`. - [x] No `console.*` in `src/hooks.server.ts` (grep verified — zero log statements emit token material). - [x] `event.locals.user` typed end-to-end (no `any` casts in public types). - [x] Cookie name is exactly `westside_admin_session` (matches #14's expectation and #2's spec). - [x] Imports use `$lib/server/keycloak` style (matches #14). - [x] `keycloak.ts` not modified (frozen at #14 merge). - [x] No new `keycloak.ts` exports requested — only consumes `decryptCookiePayload`, `encryptCookiePayload`, `refreshTokensIfNeeded`, `verifyKeycloakJwt`, `TokenRefreshError`, `KeycloakTokens`. - [x] `TokenRefreshError` handled per AC: clear cookie + 302, never 5xx. - [x] Missing-admin uses `event.url.pathname = ...` rewrite (NOT a 302 — matches #15 constraint). - [x] `/health` exclusion is in place (k8s probe pass-through preserved per `pal-e-deployments/overlays/westside-admin/prod/`). - [x] PR body includes funnel-auth review (below) per `feedback_funnel_requires_auth`. ## Funnel-Auth Review (per `feedback_funnel_requires_auth`) `westside-admin` is fronted by a Tailscale Funnel ingress (verified in `pal-e-deployments/overlays/westside-admin/prod/`), which exposes the service to the public internet. Per the 2026-04-10 PII-leak post-mortem, every funnel-exposed ingress requires verified, documented auth. This PR is the funnel-auth enforcement point. **How the gate works.** The `handle` hook runs once per request inside the SvelteKit server. By default it requires: 1. A valid AES-256-GCM `westside_admin_session` cookie that decrypts to a `KeycloakTokens` shape, AND 2. The embedded access token verifies against Keycloak's JWKS (signature, issuer, audience, expiry — all validated by `verifyKeycloakJwt` in #14), AND 3. The verified payload's `realm_access.roles` includes `admin`. Any failure on (1) or (2) results in `throw redirect(302, '/auth/login?redirect=<original>')` — not a 5xx, not a passthrough. Failure on (3) results in an internal rewrite to `UNAUTHORIZED_PATH` (no 302) so the user sees a 403 page at the URL they hit. The gate is **default-deny**: every route is protected unless explicitly excluded. **Excluded paths and why each exclusion is bounded.** - `/auth/*` — sub-task #16's OIDC endpoints (`/auth/login`, `/auth/callback`, `/auth/logout`). These cannot themselves require an existing session because they *establish* the session. The exclusion is bounded by URL prefix (`pathname.startsWith('/auth/')` plus exact `'/auth'`) and these endpoints will be reviewed against their own funnel-auth contract in #16's PR (state validation, code exchange, SLO). They expose only the OIDC dance; no application data. - `/health` — k8s liveness/readiness probes. Probes cannot authenticate; the probe pod has no Keycloak credentials. Per `pal-e-deployments/overlays/westside-admin/prod/deployment-patch.yaml`, the probes hit `/health` directly. Blocking this would break the deployment's probe gating and crash-loop the pod. The exclusion is bounded by exact match on `/health` plus prefix on `/health/` for any future sub-paths; the existing handler at `src/routes/health/+server.ts` returns only `{"status":"ok"}` — zero PII, zero state, zero auth bypass. - The `(unauthorized)` SvelteKit route group (#17) is **not** in the URL-pattern exclusion list. SvelteKit route groups produce no URL prefix, so URL-level exclusion is dead code. The route is reached *only* via internal rewrite from this hook (after authentication succeeds and the admin check fails). An anonymous user can never observe the unauthorized page because they 302 to login first. This means there is no path by which an unauthenticated request can reach a non-`/auth/*` non-`/health` rendered page. **No data egress on the unauthenticated path.** The redirect to `/auth/login` carries only the original path + search string in a URL-encoded `redirect` query param. No cookies, no headers, no JWTs are echoed in the response. The 302 response itself sets no cookies (we use `cookies.delete` only when clearing a known-bad session, which strips the Set-Cookie value). **No tokens in logs.** Verified by `grep -n 'console\.' src/hooks.server.ts` returning empty. All error paths surface typed exceptions (caught silently for the JWT-verify case, propagated for unexpected refresh errors so SvelteKit's `handleError` sees them — but not the token material). ## Coordination Notes The `(unauthorized)` rewrite target is `/__unauthorized` (double-leading-underscore convention, signals "hook-managed internal route"). Sub-task #17 owns `src/routes/(unauthorized)/+page.svelte` per its issue body. **Forward-compat note:** because SvelteKit route groups produce no URL prefix, `src/routes/(unauthorized)/+page.svelte` would resolve to `/`, colliding with the existing root page. #17 will need to land its 403 page at a nested path inside the group (e.g., `src/routes/(unauthorized)/__unauthorized/+page.svelte`) so the URL matches `UNAUTHORIZED_PATH` in this hook. If #17 settles on a different path, change `UNAUTHORIZED_PATH` here in lockstep — it is a single-line constant at the top of the hook. ## Related Notes - Closes `forgejo_admin/westside-admin#15` - Parent: `forgejo_admin/westside-admin#2` - Sibling sub-task (merged): `forgejo_admin/westside-admin#14` — provides the `keycloak.ts` lib this PR consumes (frozen, not modified). - Sibling sub-tasks not yet shipped (runtime dependencies for the redirect/rewrite targets): - `forgejo_admin/westside-admin#16` — `/auth/login`, `/auth/callback`, `/auth/logout` endpoints. - `forgejo_admin/westside-admin#17` — `(unauthorized)` 403 page. - `arch-dataflow-westside-admin` Flow 1 - `feedback_funnel_requires_auth`

forgejo_admin added 1 commit 2026-05-03 16:18:03 +00:00

feat(auth): hooks.server.ts admin role gate (closes #15) ... 1b8c6c504b

Per-request auth gate that consumes the keycloak.ts lib from #14:

- src/hooks.server.ts: handle export only. Reads the
  westside_admin_session cookie, decrypts via decryptCookiePayload,
  refreshes via refreshTokensIfNeeded (lazy, exp - now < 30s),
  re-encrypts on refresh with HttpOnly/Secure/SameSite=Lax. Verifies
  the access JWT, materializes event.locals.user from claims, and
  enforces realm role 'admin'. Missing-admin internally rewrites to
  /__unauthorized (no 302, URL preserved). All failure modes (no
  cookie, tampered, malformed, refresh-rejected, JWT verify-fail)
  treat the request as anonymous and 302 to /auth/login with deep-link
  redirect. /health and /auth/* bypass the gate.

- src/app.d.ts: extends App.Locals with a strict
  user: { sub, email, name, roles } shape. No any casts.

No log statement in this hook emits the JWT, refresh token, cookie
ciphertext, or COOKIE_SIGNING_KEY (per #15 AC + #14 invariants).

Verified:
- npm run check       0 errors / 0 warnings / 337 files
- npm test            25/25 (keycloak.test.ts unchanged)
- eslint              clean on the two changed files
- npm run build       SvelteKit wires hooks.server.js chunk

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

forgejo_admin commented 2026-05-03 16:19:12 +00:00

Author

Owner

Copy link

Dev self-review (pre-QA)

Posting findings here so they don't get rediscovered. None are blockers; all are documented in code or PR body. Surfacing for QA visibility.

Verified clean

• npm run check -- 0 errors / 0 warnings / 337 files
• npm test -- 25/25 (keycloak.test.ts unchanged)
• eslint src/hooks.server.ts src/app.d.ts -- clean
• prettier --check src/hooks.server.ts src/app.d.ts -- clean
• npm run build -- adapter-node builds; SvelteKit emits hooks.server.js chunk (10.53 kB)
• grep 'console\.' src/hooks.server.ts -- empty (no token leakage risk)

Knowingly-shipped tradeoffs

1. Cookie Max-Age uses access-token expiry, not refresh-token expiry. Issue #15 AC reads Max-Age=<refresh_expiry_seconds>, but #14's frozen KeycloakTokens interface only exposes exp (access_token expiry); refresh_expires_in from the Keycloak token response is dropped in refreshTokensIfNeeded. I used tokens.exp - now with a 60s floor. Consequence: the cookie expires when the access token does, forcing a fresh login slightly earlier than the refresh window allowed. Trade is "honest staleness" vs "request a new export from frozen lib." I chose to ship without modifying the lib (per agent constraint). If QA wants the spec-literal behavior, the lib needs an addition in a follow-up PR.

2. UNAUTHORIZED_PATH = '/__unauthorized'. SvelteKit route groups (name) produce no URL prefix, so src/routes/(unauthorized)/+page.svelte would resolve to / and collide with the existing root page. I picked /__unauthorized as a hook-internal path that #17 will need to match (e.g. src/routes/(unauthorized)/__unauthorized/+page.svelte). This is coordination scope with #17, called out in the PR body.

3. App.Locals.user is non-optional in the type, but the hook doesn't populate it for /health or /auth/* (excluded paths). Spec explicitly says user: { ... } (non-optional), and the intended consumer is "any normal route the hook lets through" -- those always have user populated. Excluded routes (/health, /auth/*) are plumbing and don't consume event.locals.user. Type contract holds for the application surface; just noting the asymmetry.

4. No vitest tests added in this PR. Per #15 test expectations: "No vitest target — wire this up only when sub-task 4 lands and we have an end-to-end fixture." Test plan in PR body lists the manual ACs for when #16+#17 land.

5. Unexpected error class from refreshTokensIfNeeded is rethrown (not caught as anonymous) so it surfaces to SvelteKit's handleError. The lib only documents TokenRefreshError as throwable; anything else is a bug worth surfacing, not swallowing. AC only mentions TokenRefreshError, so this is consistent with spec.

Hard scope respected

• Did NOT touch: src/lib/server/keycloak.ts, src/lib/server/keycloak.test.ts, src/routes/auth/*, src/routes/(unauthorized)/*, src/routes/health/*.
• Did NOT request any new exports from keycloak.ts.

Ready for QA.

## Dev self-review (pre-QA) Posting findings here so they don't get rediscovered. None are blockers; all are documented in code or PR body. Surfacing for QA visibility. ### Verified clean - `npm run check` -- 0 errors / 0 warnings / 337 files - `npm test` -- 25/25 (keycloak.test.ts unchanged) - `eslint src/hooks.server.ts src/app.d.ts` -- clean - `prettier --check src/hooks.server.ts src/app.d.ts` -- clean - `npm run build` -- adapter-node builds; SvelteKit emits `hooks.server.js` chunk (10.53 kB) - `grep 'console\.' src/hooks.server.ts` -- empty (no token leakage risk) ### Knowingly-shipped tradeoffs 1. **Cookie `Max-Age` uses access-token expiry, not refresh-token expiry.** Issue #15 AC reads `Max-Age=<refresh_expiry_seconds>`, but #14's frozen `KeycloakTokens` interface only exposes `exp` (access_token expiry); `refresh_expires_in` from the Keycloak token response is dropped in `refreshTokensIfNeeded`. I used `tokens.exp - now` with a 60s floor. Consequence: the cookie expires when the access token does, forcing a fresh login slightly earlier than the refresh window allowed. Trade is "honest staleness" vs "request a new export from frozen lib." I chose to ship without modifying the lib (per agent constraint). If QA wants the spec-literal behavior, the lib needs an addition in a follow-up PR. 2. **`UNAUTHORIZED_PATH = '/__unauthorized'`.** SvelteKit route groups `(name)` produce no URL prefix, so `src/routes/(unauthorized)/+page.svelte` would resolve to `/` and collide with the existing root page. I picked `/__unauthorized` as a hook-internal path that #17 will need to match (e.g. `src/routes/(unauthorized)/__unauthorized/+page.svelte`). This is coordination scope with #17, called out in the PR body. 3. **`App.Locals.user` is non-optional in the type, but the hook doesn't populate it for `/health` or `/auth/*` (excluded paths).** Spec explicitly says `user: { ... }` (non-optional), and the intended consumer is "any normal route the hook lets through" -- those always have user populated. Excluded routes (`/health`, `/auth/*`) are plumbing and don't consume `event.locals.user`. Type contract holds for the application surface; just noting the asymmetry. 4. **No vitest tests added in this PR.** Per #15 test expectations: "No vitest target — wire this up only when sub-task 4 lands and we have an end-to-end fixture." Test plan in PR body lists the manual ACs for when #16+#17 land. 5. **Unexpected error class from `refreshTokensIfNeeded` is rethrown** (not caught as anonymous) so it surfaces to SvelteKit's `handleError`. The lib only documents `TokenRefreshError` as throwable; anything else is a bug worth surfacing, not swallowing. AC only mentions `TokenRefreshError`, so this is consistent with spec. ### Hard scope respected - Did NOT touch: `src/lib/server/keycloak.ts`, `src/lib/server/keycloak.test.ts`, `src/routes/auth/*`, `src/routes/(unauthorized)/*`, `src/routes/health/*`. - Did NOT request any new exports from `keycloak.ts`. Ready for QA.

forgejo_admin referenced this pull request 2026-05-03 16:22:15 +00:00

feat(auth): /auth/login + /callback + /logout endpoints (closes #16) #21

forgejo_admin commented 2026-05-03 16:23:03 +00:00

Author

Owner

Copy link

PR #20 Review

DOMAIN REVIEW

Tech stack identified: TypeScript / SvelteKit server hooks / OIDC session lifecycle / k8s probe interaction. This is the funnel-auth enforcement point for westside-admin, fronted by a Tailscale Funnel — feedback_funnel_requires_auth applies in full force.

Funnel-auth gate behavior — verified.

The hook is default-deny with two URL-pattern exclusions (/auth/*, /health / /health/*) and one rewrite target (/__unauthorized). I verified each path against ground truth on main:

• /auth/* exclusion is bounded (exact /auth plus prefix /auth/) and necessary — sub-task #16's OIDC endpoints establish the session, so they can't themselves require one.
• /health exclusion is bounded (exact /health plus prefix /health/) and necessary — src/routes/health/+server.ts returns {"status":"ok"} only, no PII, no state, no auth bypass surface. k8s liveness/readiness probes (per pal-e-deployments/overlays/westside-admin/prod/) cannot authenticate.
• (unauthorized) route group is correctly NOT in the URL-pattern allowlist. Confirmed via SvelteKit semantics: route groups produce no URL prefix. Anonymous users 302 to /auth/login before they can ever observe the rewrite target.

Failure-mode contract — verified against the lib's frozen surface.

Cross-checked every claim against src/lib/server/keycloak.ts on main (post-#18 merge):

Failure path	Lib contract	Hook behavior	Verdict
No cookie	n/a	302 to /auth/login?redirect=<original>	✓
Tampered/wrong-key/malformed cookie	decryptCookiePayload returns null (line 358, never throws)	delete cookie + 302	✓
Structurally invalid payload	n/a	asKeycloakTokens returns null → delete + 302	✓
TokenRefreshError	Thrown by refreshTokensIfNeeded on non-2xx (line 525)	Caught explicitly + delete + 302, never 5xx	✓
Other refresh errors	Lib documents only TokenRefreshError from this fn	Rethrown to SvelteKit handleError	✓ correct — bug-surfacing, not swallowing
JWT verify fails	verifyKeycloakJwt throws JwtVerificationError or JwksUnreachableError (line 283)	Catch-all → delete + 302	✓
Valid + admin	n/a	resolve(event) with locals.user populated	✓
Valid + missing admin	n/a	event.url.pathname = '/__unauthorized' (rewrite, NOT 302)	✓ — matches AC #3

JWT/token logging — verified clean. Diff contains zero console.* statements in src/hooks.server.ts. No risk of leaking JWT, refresh token, cookie ciphertext, or COOKIE_SIGNING_KEY. The lib itself has one sanitized JWKS-unreachable warning (line 249) which is the expected source for that signal — the hook does not duplicate.

TypeScript strict — verified. No any casts in App.Locals.user. The internal as Record<string, unknown> cast (buildUser(verified.payload as Record<string, unknown>)) is structurally a narrow over JWTPayload (which already declares [propName: string]: unknown). Defensible. RealmAccess inline cast same story.

Out-of-bounds files — verified untouched. Diff is exactly src/hooks.server.ts (new, 269 lines) + src/app.d.ts (4-line edit: -1 +35 with comment block). src/lib/server/keycloak.ts, src/routes/auth/*, src/routes/(unauthorized)/*, src/routes/health/* all confirmed unmodified.

---

Scrutiny on Flagged Deviations

Deviation 1: Max-Age derived from access-token exp rather than refresh_expires_in.

I read the frozen KeycloakTokens interface at src/lib/server/keycloak.ts:476-481. It exposes exactly { access_token, refresh_token, exp } — no refresh_expires_in. The lib's RefreshResponseBody (line 483) receives Keycloak's response which DOES include refresh_expires_in via the [k: string]: unknown catch-all, but it's dropped on the way into the typed KeycloakTokens shape returned at line 532-536. The dev's claim that the frozen lib doesn't expose refresh_expires_in is factually correct.

However, the dev's choice has a real UX consequence I want explicit about: with Keycloak realm defaults (5min access token / 30min refresh token), the cookie's Max-Age is currently ~5min. The browser discards the cookie at access-token expiry and the user is bounced to /auth/login — even though the refresh token was still valid for another 25 minutes. This violates the spirit of AC "lazy refresh tokens server-side when access_token within 30s of expiry" because the cookie is gone before the server ever reaches the refresh path. The 30s refresh-window logic in the lib only fires for requests still in-flight at the boundary; cross-request refresh is dead weight under this cookie policy.

Available paths the dev did not take:

1. Inline-decode the refresh JWT. Keycloak issues refresh tokens as JWTs by default. The hook could JSON.parse(Buffer.from(refresh_token.split('.')[1], 'base64url').toString('utf8')).exp to read the refresh expiry without modifying the lib. This is a mild layering violation (the OIDC spec treats refresh tokens as opaque to the client) but solves the UX issue without expanding the frozen interface.
2. Extend KeycloakTokens with refresh_exp in a follow-up PR. The lib's refreshTokensIfNeeded already has expires_in for access; adding refresh_expires_in mapping is one line. Not in scope here.

Verdict on Deviation 1: APPROVABLE because (a) the frozen-lib constraint is the right cross-PR contract per the #14 freeze, (b) the dev's "safe non-leaking" reasoning is defensible — refresh_expires_in isn't strictly OIDC-defined, only Keycloak-defined, (c) the inline-JWT-decode workaround is itself a layering violation worth a separate decision, not a same-PR retrofit. REQUIRED FOLLOW-UP TICKET (must be filed before merge): "Extend KeycloakTokens to surface refresh_exp; update hooks.server.ts cookie Max-Age to refresh-token expiry." Reference this PR in the body. Until that ticket lands, accept the 5-minute cookie lifespan as a known degradation documented in the PR.

Deviation 2: UNAUTHORIZED_PATH = '/__unauthorized'.

Verified: SvelteKit route groups (parens) produce no URL prefix. src/routes/(unauthorized)/+page.svelte would resolve to / and collide with the existing home page (/home/ldraney/westside-admin/src/routes/+page.svelte — confirmed exists, content is the scaffold lede). The dev's /__unauthorized synthetic path with the documented "double-leading-underscore = hook-managed internal route" convention is sensible.

Alternative considered: locals.unauthorized flag set by a +layout.server.ts and conditionally rendered. Cleaner per pure SvelteKit idiom — no synthetic path to coordinate. BUT requires every current and future +page.server.ts to know about and respect the flag, which is the opposite of the centralized default-deny gate this PR is implementing. URL-rewrite is the more centralized choice. Dev's call is defensible.

Verdict on Deviation 2: APPROVED. The Coordination Notes section in the PR body correctly documents that #17's route file must land at src/routes/(unauthorized)/__unauthorized/+page.svelte (nested inside the group) so the URL matches. REQUIRED COORDINATION ACTION (must happen before #17 dispatches): Update issue #17's body to specify the exact file path src/routes/(unauthorized)/__unauthorized/+page.svelte and the URL /__unauthorized it must match. Without this, #17's dev agent will likely place the page at (unauthorized)/+page.svelte and the rewrite target will 404.

---

BLOCKERS

None. All BLOCKER criteria from the QA SOP cleared:

• New functionality has documented test plan; vitest skip is justified by the integration boundary + #15 explicit deferral. Existing 25/25 keycloak.test.ts continues to pass per dev's claim (matches main's test count post-#18).
• No unvalidated user input — every input passes through decryptCookiePayload (null-on-tamper) and asKeycloakTokens (structural narrow) before any lib call.
• No secrets in code — only the cookie-name constant westside_admin_session, which is part of the cross-PR contract.
• No DRY violation in auth/security paths — single handle function, no duplication of decrypt/refresh/verify logic.

NITS

1. Cookie Max-Age follow-up ticket REQUIRED before merge (see Deviation 1 above). File against forgejo_admin/westside-admin, reference this PR. Body should propose: extend KeycloakTokens to surface refresh_exp, update hook cookie Max-Age accordingly. Without this ticket logged, the 5-minute UX degradation has no exit path.
2. Issue #17 body update REQUIRED before #17 dispatches (see Deviation 2 above). Specify file path src/routes/(unauthorized)/__unauthorized/+page.svelte and URL /__unauthorized. This is not a blocker for #20 merge — it's a coordination requirement for the next sub-task.
3. Cookie Max-Age = Math.max(60, exp - nowSec) — the 60-second floor is sensible for clock-skew tolerance but is unannotated. Consider a comment noting the floor exists for skew handling. Cosmetic.
4. buildUser falls back to '' for missing string claims. Documented in the JSDoc on App.Locals.user. Defensible — UI code can render without optional-chaining. Worth flagging because some future consumer might want null to distinguish "missing" from "empty"; the locked shape forecloses that. Not a change request, just a captured decision.
5. Test count claim: Dev says 25/25. The lib has its own test file, not visible in this diff. Trust-but-verify checkbox: confirm npm test output in CI matches when the PR is rebased green. (Issue #19 — wire vitest into Woodpecker — is open; CI will eventually enforce this automatically.)

SOP COMPLIANCE

• Branch named after issue: 15-hooks-server-ts — matches {issue-number}-{kebab-purpose} convention.
• PR body follows template-pr-body: Summary, Changes, Test Plan, Review Checklist, Related Notes all present.
• PR body includes funnel-auth review per feedback_funnel_requires_auth — present and thorough; addresses 2026-04-10 PII-leak post-mortem requirement; documents how each exclusion is bounded and why.
• OIDC client config compliance per sop-keycloak-client-creation: Hook consumes KEYCLOAK_CLIENT_ID (audience), KEYCLOAK_CLIENT_SECRET (refresh basic auth), KEYCLOAK_URL, KEYCLOAK_REALM, COOKIE_SIGNING_KEY — all via the lib's requireEnv. Confidential client (matches westside-admin pattern in the SOP). State validation is consuming-app responsibility but lives in #16 (/auth/callback), not this PR — correctly out of scope.
• No secrets committed. Verified: only the cookie name constant is in source.
• No scope creep. Diff is exactly the 2 files in #15's File Targets.
• Hard scope respected: keycloak.ts, routes/auth/*, routes/(unauthorized)/*, routes/health/* untouched.
• Related references the project (arch-dataflow-westside-admin Flow 1) and the funnel-auth memory.
• No plan slug needed (kanban-managed, not plan-managed work).

PROCESS OBSERVATIONS

• Deployment frequency impact: Low blast-on-merge — the hook ships behind a route gate (no (unauthorized) page exists yet, so missing-admin requests will 404 until #17 lands; valid-admin requests will work). End-to-end SSO loop only completes after #16 + #17 also ship. The dev correctly defers manual integration ACs to the post-#16/#17 milestone. This is honest and matches the issue body.
• Change failure risk: Maximum theoretical surface (every request gates through this hook, on a public Tailscale funnel) but well-mitigated:
  • Default-deny posture means a bug fails closed (302 to login) not open (allow request).
  • All error paths converge to "treat as anonymous + clear cookie + 302". No 5xx on user-facing paths.
  • The lib does the security-critical work (JWT verify, AES-GCM decrypt, constant-time state); the hook is glue + role gate. Smaller-blast-radius design.
  • Out-of-bounds enforcement is mechanical (file paths in diff) and verified clean.
• Documentation gaps:
  • The 5-minute cookie UX degradation needs a follow-up ticket logged (NIT 1) to track exit.
  • Issue #17 body needs the path-coordination update (NIT 2) before #17 dispatches.
  • Both gaps are coordination items, not code issues — log them as Forgejo issues + board items per feedback_discovered_scope_always_tracked.
• DORA traceability: PR closes #15 (sub-task 2 of 4 from #2). story label / arch label / type label all set per review-1135-2026-05-03's traceability section. arch:keycloak label still has no backing note — same gap as siblings #14, #16, #17. Not a per-ticket blocker; carry-over scope decision for Ava per the review note.

---

VERDICT: APPROVED

Approved with two REQUIRED follow-ups logged before merge:

1. File a Forgejo issue: "Surface refresh_exp from keycloak.ts lib + use for cookie Max-Age" — reference this PR.
2. Update issue #17 body to specify file path src/routes/(unauthorized)/__unauthorized/+page.svelte and URL /__unauthorized matching UNAUTHORIZED_PATH in this hook.

Both are coordination items, not code changes — they do not require this PR to be revised. Code itself is clean, well-tested-where-testable, well-documented, and faithful to the AC drift acknowledged in the issue body.

## PR #20 Review ### DOMAIN REVIEW **Tech stack identified:** TypeScript / SvelteKit server hooks / OIDC session lifecycle / k8s probe interaction. This is the funnel-auth enforcement point for `westside-admin`, fronted by a Tailscale Funnel — `feedback_funnel_requires_auth` applies in full force. **Funnel-auth gate behavior — verified.** The hook is default-deny with two URL-pattern exclusions (`/auth/*`, `/health` / `/health/*`) and one rewrite target (`/__unauthorized`). I verified each path against ground truth on `main`: - `/auth/*` exclusion is bounded (exact `/auth` plus prefix `/auth/`) and necessary — sub-task #16's OIDC endpoints establish the session, so they can't themselves require one. - `/health` exclusion is bounded (exact `/health` plus prefix `/health/`) and necessary — `src/routes/health/+server.ts` returns `{"status":"ok"}` only, no PII, no state, no auth bypass surface. k8s liveness/readiness probes (per `pal-e-deployments/overlays/westside-admin/prod/`) cannot authenticate. - `(unauthorized)` route group is correctly **NOT** in the URL-pattern allowlist. Confirmed via SvelteKit semantics: route groups produce no URL prefix. Anonymous users 302 to `/auth/login` before they can ever observe the rewrite target. **Failure-mode contract — verified against the lib's frozen surface.** Cross-checked every claim against `src/lib/server/keycloak.ts` on `main` (post-#18 merge): | Failure path | Lib contract | Hook behavior | Verdict | |--------------|--------------|---------------|---------| | No cookie | n/a | 302 to `/auth/login?redirect=<original>` | ✓ | | Tampered/wrong-key/malformed cookie | `decryptCookiePayload` returns `null` (line 358, never throws) | `delete` cookie + 302 | ✓ | | Structurally invalid payload | n/a | `asKeycloakTokens` returns `null` → `delete` + 302 | ✓ | | `TokenRefreshError` | Thrown by `refreshTokensIfNeeded` on non-2xx (line 525) | Caught explicitly + `delete` + 302, **never 5xx** | ✓ | | Other refresh errors | Lib documents only `TokenRefreshError` from this fn | Rethrown to SvelteKit `handleError` | ✓ correct — bug-surfacing, not swallowing | | JWT verify fails | `verifyKeycloakJwt` throws `JwtVerificationError` or `JwksUnreachableError` (line 283) | Catch-all → `delete` + 302 | ✓ | | Valid + admin | n/a | `resolve(event)` with `locals.user` populated | ✓ | | Valid + missing admin | n/a | `event.url.pathname = '/__unauthorized'` (rewrite, NOT 302) | ✓ — matches AC #3 | **JWT/token logging — verified clean.** Diff contains zero `console.*` statements in `src/hooks.server.ts`. No risk of leaking JWT, refresh token, cookie ciphertext, or `COOKIE_SIGNING_KEY`. The lib itself has one sanitized JWKS-unreachable warning (line 249) which is the expected source for that signal — the hook does not duplicate. **TypeScript strict — verified.** No `any` casts in `App.Locals.user`. The internal `as Record<string, unknown>` cast (`buildUser(verified.payload as Record<string, unknown>)`) is structurally a narrow over `JWTPayload` (which already declares `[propName: string]: unknown`). Defensible. `RealmAccess` inline cast same story. **Out-of-bounds files — verified untouched.** Diff is exactly `src/hooks.server.ts` (new, 269 lines) + `src/app.d.ts` (4-line edit: -1 +35 with comment block). `src/lib/server/keycloak.ts`, `src/routes/auth/*`, `src/routes/(unauthorized)/*`, `src/routes/health/*` all confirmed unmodified. --- ### Scrutiny on Flagged Deviations **Deviation 1: `Max-Age` derived from access-token `exp` rather than `refresh_expires_in`.** I read the frozen `KeycloakTokens` interface at `src/lib/server/keycloak.ts:476-481`. It exposes exactly `{ access_token, refresh_token, exp }` — no `refresh_expires_in`. The lib's `RefreshResponseBody` (line 483) receives Keycloak's response which DOES include `refresh_expires_in` via the `[k: string]: unknown` catch-all, but it's dropped on the way into the typed `KeycloakTokens` shape returned at line 532-536. **The dev's claim that the frozen lib doesn't expose `refresh_expires_in` is factually correct.** **However**, the dev's choice has a real UX consequence I want explicit about: with Keycloak realm defaults (5min access token / 30min refresh token), the cookie's `Max-Age` is currently ~5min. The browser discards the cookie at access-token expiry and the user is bounced to `/auth/login` — even though the refresh token was still valid for another 25 minutes. This **violates the spirit** of AC "lazy refresh tokens server-side when access_token within 30s of expiry" because the cookie is gone before the server ever reaches the refresh path. The 30s refresh-window logic in the lib only fires for requests still in-flight at the boundary; cross-request refresh is dead weight under this cookie policy. **Available paths the dev did not take:** 1. **Inline-decode the refresh JWT.** Keycloak issues refresh tokens as JWTs by default. The hook could `JSON.parse(Buffer.from(refresh_token.split('.')[1], 'base64url').toString('utf8')).exp` to read the refresh expiry without modifying the lib. This is a mild layering violation (the OIDC spec treats refresh tokens as opaque to the client) but solves the UX issue without expanding the frozen interface. 2. **Extend `KeycloakTokens` with `refresh_exp` in a follow-up PR.** The lib's `refreshTokensIfNeeded` already has `expires_in` for access; adding `refresh_expires_in` mapping is one line. Not in scope here. **Verdict on Deviation 1:** APPROVABLE because (a) the frozen-lib constraint is the right cross-PR contract per the #14 freeze, (b) the dev's "safe non-leaking" reasoning is defensible — `refresh_expires_in` isn't strictly OIDC-defined, only Keycloak-defined, (c) the inline-JWT-decode workaround is itself a layering violation worth a separate decision, not a same-PR retrofit. **REQUIRED FOLLOW-UP TICKET (must be filed before merge):** "Extend `KeycloakTokens` to surface `refresh_exp`; update `hooks.server.ts` cookie `Max-Age` to refresh-token expiry." Reference this PR in the body. Until that ticket lands, accept the 5-minute cookie lifespan as a known degradation documented in the PR. **Deviation 2: `UNAUTHORIZED_PATH = '/__unauthorized'`.** Verified: SvelteKit route groups (parens) produce no URL prefix. `src/routes/(unauthorized)/+page.svelte` would resolve to `/` and collide with the existing home page (`/home/ldraney/westside-admin/src/routes/+page.svelte` — confirmed exists, content is the scaffold lede). The dev's `/__unauthorized` synthetic path with the documented "double-leading-underscore = hook-managed internal route" convention is sensible. **Alternative considered: `locals.unauthorized` flag set by a `+layout.server.ts` and conditionally rendered.** Cleaner per pure SvelteKit idiom — no synthetic path to coordinate. BUT requires every current and future `+page.server.ts` to know about and respect the flag, which is the opposite of the centralized default-deny gate this PR is implementing. URL-rewrite is the more centralized choice. Dev's call is defensible. **Verdict on Deviation 2:** APPROVED. The Coordination Notes section in the PR body correctly documents that #17's route file must land at `src/routes/(unauthorized)/__unauthorized/+page.svelte` (nested inside the group) so the URL matches. **REQUIRED COORDINATION ACTION (must happen before #17 dispatches):** Update issue #17's body to specify the exact file path `src/routes/(unauthorized)/__unauthorized/+page.svelte` and the URL `/__unauthorized` it must match. Without this, #17's dev agent will likely place the page at `(unauthorized)/+page.svelte` and the rewrite target will 404. --- ### BLOCKERS None. All BLOCKER criteria from the QA SOP cleared: - New functionality has documented test plan; vitest skip is justified by the integration boundary + #15 explicit deferral. Existing 25/25 keycloak.test.ts continues to pass per dev's claim (matches main's test count post-#18). - No unvalidated user input — every input passes through `decryptCookiePayload` (null-on-tamper) and `asKeycloakTokens` (structural narrow) before any lib call. - No secrets in code — only the cookie-name constant `westside_admin_session`, which is part of the cross-PR contract. - No DRY violation in auth/security paths — single `handle` function, no duplication of decrypt/refresh/verify logic. ### NITS 1. **Cookie `Max-Age` follow-up ticket REQUIRED before merge** (see Deviation 1 above). File against `forgejo_admin/westside-admin`, reference this PR. Body should propose: extend `KeycloakTokens` to surface `refresh_exp`, update hook cookie `Max-Age` accordingly. Without this ticket logged, the 5-minute UX degradation has no exit path. 2. **Issue #17 body update REQUIRED before #17 dispatches** (see Deviation 2 above). Specify file path `src/routes/(unauthorized)/__unauthorized/+page.svelte` and URL `/__unauthorized`. This is not a blocker for #20 merge — it's a coordination requirement for the next sub-task. 3. **Cookie `Max-Age = Math.max(60, exp - nowSec)`** — the 60-second floor is sensible for clock-skew tolerance but is unannotated. Consider a comment noting the floor exists for skew handling. Cosmetic. 4. **`buildUser` falls back to `''` for missing string claims.** Documented in the JSDoc on `App.Locals.user`. Defensible — UI code can render without optional-chaining. Worth flagging because some future consumer might want `null` to distinguish "missing" from "empty"; the locked shape forecloses that. Not a change request, just a captured decision. 5. **Test count claim:** Dev says `25/25`. The lib has its own test file, not visible in this diff. Trust-but-verify checkbox: confirm `npm test` output in CI matches when the PR is rebased green. (Issue #19 — wire vitest into Woodpecker — is open; CI will eventually enforce this automatically.) ### SOP COMPLIANCE - [x] Branch named after issue: `15-hooks-server-ts` — matches `{issue-number}-{kebab-purpose}` convention. - [x] PR body follows `template-pr-body`: Summary, Changes, Test Plan, Review Checklist, Related Notes all present. - [x] PR body includes funnel-auth review per `feedback_funnel_requires_auth` — present and thorough; addresses 2026-04-10 PII-leak post-mortem requirement; documents how each exclusion is bounded and why. - [x] OIDC client config compliance per `sop-keycloak-client-creation`: Hook consumes `KEYCLOAK_CLIENT_ID` (audience), `KEYCLOAK_CLIENT_SECRET` (refresh basic auth), `KEYCLOAK_URL`, `KEYCLOAK_REALM`, `COOKIE_SIGNING_KEY` — all via the lib's `requireEnv`. Confidential client (matches `westside-admin` pattern in the SOP). State validation is consuming-app responsibility but lives in #16 (`/auth/callback`), not this PR — correctly out of scope. - [x] No secrets committed. Verified: only the cookie name constant is in source. - [x] No scope creep. Diff is exactly the 2 files in #15's File Targets. - [x] Hard scope respected: `keycloak.ts`, `routes/auth/*`, `routes/(unauthorized)/*`, `routes/health/*` untouched. - [x] Related references the project (`arch-dataflow-westside-admin` Flow 1) and the funnel-auth memory. - [x] No plan slug needed (kanban-managed, not plan-managed work). ### PROCESS OBSERVATIONS - **Deployment frequency impact:** Low blast-on-merge — the hook ships behind a route gate (no `(unauthorized)` page exists yet, so missing-admin requests will 404 until #17 lands; valid-admin requests will work). End-to-end SSO loop only completes after #16 + #17 also ship. The dev correctly defers manual integration ACs to the post-#16/#17 milestone. This is honest and matches the issue body. - **Change failure risk:** Maximum theoretical surface (every request gates through this hook, on a public Tailscale funnel) but well-mitigated: - Default-deny posture means a bug fails closed (302 to login) not open (allow request). - All error paths converge to "treat as anonymous + clear cookie + 302". No 5xx on user-facing paths. - The lib does the security-critical work (JWT verify, AES-GCM decrypt, constant-time state); the hook is glue + role gate. Smaller-blast-radius design. - Out-of-bounds enforcement is mechanical (file paths in diff) and verified clean. - **Documentation gaps:** - The 5-minute cookie UX degradation needs a follow-up ticket logged (NIT 1) to track exit. - Issue #17 body needs the path-coordination update (NIT 2) before #17 dispatches. - Both gaps are coordination items, not code issues — log them as Forgejo issues + board items per `feedback_discovered_scope_always_tracked`. - **DORA traceability:** PR closes #15 (sub-task 2 of 4 from #2). story label / arch label / type label all set per `review-1135-2026-05-03`'s traceability section. `arch:keycloak` label still has no backing note — same gap as siblings #14, #16, #17. Not a per-ticket blocker; carry-over scope decision for Ava per the review note. --- ### VERDICT: APPROVED Approved with two REQUIRED follow-ups logged before merge: 1. File a Forgejo issue: "Surface `refresh_exp` from `keycloak.ts` lib + use for cookie `Max-Age`" — reference this PR. 2. Update issue #17 body to specify file path `src/routes/(unauthorized)/__unauthorized/+page.svelte` and URL `/__unauthorized` matching `UNAUTHORIZED_PATH` in this hook. Both are coordination items, not code changes — they do not require this PR to be revised. Code itself is clean, well-tested-where-testable, well-documented, and faithful to the AC drift acknowledged in the issue body.

forgejo_admin referenced this pull request 2026-05-03 16:24:06 +00:00

feat(auth): expose refresh_expires_in via KeycloakTokens so hook cookie Max-Age tracks refresh window #22

forgejo_admin referenced this pull request 2026-05-03 16:24:36 +00:00

(unauthorized) route group: 403 page with sign-out button (consumes #15) #17

forgejo_admin merged commit 0480bd11f9 into main 2026-05-03 18:49:48 +00:00

forgejo_admin deleted branch 15-hooks-server-ts 2026-05-03 18:49:48 +00:00

forgejo_admin referenced this pull request from a commit 2026-05-03 18:49:49 +00:00

feat(auth): hooks.server.ts admin role gate (closes #15) (#20)

forgejo_admin referenced this pull request from a commit 2026-05-03 19:36:08 +00:00

feat(auth): (unauthorized) 403 page (closes #17)

forgejo_admin referenced this pull request 2026-05-03 19:37:03 +00:00

feat(auth): (unauthorized) 403 page (closes #17) #23

forgejo_admin referenced this pull request 2026-05-03 20:29:32 +00:00

Keycloak cookie SSR auth + admin role gate (hooks.server.ts) #2

forgejo_admin referenced this pull request 2026-05-03 20:52:56 +00:00

[BUG] Cookie SSR auth loops infinitely — session cookie value (4719B) exceeds browser per-cookie limit #24

forgejo_admin referenced this pull request 2026-05-03 21:03:33 +00:00

fix(auth): drop id_token from session cookie payload (closes #24) #25

forgejo_admin referenced this pull request 2026-05-03 21:23:29 +00:00

[BUG] verifyKeycloakJwt rejects valid tokens — Keycloak default access token aud=account, not westside-admin #26

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

forgejo_admin/westside-admin!20

No description provided.