forgejo_admin/westside-admin
1
0
Fork 0
Code Issues 6 Pull requests Projects Releases Packages Wiki Activity Actions

fix(auth): accept azp claim for client identity (closes #26) #27

Merged
forgejo_admin merged 1 commit from 26-jwt-aud-azp-fix into main 2026-05-03 21:40:30 +00:00
Conversation 1 Commits 1 Files changed 3 +246 -6
forgejo_admin commented 2026-05-03 21:34:44 +00:00
Owner
Copy link

Summary

verifyKeycloakJwt was rejecting every live access token because Keycloak's default access tokens carry aud=account, azp=westside-admin (no audience mapper configured on the client). This PR softens the client-identity check to accept aud containing the client_id OR azp equal to the client_id, per OIDC Core §2. SSO completes end-to-end against the existing Keycloak deployment with no admin-console changes required.

Closes #26.

Changes

  • src/lib/server/keycloak.ts — verifyKeycloakJwt: drop audience: expectedAudience() from the jose jwtVerify options (jose no longer auto-rejects on aud). After verify, manually check aud === clientId || aud.includes(clientId) || azp === clientId. Throw JwtVerificationError(claim aud/azp invalid (aud=…, azp=…)) when both fail. Pass-through our own typed error so the manual rejection isn't double-wrapped by the jose-error-mapper.
  • src/lib/server/keycloak.test.ts — extend JwtOpts with optional azp (passed as a JWT body claim); broaden aud to string | string[] so we can build the array case; rename the existing rejects a token with the wrong audience case to rejects a token with the wrong audience AND wrong azp (truly wrong client) and set both claims to non-matching values (otherwise it becomes a false-pass under the new logic); add the 6-row accept/reject matrix from #26.
  • scripts/verify-aud-azp.mjs — standalone harness that imports the real exported verifyKeycloakJwt, stubs the JWKS endpoint via __setFetchForTests, and exercises every matrix case end-to-end. Mirrors the measure-cookie-size.mjs pattern from #25. Run with npx tsx scripts/verify-aud-azp.mjs.

Live JWT-claims Evidence (from #26)

Decoded the live access token claims for Lucas's session at SHA 785a1ca3 (post-#25 deploy):

iss: https://keycloak.tail5b443a.ts.net/realms/westside-basketball   matches expectedIssuer
aud: account                                                          does NOT match expected 'westside-admin'  <-- the bug
azp: westside-admin                                                   matches client_id but jose was not consulting it
sub: 19bfe0df-7fbc-463c-97df-59d77901421e
typ: Bearer
realm_access.roles: [..., admin, ...]

Pre-fix: audience: expectedAudience() in jwtVerify causes JWTClaimValidationFailed on aud === 'account'. Hook's catch {} falls through to anonymous → 302 → infinite redirect.

Post-fix: aud-mismatch is no longer fatal when azp === client_id, so the Keycloak default shape verifies cleanly.

Accept/Reject Matrix (new test coverage)

aud azp Expected Test result
account westside-admin ACCEPT (Keycloak default — the bug) PASS
westside-admin westside-admin ACCEPT PASS
westside-admin other ACCEPT (audience mapper case) PASS
account other REJECT PASS
[westside-admin, account] westside-admin ACCEPT (aud is array) PASS
other other REJECT (truly wrong client) PASS

Validation Script Output

Per feedback_validate_before_done — running the standalone matrix harness against the actual exported verifyKeycloakJwt:

$ npx tsx scripts/verify-aud-azp.mjs
aud/azp verification matrix (issue #26)
=========================================
  [PASS] expect=ACCEPT actual=ACCEPT aud=account, azp=westside-admin (Keycloak default — the bug)
  [PASS] expect=ACCEPT actual=ACCEPT aud=westside-admin, azp=westside-admin
  [PASS] expect=ACCEPT actual=ACCEPT aud=westside-admin, azp=other (audience mapper case)
  [PASS] expect=REJECT actual=REJECT aud=account, azp=other (no match)
  [PASS] expect=ACCEPT actual=ACCEPT aud=[westside-admin, account], azp=westside-admin (aud array)
  [PASS] expect=REJECT actual=REJECT aud=other, azp=other (truly wrong client)
=========================================
all 6 matrix cases match expected outcomes

This is the live aud=account shape that broke prod — exit code 0, all 6 cases match expected outcomes against the real exported function.

Funnel-Auth Review (per feedback_funnel_requires_auth)

The westside-admin app sits behind a Tailscale funnel + Keycloak OIDC. This PR softens the JWT identity check; below is the security argument for why it's not a regression.

  • azp is OIDC-spec-conformant for client identity. OIDC Core §2 defines azp as "the party to which the ID Token was issued" (commonly the client_id). Major IdPs (Keycloak, Auth0, Okta) populate it with the requesting client_id by default. Accepting it satisfies "did this token originate from a request my client made?" — exactly the question we're asking.
  • No surface expansion. Pre-fix logic accepted: aud === client_id. Post-fix logic accepts: aud === client_id OR aud.includes(client_id) OR azp === client_id. We reject any token where ALL THREE are false. The signature, issuer, and exp checks are unchanged — every other invariant from #14's threat model is intact.
  • Attacker model. To forge a verifying token, an attacker needs a Keycloak signing key (the iss and kid filter to our realm's JWKS). Adding azp to the accept set does not lower that bar — the attacker still needs the private key to sign a token bearing iss=our-realm AND azp=westside-admin. Compromised signing key = game-over regardless of which claim we read for client identity.
  • Cross-client confusion. A token issued to a DIFFERENT Keycloak client in our realm (e.g. a hypothetical westside-public) would have azp=westside-public, aud=…. Neither matches westside-admin, so we reject. Verified by the aud=other, azp=other case in the matrix.
  • aud=account token in the wild. The account audience is Keycloak's built-in account-management client scope, present on every default-scoped client in every realm. By itself it does not grant admin access — the role check downstream of verifyKeycloakJwt (in the hook) gates admin actions on realm_access.roles containing admin, which only Lucas's user has. The fix unblocks that check from firing; it does not weaken it.

Conclusion: accepting azp is OIDC-spec-conformant and reduces no security invariant. The funnel-auth gate becomes operational rather than non-functional.

Test Plan

  • npm test — 31 cases pass (was 25, added 6 matrix cases + replaced 1)
  • npm run check — 0 errors, 0 warnings
  • npx prettier --check src/lib/server/keycloak.ts src/lib/server/keycloak.test.ts scripts/verify-aud-azp.mjs — clean
  • npx eslint src/lib/server/keycloak.ts src/lib/server/keycloak.test.ts — no errors
  • npm run build — succeeds (adapter-node, server bundle 127.66 kB)
  • npx tsx scripts/verify-aud-azp.mjs — exit 0, all 6 matrix cases PASS
  • Post-merge: redeploy → curl SSO round-trip → verify / returns 200 after callback Set-Cookie (ACs 1, 2 from #26)
  • Post-merge: Playwright Chromium → admin user logs in → app renders at / → event.locals.user.name populated (AC 3 from #26)
  • Post-merge: non-admin user → renders the (unauthorized) 403 page

Coordination with #22

#22 (open — refresh_expires_in exposure on KeycloakTokens) also touches src/lib/server/keycloak.ts. Different function (refreshTokensIfNeeded + the KeycloakTokens interface), no semantic conflict with verifyKeycloakJwt. Whichever lands second rebases — likely a trivial three-way merge since the line ranges are non-overlapping. No code changes needed in this PR for #22 awareness.

Out of Scope (per ticket)

  • src/routes/auth/* — unchanged; they don't call verifyKeycloakJwt directly.
  • src/hooks.server.ts — unchanged; gets the new behavior through the unchanged verifyKeycloakJwt signature.
  • pal-e-deployments/*, (unauthorized)/* — unchanged.
  • Path A (audience-mapper Keycloak admin-console fix) — explicitly NOT pursued; Path B keeps SOPs simple and works for any future client.

Review Checklist

  • Scope is bounded to src/lib/server/keycloak.ts and src/lib/server/keycloak.test.ts plus the new scripts/verify-aud-azp.mjs validation harness — no edits to src/routes/auth/*, src/hooks.server.ts, deployments, or unrelated files
  • No tokens, ciphertext, or signing-key material added to log statements (the keycloak.ts source-hygiene unit test still passes)
  • Existing rejects-on-wrong-aud case was UPDATED, not deleted — now sets both aud and azp to non-matching values so it isn't a false-pass under the new logic
  • Funnel-Auth Review (above) addresses why accepting azp is OIDC-spec-conformant and not a security regression, per feedback_funnel_requires_auth
  • Standalone validation script output included in PR body, per feedback_validate_before_done
  • Coordination note for sibling open PR #22 included
  • Reviewer: confirm the manual aud/azp branch matches OIDC Core §2 wording for azp
  • Reviewer: confirm no behavioral change for the existing 6 JWT verify cases (signature, issuer, exp, malformed, missing-key) — only the aud check moved out-of-jose
  • Reviewer: spot-check that JwtVerificationError thrown manually is NOT re-wrapped by the catch block (the if (err instanceof JwtVerificationError) throw err; guard at the top of catch handles this)

Related Notes

  • feedback_funnel_requires_auth — funnel-auth gate is non-functional without this fix even though every individual component was "approved"
  • feedback_validate_before_done — drove the standalone harness output in this PR body; gate that didn't catch this last time catches it this time
  • sop-keycloak-client-creation — no SOP change required by Path B (works for any client; Path A would have needed a per-client audience mapper step)

Related

  • Closes forgejo_admin/westside-admin#26
  • Sibling forgejo_admin/westside-admin#22 (open — refresh_exp; overlapping lib file, different function)
  • Sibling forgejo_admin/westside-admin#24 / PR #25 (closed — cookie size fix; SAME bug surface, different root cause; this PR completes the loop unmasked by #25)
## Summary `verifyKeycloakJwt` was rejecting every live access token because Keycloak's default access tokens carry `aud=account, azp=westside-admin` (no audience mapper configured on the client). This PR softens the client-identity check to accept `aud` containing the client_id OR `azp` equal to the client_id, per OIDC Core §2. SSO completes end-to-end against the existing Keycloak deployment with no admin-console changes required. Closes #26. ## Changes - `src/lib/server/keycloak.ts` — `verifyKeycloakJwt`: drop `audience: expectedAudience()` from the jose `jwtVerify` options (jose no longer auto-rejects on aud). After verify, manually check `aud === clientId || aud.includes(clientId) || azp === clientId`. Throw `JwtVerificationError(claim aud/azp invalid (aud=…, azp=…))` when both fail. Pass-through our own typed error so the manual rejection isn't double-wrapped by the jose-error-mapper. - `src/lib/server/keycloak.test.ts` — extend `JwtOpts` with optional `azp` (passed as a JWT body claim); broaden `aud` to `string | string[]` so we can build the array case; rename the existing `rejects a token with the wrong audience` case to `rejects a token with the wrong audience AND wrong azp (truly wrong client)` and set both claims to non-matching values (otherwise it becomes a false-pass under the new logic); add the 6-row accept/reject matrix from #26. - `scripts/verify-aud-azp.mjs` — standalone harness that imports the real exported `verifyKeycloakJwt`, stubs the JWKS endpoint via `__setFetchForTests`, and exercises every matrix case end-to-end. Mirrors the `measure-cookie-size.mjs` pattern from #25. Run with `npx tsx scripts/verify-aud-azp.mjs`. ## Live JWT-claims Evidence (from #26) Decoded the live access token claims for Lucas's session at SHA `785a1ca3` (post-#25 deploy): ``` iss: https://keycloak.tail5b443a.ts.net/realms/westside-basketball matches expectedIssuer aud: account does NOT match expected 'westside-admin' <-- the bug azp: westside-admin matches client_id but jose was not consulting it sub: 19bfe0df-7fbc-463c-97df-59d77901421e typ: Bearer realm_access.roles: [..., admin, ...] ``` Pre-fix: `audience: expectedAudience()` in `jwtVerify` causes `JWTClaimValidationFailed` on `aud === 'account'`. Hook's `catch {}` falls through to anonymous → 302 → infinite redirect. Post-fix: aud-mismatch is no longer fatal when `azp === client_id`, so the Keycloak default shape verifies cleanly. ## Accept/Reject Matrix (new test coverage) | `aud` | `azp` | Expected | Test result | |---|---|---|---| | `account` | `westside-admin` | ACCEPT (Keycloak default — the bug) | PASS | | `westside-admin` | `westside-admin` | ACCEPT | PASS | | `westside-admin` | `other` | ACCEPT (audience mapper case) | PASS | | `account` | `other` | REJECT | PASS | | `[westside-admin, account]` | `westside-admin` | ACCEPT (aud is array) | PASS | | `other` | `other` | REJECT (truly wrong client) | PASS | ## Validation Script Output Per `feedback_validate_before_done` — running the standalone matrix harness against the actual exported `verifyKeycloakJwt`: ``` $ npx tsx scripts/verify-aud-azp.mjs aud/azp verification matrix (issue #26) ========================================= [PASS] expect=ACCEPT actual=ACCEPT aud=account, azp=westside-admin (Keycloak default — the bug) [PASS] expect=ACCEPT actual=ACCEPT aud=westside-admin, azp=westside-admin [PASS] expect=ACCEPT actual=ACCEPT aud=westside-admin, azp=other (audience mapper case) [PASS] expect=REJECT actual=REJECT aud=account, azp=other (no match) [PASS] expect=ACCEPT actual=ACCEPT aud=[westside-admin, account], azp=westside-admin (aud array) [PASS] expect=REJECT actual=REJECT aud=other, azp=other (truly wrong client) ========================================= all 6 matrix cases match expected outcomes ``` This is the live aud=account shape that broke prod — exit code 0, all 6 cases match expected outcomes against the real exported function. ## Funnel-Auth Review (per `feedback_funnel_requires_auth`) The westside-admin app sits behind a Tailscale funnel + Keycloak OIDC. This PR softens the JWT identity check; below is the security argument for why it's not a regression. - **`azp` is OIDC-spec-conformant for client identity.** OIDC Core §2 defines `azp` as "the party to which the ID Token was issued" (commonly the client_id). Major IdPs (Keycloak, Auth0, Okta) populate it with the requesting client_id by default. Accepting it satisfies "did this token originate from a request my client made?" — exactly the question we're asking. - **No surface expansion.** Pre-fix logic accepted: `aud === client_id`. Post-fix logic accepts: `aud === client_id` OR `aud.includes(client_id)` OR `azp === client_id`. We reject any token where ALL THREE are false. The signature, issuer, and exp checks are unchanged — every other invariant from #14's threat model is intact. - **Attacker model.** To forge a verifying token, an attacker needs a Keycloak signing key (the `iss` and `kid` filter to our realm's JWKS). Adding `azp` to the accept set does not lower that bar — the attacker still needs the private key to sign a token bearing `iss=our-realm` AND `azp=westside-admin`. Compromised signing key = game-over regardless of which claim we read for client identity. - **Cross-client confusion.** A token issued to a DIFFERENT Keycloak client in our realm (e.g. a hypothetical `westside-public`) would have `azp=westside-public, aud=…`. Neither matches `westside-admin`, so we reject. Verified by the `aud=other, azp=other` case in the matrix. - **`aud=account` token in the wild.** The `account` audience is Keycloak's built-in account-management client scope, present on every default-scoped client in every realm. By itself it does not grant admin access — the role check downstream of `verifyKeycloakJwt` (in the hook) gates admin actions on `realm_access.roles` containing `admin`, which only Lucas's user has. The fix unblocks that check from firing; it does not weaken it. Conclusion: accepting `azp` is OIDC-spec-conformant and reduces no security invariant. The funnel-auth gate becomes operational rather than non-functional. ## Test Plan - [x] `npm test` — 31 cases pass (was 25, added 6 matrix cases + replaced 1) - [x] `npm run check` — 0 errors, 0 warnings - [x] `npx prettier --check src/lib/server/keycloak.ts src/lib/server/keycloak.test.ts scripts/verify-aud-azp.mjs` — clean - [x] `npx eslint src/lib/server/keycloak.ts src/lib/server/keycloak.test.ts` — no errors - [x] `npm run build` — succeeds (adapter-node, server bundle 127.66 kB) - [x] `npx tsx scripts/verify-aud-azp.mjs` — exit 0, all 6 matrix cases PASS - [ ] Post-merge: redeploy → curl SSO round-trip → verify `/` returns 200 after callback Set-Cookie (ACs 1, 2 from #26) - [ ] Post-merge: Playwright Chromium → admin user logs in → app renders at `/` → `event.locals.user.name` populated (AC 3 from #26) - [ ] Post-merge: non-admin user → renders the `(unauthorized)` 403 page ## Coordination with #22 `#22` (open — `refresh_expires_in` exposure on `KeycloakTokens`) also touches `src/lib/server/keycloak.ts`. Different function (`refreshTokensIfNeeded` + the `KeycloakTokens` interface), no semantic conflict with `verifyKeycloakJwt`. Whichever lands second rebases — likely a trivial three-way merge since the line ranges are non-overlapping. No code changes needed in this PR for #22 awareness. ## Out of Scope (per ticket) - `src/routes/auth/*` — unchanged; they don't call `verifyKeycloakJwt` directly. - `src/hooks.server.ts` — unchanged; gets the new behavior through the unchanged `verifyKeycloakJwt` signature. - `pal-e-deployments/*`, `(unauthorized)/*` — unchanged. - Path A (audience-mapper Keycloak admin-console fix) — explicitly NOT pursued; Path B keeps SOPs simple and works for any future client. ## Review Checklist - [x] Scope is bounded to `src/lib/server/keycloak.ts` and `src/lib/server/keycloak.test.ts` plus the new `scripts/verify-aud-azp.mjs` validation harness — no edits to `src/routes/auth/*`, `src/hooks.server.ts`, deployments, or unrelated files - [x] No tokens, ciphertext, or signing-key material added to log statements (the `keycloak.ts` source-hygiene unit test still passes) - [x] Existing `rejects-on-wrong-aud` case was UPDATED, not deleted — now sets both `aud` and `azp` to non-matching values so it isn't a false-pass under the new logic - [x] Funnel-Auth Review (above) addresses why accepting `azp` is OIDC-spec-conformant and not a security regression, per `feedback_funnel_requires_auth` - [x] Standalone validation script output included in PR body, per `feedback_validate_before_done` - [x] Coordination note for sibling open PR #22 included - [ ] Reviewer: confirm the manual aud/azp branch matches OIDC Core §2 wording for `azp` - [ ] Reviewer: confirm no behavioral change for the existing 6 JWT verify cases (signature, issuer, exp, malformed, missing-key) — only the aud check moved out-of-jose - [ ] Reviewer: spot-check that `JwtVerificationError` thrown manually is NOT re-wrapped by the catch block (the `if (err instanceof JwtVerificationError) throw err;` guard at the top of catch handles this) ## Related Notes - `feedback_funnel_requires_auth` — funnel-auth gate is non-functional without this fix even though every individual component was "approved" - `feedback_validate_before_done` — drove the standalone harness output in this PR body; gate that didn't catch this last time catches it this time - `sop-keycloak-client-creation` — no SOP change required by Path B (works for any client; Path A would have needed a per-client audience mapper step) ## Related - Closes `forgejo_admin/westside-admin#26` - Sibling `forgejo_admin/westside-admin#22` (open — `refresh_exp`; overlapping lib file, different function) - Sibling `forgejo_admin/westside-admin#24` / PR `#25` (closed — cookie size fix; SAME bug surface, different root cause; this PR completes the loop unmasked by #25)
fix(auth): accept azp claim for client identity (closes #26)
All checks were successful
ci/woodpecker/pr/woodpecker Pipeline was successful
Details
60c43ac1b8 
Keycloak default access tokens carry `aud=account, azp=westside-admin`
because no audience mapper is configured on the client. The previous
`verifyKeycloakJwt` passed `audience: expectedAudience()` to jose's
jwtVerify, which rejected every live token with JWTClaimValidationFailed
on aud — masking #24 until that bug was fixed, then breaking SSO.

Per OIDC Core §2, `azp` (authorized party) is "the party to which the
ID Token was issued." Accepting `aud` containing client_id OR `azp`
equal to client_id is spec-conformant and handles both Keycloak default
deployments and operators who add an audience mapper later.

Changes:
- src/lib/server/keycloak.ts: drop `audience` arg from jwtVerify; manual
  aud/azp check after; pass-through our typed JwtVerificationError.
- src/lib/server/keycloak.test.ts: extend JwtOpts with `azp`; update the
  existing wrong-aud case to also set non-matching azp (avoids false
  pass); add 6-row accept/reject matrix.
- scripts/verify-aud-azp.mjs: standalone validation harness mirroring
  measure-cookie-size.mjs — exercises the matrix against the real
  exported verifyKeycloakJwt so reviewers see live ACCEPT/REJECT.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
forgejo_admin commented 2026-05-03 21:37:41 +00:00
Author
Owner
Copy link

PR #27 Review

DOMAIN REVIEW

Stack: SvelteKit (adapter-node) + TypeScript + jose 5.x + Vitest. Server-side OIDC/JWT primitives. Funnel-fronted via Tailscale + Keycloak — feedback_funnel_requires_auth applies.

Cryptographic correctness — OIDC Core §2 conformance.

The fix replaces jose's built-in audience: expectedAudience() enforcement with a manual post-verify check: aud === clientId || aud.includes(clientId) || azp === clientId. Per OIDC Core §2 (azp = "the party to which the ID Token was issued"), accepting azp === clientId as a client-identity assertion is spec-conformant. Major IdPs (Keycloak, Auth0, Okta) populate azp with the requesting client_id by default. The funnel-auth review in the PR body correctly establishes:

  • Signature, issuer, exp checks unchanged — all #14 invariants intact.
  • Attacker model unchanged: forging requires the realm signing key regardless of which claim we read for client identity.
  • Cross-client confusion: a token issued to a different Keycloak client in the same realm has azp=other-client, aud=... → both branches false → REJECT. Verified by matrix row 6 (aud=other, azp=other).
  • The downstream realm_access.roles admin gate in hooks.server.ts is untouched — aud=account tokens do NOT bypass admin authz, they merely pass JWT-shape validation so the role gate can fire.

No security regression. azp is the right OIDC claim for "did this token originate from a request my client made?"

Test matrix completeness.

All 6 cases from #26's body are present in keycloak.test.ts (lines 232–331):

# aud azp Expected Present
1 account westside-admin ACCEPT yes (the bug case)
2 westside-admin westside-admin ACCEPT yes
3 westside-admin other ACCEPT yes (mapper case)
4 account other REJECT yes
5 [westside-admin, account] westside-admin ACCEPT yes (array case)
6 other other REJECT yes

No-regression on the original wrong-aud test.

The existing rejects a token with the wrong audience case (pre-PR line 212–222) was correctly UPDATED, not deleted. Both aud and azp now set to 'some-other-client'. Without this update the test would have become a false-pass under the new logic (a wrong-aud-only token would fail-but-not-because-of-the-claim-the-test-named). The rename to "wrong audience AND wrong azp (truly wrong client)" makes the intent explicit. Correctly done.

Standalone validation script (scripts/verify-aud-azp.mjs).

Imports the REAL verifyKeycloakJwt from ../src/lib/server/keycloak.ts (not a re-implementation), stubs JWKS via __setFetchForTests, signs JWTs with the same jose primitives prod uses, runs all 6 matrix cases, exits non-zero on any fail. Production code path is exercised end-to-end. PR body shows all 6 cases PASS against the live aud=account shape that broke prod.

Error message hygiene.

The new rejection error reads claim aud/azp invalid (aud=..., azp=...). The REJECT matrix test (line 264) asserts reason: expect.stringContaining('aud/azp') — confirms the new tag distinguishes from the legacy jose-mapped aud error so the existing wrong-iss test (which asserts stringContaining('iss')) still works alongside it. Token bytes are not embedded in the error — only claim values. The existing source-hygiene test (line 595) still passes per PR body.

BLOCKERS

None.

NITS

  1. keycloak.ts rendered as Binary files differ in the Forgejo diff. Almost certainly a Forgejo display quirk (non-ASCII char in a hunk?) rather than an actual binary. The PR body describes the source change clearly and the tests + script + matrix output are all consistent with that change. Recommend reviewers spot-check via git show 26-jwt-aud-azp-fix:src/lib/server/keycloak.ts to eyeball the manual aud/azp branch and the if (err instanceof JwtVerificationError) throw err; guard at the top of the catch block. Not blocking — the test surface and standalone script provide independent evidence.

  2. Manual check ordering. For an array aud, aud === clientId is always false (string vs array), so aud.includes(clientId) carries the array case. Fine. A one-line comment in source explaining the short-circuit order would aid future maintainers — defer to source spot-check.

  3. Process gap covered by #19. This is the third PR (#23, #25, #27) closing a bug surfaced after the "approved" lib hit the live Keycloak default token. verify-aud-azp.mjs is exactly the kind of harness that should run in CI to catch this class pre-merge. Recommend a one-line follow-up comment on #19 to wire npx tsx scripts/verify-aud-azp.mjs into the Woodpecker pipeline alongside npm test. Not blocking this PR.

SOP COMPLIANCE

  • Branch named after issue: 26-jwt-aud-azp-fix follows {issue-number}-{kebab-case-purpose}.
  • PR body follows template: Summary, Changes, Test Plan, Related — all present.
  • Closes #26 linkage present.
  • Funnel-Auth Review section present and substantive (per feedback_funnel_requires_auth — funnel-fronted ingress).
  • Standalone validation output in PR body (per feedback_validate_before_done).
  • #22 collision note present and accurate (different function, no semantic conflict).
  • #25 sibling reference present (cookie-size fix that masked this bug).
  • No tokens, ciphertext, or signing-key material in source/logs (source-hygiene test still passes).
  • Scope bounded: src/lib/server/keycloak.ts, src/lib/server/keycloak.test.ts, scripts/verify-aud-azp.mjs. No edits to src/hooks.server.ts, src/routes/auth/*, src/routes/(unauthorized)/*, or pal-e-deployments/*.
  • npm test 31 pass, npm run check 0/0, prettier + eslint clean, npm run build succeeds, npx tsx scripts/verify-aud-azp.mjs exits 0 with all 6 matrix PASS.
  • No secrets / .env / credentials committed.
  • Post-merge ACs (curl SSO round-trip, Playwright admin login, non-admin 403) correctly LEFT UNCHECKED — per feedback_validate_before_done these tick post-deploy, not at QA gate.

PROCESS OBSERVATIONS

  • Deployment frequency impact: unblocks the funnel-auth gate that has been non-functional in prod since #14 merged. Restores SSO end-to-end without admin-console changes (Path B chosen over Path A's per-client audience-mapper procedure — keeps sop-keycloak-client-creation simple).
  • Change failure risk: low. The fix narrows the previously-too-strict aud-only check to an OIDC-spec-conformant aud-OR-azp check. Signature, issuer, exp, malformed, missing-key paths are unchanged. Six new matrix tests + 1 updated test + standalone harness give high confidence.
  • Documentation gap: none for this PR. Follow-up: file a CI ticket (or comment on #19) to add npx tsx scripts/verify-aud-azp.mjs to the Woodpecker pipeline so this class of bug — unit tests pass but live-IdP shape unmasks an integration gap — gets caught pre-merge next time. Three iterations (#23, #25, #27) on a single auth surface suggests the harness gap is the systemic issue, not any one PR's correctness.

VERDICT: APPROVED

## PR #27 Review ### DOMAIN REVIEW **Stack:** SvelteKit (adapter-node) + TypeScript + jose 5.x + Vitest. Server-side OIDC/JWT primitives. Funnel-fronted via Tailscale + Keycloak — `feedback_funnel_requires_auth` applies. **Cryptographic correctness — OIDC Core §2 conformance.** The fix replaces jose's built-in `audience: expectedAudience()` enforcement with a manual post-verify check: `aud === clientId || aud.includes(clientId) || azp === clientId`. Per OIDC Core §2 (`azp` = "the party to which the ID Token was issued"), accepting `azp === clientId` as a client-identity assertion is spec-conformant. Major IdPs (Keycloak, Auth0, Okta) populate `azp` with the requesting `client_id` by default. The funnel-auth review in the PR body correctly establishes: - Signature, issuer, exp checks unchanged — all #14 invariants intact. - Attacker model unchanged: forging requires the realm signing key regardless of which claim we read for client identity. - Cross-client confusion: a token issued to a different Keycloak client in the same realm has `azp=other-client, aud=...` → both branches false → REJECT. Verified by matrix row 6 (`aud=other, azp=other`). - The downstream `realm_access.roles` admin gate in `hooks.server.ts` is untouched — `aud=account` tokens do NOT bypass admin authz, they merely pass JWT-shape validation so the role gate can fire. No security regression. `azp` is the right OIDC claim for "did this token originate from a request my client made?" **Test matrix completeness.** All 6 cases from #26's body are present in `keycloak.test.ts` (lines 232–331): | # | aud | azp | Expected | Present | |---|-----|-----|----------|---------| | 1 | `account` | `westside-admin` | ACCEPT | yes (the bug case) | | 2 | `westside-admin` | `westside-admin` | ACCEPT | yes | | 3 | `westside-admin` | `other` | ACCEPT | yes (mapper case) | | 4 | `account` | `other` | REJECT | yes | | 5 | `[westside-admin, account]` | `westside-admin` | ACCEPT | yes (array case) | | 6 | `other` | `other` | REJECT | yes | **No-regression on the original wrong-aud test.** The existing `rejects a token with the wrong audience` case (pre-PR line 212–222) was correctly UPDATED, not deleted. Both `aud` and `azp` now set to `'some-other-client'`. Without this update the test would have become a false-pass under the new logic (a wrong-aud-only token would fail-but-not-because-of-the-claim-the-test-named). The rename to "wrong audience AND wrong azp (truly wrong client)" makes the intent explicit. Correctly done. **Standalone validation script (`scripts/verify-aud-azp.mjs`).** Imports the REAL `verifyKeycloakJwt` from `../src/lib/server/keycloak.ts` (not a re-implementation), stubs JWKS via `__setFetchForTests`, signs JWTs with the same jose primitives prod uses, runs all 6 matrix cases, exits non-zero on any fail. Production code path is exercised end-to-end. PR body shows all 6 cases PASS against the live `aud=account` shape that broke prod. **Error message hygiene.** The new rejection error reads `claim aud/azp invalid (aud=..., azp=...)`. The REJECT matrix test (line 264) asserts `reason: expect.stringContaining('aud/azp')` — confirms the new tag distinguishes from the legacy jose-mapped `aud` error so the existing wrong-iss test (which asserts `stringContaining('iss')`) still works alongside it. Token bytes are not embedded in the error — only claim values. The existing source-hygiene test (line 595) still passes per PR body. ### BLOCKERS None. ### NITS 1. **`keycloak.ts` rendered as `Binary files differ` in the Forgejo diff.** Almost certainly a Forgejo display quirk (non-ASCII char in a hunk?) rather than an actual binary. The PR body describes the source change clearly and the tests + script + matrix output are all consistent with that change. Recommend reviewers spot-check via `git show 26-jwt-aud-azp-fix:src/lib/server/keycloak.ts` to eyeball the manual aud/azp branch and the `if (err instanceof JwtVerificationError) throw err;` guard at the top of the catch block. Not blocking — the test surface and standalone script provide independent evidence. 2. **Manual check ordering.** For an array `aud`, `aud === clientId` is always false (string vs array), so `aud.includes(clientId)` carries the array case. Fine. A one-line comment in source explaining the short-circuit order would aid future maintainers — defer to source spot-check. 3. **Process gap covered by #19.** This is the third PR (#23, #25, #27) closing a bug surfaced after the "approved" lib hit the live Keycloak default token. `verify-aud-azp.mjs` is exactly the kind of harness that should run in CI to catch this class pre-merge. Recommend a one-line follow-up comment on #19 to wire `npx tsx scripts/verify-aud-azp.mjs` into the Woodpecker pipeline alongside `npm test`. Not blocking this PR. ### SOP COMPLIANCE - [x] Branch named after issue: `26-jwt-aud-azp-fix` follows `{issue-number}-{kebab-case-purpose}`. - [x] PR body follows template: Summary, Changes, Test Plan, Related — all present. - [x] `Closes #26` linkage present. - [x] Funnel-Auth Review section present and substantive (per `feedback_funnel_requires_auth` — funnel-fronted ingress). - [x] Standalone validation output in PR body (per `feedback_validate_before_done`). - [x] `#22` collision note present and accurate (different function, no semantic conflict). - [x] `#25` sibling reference present (cookie-size fix that masked this bug). - [x] No tokens, ciphertext, or signing-key material in source/logs (source-hygiene test still passes). - [x] Scope bounded: `src/lib/server/keycloak.ts`, `src/lib/server/keycloak.test.ts`, `scripts/verify-aud-azp.mjs`. No edits to `src/hooks.server.ts`, `src/routes/auth/*`, `src/routes/(unauthorized)/*`, or `pal-e-deployments/*`. - [x] `npm test` 31 pass, `npm run check` 0/0, prettier + eslint clean, `npm run build` succeeds, `npx tsx scripts/verify-aud-azp.mjs` exits 0 with all 6 matrix PASS. - [x] No secrets / .env / credentials committed. - [x] Post-merge ACs (curl SSO round-trip, Playwright admin login, non-admin 403) correctly LEFT UNCHECKED — per `feedback_validate_before_done` these tick post-deploy, not at QA gate. ### PROCESS OBSERVATIONS - **Deployment frequency impact:** unblocks the funnel-auth gate that has been non-functional in prod since #14 merged. Restores SSO end-to-end without admin-console changes (Path B chosen over Path A's per-client audience-mapper procedure — keeps `sop-keycloak-client-creation` simple). - **Change failure risk:** low. The fix narrows the previously-too-strict aud-only check to an OIDC-spec-conformant aud-OR-azp check. Signature, issuer, exp, malformed, missing-key paths are unchanged. Six new matrix tests + 1 updated test + standalone harness give high confidence. - **Documentation gap:** none for this PR. Follow-up: file a CI ticket (or comment on #19) to add `npx tsx scripts/verify-aud-azp.mjs` to the Woodpecker pipeline so this class of bug — unit tests pass but live-IdP shape unmasks an integration gap — gets caught pre-merge next time. Three iterations (#23, #25, #27) on a single auth surface suggests the harness gap is the systemic issue, not any one PR's correctness. ### VERDICT: APPROVED
forgejo_admin deleted branch 26-jwt-aud-azp-fix 2026-05-03 21:40:30 +00:00
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
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!27
No description provided.