fix(security): move Hasura Auth tokens from response body to httpOnly cookies - #26
Merged
Merged
Conversation
… cookies signin.ts and signup.ts previously proxied Hasura Auth and returned accessToken/refreshToken directly in the JSON response body, which any client-side code would have to store somewhere readable by JS (localStorage/sessionStorage/a variable) — an XSS token-theft risk. No signin/signup UI was wired up to these routes yet, but the insecure pattern was live in the API layer. Ports the httpOnly-cookie session pattern already shipped in praycalc/web (ADR-010, no-localstorage-token): tokens are now set as httpOnly/secure/sameSite=lax cookies (ci_access_token/ci_refresh_token) by same-origin /api/auth/* routes, and only a non-sensitive profile (email/displayName/initials + a plain expiry timestamp) is kept in localStorage. Adds the missing refresh.ts and signout.ts routes so a session can actually be renewed/ended. Also migrates TutorIsland, SettingsIsland, and the tutor API routes (progress/message/session) off the chatislam_token localStorage/Bearer pattern onto the httpOnly cookie, and fixes signin/signup to read import.meta.env.PUBLIC_AUTH_URL (the actually-wired env var per astro.config.ts + .env.example) instead of the dead process.env.NEXT_PUBLIC_AUTH_URL leftover from this app's pre-Astro Next.js code. JWT signature verification for the tutor routes' cookie-sourced token is tracked as a separate follow-up, not bundled into this change.
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
🟢 Rampart Security Gate — CLEAN
Totals: 0 critical · 0 high · 0 medium · 0 low Mode: |
…idebar useFeynman.ts and ChatSidebar.tsx still read a raw bearer token from localStorage (chatislam_token) and sent it as an Authorization header, the same pattern already fixed in TutorIsland/SettingsIsland/tutor routes by the prior commit on this branch. useFeynman.ts now sends credentials: 'same-origin' to /api/research so the httpOnly ci_access_token cookie is used instead; /api/research's parseUserId is updated to read the cookie (via readAccessToken) rather than an Authorization header, matching tutor/progress.ts and tutor/session.ts. ChatSidebar.tsx called Hasura directly from the client with the raw token — architecturally incompatible with an httpOnly cookie, since JS can never read it. Added GET /api/chat/sessions as a same-origin proxy (same admin-secret + user_id-filtered pattern as tutor/progress.ts) so the component can drop the client-held token entirely. Sign-in gating in both components now uses getSession() from @/lib/session.
acamarata
added a commit
that referenced
this pull request
Sep 2, 2026
…laims Six API routes each hand-rolled a base64 decode of the JWT payload and read `x-hasura-user-id` straight out of it without ever calling jwtVerify(). No signature check means a forged token grants any identity: full auth bypass on consent, research, chat/sessions, tutor/message, tutor/progress and tutor/session. chat/sessions.ts even carried the comment "no signature verification here — tracked as a separate follow-up". All six now delegate to a shared, signature-verified helper (src/lib/auth/verify-jwt.ts — jose.jwtVerify, HS256 only), mirroring ummat/app/web's getSession() reference pattern (S12-03/H1). The helper fails CLOSED: a missing HASURA_GRAPHQL_JWT_SECRET re-throws rather than returning null, so a misconfigured deployment cannot silently fall back to trusting unverified claims. Documented in web/.env.example. PR #26 moved these tokens from the response body into httpOnly cookies, which stops client-side theft but does nothing about forgery — the cookie value was still decoded unverified. This closes that half. Note chat/sessions.ts is NOT in the original local version of this fix; it arrived with #26 afterwards. Re-applying against current main instead of force-resolving the rebase conflict is what caught it, so this covers six routes rather than five. Verified: tsc --noEmit clean (0 errors); 21 test files / 402 tests pass, including 10 new verify-jwt cases covering forged, expired and malformed tokens. No raw `split('.')[1]` decode remains anywhere under web/src.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
signin.ts/signup.tspreviously proxied Hasura Auth and returnedaccessToken/refreshTokendirectly in the JSON response body — an XSS token-theft risk for whatever client code eventually stored them. No signin/signup UI was wired up yet, but the insecure pattern was live in the API layer and needed fixing before a UI got built on top of it.praycalc/web(ADR-010, no-localstorage-token): tokens are now set ashttpOnly/secure/sameSite=laxcookies (ci_access_token/ci_refresh_token) by same-origin/api/auth/*routes. Only a non-sensitive profile (email/displayName/initials + a plain expiry timestamp) is kept inlocalStorage(lib/session.ts).refresh.tsandsignout.tsroutes so a session set up this way can actually be renewed and ended.TutorIsland,SettingsIsland, and the tutor API routes (progress/message/session) off thechatislam_tokenlocalStorage/Bearer pattern onto the httpOnly cookie.signin/signupto readimport.meta.env.PUBLIC_AUTH_URL(the actually-wired env var perastro.config.ts+.env.example) instead of the deadprocess.env.NEXT_PUBLIC_AUTH_URLleftover from this app's pre-Astro Next.js code — that env var was never wired to a deployed value.Out of scope (filed separately)
chatislam_tokenlocalStorage reads inhooks/useFeynman.tsandcomponents/chat/ChatSidebar.tsx, outside this PR's explicit scope./api/settings/byo-keyremains unimplemented (pre-existing gap, unrelated to this fix) — only the client fetch call was updated for consistency.Test plan
pnpm vitest run— 392 passed, 1 pre-existing skippnpm run typecheck(astro check) — 0 errors__tests__/session.test.ts,__tests__/auth-client.test.ts— cover profile round-trip, no-raw-tokens-returned, same-origin credentials usage, error handling