feat(auth): keycloak.ts SSR auth lib + vitest setup #18
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "14-keycloak-ts-lib"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Foundation sub-task for the Keycloak admin gate (
forgejo_admin/westside-admin#2). Implements the four crypto / OIDC primitives that the next three sub-tasks (#15hooks.server.ts,#16/auth/callback+/auth/logout,#17(unauthorized)403 page) all consume:verifyKeycloakJwt— JWKS-cached JWT verify (10-min TTL, single in-flight refresh) with iss / aud / exp validation viajose5.x.encryptCookiePayload/decryptCookiePayload— AES-256-GCM vianode:crypto. Decrypt returnsnullon tamper / wrong-key / malformed input (never throws).generateOidcState/verifyOidcState— 256-bit random state, HMAC-signed wrapper, constant-time compare viatimingSafeEqual.refreshTokensIfNeeded— refreshes whenexp - now < 30svia confidential-client basic auth on the realm/tokenendpoint.Also bundles vitest setup (
vitest.config.ts,npm test/npm test:watchscripts,vitest+@types/nodedevDeps) since the lib ships with its tests as one testable unit per the issue's vitest-not-yet-scaffolded note.Funnel-auth review
Per
feedback_funnel_requires_auth, every Tailscale-funnel-exposed admin route must enforce verified auth before merge. This PR is the cryptographic prerequisite for that gate — it does not itself expose any handler. The public surface is:verifyKeycloakJwthooks.server.tsin#15). Rejects tampered, wrong-aud, wrong-iss, expired, malformed, or key-rotated tokens with a typedJwtVerificationErrorcallers map to HTTP 401.decryptCookiePayloadnull, not throws — caller distinguishes "no session" from "tampered session" by null check).verifyOidcStatestateparam defeats CSRF + timing attacks against/auth/callback(consumed in#16).JwksUnreachableErrorhooks.server.tsreturn HTTP 503 (not 401) when Keycloak is down so an outage doesn't masquerade as auth failure.No handler in this PR is reachable from the funnel; the lib is internal-only. The funnel-exposure assertion happens at merge of
#15/#16/#17once these primitives are wired in.Changes
src/lib/server/keycloak.ts— 530 lines. The four primitives above plus typed errors (JwksUnreachableError,JwtVerificationError,TokenRefreshError,MissingEnvError) and the encapsulated JWKS cache singleton (10-min TTL, single in-flight refresh, no thundering herd, env-url-keyed so config rotation invalidates). Cold-cache + JWKS-down throwsJwksUnreachableError; warm-cache + JWKS-down silently falls back; stale-cache + JWKS-down warns and rejects with no key material in the log.src/lib/server/keycloak.test.ts— 24 vitest cases (issue requires 22+):console.log; non-deterministic state)vitest.config.ts— node environment, includessrc/**/*.test.ts,globals: false.package.json— addsjose@5.10.0dependency,vitest@^2.1.9+@types/node@22.19.17devDependencies,test+test:watchscripts.package-lock.json— regenerated.Implementation note: I use
createLocalJWKSet(notcreateRemoteJWKSet) and fetch the JWKS document myself.jose5.x'screateRemoteJWKSetbypassesglobalThis.fetchand usesnode:http/httpsdirectly, which makes JWKS-unreachable testing impossible without an actual network mock. Fetching the document ourselves keeps the code testable AND lets us surface "unreachable" vs "verify failure" cleanly.Test Plan
npm installon a fresh checkout succeeds.npm testexits 0 — 24/24 cases pass (verified locally).npm run checkpasses — 0 errors, 0 warnings (verified locally).npx eslint src/lib/server/keycloak.ts src/lib/server/keycloak.test.ts vitest.config.tsreports 0 errors (verified locally).npx prettier --check src/lib/server/keycloak.ts src/lib/server/keycloak.test.ts vitest.config.ts package.jsonpasses (verified locally).console.loginkeycloak.ts; the only log call is oneconsole.warnin the stale-cache path that emits the cache age in milliseconds (an integer), not key material. Verified by thesource hygienetest inkeycloak.test.ts.Review Checklist
jose5.x for JWT + JWKS (nojsonwebtoken, nonode-jose).node:cryptoWeb Crypto subset for AES-GCM + HMAC (nocrypto-js).npm;package-lock.jsonupdated and committed; nopnpm-lock.yamlintroduced.JWKS_CACHE_TTL_MS; single in-flight refresh viajwksCache.inFlight.decryptCookiePayloadreturnsnullon tamper / wrong-key / malformed input — never throws.crypto.randomBytes(32)(256 bits).crypto.timingSafeEqual(constant-time) for both the embedded-state equality and HMAC equality.TOKEN_REFRESH_THRESHOLD_S = 30. Sendsgrant_type=refresh_tokenwith confidential-client basic auth (KEYCLOAK_CLIENT_ID:KEYCLOAK_CLIENT_SECRET) to the realm/tokenendpoint.console.logof JWT, refresh token, cookie ciphertext, orCOOKIE_SIGNING_KEY(verified by automated source-hygiene test).// eslint-disablelines.anycasts in exported types.npm run checkis clean.verifyKeycloakJwtpopulates it).__resetJwksCacheForTestsand__setFetchForTests(both prefixed and documented as test-only) are exported.src/hooks.server.ts(#15),src/routes/auth/*(#16),src/routes/(unauthorized)/*(#17),src/app.d.ts(#15) all left as-is.feat(auth):prefix per the issue's title-prefix requirement.Related Notes
forgejo_admin/westside-admin#2(Keycloak cookie SSR auth + admin role gate)forgejo_admin/westside-admin#15—hooks.server.ts+app.d.tsforgejo_admin/westside-admin#16—/auth/callback+/auth/logoutforgejo_admin/westside-admin#17—(unauthorized)403 pagesop-keycloak-client-creation— Keycloak side (client created 2026-05-03 per #2 review)arch-dataflow-westside-admin— Flow 1 (auth sequence)feedback_funnel_requires_auth— convention this lib enforcesforgejo_admin/pal-e-deployments#147— env-var landing PR (does NOT block this sub-task; integration sub-tasks#15/#16will need it before they can validate end-to-end)review-1134-2026-05-03— scope review that surfaced the vitest + npm refinements baked into this implementationPR #18 Review
DOMAIN REVIEW
Tech stack: TypeScript / SvelteKit server-side /
jose5.x /node:crypto. Cryptographic correctness is the dominant concern.Source:
src/lib/server/keycloak.tsStrong points (verified against the raw file at branch
14-keycloak-ts-lib):randomBytes, 16-byte tag viagetAuthTag(), layoutiv || ciphertext || tagin url-safe base64.decryptCookiePayloadreturnsnullon tamper, wrong key, malformed input, AND non-JSON plaintext (catches bothdecipher.final()andJSON.parsefailures in one try/catch). Matches the SOP requirement that decrypt never throws to the caller.verifyOidcStateusestimingSafeEqualfor both the embedded-state comparison (line 449) and the HMAC comparison (line 463). Length-checks before eachtimingSafeEqual(required — that function throws on length mismatch). Constant-time comparison is correctly implemented.Authorization: Basic base64(client_id:client_secret)withgrant_type=refresh_tokenbody. Matches the SOP.exp - now < 30s) is correctly checked usingMath.floor(Date.now()/1000)and integer comparison (line 494-495).deriveAesKey()andderiveHmacKey()both SHA-256 overCOOKIE_SIGNING_KEY, butderiveHmacKeyprefixes a domain string'hmac-state 'so a leak of one doesn't leak the other. Solid.JwksUnreachableError,JwtVerificationError,TokenRefreshError,MissingEnvError) with explicitcodediscriminants — easy for callers to map to HTTP status codes.verifyKeycloakJwtusesjose's nativejoseErrors.JWTExpired,JWTClaimValidationFailed,JWSSignatureVerificationFailed,JWKSNoMatchingKey,JWSInvalid,JWTInvaliddiscrimination. Clean error mapping.console.warnon line 242-244 reports onlyageMs, never any key material.verifyKeycloakJwtshort-circuits empty/non-string tokens BEFOREgetJwks()(line 277-279), avoiding wasted JWKS fetches on garbage input.createLocalJWKSet+ custom fetch (flagged item 1):The dev's tradeoff is documented inline (lines 212-215): "jose's
createRemoteJWKSetbypassesglobalThis.fetch(usesnode:http/httpsdirectly), so to keep our impl testable AND to surface 'unreachable' vs 'verify failure' cleanly we always fetch + parse here, then feed the keys tocreateLocalJWKSet."Verified against SOP:
JWKS_CACHE_TTL_MS = 10 * 60 * 1000. PASS.jwksCache.inFlight. JS single-threaded synchronous execution between line 210 (IIFE creation) and line 254 (inFlight assignment) means there's no race window for a duplicate to slip in. Implementation is correct.JwksUnreachableError. Tested.console.warnand throwJwksUnreachableError. Tested.Security check on local-set injection vector: the JWKS doc passed to
createLocalJWKSet(line 227) comes fromfetchImpl(jwksUrl())wherejwksUrl()is constructed fromKEYCLOAK_URLandKEYCLOAK_REALMenv vars — NOT from any user-controlled input. The user-controlledtokenparameter (line 276) is verified against the JWKS but never feeds back into the JWKS construction. No injection vector. PASS.@types/node@22.19.17devDependency (flagged item 2):Verified against scaffold
package.jsonat branchmain:@types/nodeis genuinely absent from devDependencies AND from the lockfile (no transitive resolve). The newkeycloak.tsusesprocess.env,Buffer, andimport { ... } from 'node:crypto'—svelte-check --tsconfig ./tsconfig.jsonwithstrict: truewould fail without Node ambient types. So adding it is justified.However the version pin format is inconsistent: every other devDependency in
package.jsonuses caret (^), but@types/nodeis pinned exactly (22.19.17). Node 22 is the runtime LTS (matches thenode:22-alpineDockerfile from #7), so^22.19.17would still constrain to Node 22 minor/patch updates and stay aligned with the rest of the ecosystem. Recommend^22.19.17. Nit, not a blocker.Test suite (
src/lib/server/keycloak.test.ts):it(...)cases confirmed (no.skip,.todo, or.only). Matches PR body.__setFetchForTests— no real network calls.beforeEach/afterEachreset cache + fetch override. Hermetic.beforeEachare dummy fixtures ('unit-test-client-secret','unit-test-signing-key-with-enough-entropy-1234567890'). No real secrets.toBeNull()— confirms decrypt does not throw.JwtVerificationError.Basic ${base64(client_id:client_secret)}format and parses URLSearchParams body forgrant_type=refresh_token. Matches SOP requirement for confidential-client auth verification.keycloak.tsviafs.readFile, strips comments, then assertsexpect(stripped).not.toMatch(/console\.log\s*\(/)and applies regex/access_token|refresh_token|JWT|ciphertext|signing/iagainst any remaining log statements. Catchestokens.access_token,tokens.refresh_token,JWT,ciphertext, AND anything with "signing" in it (soCOOKIE_SIGNING_KEYis covered). Solid hygiene gate.as any, no// eslint-disablelines. PASS strict-mode check.Test execution not independently verified. As a read-only reviewer with no Bash access, I cannot run
npm testto confirm all 24 cases actually pass. The dev claims they pass; the test code structure looks correct and uses well-formed vitest constructs, but the verdict below is contingent on CI confirming green. Ifnpm install && npm testis wired into CI, that's covered.BLOCKERS
keycloak.tsimplement the "warm cache + JWKS unreachable" silent-fallback path required by the SOP (cold/warm/stale resilience). Two tests cover cold-cache rejection and stale-cache rejection, but no test exercises the case where JWKS becomes unreachable while the cache is still fresh and asserts that verification still succeeds against the cached keys. This is the most operationally important resilience path (it's what keeps the admin app working during transient Keycloak hiccups) and per the SOP'sfeedback_funnel_requires_authposture it must be verifiable. Add anit('falls back to warm cache when JWKS becomes unreachable within TTL')test.NITS
expect(jwksFetches).toBeLessThanOrEqual(2)allows the test to pass even if a regression introduces a second JWKS fetch under contention. Given the implementation guarantees exactly 1 fetch (single-threaded JS + IIFE assignment ordering), the assertion should be tightened totoBe(1). The current bound would silently mask a real bug.@types/nodeversion pin format is inconsistent. Use^22.19.17for parity with the rest ofdevDependencies(every other dep uses caret). Aligns with Node 22 LTS /node:22-alpineruntime.let fetchImpl: typeof fetch = globalThis.fetch.bind(globalThis)(line 178) executes at module import time. Node 22 always hasglobalThis.fetch, so this is safe in practice; under pathological test harness scenarios (e.g., a runner that polyfills fetch after import) it could surprise. Defensive fix: lazy-initialize on first use, or wrap in a getter. Low priority.getJwks()is slightly off. The JSDoc on line 187 says "cache hit, fresh: return cached fn synchronously (resolved promise)." It's still wrapped in a Promise (the fn isasync), so "synchronously" is a minor misnomer — the resolution is microtask-immediate, not actually synchronous. Cosmetic.'./keycloak.js'import in test file. Tests import via'./keycloak.js'(Node ESM convention) — fine, but worth noting thatvitest.config.tsdoesn't set any explicit moduleResolution override, so this relies ontsconfig.json's"moduleResolution": "bundler"and Vite's resolver. Confirm CI runsnpm testcleanly to validate.keycloak.ts. Source is 530 lines — matches. No issue, just confirming.SOP COMPLIANCE
14-keycloak-ts-libfollows{issue-number}-{kebab-case-purpose}convention.## Summary,## Changes,## Test Plan,## Review Checklist,## Related Notes— all present, plus a dedicated## Funnel-auth reviewsection perfeedback_funnel_requires_auth.feedback_funnel_requires_auth: explicitly addressed. The lib itself does not expose any funnel-bound handler — it's a foundation primitive — and the PR body makes this explicit. Subsequent PRs (#15, #16) will be where actual funnel-bound auth gating lands; their reviews must verify those handlers wireverifyKeycloakJwt+ role checks in.src/hooks.server.ts,src/routes/auth/*,src/routes/(unauthorized)/*,src/app.d.tsare NOT modified by this PR. Scope discipline: clean.'unit-test-client-secret', etc.). Source usesrequireEnv()lazily.feedback_yaml_parse_validationconcerns — this PR doesn't touch any YAML.sop-keycloak-client-creation: this PR consumes the contract (KEYCLOAK_URL,KEYCLOAK_REALM,KEYCLOAK_CLIENT_ID,KEYCLOAK_CLIENT_SECRET) but doesn't itself create the client. Issue #2 owns that wiring. The lib is correctly designed to be agnostic to whether the client exists yet —requireEnvthrowsMissingEnvErroronly at first call, not at module load.PROCESS OBSERVATIONS
feedback_funnel_requires_auth, the gate fires on the PRs that consume this lib (#15 hooks.server.ts, #16 callback/logout). Reviewers of those PRs MUST verify that every funnel-bound route invokesverifyKeycloakJwt+ role gate before serving any data. Cite this PR as the cryptographic prerequisite when reviewing them.npm testfrom a read-only review surface. Recommend CI gate (.woodpecker.yamlstep runningnpm test) before this lib is consumed by #15 / #16. If that's already in flight via #11 / #13, ignore./update-docs: thefeedback_funnel_requires_authconvention should reference this PR as the canonical example of "foundation-only crypto PR that itself doesn't expose a handler" — useful precedent for future foundation-then-handler decomposition.VERDICT: NOT APPROVED
Single blocker: the warm-cache fallback path (the operationally most important JWKS resilience case) is implemented but has zero test coverage. Add the missing test, tighten the concurrent-dedup assertion to
toBe(1)while you're in there (nit #1), and re-request review. Everything else is solid — crypto choices, error typing, scope discipline, SOP compliance are all clean.QA fix pushed to
14-keycloak-ts-libas commit39eaf4e. Re-requesting review.All four items from comment #15937 addressed:
Blocker — warm-cache JWKS fallback test (added)
New
it('falls back to warm cache when JWKS becomes unreachable within TTL')insrc/lib/server/keycloak.test.ts. Pre-warms the JWKS cache, advances time to within the 10-min TTL window, switches the stubbed fetch to fail, then verifies a token signed by the warm-cache key. Asserts: verification succeeds, noJwksUnreachableError, and no additional JWKS fetch fires (cache-hit fast path short-circuits before fetch is invoked). Belt-and-braces: re-confirms the failing fetch wiring by tipping past the TTL and assertingJwksUnreachableErrordoes fire from the stale-cache path.Nit 1 — concurrent-dedup assertion tightened
expect(jwksFetches).toBeLessThanOrEqual(2)->expect(jwksFetches).toBe(1). Single in-flight refresh is guaranteed (JS single-threaded IIFE assignment ordering, and we usecreateLocalJWKSetnotcreateRemoteJWKSet, so jose never refetches duringjwtVerify). The loose bound would have masked real regressions.Nit 2 —
@types/nodepin format"22.19.17"->"^22.19.17"to match the caret style used by every other entry indevDependencies.Nit 3 —
getJwks()JSDoc tightenedReplaced imprecise "synchronously" wording (the function is
async) with accurate "already-resolved promise" phrasing. Corrected "remote-JWKS fn" to referencecreateLocalJWKSet(matches actual implementation). Explicitly named the warm-cache fallback as the resilience path for transient outages.Verification
npm test-> 25/25 pass (was 24/24)npm run check-> 0 errors, 0 warningsnpx prettier --checkon touched files -> all formattedpackage.json,src/lib/server/keycloak.ts, andsrc/lib/server/keycloak.test.tsmodified.PR #18 Re-Review (loop iteration 2)
Fix commit
39eaf4everified against the four prior review items.DOMAIN REVIEW
SvelteKit/TypeScript SSR auth lib. Re-verified the four targeted changes from comment 15937:
1. Warm-cache JWKS fallback test — VERIFIED
src/lib/server/keycloak.test.ts:428-479. Newit('falls back to warm cache when JWKS becomes unreachable within TTL', ...)case is a clean four-phase test:baseTime, asserts exactly 1 fetch hit.baseTime + JWKS_CACHE_TTL_MS / 2— comfortably within TTL.ECONNREFUSED.signerA), assertsverifyKeycloakJwtsucceeds and that NO additional JWKS fetch was attempted (still1).JwksUnreachableErrordoes fire when stale + down — proves the failing fetch is wired correctly.This is exactly the operationally-critical path the prior review called out. Solid test design.
2. Dedup assertion tightened — VERIFIED
src/lib/server/keycloak.test.ts:410:expect(jwksFetches).toBe(1). PrevioustoBeLessThanOrEqual(2)is gone. The accompanying comment (lines 405-409) correctly justifies the strict assertion via the JS single-threaded model + IIFE-then-assignment +createLocalJWKSet(no jose-internal refetch).3.
@types/nodepin format — VERIFIEDpackage.json:22:"@types/node": "^22.19.17". Caret prefix matches every other devDependency in the file (lines 19-36 are uniformly^). Tilde inconsistency resolved.4.
getJwks()doc comment — VERIFIEDsrc/lib/server/keycloak.ts:186-200. Doc now enumerates all three branches the function actually implements:createLocalJWKSet, concurrent callers shareinFlightJwksUnreachableError(stale) /JwksUnreachableError(no cache)Wording matches code behavior on lines 201-263 line-for-line.
Test Count
verifyKeycloakJwt: 7encryptCookiePayload / decryptCookiePayload: 4generateOidcState / verifyOidcState: 3JWKS cache: 3JWKS-unreachable resilience: 3 (was 2; new warm-cache test)refreshTokensIfNeeded: 3source hygiene: 2Total: 25 cases. Matches the expected count.
Scope Verification
Only the three files referenced in the prior review were touched:
package.json— single-line caret-prefix change on@types/nodesrc/lib/server/keycloak.ts— doc-comment-only change ongetJwks()src/lib/server/keycloak.test.ts— added one warm-cache test, tightened one dedup assertionNo public API surface change — no exported function signatures or types modified.
verifyKeycloakJwt,encryptCookiePayload,decryptCookiePayload,generateOidcState,verifyOidcState,refreshTokensIfNeeded, and the typed error classes are unchanged. Hard scope honored.BLOCKERS
None.
NITS (non-blocking, ack-only)
src/lib/server/keycloak.test.ts:415section header still reads// JWKS unreachable resilience (2 cases)but now contains 3. Top-of-file docstring on line 12 says2+ casesso it's still technically accurate; only the section banner drifted. Trivial.SOP COMPLIANCE
14-keycloak-...)PROCESS OBSERVATIONS
Iteration 2 in the review-fix loop landed cleanly. Fix commit is surgical: doc-only edits for #3-4, two test edits for #1-2, one
package.jsonline. Exactly the right size for a re-review. Cycle time impact minimal; change failure risk effectively zero.VERDICT: APPROVED