Skip to content

fix(auth): require JWT-derived identity on every write endpoint#38

Closed
bryanfawcett wants to merge 5 commits into
mainfrom
claude/critical-auth-jwt-bearer
Closed

fix(auth): require JWT-derived identity on every write endpoint#38
bryanfawcett wants to merge 5 commits into
mainfrom
claude/critical-auth-jwt-bearer

Conversation

@bryanfawcett
Copy link
Copy Markdown
Contributor

Summary

Closes the #1 finding from the recent security review: ~10 worker write endpoints were trusting body.userId / body.hostId for the actor identity (and a couple had no auth at all), while the frontend's admin pages sent cookies that the JWKS-validated worker doesn't read and every write helper in src/lib/api.ts had an optional sessionJwt that no call site passed. Net effect: any browser on a trusted origin could act as any user.

After this PR every mutating worker endpoint derives the actor from the WorkOS JWT (subidentity.person.id lookup), and every frontend write call threads the access token through.

Worker — auth foundation (commit 1)

  • New worker/src/auth/identity.ts:
    • resolvePersonId(env, workosUserId) — canonical JWT-subject → identity.person.id lookup
    • requireRequesterPersonId(c) — returns either the person id or a 401 Response for the handler to short-circuit on
  • worker/src/middleware/auth.ts:
    • validateApiKey() no longer falls back to Authorization: Bearer (was conflating machine API keys with per-user JWTs — a leaked user JWT in the X-API-Key slot could have bypassed origin checks)
    • writeAuth now treats PATCH as mutating alongside POST / PUT / DELETE

Worker — every write endpoint hardened (commit 2)

File Endpoint Change
events.ts POST / organizer_person_id from JWT; body.organizerPersonId ignored
events.ts PUT /:id Must be event organizer, else 403
events.ts POST /:id/cancel Organizer-only; audit row carries requester as actorId
events.ts DELETE /:id Organizer-only; audit row carries actorId
events.ts POST /:id/reviews Author from JWT; body.userId ignored
registrations.ts GET / JWT required; user_id query must equal requester OR (with event_id) requester is organizer OR admin
registrations.ts POST / agent_person_id from JWT; userId dropped from required fields
registrations.ts PUT /:id Refactored to shared helper
registrations.ts DELETE /:id Registrant OR organizer; emits registration.cancelled audit row
checkin.ts POST /events/:id/checkin Organizer-only (paired-kiosk flow is separate)
waitlist.ts POST /events/:id/waitlist Person from JWT; body.userId dropped
waitlist.ts DELETE /events/:id/waitlist Same
series.ts POST / hostId from JWT; on template promotion the caller must already own the event
series.ts PUT /:id Series parent's organizer only
series.ts DELETE /:id Series parent's organizer only
reviews.ts POST /:id/helpful JWT required; body.userId dropped
payments.ts POST /create Requester must equal rsvp.agent_person_id
users.ts DELETE /:id Self OR admin; audit row's actorId is the requester
links.ts POST / Added writeAuth (route had no auth at all); created_by from JWT

Verified clean: grep -rn 'body.userId\|body\.hostId\|body\["userId"\]' worker/src/routes/ returns no matches.

Worker — tests (commit 3)

Updated 6 test files + the shared mock layer to cover the new flow. Every write test mocks getAuthenticatedUser via vi.mock("../auth/workos") and adds a person GET to satisfy resolvePersonId. New authedOriginHeaders(token) helper in mocks.ts for the common case where a route needs both writeAuth origin AND a Bearer token. New tests include explicit 401/403 coverage and assert that body-side identity is ignored. Worker test count: 231 → 243 (all passing).

Frontend — api.ts + auth-context (commit 4)

  • src/lib/api.ts — every write helper has sessionJwt as a required parameter now (was optional and unused). Body-side userId / hostId / createdBy dropped from the signatures since the worker ignores them.
  • src/components/auth/auth-context.tsxAuthContext now exposes accessToken and getAccessToken() so consumers don't need to re-import the WorkOS hook.
  • src/components/auth/auth-guard.tsx — supports optional requiredRole prop; signed-in users without the role see a "Not authorized" placeholder instead of being redirected (avoids loops).
  • src/components/ui/event-ratings.tsx and src/lib/use-tracked-link.ts — threaded the access token through existing flows.

Frontend — call sites + admin Bearer + kraal bug (commit 5)

Threaded access token through:

  • src/app/events/[id]/rsvp-button.tsx
  • src/app/events/[id]/manage/page.tsx (3 call sites)
  • src/app/events/create/create-event-form.tsx
  • src/app/events/[id]/kiosk/host/page.tsx

Admin pages switched from credentials: "include" to Authorization: Bearer ${accessToken}:

  • src/app/admin/page.tsx
  • src/app/admin/users/page.tsx
  • src/app/admin/events/page.tsx
  • src/app/admin/support/page.tsx

The admin layout (src/app/admin/layout.tsx) already enforces a role gate client-side, so the new fetches wait for useAuth().isLoading to resolve before firing.

Kraal personId snake_case bug fixed:

  • src/app/kraal/kraal-index-client.tsx
  • src/app/kraal/[id]/kraal-detail-client.tsx

The buggy cast was (user as { person_id?: string } | null)?.person_id — but NhimbeUser exposes the field as camelCase personId. Dropped the cast and read user?.personId directly.

Pre-existing bugs surfaced

  • src/app/events/[id]/kiosk/page.tsx — the paired-kiosk frontend calls /api/events/:id/checkin, which the spec defines as organizer-only (the kiosk flow is supposed to use the separate session-token path in worker/src/routes/kiosk.ts, but there's no check-in endpoint on that path yet). After this PR the paired kiosk will 401 because it has no JWT. Marked with a FIXME and left for a follow-up that adds a kiosk-session-authenticated check-in endpoint.
  • worker/src/utils/audit.ts writes to system.activity_logs, but the audit.ts doc-string mentions audit_logs — minor naming inconsistency, not touched.
  • worker/src/routes/categories.ts — the spec referenced an inline resolvePersonId at ~line 210, but the file is 99 lines and has no such inline helper on this branch (categories is now DB-only). No-op for that step.

Test plan

  • cd worker && npx tsc --noEmit — passes
  • cd worker && npx vitest run — 243/243 passing (was 231)
  • npm run lint — 0 errors
  • npx vitest run (root) — 162/162 passing
  • npm run build — succeeds
  • grep -rn 'body.userId\|body\.hostId\|body\["userId"\]' worker/src/routes/ — no matches
  • Manual smoke: sign in, create an event, RSVP, cancel, host-approve, host-cancel — verify each call carries a Bearer token in the network panel and the worker accepts it
  • Manual smoke: try the impersonation case — send a POST /api/events with someone else's organizerPersonId in the body and confirm the resulting row's organizer_person_id is the requester's id, not the body value
  • Manual smoke: confirm admin pages render data when signed in as an admin and 401 otherwise

https://claude.ai/code/session_01Dp6YFZCHz1HjL9svPWmso2


Generated by Claude Code

claude added 5 commits May 17, 2026 16:17
Introduces worker/src/auth/identity.ts with two helpers that every write
endpoint now uses:

  - resolvePersonId(env, workosUserId) — maps the JWT subject to
    identity.person.id via PostgREST (the canonical lookup that was
    previously duplicated inline in several routes).
  - requireRequesterPersonId(c) — wraps getAuthenticatedUser +
    resolvePersonId, returning either the resolved person id or a fully-
    formed 401 Response. Route handlers can short-circuit with one if-check.

Also tightens middleware/auth.ts:

  - validateApiKey() drops its fallback to Authorization: Bearer. The dual
    header path conflated machine API keys with per-user WorkOS access
    tokens, so a stolen JWT placed in the X-API-Key slot would have
    bypassed origin checks. Machine credentials must come on X-API-Key only.
  - writeAuth now includes PATCH in the mutating-verb list alongside
    POST/PUT/DELETE.

https://claude.ai/code/session_01Dp6YFZCHz1HjL9svPWmso2
Closes the systemic authorisation gap where ~10 worker write endpoints
trusted body.userId / body.hostId for actor identity instead of deriving
it from the WorkOS JWT. After this commit every mutating handler resolves
the requester via requireRequesterPersonId() and rejects requests that
either lack a JWT or whose JWT subject has no live identity.person row.

Per-endpoint changes:

  events.ts
    POST /            — organizer_person_id comes from JWT; body's
                        organizerPersonId / organizer.identifier are
                        ignored (impersonation hole closed).
    PUT /:id          — must equal event.organizer_person_id, else 403.
    POST /:id/cancel  — organizer-only; audit row carries the requester
                        as actorId.
    DELETE /:id       — organizer-only; audit row carries actorId.
    POST /:id/reviews — author comes from JWT; body.userId ignored.
    (CSV export already had organizer auth — refactored to the shared
     helper.)

  registrations.ts
    GET  /            — JWT required. user_id query must equal requester
                        OR (with event_id) requester must be the event
                        organizer OR an admin (getAdminUser fallback).
    POST /            — agent_person_id from JWT; userId dropped from
                        required-fields list and ignored if sent.
    PUT  /:id         — same host vs registrant logic, now via the
                        shared helper instead of inline lookup.
    DELETE /:id       — must be registrant OR event organizer; emits a
                        registration.cancelled audit row.

  checkin.ts
    POST /events/:id/checkin — organizer-only. The paired-kiosk session
                        flow is separate (routes/kiosk.ts) and untouched.

  waitlist.ts
    POST  /events/:id/waitlist — person_id from JWT; body.userId dropped.
    DELETE /events/:id/waitlist — same.
    GET    /events/:id/waitlist — refactored to the shared helper.

  series.ts
    POST   /          — hostId from JWT; body.hostId dropped. When a
                        templateEventId is supplied the caller must already
                        own that event row.
    PUT  /:id         — series parent's organizer only.
    DELETE /:id       — series parent's organizer only.

  reviews.ts
    POST /:id/helpful — JWT required; body.userId dropped.

  payments.ts
    POST /create      — requester must equal rsvp.agent_person_id, so a
                        third party can't initiate payment on someone
                        else's registration.

  users.ts
    DELETE /:id       — requester must be self OR admin; 403 otherwise.
                        Audit row's actorId is now the requester (not the
                        deleted user), and details.self_delete flags
                        which case applied.

  links.ts
    Mounted writeAuth on "*" (route had no auth middleware at all).
    POST / now requires a JWT; created_by is derived from it, dropping
    body.createdBy.

https://claude.ai/code/session_01Dp6YFZCHz1HjL9svPWmso2
Updates the 6 affected test files (events, registrations, payments,
reviews, users, security) plus the shared mock layer. Each write test
now mocks getAuthenticatedUser via vi.mock("../auth/workos") and adds a
person GET handler so resolvePersonId resolves cleanly.

mocks.ts
  - New authedOriginHeaders(token) helper for the common case where a
    route needs BOTH the trusted-origin path (writeAuth) AND a Bearer
    token (requireRequesterPersonId).

security.test.ts
  - Mirrors production: validateApiKey no longer falls back to
    Authorization: Bearer. The "validates correct key from Authorization
    header" case is flipped to assert rejection, matching the new policy.

events.test.ts (27 tests)
  - POST / now derives organizer from JWT; new test confirms a body-side
    organizerPersonId is ignored (impersonation guard).
  - PUT/cancel/DELETE assert 403 for non-organizers and audit rows
    carrying the requester as actorId.
  - Reviews assert author comes from JWT, not body.

registrations.test.ts (27 tests)
  - GET re-tested with the new authz matrix (self / event-organizer /
    admin / forbidden third party).
  - POST drops the userId-missing check; identity flows from JWT.
  - DELETE adds 401/403/404 cases and verifies the cancellation audit row.

payments.test.ts (20 tests)
  - 403 case for non-registrant payment initiation added.
  - Happy paths thread a person GET to satisfy resolvePersonId.

reviews.test.ts (4 tests)
  - 401 added; helpful-vote no longer accepts a body.userId.

users.test.ts (19 tests)
  - DELETE adds 401 (no JWT) and 403 (not self, not admin) cases.
  - The self-delete happy path now needs two sequential person GETs:
    first to resolve the requester, second to load the target row.

https://claude.ai/code/session_01Dp6YFZCHz1HjL9svPWmso2
… useAuth

src/lib/api.ts — every write helper that previously had an optional
sessionJwt parameter (which no call site was passing) is now required.
TypeScript will surface any missed call site at build time. The redundant
body.userId / body.hostId fields are dropped from the write helpers since
the worker derives identity from the JWT now and ignores them anyway.

Affected helpers: createEvent, updateEvent, deleteEvent, registerForEvent,
updateRegistrationStatus, cancelRegistration, submitEventReview,
markReviewHelpful, createTrackedLink, checkinRegistration.

src/components/auth/auth-context.tsx — AuthContext now exposes
`accessToken` (current snapshot from useAccessToken) and
`getAccessToken()` (force-refresh path) so consumer components can pass
the JWT into write helpers without re-importing the WorkOS hook
everywhere.

src/components/auth/auth-guard.tsx — supports an optional `requiredRole`
prop. When supplied, signed-in users without the role see a
"Not authorized" placeholder instead of being redirected (avoids redirect
loops for the wrong-account case).

src/components/ui/event-ratings.tsx + src/lib/use-tracked-link.ts —
threaded the access token through the existing helpful-vote and
tracked-link flows.

https://claude.ai/code/session_01Dp6YFZCHz1HjL9svPWmso2
…ersonId

Call sites for the API helpers now read accessToken from useAuth() and
pass it in:

  src/app/events/[id]/rsvp-button.tsx   — registerForEvent
  src/app/events/[id]/manage/page.tsx   — updateRegistrationStatus +
                                           deleteEvent (3 call sites)
  src/app/events/create/create-event-form.tsx — createEvent
  src/app/events/[id]/kiosk/host/page.tsx — checkinRegistration

Admin pages switch from credentials:"include" cookies (which the
JWKS-validated worker doesn't read) to Authorization: Bearer ${accessToken}.
The bearer is sourced from useAuth() and the fetches wait for the
AuthProvider to finish syncing before firing.

  src/app/admin/page.tsx          — /api/admin/stats
  src/app/admin/users/page.tsx    — list + suspend/activate
  src/app/admin/events/page.tsx   — list + DELETE
  src/app/admin/support/page.tsx  — list + status PUT + reply POST

Kraal pages: the (user as { person_id?: string })?.person_id snake_case
cast was wrong — NhimbeUser exposes the field as camelCase `personId`.
Drop the cast and read user?.personId directly.

  src/app/kraal/kraal-index-client.tsx
  src/app/kraal/[id]/kraal-detail-client.tsx

Pre-existing bug noted in src/app/events/[id]/kiosk/page.tsx: the
paired-kiosk flow calls /api/events/:id/checkin which is now
organizer-only (was already the wrong endpoint — kiosks should use a
session-token endpoint that doesn't exist yet). A FIXME marks the gap.

https://claude.ai/code/session_01Dp6YFZCHz1HjL9svPWmso2
@vercel
Copy link
Copy Markdown

vercel Bot commented May 17, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
nhimbe Ready Ready Preview, Comment May 17, 2026 4:20pm

Request Review

@cloudflare-workers-and-pages
Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
mukoko-nhimbe-api 3f51191 Commit Preview URL

Branch Preview URL
May 17 2026, 04:19 PM

@claude
Copy link
Copy Markdown

claude Bot commented May 17, 2026

Claude finished @bryanfawcett's task —— View job


Security Review of Authentication Hardening

Todo List:

  • Review new identity resolution system (worker/src/auth/identity.ts)
  • Check authentication middleware changes
  • Review hardened write endpoints for security issues
  • Analyze frontend JWT threading implementation
  • Check for performance implications
  • Verify adherence to project conventions
  • Look for potential bugs or edge cases
  • Review test coverage for new auth flow

SECURITY REVIEW - NO CRITICAL ISSUES FOUND

This PR successfully closes a critical security vulnerability. The implementation is well-architected with strong security practices throughout.

Security Strengths

🔐 Robust Identity Resolution

  • JWT-first authentication: All write endpoints now derive identity from WorkOS JWT instead of trusting request bodies
  • Soft-delete protection: deleted_at=is.null filters prevent access by deactivated users
  • Type-safe error handling: requireRequesterPersonId() returns string | Response preventing auth bypass bugs

🛡️ Hardened Authorization

  • Verified body parameter removal: grep confirms no remaining body.userId|body.hostId references in routes
  • Owner-only operations: Event updates/deletes require requesterPersonId === organizerPersonId
  • Registration authorization: Payment creation verifies requester === rsvp.agent_person_id at payments.ts:68

🔒 Enhanced Middleware Security

  • API key isolation: Removed dangerous fallback from Authorization: Bearer to X-API-Key header (prevents JWT-in-API-key bypass)
  • PATCH verb coverage: Added PATCH alongside POST/PUT/DELETE in writeAuth middleware
  • Timing-safe comparisons: API key validation uses crypto.subtle.timingSafeEqual()

Performance Assessment

Minimal Performance Impact

  • Single DB lookup per request: resolvePersonId() adds one fast indexed query (workos_user_id lookup)
  • No N+1 issues: Identity resolution happens once per request, not per operation
  • Cached JWT validation: WorkOS JWKS caching (1hr TTL) prevents repeated key fetches

Code Quality & Conventions

Excellent Adherence to Project Standards

  • Structured logging: Proper [mukoko] prefixes maintained
  • Error handling: Consistent use of notFound(), forbidden(), badRequest() helpers
  • Type safety: All new code follows strict TypeScript patterns from CLAUDE.md
  • Circuit breaker ready: New supabaseFetch() calls integrate with existing resilience patterns

Frontend JWT Threading

  • Required parameters: All write methods now mandate sessionJwt: string parameter
  • Admin Bearer auth: Admin pages correctly switched from cookies to Authorization: Bearer ${accessToken}
  • Context exposure: AuthContext properly exposes accessToken and getAccessToken() for consumers

Test Coverage Assessment

Comprehensive Test Updates

  • Mock architecture: Tests properly mock getAuthenticatedUser() and add person lookup stubs
  • Auth coverage: New tests verify 401/403 responses and ignore body-side identity parameters
  • Helper functions: authedOriginHeaders() simplifies testing authenticated routes

Edge Cases Identified

⚠️ One Known Issue (Acknowledged)

  • Kiosk check-in: src/app/events/[id]/kiosk/page.tsx will 401 after this PR since it lacks JWT but calls organizer-only /api/events/:id/checkin
  • Status: Marked with FIXME comment; planned for follow-up to add kiosk-session check-in endpoint

Race Conditions Handled

  • Registration capacity: Atomic checks in registrations.ts prevent overselling
  • Audit logging: Proper actorId tracking for all destructive operations

Migration Safety

Safe Deployment Path

  • Backward compatible: Frontend changes are additive (access tokens now required vs optional)
  • Database unchanged: No schema migrations required
  • Rollback ready: Changes are code-only and reversible

Recommendation: ✅ APPROVE

This PR effectively addresses the #1 security finding with a well-architected solution. The JWT-derived identity system provides strong authentication guarantees while maintaining good performance characteristics. Code quality is excellent and follows all project conventions.

Next Steps:

  1. Address the kiosk check-in 401 issue in a follow-up PR
  2. Consider adding integration tests for the full auth flow in a future iteration

Copy link
Copy Markdown
Contributor Author

Closing — consolidating into PR #36 per the "all in one PR" directive. Cherry-picking the 5 commits onto claude/worker-observability-and-categories. The known kiosk regression flagged in this PR's body will be fixed inside #36 (kiosk-session-authenticated check-in endpoint) so we don't ship a regression to prod. Branch claude/critical-auth-jwt-bearer will stay around for reference until #36 merges.


Generated by Claude Code

bryanfawcett pushed a commit that referenced this pull request May 17, 2026
The auth-hardening pass earlier in this PR made
POST /api/events/:eventId/checkin organizer-only — correct for the host's
own page, wrong for a paired kiosk that holds a kiosk session token (not
a WorkOS JWT). The paired-kiosk page would 401 on every QR scan until
this lands.

Worker:
- New POST /api/kiosk/checkin on worker/src/routes/kiosk.ts. Reads the
  X-Kiosk-Token header (kept off the Authorization slot so the two auth
  contexts stay separate), SHA-256-hashes it, validates the session via
  device.session join device.device, and resolves the bound event from
  the device's context_entity_id.
- Cross-event guard: the request's registration must belong to the
  device's bound event, so a stolen token can't be used to check guests
  in at unrelated events. Only device_type='kiosk' can check in —
  signage screens stay display-only.
- Idempotency: returns 409 with the existing checked_in_at when the
  guest is already checked in (matches the host endpoint's contract).

Frontend:
- New checkinViaKiosk(eventId, registrationId, kioskToken) helper in
  src/lib/api.ts that sends X-Kiosk-Token.
- src/app/events/[id]/kiosk/page.tsx now uses it; FIXME removed.

Closes the known regression that PR-#38's auth campaign introduced.

https://claude.ai/code/session_01Dp6YFZCHz1HjL9svPWmso2
@bryanfawcett bryanfawcett deleted the claude/critical-auth-jwt-bearer branch May 21, 2026 08:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants