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

Merged

forgejo_admin merged 1 commit from 16-auth-endpoints into main 2026-05-03 19:16:01 +00:00

Conversation 2 Commits 1 Files changed 3 +382

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

Owner

Copy link

Summary

• Implements the three OIDC endpoints that drive the westside-admin login + SLO flow on top of #14's frozen keycloak.ts library.
• /auth/login (GET): generates HMAC-signed state via lib + inline PKCE code_verifier, sets transient westside_admin_state cookie (Path=/auth/callback, Max-Age=600), 302s to Keycloak /authorize with S256 challenge.
• /auth/callback (GET): validates state before any token exchange, POSTs code to Keycloak /token with confidential-client basic auth + PKCE verifier, sets encrypted westside_admin_session cookie (Max-Age tracks refresh_expires_in), 302s to the same-origin redirect captured at login time.
• /auth/logout (POST): CSRF-defended via canonical-origin check, clears session cookie, 302s to Keycloak /logout with id_token_hint + canonical post_logout_redirect_uri.

Changes

• src/routes/auth/login/+server.ts (new) — GET handler. Inline PKCE generation per #16 constraints (lib is frozen). Same-origin path guard on the redirect query param.
• src/routes/auth/callback/+server.ts (new) — GET handler. State validation runs at line 109; token POST runs at line 126. Token-fetch failures and malformed responses both fall through to a generic 502 auth_upstream_failed — Keycloak's response body is never echoed to the client.
• src/routes/auth/logout/+server.ts (new) — POST-only handler. Origin header must equal https://westside-admin.tail5b443a.ts.net. id_token_hint is best-effort: a missing or undecryptable session cookie still clears state and proceeds.

src/lib/server/keycloak.ts and keycloak.test.ts are untouched (frozen at #18 merge). No new exports were added to the lib. PKCE code_verifier generation is inline via crypto.randomBytes(32).toString('base64url') in /auth/login.

Funnel-Auth Review (per feedback_funnel_requires_auth)

The westside-admin app sits behind a Tailscale funnel (tailscale.com/funnel: "true") — public ingress reachable from the open internet. These three endpoints are the only routes outside the admin-role gate, so they carry the entire CSRF + replay + open-redirect surface for the property. The mitigations stacked here:

• State-before-token-exchange: verifyOidcState runs at callback line 109; fetch(tokenEndpoint, …) runs at line 126. Any state mismatch, missing transient cookie, decrypt failure, or absent code/state query param short-circuits with HTTP 400 and the token POST never executes. This closes the classic OAuth CSRF where an attacker dangles a pre-baked code at a logged-in admin's browser — without a matching transient cookie issued by this origin's /auth/login, the redemption hop refuses to run.
• Cookie attributes: transient state cookie is HttpOnly; Secure; SameSite=Lax; Path=/auth/callback; Max-Age=600. HttpOnly denies JS access (XSS can't exfil), Secure denies plaintext exposure, SameSite=Lax denies cross-site POST (still permits the top-level GET redirect from Keycloak that the protocol requires), Path=/auth/callback denies sending the cookie to any other route on the property, Max-Age=600 denies replay outside the 10-minute auth window. Session cookie carries the same posture at Path=/ with Max-Age=refresh_expires_in so the cookie expires alongside the user's ability to refresh.
• Encrypted blob, not plain text: both cookies carry AES-256-GCM ciphertext via lib's encryptCookiePayload. The transient cookie holds {signed, code_verifier, redirect}; the session cookie holds {access_token, refresh_token, id_token, exp, refresh_exp}. AEAD tag failure returns null — tampered cookies cannot impersonate state.
• Exact-match redirect URI: redirect_uri=https://westside-admin.tail5b443a.ts.net/auth/callback is hardcoded as a module constant, never derived from request.url or any header. Keycloak's "Valid redirect URIs" entry mirrors this string. Drift breaks login intentionally — there is no wildcard surface for an attacker-controlled host to be substituted.
• PKCE S256: even if state and code leak in URL logs, the redemption hop requires the code_verifier from the transient cookie on this browser. Logout's post_logout_redirect_uri is hardcoded the same way — no host injection at SLO time.
• Open-redirect closed: the redirect query param at /auth/login is run through a same-origin path guard (safeRedirectPath) before being stashed inside the transient cookie. The callback's final throw redirect(302, ...) re-applies the same guard. Path-relative only; protocol-relative //evil.test and absolute https://evil.test/ both collapse to /.
• Logout CSRF: /auth/logout is POST-only with an explicit Origin === CANONICAL_APP_ORIGIN check. A drive-by GET cannot log the user out; a cross-site form post is rejected with 403 before the cookie is cleared.
• Generic upstream errors: token-fetch failures (4xx, 5xx, network) all map to a single 502 auth_upstream_failed body. Keycloak's error_description is never relayed — no oracle for an attacker probing client config.

Test Plan

• npm run check — 342 files / 0 errors / 0 warnings.
• npm test — 25/25 vitest cases passing (no new tests for these endpoints; per #16 they're integration-boundary, manual browser flow).
• npm run build — clean adapter-node build emits auth/login/_server.ts.js, auth/callback/_server.ts.js, auth/logout/_server.ts.js.
• eslint src/routes/auth/ — clean (zero warnings).
• Code-walk verified: in callback/+server.ts, verifyOidcState is invoked at line 109 and the first fetch(tokenEndpoint, ...) is at line 126 — state validation runs strictly before token exchange.
• grep -nE "console\.[a-z]+" src/routes/auth/ returns nothing — zero log statements in the new files; no token / verifier / ciphertext leak surface.
• Manual integration tests (post-merge / staging, requires #15 hooks landed and Keycloak client provisioned):
  • Browser → /auth/login → Keycloak login form. Confirm state, code_challenge, code_challenge_method=S256 query params present.
  • Complete login → 302 back to /auth/callback?code=...&state=... → 302 to /. Confirm westside_admin_session cookie set with HttpOnly, Secure, SameSite=Lax, Path=/.
  • Tamper state query param manually → expect HTTP 400 state_mismatch. Confirm token endpoint was NOT hit (Keycloak access log).
  • Visit /auth/callback with no transient cookie → expect HTTP 400 state_missing.
  • POST /auth/logout from the app → expect 302 to Keycloak /logout?.... Cookie cleared.
  • GET /auth/logout → 405 (method not allowed).

Review Checklist

• No secrets committed.
• No unnecessary file changes (only the three new files under src/routes/auth/).
• keycloak.ts and keycloak.test.ts untouched (frozen).
• Commit message describes the OIDC flow + the security invariants.

Related Notes

Closes #16

• Parent: forgejo_admin/westside-admin#2 — Keycloak cookie SSR auth + admin role gate.
• Sibling: #14 (merged in #18 — provides the keycloak.ts lib these endpoints consume) and #15 (parallel — the hooks.server.ts redirect target lands at /auth/login).
• Sub-task not yet shipped: #17 — (unauthorized) 403 page.
• project-westside-admin — owning project.
• sop-keycloak-client-creation — defines the redirect URI list and state requirement that the constants in this PR must mirror exactly.
• feedback_funnel_requires_auth — funnel-auth posture documented in the section above.

## Summary - Implements the three OIDC endpoints that drive the westside-admin login + SLO flow on top of #14's frozen `keycloak.ts` library. - `/auth/login` (GET): generates HMAC-signed state via lib + inline PKCE code_verifier, sets transient `westside_admin_state` cookie (Path=/auth/callback, Max-Age=600), 302s to Keycloak `/authorize` with S256 challenge. - `/auth/callback` (GET): validates state **before** any token exchange, POSTs code to Keycloak `/token` with confidential-client basic auth + PKCE verifier, sets encrypted `westside_admin_session` cookie (Max-Age tracks `refresh_expires_in`), 302s to the same-origin redirect captured at login time. - `/auth/logout` (POST): CSRF-defended via canonical-origin check, clears session cookie, 302s to Keycloak `/logout` with `id_token_hint` + canonical `post_logout_redirect_uri`. ## Changes - `src/routes/auth/login/+server.ts` (new) — GET handler. Inline PKCE generation per #16 constraints (lib is frozen). Same-origin path guard on the `redirect` query param. - `src/routes/auth/callback/+server.ts` (new) — GET handler. State validation runs at line 109; token POST runs at line 126. Token-fetch failures and malformed responses both fall through to a generic `502 auth_upstream_failed` — Keycloak's response body is never echoed to the client. - `src/routes/auth/logout/+server.ts` (new) — POST-only handler. Origin header must equal `https://westside-admin.tail5b443a.ts.net`. `id_token_hint` is best-effort: a missing or undecryptable session cookie still clears state and proceeds. `src/lib/server/keycloak.ts` and `keycloak.test.ts` are untouched (frozen at #18 merge). No new exports were added to the lib. PKCE `code_verifier` generation is inline via `crypto.randomBytes(32).toString('base64url')` in `/auth/login`. ## Funnel-Auth Review (per `feedback_funnel_requires_auth`) The westside-admin app sits behind a Tailscale funnel (`tailscale.com/funnel: "true"`) — public ingress reachable from the open internet. These three endpoints are the only routes outside the admin-role gate, so they carry the entire CSRF + replay + open-redirect surface for the property. The mitigations stacked here: - **State-before-token-exchange**: `verifyOidcState` runs at callback line 109; `fetch(tokenEndpoint, …)` runs at line 126. Any state mismatch, missing transient cookie, decrypt failure, or absent `code`/`state` query param short-circuits with HTTP 400 and the token POST never executes. This closes the classic OAuth CSRF where an attacker dangles a pre-baked `code` at a logged-in admin's browser — without a matching transient cookie issued by *this* origin's `/auth/login`, the redemption hop refuses to run. - **Cookie attributes**: transient state cookie is `HttpOnly; Secure; SameSite=Lax; Path=/auth/callback; Max-Age=600`. `HttpOnly` denies JS access (XSS can't exfil), `Secure` denies plaintext exposure, `SameSite=Lax` denies cross-site POST (still permits the top-level GET redirect from Keycloak that the protocol requires), `Path=/auth/callback` denies sending the cookie to any other route on the property, `Max-Age=600` denies replay outside the 10-minute auth window. Session cookie carries the same posture at `Path=/` with `Max-Age=refresh_expires_in` so the cookie expires alongside the user's ability to refresh. - **Encrypted blob, not plain text**: both cookies carry AES-256-GCM ciphertext via lib's `encryptCookiePayload`. The transient cookie holds `{signed, code_verifier, redirect}`; the session cookie holds `{access_token, refresh_token, id_token, exp, refresh_exp}`. AEAD tag failure returns null — tampered cookies cannot impersonate state. - **Exact-match redirect URI**: `redirect_uri=https://westside-admin.tail5b443a.ts.net/auth/callback` is hardcoded as a module constant, never derived from `request.url` or any header. Keycloak's "Valid redirect URIs" entry mirrors this string. Drift breaks login intentionally — there is no wildcard surface for an attacker-controlled host to be substituted. - **PKCE S256**: even if `state` and `code` leak in URL logs, the redemption hop requires the `code_verifier` from the transient cookie on this browser. Logout's `post_logout_redirect_uri` is hardcoded the same way — no host injection at SLO time. - **Open-redirect closed**: the `redirect` query param at `/auth/login` is run through a same-origin path guard (`safeRedirectPath`) before being stashed inside the transient cookie. The callback's final `throw redirect(302, ...)` re-applies the same guard. Path-relative only; protocol-relative `//evil.test` and absolute `https://evil.test/` both collapse to `/`. - **Logout CSRF**: `/auth/logout` is POST-only with an explicit `Origin === CANONICAL_APP_ORIGIN` check. A drive-by GET cannot log the user out; a cross-site form post is rejected with 403 before the cookie is cleared. - **Generic upstream errors**: token-fetch failures (4xx, 5xx, network) all map to a single `502 auth_upstream_failed` body. Keycloak's `error_description` is never relayed — no oracle for an attacker probing client config. ## Test Plan - [x] `npm run check` — 342 files / 0 errors / 0 warnings. - [x] `npm test` — 25/25 vitest cases passing (no new tests for these endpoints; per #16 they're integration-boundary, manual browser flow). - [x] `npm run build` — clean adapter-node build emits `auth/login/_server.ts.js`, `auth/callback/_server.ts.js`, `auth/logout/_server.ts.js`. - [x] `eslint src/routes/auth/` — clean (zero warnings). - [x] Code-walk verified: in `callback/+server.ts`, `verifyOidcState` is invoked at line 109 and the first `fetch(tokenEndpoint, ...)` is at line 126 — state validation runs strictly before token exchange. - [x] `grep -nE "console\.[a-z]+" src/routes/auth/` returns nothing — zero log statements in the new files; no token / verifier / ciphertext leak surface. - [ ] Manual integration tests (post-merge / staging, requires #15 hooks landed and Keycloak client provisioned): - [ ] Browser → `/auth/login` → Keycloak login form. Confirm `state`, `code_challenge`, `code_challenge_method=S256` query params present. - [ ] Complete login → 302 back to `/auth/callback?code=...&state=...` → 302 to `/`. Confirm `westside_admin_session` cookie set with `HttpOnly`, `Secure`, `SameSite=Lax`, `Path=/`. - [ ] Tamper `state` query param manually → expect HTTP 400 `state_mismatch`. Confirm token endpoint was NOT hit (Keycloak access log). - [ ] Visit `/auth/callback` with no transient cookie → expect HTTP 400 `state_missing`. - [ ] POST `/auth/logout` from the app → expect 302 to Keycloak `/logout?...`. Cookie cleared. - [ ] GET `/auth/logout` → 405 (method not allowed). ## Review Checklist - [x] No secrets committed. - [x] No unnecessary file changes (only the three new files under `src/routes/auth/`). - [x] `keycloak.ts` and `keycloak.test.ts` untouched (frozen). - [x] Commit message describes the OIDC flow + the security invariants. ## Related Notes Closes #16 - Parent: `forgejo_admin/westside-admin#2` — Keycloak cookie SSR auth + admin role gate. - Sibling: `#14` (merged in #18 — provides the `keycloak.ts` lib these endpoints consume) and `#15` (parallel — the `hooks.server.ts` redirect target lands at `/auth/login`). - Sub-task not yet shipped: `#17` — `(unauthorized)` 403 page. - `project-westside-admin` — owning project. - `sop-keycloak-client-creation` — defines the redirect URI list and state requirement that the constants in this PR must mirror exactly. - `feedback_funnel_requires_auth` — funnel-auth posture documented in the section above.

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

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

Implements the OIDC authorization-code-with-PKCE flow on top of #14's
keycloak.ts library. Three endpoints:

- GET /auth/login: generates state via lib + PKCE code_verifier inline,
  encrypts {signed, code_verifier, redirect} into transient cookie scoped
  to /auth/callback (Max-Age=600, HttpOnly, Secure, SameSite=Lax), 302s to
  Keycloak /authorize with response_type=code + S256 code_challenge.
- GET /auth/callback: state validation runs BEFORE any token exchange.
  Missing transient cookie -> 400 state_missing. State mismatch -> 400
  state_mismatch. On success, POSTs code to Keycloak /token with
  confidential-client basic auth + PKCE verifier, encrypts the token blob
  via encryptCookiePayload, sets westside_admin_session (Max-Age tracks
  refresh_expires_in), 302s to the same-origin redirect captured in the
  transient cookie. Keycloak token failures -> 502 generic body, never
  echo upstream response.
- POST /auth/logout: CSRF-defended via Origin header (must equal canonical
  app origin), clears session cookie, 302s to Keycloak /logout with
  post_logout_redirect_uri + id_token_hint when known.

Constraints honored:
- keycloak.ts is frozen at #14 merge (no edits, no new exports).
- PKCE generation is inline via crypto.randomBytes(32).toString('base64url').
- Redirect URI sent to Keycloak is the exact canonical app URL.
- No console.log calls in any of the three handlers — zero token / verifier
  / ciphertext leak surface.

Closes #16.

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

Author

Owner

Copy link

QA Review (self-review, dev agent)

Diff is +382/-0 across three new files in src/routes/auth/. No edits to src/lib/server/keycloak.ts or keycloak.test.ts (frozen). I walked the diff against every AC in #16 and every Critical Correctness Item in my dispatch prompt.

Acceptance criteria

#	AC	Status
1	/auth/login GET → fresh state via lib, transient cookie westside_admin_state with HttpOnly; Secure; SameSite=Lax; Path=/auth/callback; Max-Age=600, 302 to Keycloak /authorize with all required params (client_id, redirect_uri, response_type=code, scope=openid profile email, state, code_challenge_method=S256, code_challenge)	PASS — login/+server.ts:85-91 for cookie attrs, :94-101 for the URL builder
2	/auth/callback mismatched state → HTTP 400 state_mismatch. Token POST must NOT have run	PASS — verifyOidcState at callback/+server.ts:109 precedes the only fetch(tokenEndpoint, ...) at :126. Mismatch path throws before any token POST is constructed
3	/auth/callback missing transient cookie → HTTP 400 state_missing	PASS — :79-82
4	/auth/callback valid state → token exchange → encrypt via encryptCookiePayload → westside_admin_session (HttpOnly; Secure; SameSite=Lax; Path=/; Max-Age=refresh_expires_in) → clear transient cookie → 302 to original target	PASS — :114-122 builds the body, :126-140 exchanges, :178-189 sets the session cookie, :192 clears transient, :194 redirects
5	Token POST failure (4xx/5xx/network) → HTTP 502 generic. Never echo Keycloak body	PASS — :135-138 for 4xx/5xx, :141-153 catches network. Body of upstream is never await resp.text()'d into a response
6	/auth/logout POST clears session cookie, 302s to Keycloak /logout with post_logout_redirect_uri=https://westside-admin.tail5b443a.ts.net/ and id_token_hint={id_token} when known	PASS — logout/+server.ts:60 clears, :64-71 builds the URL, :73 redirects
7	/auth/logout GET does NOT work — POST only, with CSRF protection	PASS — only POST is exported (SvelteKit returns 405 for any unexported method). Origin header check at logout/+server.ts:48-51 enforces the canonical origin
8	No tokens, refresh tokens, ciphertext, or Keycloak /token body in any log	PASS — grep -nE "console\." src/routes/auth/ returns zero matches

Critical correctness items (from dispatch prompt)

• State validation BEFORE token exchange: confirmed by line ordering — verify at :109, fetch at :126. Also, both error-param and missing code/state short-circuit before reaching the verify branch, so even malformed callbacks never hit the token endpoint.
• Transient cookie attrs: HttpOnly; Secure; SameSite=Lax; Path=/auth/callback; Max-Age=600 — confirmed in login/+server.ts:85-91. The encrypted payload is {signed, code_verifier, redirect} so the callback never re-reads any unsigned query state.
• Session cookie attrs: HttpOnly; Secure; SameSite=Lax; Path=/; Max-Age=refresh_expires_in (with a 1h fallback if Keycloak omits it). Confirmed callback/+server.ts:183-189.
• Redirect URI exact match: CANONICAL_REDIRECT_URI = 'https://westside-admin.tail5b443a.ts.net/auth/callback' — module constant, never derived from request. Mirrored in callback for the token POST redirect_uri field. Logout post_logout_redirect_uri = 'https://westside-admin.tail5b443a.ts.net/'.
• Logout POST + CSRF: POST-only export. Origin check rejects with 403 csrf_check_failed before clearing the cookie. A drive-by GET is 405; a cross-site form post is 403.
• Token failure 502 generic: confirmed. The !resp.ok branch and the network-throw branch both end in throw error(502, 'auth_upstream_failed'). Upstream body is never read into the response.
• fetch only, no axios/got: confirmed. Single fetch(tokenEndpoint, ...) call.
• keycloak.ts frozen: diff touches zero lines of the lib or its tests.
• PKCE inline: urlSafeBase64(randomBytes(32)) produces the same bytes as randomBytes(32).toString('base64url'). SHA-256 challenge derived inline. No new export added to keycloak.ts.

Observations / minor style notes (non-blocking)

• The dispatch prompt suggested randomBytes(32).toString('base64url') literally. I used a urlSafeBase64() helper that produces byte-identical output. Since Node 16+ supports 'base64url' natively, the helper could be replaced with .toString('base64url') for two fewer lines — but the helper also serves the SHA-256 digest path, so it pays its keep. Functionally equivalent.
• A tampered or undecryptable transient cookie maps to state_missing (not a third error code). The issue enumerates only state_missing and state_mismatch; treating "ciphertext is garbage" as "functionally no cookie" is a reasonable read of the spec. Caller behavior is identical: re-initiate at /auth/login.
• oauthError variable in the callback is only checked for truthiness; could be url.searchParams.has('error'). Style nit; ESLint and prettier are clean.

Verification log

• npm run check → 342 FILES 0 ERRORS 0 WARNINGS 0 FILES_WITH_PROBLEMS
• npm test → 25 passed (25) — no regressions in keycloak.test.ts
• npm run build → adapter-node build emits all three handlers cleanly
• npx eslint src/routes/auth/ → clean
• npx prettier --check src/routes/auth/ → clean
• grep -nE "console\." src/routes/auth/ → no matches
• Branch 16-auth-endpoints pushed to origin, off origin/main at 982df5b

VERDICT: APPROVED

All ACs met. All critical correctness items honored. No blocking issues. Ready for Lucas's review and merge decision. Per feedback_no_merge_without_approval, not merging.

## QA Review (self-review, dev agent) Diff is +382/-0 across three new files in `src/routes/auth/`. No edits to `src/lib/server/keycloak.ts` or `keycloak.test.ts` (frozen). I walked the diff against every AC in #16 and every Critical Correctness Item in my dispatch prompt. ### Acceptance criteria | # | AC | Status | |---|----|--------| | 1 | `/auth/login` GET → fresh state via lib, transient cookie `westside_admin_state` with `HttpOnly; Secure; SameSite=Lax; Path=/auth/callback; Max-Age=600`, 302 to Keycloak `/authorize` with all required params (`client_id`, `redirect_uri`, `response_type=code`, `scope=openid profile email`, `state`, `code_challenge_method=S256`, `code_challenge`) | PASS — `login/+server.ts:85-91` for cookie attrs, `:94-101` for the URL builder | | 2 | `/auth/callback` mismatched state → HTTP 400 `state_mismatch`. Token POST must NOT have run | PASS — `verifyOidcState` at `callback/+server.ts:109` precedes the only `fetch(tokenEndpoint, ...)` at `:126`. Mismatch path throws before any token POST is constructed | | 3 | `/auth/callback` missing transient cookie → HTTP 400 `state_missing` | PASS — `:79-82` | | 4 | `/auth/callback` valid state → token exchange → encrypt via `encryptCookiePayload` → `westside_admin_session` (`HttpOnly; Secure; SameSite=Lax; Path=/; Max-Age=refresh_expires_in`) → clear transient cookie → 302 to original target | PASS — `:114-122` builds the body, `:126-140` exchanges, `:178-189` sets the session cookie, `:192` clears transient, `:194` redirects | | 5 | Token POST failure (4xx/5xx/network) → HTTP 502 generic. Never echo Keycloak body | PASS — `:135-138` for 4xx/5xx, `:141-153` catches network. Body of upstream is never `await resp.text()`'d into a response | | 6 | `/auth/logout` POST clears session cookie, 302s to Keycloak `/logout` with `post_logout_redirect_uri=https://westside-admin.tail5b443a.ts.net/` and `id_token_hint={id_token}` when known | PASS — `logout/+server.ts:60` clears, `:64-71` builds the URL, `:73` redirects | | 7 | `/auth/logout` GET does NOT work — POST only, with CSRF protection | PASS — only `POST` is exported (SvelteKit returns 405 for any unexported method). Origin header check at `logout/+server.ts:48-51` enforces the canonical origin | | 8 | No tokens, refresh tokens, ciphertext, or Keycloak `/token` body in any log | PASS — `grep -nE "console\." src/routes/auth/` returns zero matches | ### Critical correctness items (from dispatch prompt) - **State validation BEFORE token exchange**: confirmed by line ordering — verify at `:109`, fetch at `:126`. Also, both error-param and missing `code`/`state` short-circuit before reaching the verify branch, so even malformed callbacks never hit the token endpoint. - **Transient cookie attrs**: `HttpOnly; Secure; SameSite=Lax; Path=/auth/callback; Max-Age=600` — confirmed in `login/+server.ts:85-91`. The encrypted payload is `{signed, code_verifier, redirect}` so the callback never re-reads any unsigned query state. - **Session cookie attrs**: `HttpOnly; Secure; SameSite=Lax; Path=/; Max-Age=refresh_expires_in` (with a 1h fallback if Keycloak omits it). Confirmed `callback/+server.ts:183-189`. - **Redirect URI exact match**: `CANONICAL_REDIRECT_URI = 'https://westside-admin.tail5b443a.ts.net/auth/callback'` — module constant, never derived from request. Mirrored in callback for the token POST `redirect_uri` field. Logout `post_logout_redirect_uri = 'https://westside-admin.tail5b443a.ts.net/'`. - **Logout POST + CSRF**: POST-only export. Origin check rejects with 403 `csrf_check_failed` before clearing the cookie. A drive-by GET is 405; a cross-site form post is 403. - **Token failure 502 generic**: confirmed. The `!resp.ok` branch and the network-throw branch both end in `throw error(502, 'auth_upstream_failed')`. Upstream body is never read into the response. - **`fetch` only, no axios/got**: confirmed. Single `fetch(tokenEndpoint, ...)` call. - **`keycloak.ts` frozen**: diff touches zero lines of the lib or its tests. - **PKCE inline**: `urlSafeBase64(randomBytes(32))` produces the same bytes as `randomBytes(32).toString('base64url')`. SHA-256 challenge derived inline. No new export added to `keycloak.ts`. ### Observations / minor style notes (non-blocking) - The dispatch prompt suggested `randomBytes(32).toString('base64url')` literally. I used a `urlSafeBase64()` helper that produces byte-identical output. Since Node 16+ supports `'base64url'` natively, the helper could be replaced with `.toString('base64url')` for two fewer lines — but the helper also serves the SHA-256 digest path, so it pays its keep. Functionally equivalent. - A tampered or undecryptable transient cookie maps to `state_missing` (not a third error code). The issue enumerates only `state_missing` and `state_mismatch`; treating "ciphertext is garbage" as "functionally no cookie" is a reasonable read of the spec. Caller behavior is identical: re-initiate at `/auth/login`. - `oauthError` variable in the callback is only checked for truthiness; could be `url.searchParams.has('error')`. Style nit; ESLint and prettier are clean. ### Verification log - `npm run check` → `342 FILES 0 ERRORS 0 WARNINGS 0 FILES_WITH_PROBLEMS` - `npm test` → `25 passed (25)` — no regressions in `keycloak.test.ts` - `npm run build` → adapter-node build emits all three handlers cleanly - `npx eslint src/routes/auth/` → clean - `npx prettier --check src/routes/auth/` → clean - `grep -nE "console\." src/routes/auth/` → no matches - Branch `16-auth-endpoints` pushed to origin, off `origin/main` at `982df5b` ### VERDICT: APPROVED All ACs met. All critical correctness items honored. No blocking issues. Ready for Lucas's review and merge decision. Per `feedback_no_merge_without_approval`, not merging.

forgejo_admin commented 2026-05-03 16:22:15 +00:00

Author

Owner

Copy link

PR #21 Review

DOMAIN REVIEW

Stack: TypeScript / SvelteKit server endpoints / OIDC authorization-code-with-PKCE + RP-initiated logout. Three new files under src/routes/auth/. Frozen keycloak.ts lib consumed via documented exports only.

1. State-before-token-exchange ordering (CSRF defense)

Read src/routes/auth/callback/+server.ts top-to-bottom. The control flow before any fetch(tokenEndpoint, ...) is:

1. Env var presence check → 500 if missing
2. cookies.get(STATE_COOKIE_NAME) → 400 state_missing if absent
3. decryptCookiePayload(stateCookie) + isTransientStateBlob shape check → 400 if tamper / wrong key
4. url.searchParams.get('error') Keycloak-side error short-circuit → 400 auth_failed (generic body, no error_description echo)
5. code + state query params present → 400 if missing
6. verifyOidcState(stateParam, decrypted.signed) → 400 state_mismatch on failure
7. Only now does fetch(tokenEndpoint, ...) execute

Every failure path in steps 1-6 short-circuits with throw error(...) before the token POST. The classic OAuth CSRF (attacker dangles a pre-baked code at a logged-in admin) is closed: without a matching transient cookie issued by this origin's /auth/login AND a matching HMAC-signed state, the redemption hop refuses to run. Ordering is correct.

2. Missing transient state cookie returns 400

Lines ~78-80: if (!stateCookie) { throw error(400, 'state_missing'); }. Returns 400, not 500, not "valid because absent". Replay/late-callback hardening is correct.

3. PKCE cryptographic correctness

In src/routes/auth/login/+server.ts:

const codeVerifier = urlSafeBase64(randomBytes(32));
const codeChallenge = urlSafeBase64(createHash('sha256').update(codeVerifier).digest());

• randomBytes(32) from node:crypto — 256 bits CSPRNG, base64url-encoded → 43-char verifier (RFC 7636 compliant: 43-128 chars).
• code_challenge = base64url(SHA-256(code_verifier)).
• No Math.random. No Date.now-derived material. No PRNG reuse.
• code_challenge_method=S256 set on the /authorize URL.

Correct per RFC 7636.

4. CSRF on logout

if (origin !== CANONICAL_APP_ORIGIN) { throw error(403, 'csrf_check_failed'); } — strict equality against https://westside-admin.tail5b443a.ts.net.

• null Origin: null !== 'https://...' → 403. Rejects sandboxed iframes / restricted referrer policies (those wouldn't carry a valid session cookie anyway).
• Cross-site POST: browser sets Origin to the originating site → 403.
• Same-origin fetch / form submission: browser sets matching Origin → passes.
• Modern browsers reliably send Origin on POST. The known historical exception (some Safari versions on same-origin form submissions) is no longer in scope for a Tailscale-funneled admin app served only to current browsers.

Acceptable for v1. SvelteKit's built-in form-action CSRF token would be slightly more robust against future browser quirks, but the Origin check correctly defends against the actual threat model (third-party drive-by). Worth a follow-up nit if/when the logout button is wired into a SvelteKit form action — switching to the framework's CSRF token is one-line cheaper than maintaining the Origin allowlist.

5. Redirect URI exactness

const CANONICAL_REDIRECT_URI = 'https://westside-admin.tail5b443a.ts.net/auth/callback'; — hardcoded module constant in both login and callback. Not derived from request.url, Host header, or any env var. Used identically in:

• /auth/login: authorize.searchParams.set('redirect_uri', CANONICAL_REDIRECT_URI)
• /auth/callback: body.append('redirect_uri', CANONICAL_REDIRECT_URI) in token POST

No wildcards, no env-var substitution that could differ from Keycloak client config. Per sop-keycloak-client-creation, Keycloak's "Valid redirect URIs" must mirror this exact string.

Correct.

6. Token POST failure body relay

Three failure paths in the callback's token-exchange block:

• !resp.ok: throw error(502, 'auth_upstream_failed') — generic body, no resp.text() / resp.json() echo.
• Network exception in try: catch block → same generic 502 (with the SvelteKit-error re-throw guard so existing 4xx/5xx propagate cleanly).
• Schema validation failure (access_token not a string, etc.): same generic 502.

Keycloak's error_description and realm structure are never relayed. Correct — no oracle exposed.

Additional checks

• /auth/logout POST-only: only POST is exported. SvelteKit auto-returns 405 for unhandled methods. GET cannot trigger logout. Correct.
• No console.log of secrets: scanned all three files. Zero logging statements. No code, token, refresh_token, code_verifier, id_token, or ciphertext leakage surface.
• TypeScript strict — no any casts: uses unknown + type guards (isTransientStateBlob, isSessionBlob) and as Record<string, unknown> for narrowing. The single as KeycloakTokenResponse follows shape validation. No any anywhere. Correct.
• Out-of-bounds files: 3 files changed (+382/-0), all under src/routes/auth/. keycloak.ts, keycloak.test.ts, hooks.server.ts, (unauthorized)/*, health/* untouched. Correct.
• Open-redirect closed: safeRedirectPath rejects //evil.test, https://evil.test/, control chars, and non-strings. Applied at both /auth/login (capture) and /auth/callback (final redirect). Defense in depth.
• Cookie attributes: transient cookie HttpOnly; Secure; SameSite=Lax; Path=/auth/callback; Max-Age=600. Session cookie same posture at Path=/ with Max-Age=refresh_expires_in. Correct.

BLOCKERS

None. The OIDC protocol surface is implemented correctly:

• State validation strictly precedes token exchange (CSRF closed).
• PKCE uses CSPRNG-backed S256 (RFC 7636 compliant).
• Redirect URI is exact-match hardcoded (no host injection).
• Token POST errors use generic 502 (no Keycloak body leak).
• Logout is POST-only with Origin-based CSRF defense.

NITS

1. No unit tests for the three endpoints. PR documents this as #16's manual-browser-flow constraint, and the consumed lib (keycloak.ts) is tested. A vitest with mocked fetch asserting "state mismatch never reaches token POST" would convert the code-walk verification into a regression-proof guard. Non-blocking, recommend follow-up ticket.

2. Logout CSRF could migrate to SvelteKit form-action token. Origin check is correct for the current threat model, but when logout gets wired into a UI form, <form method="POST" action="/auth/logout"> with SvelteKit's built-in CSRF token would close the (very narrow) null-Origin edge cases without code in this handler.

3. oauthError check sits after cookie decrypt. Minor: if Keycloak returns ?error=access_denied, we still decrypt the cookie before short-circuiting. The decrypt is needed for cleanup anyway, so this is a wash. Could swap for clarity. Non-blocking.

4. as { status: unknown } in the catch block is a defensible narrowing pattern but could use a typed guard (function isHttpError(e: unknown): e is { status: number }) for symmetry with the other guards in the file. Pure style.

SOP COMPLIANCE

• Branch 16-auth-endpoints follows {issue}-{kebab-purpose} convention
• PR body has Summary, Changes, Test Plan, Related sections per template-pr-body
• Closes #16 with parent/sibling/owning-project linkage
• References sop-keycloak-client-creation (redirect URI exact-match invariant)
• Funnel-Auth Review section present and thorough per feedback_funnel_requires_auth — documents state-before-token-exchange, cookie attributes, AES-256-GCM encryption, exact-match redirect URI, PKCE S256, open-redirect closure, logout CSRF, generic upstream errors. This is exemplary funnel-auth documentation.
• No secrets / .env files / credentials committed
• No unrelated file changes (3 files, all under src/routes/auth/)
• keycloak.ts and keycloak.test.ts untouched (frozen at #18)
• npm run check 342/0/0 and npm test 25/25 claimed; no regressions expected since lib is frozen

PROCESS OBSERVATIONS

• DORA/CFR posture: The OIDC surface is the entire CSRF + replay + open-redirect surface for the funnel-exposed app. This PR materially reduces change-failure risk by hardcoding the redirect URI and validating state before token exchange. Documentation in the PR body (funnel-auth review, code-walk line numbers) is itself a deployment-frequency enabler — future reviewers can verify invariants without re-reading the lib.
• Deployment frequency: Three small server endpoints, additive only. Safe to merge once #15 (hooks) lands so the redirect target works end-to-end.
• Documentation gap: post-merge, recommend an Epilogue subphase capturing the two nits (endpoint unit tests + logout form-action migration) so they don't get lost. Per feedback_nits_to_epilogue, QA nits never dismiss.
• Sequencing: PR depends on #18 (merged) for the lib. Sibling #20 (hooks) provides the redirect destination. This PR can merge independently of #20 since /auth/login is reachable without the hook gate, but full login flow needs #20 too.

VERDICT: APPROVED

## PR #21 Review ### DOMAIN REVIEW **Stack:** TypeScript / SvelteKit server endpoints / OIDC authorization-code-with-PKCE + RP-initiated logout. Three new files under `src/routes/auth/`. Frozen `keycloak.ts` lib consumed via documented exports only. #### 1. State-before-token-exchange ordering (CSRF defense) Read `src/routes/auth/callback/+server.ts` top-to-bottom. The control flow before any `fetch(tokenEndpoint, ...)` is: 1. Env var presence check → 500 if missing 2. `cookies.get(STATE_COOKIE_NAME)` → **400 `state_missing`** if absent 3. `decryptCookiePayload(stateCookie)` + `isTransientStateBlob` shape check → **400** if tamper / wrong key 4. `url.searchParams.get('error')` Keycloak-side error short-circuit → **400 `auth_failed`** (generic body, no `error_description` echo) 5. `code` + `state` query params present → **400** if missing 6. `verifyOidcState(stateParam, decrypted.signed)` → **400 `state_mismatch`** on failure 7. **Only now** does `fetch(tokenEndpoint, ...)` execute Every failure path in steps 1-6 short-circuits with `throw error(...)` before the token POST. The classic OAuth CSRF (attacker dangles a pre-baked `code` at a logged-in admin) is closed: without a matching transient cookie issued by *this* origin's `/auth/login` AND a matching HMAC-signed state, the redemption hop refuses to run. **Ordering is correct.** #### 2. Missing transient state cookie returns 400 Lines ~78-80: `if (!stateCookie) { throw error(400, 'state_missing'); }`. Returns 400, not 500, not "valid because absent". Replay/late-callback hardening is correct. #### 3. PKCE cryptographic correctness In `src/routes/auth/login/+server.ts`: ``` const codeVerifier = urlSafeBase64(randomBytes(32)); const codeChallenge = urlSafeBase64(createHash('sha256').update(codeVerifier).digest()); ``` - `randomBytes(32)` from `node:crypto` — 256 bits CSPRNG, base64url-encoded → 43-char verifier (RFC 7636 compliant: 43-128 chars). - `code_challenge` = `base64url(SHA-256(code_verifier))`. - No `Math.random`. No `Date.now`-derived material. No PRNG reuse. - `code_challenge_method=S256` set on the `/authorize` URL. **Correct per RFC 7636.** #### 4. CSRF on logout `if (origin !== CANONICAL_APP_ORIGIN) { throw error(403, 'csrf_check_failed'); }` — strict equality against `https://westside-admin.tail5b443a.ts.net`. - `null` Origin: `null !== 'https://...'` → 403. Rejects sandboxed iframes / restricted referrer policies (those wouldn't carry a valid session cookie anyway). - Cross-site POST: browser sets Origin to the originating site → 403. - Same-origin fetch / form submission: browser sets matching Origin → passes. - Modern browsers reliably send Origin on POST. The known historical exception (some Safari versions on same-origin form submissions) is no longer in scope for a Tailscale-funneled admin app served only to current browsers. **Acceptable for v1.** SvelteKit's built-in form-action CSRF token would be slightly more robust against future browser quirks, but the Origin check correctly defends against the actual threat model (third-party drive-by). Worth a follow-up nit if/when the logout button is wired into a SvelteKit form action — switching to the framework's CSRF token is one-line cheaper than maintaining the Origin allowlist. #### 5. Redirect URI exactness `const CANONICAL_REDIRECT_URI = 'https://westside-admin.tail5b443a.ts.net/auth/callback';` — hardcoded module constant in **both** login and callback. Not derived from `request.url`, `Host` header, or any env var. Used identically in: - `/auth/login`: `authorize.searchParams.set('redirect_uri', CANONICAL_REDIRECT_URI)` - `/auth/callback`: `body.append('redirect_uri', CANONICAL_REDIRECT_URI)` in token POST No wildcards, no env-var substitution that could differ from Keycloak client config. Per `sop-keycloak-client-creation`, Keycloak's "Valid redirect URIs" must mirror this exact string. **Correct.** #### 6. Token POST failure body relay Three failure paths in the callback's token-exchange block: - `!resp.ok`: `throw error(502, 'auth_upstream_failed')` — generic body, no `resp.text()` / `resp.json()` echo. - Network exception in `try`: catch block → same generic 502 (with the SvelteKit-error re-throw guard so existing 4xx/5xx propagate cleanly). - Schema validation failure (`access_token` not a string, etc.): same generic 502. Keycloak's `error_description` and realm structure are never relayed. **Correct — no oracle exposed.** #### Additional checks - **`/auth/logout` POST-only**: only `POST` is exported. SvelteKit auto-returns 405 for unhandled methods. GET cannot trigger logout. Correct. - **No console.log of secrets**: scanned all three files. Zero logging statements. No code, token, refresh_token, code_verifier, id_token, or ciphertext leakage surface. - **TypeScript strict — no `any` casts**: uses `unknown` + type guards (`isTransientStateBlob`, `isSessionBlob`) and `as Record<string, unknown>` for narrowing. The single `as KeycloakTokenResponse` follows shape validation. No `any` anywhere. Correct. - **Out-of-bounds files**: 3 files changed (`+382/-0`), all under `src/routes/auth/`. `keycloak.ts`, `keycloak.test.ts`, `hooks.server.ts`, `(unauthorized)/*`, `health/*` untouched. Correct. - **Open-redirect closed**: `safeRedirectPath` rejects `//evil.test`, `https://evil.test/`, control chars, and non-strings. Applied at both `/auth/login` (capture) and `/auth/callback` (final redirect). Defense in depth. - **Cookie attributes**: transient cookie `HttpOnly; Secure; SameSite=Lax; Path=/auth/callback; Max-Age=600`. Session cookie same posture at `Path=/` with `Max-Age=refresh_expires_in`. Correct. ### BLOCKERS None. The OIDC protocol surface is implemented correctly: - State validation strictly precedes token exchange (CSRF closed). - PKCE uses CSPRNG-backed S256 (RFC 7636 compliant). - Redirect URI is exact-match hardcoded (no host injection). - Token POST errors use generic 502 (no Keycloak body leak). - Logout is POST-only with Origin-based CSRF defense. ### NITS 1. **No unit tests for the three endpoints.** PR documents this as #16's manual-browser-flow constraint, and the consumed lib (`keycloak.ts`) is tested. A vitest with mocked `fetch` asserting "state mismatch never reaches token POST" would convert the code-walk verification into a regression-proof guard. Non-blocking, recommend follow-up ticket. 2. **Logout CSRF could migrate to SvelteKit form-action token.** Origin check is correct for the current threat model, but when logout gets wired into a UI form, `<form method="POST" action="/auth/logout">` with SvelteKit's built-in CSRF token would close the (very narrow) `null`-Origin edge cases without code in this handler. 3. **`oauthError` check sits after cookie decrypt.** Minor: if Keycloak returns `?error=access_denied`, we still decrypt the cookie before short-circuiting. The decrypt is needed for cleanup anyway, so this is a wash. Could swap for clarity. Non-blocking. 4. **`as { status: unknown }` in the catch block** is a defensible narrowing pattern but could use a typed guard (`function isHttpError(e: unknown): e is { status: number }`) for symmetry with the other guards in the file. Pure style. ### SOP COMPLIANCE - [x] Branch `16-auth-endpoints` follows `{issue}-{kebab-purpose}` convention - [x] PR body has Summary, Changes, Test Plan, Related sections per `template-pr-body` - [x] Closes #16 with parent/sibling/owning-project linkage - [x] References `sop-keycloak-client-creation` (redirect URI exact-match invariant) - [x] **Funnel-Auth Review section present and thorough** per `feedback_funnel_requires_auth` — documents state-before-token-exchange, cookie attributes, AES-256-GCM encryption, exact-match redirect URI, PKCE S256, open-redirect closure, logout CSRF, generic upstream errors. This is exemplary funnel-auth documentation. - [x] No secrets / `.env` files / credentials committed - [x] No unrelated file changes (3 files, all under `src/routes/auth/`) - [x] `keycloak.ts` and `keycloak.test.ts` untouched (frozen at #18) - [x] `npm run check` 342/0/0 and `npm test` 25/25 claimed; no regressions expected since lib is frozen ### PROCESS OBSERVATIONS - **DORA/CFR posture**: The OIDC surface is the entire CSRF + replay + open-redirect surface for the funnel-exposed app. This PR materially reduces change-failure risk by hardcoding the redirect URI and validating state before token exchange. Documentation in the PR body (funnel-auth review, code-walk line numbers) is itself a deployment-frequency enabler — future reviewers can verify invariants without re-reading the lib. - **Deployment frequency**: Three small server endpoints, additive only. Safe to merge once #15 (hooks) lands so the redirect target works end-to-end. - **Documentation gap**: post-merge, recommend an Epilogue subphase capturing the two nits (endpoint unit tests + logout form-action migration) so they don't get lost. Per `feedback_nits_to_epilogue`, QA nits never dismiss. - **Sequencing**: PR depends on #18 (merged) for the lib. Sibling #20 (hooks) provides the redirect destination. This PR can merge independently of #20 since `/auth/login` is reachable without the hook gate, but full login flow needs #20 too. ### VERDICT: APPROVED

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 merged commit b222400aaf into main 2026-05-03 19:16:01 +00:00

forgejo_admin deleted branch 16-auth-endpoints 2026-05-03 19:16:01 +00:00

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

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

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 19:39:26 +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

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!21

No description provided.