feat: use httpOnly cookies instead of localStorage for user sessions - #269
feat: use httpOnly cookies instead of localStorage for user sessions#269Diyaaa-12 wants to merge 8 commits into
Conversation
|
Greetings, @Diyaaa-12. Thy scroll hath arrived unblemished — the runes align and no conflict bars the path. The council shall now convene over its contents, weighing each incantation by candlelight. Tarry a while, brave adventurer; we shall investigate and return to thee with our verdict. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAuthentication moves session tokens from UI local storage to an HttpOnly cookie. API routes set and clear the cookie, middleware reads it as a fallback, credentialed UI requests stop sending Bearer headers, and profile-only session state is retained. ChangesCookie Session Authentication
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant UI
participant API
participant Browser
UI->>API: Submit sign-in or sign-up
API->>Browser: Set auth_token cookie
UI->>API: Send credentialed API request
Browser->>API: Attach auth_token cookie
API->>UI: Return authenticated response
UI->>API: Submit sign-out
API->>Browser: Clear auth_token cookie
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/api/src/middleware/auth.ts (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
AUTH_COOKIE_NAMEis duplicated across two files. Bothcookie-options.tsandauth.tsindependently defineconst AUTH_COOKIE_NAME = 'auth_token'. If one is updated and the other isn't, the cookie set on login won't match the cookie read during authentication, silently breaking the auth flow. Export fromcookie-options.tsand import inauth.ts.
packages/api/src/routes/auth/cookie-options.ts#L3-3: changeconsttoexport constforAUTH_COOKIE_NAME.packages/api/src/middleware/auth.ts#L18-18: remove the local declaration and importAUTH_COOKIE_NAMEfrom../routes/auth/cookie-options.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/api/src/middleware/auth.ts` at line 1, Use a single shared AUTH_COOKIE_NAME definition: export AUTH_COOKIE_NAME from cookie-options.ts, then remove the local declaration in auth.ts and import the exported symbol from ../routes/auth/cookie-options.
🧹 Nitpick comments (1)
packages/ui/src/lib/api/ApiClient.ts (1)
16-27: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove unnecessary dynamic
import('next/navigation').The import resolves but its exports are never used — all three branches (
.then,.catch, outertry/catch) simply setwindow.location.href = SIGN_IN_PATH. The dynamic import adds an async hop before the redirect for no benefit.♻️ Proposed fix
function handleUnauthorized(): void { if (typeof window === 'undefined') return; window.localStorage.removeItem(PROFILE_STORAGE_KEY); const current = window.location.pathname.replace(/\/$/, ''); if (current !== SIGN_IN_PATH) { - try { - import('next/navigation') - .then(() => { - window.location.href = SIGN_IN_PATH; - }) - .catch(() => { - window.location.href = SIGN_IN_PATH; - }); - } catch { - window.location.href = SIGN_IN_PATH; - } + window.location.href = SIGN_IN_PATH; } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/lib/api/ApiClient.ts` around lines 16 - 27, Remove the unnecessary dynamic import and its surrounding promise and try/catch handling in the redirect logic. Update the affected method in ApiClient so it directly assigns window.location.href to SIGN_IN_PATH, preserving the existing redirect behavior without the asynchronous hop.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/api/src/middleware/auth.ts`:
- Line 18: Remove the local AUTH_COOKIE_NAME declaration from auth.ts, export
the existing AUTH_COOKIE_NAME from cookie-options.ts, and import that shared
symbol in auth.ts so login cookie creation and authentication reads use one
source of truth.
In `@packages/api/src/routes/auth/sign-out.route.ts`:
- Around line 6-9: Update the sign-out route handler to replace the direct
res.json call with the established utilService success response method,
returning that call after clearAuthCookie(res). Use the appropriate success
response without a { message } payload, preserving the existing sign-out
behavior.
---
Outside diff comments:
In `@packages/api/src/middleware/auth.ts`:
- Line 1: Use a single shared AUTH_COOKIE_NAME definition: export
AUTH_COOKIE_NAME from cookie-options.ts, then remove the local declaration in
auth.ts and import the exported symbol from ../routes/auth/cookie-options.
---
Nitpick comments:
In `@packages/ui/src/lib/api/ApiClient.ts`:
- Around line 16-27: Remove the unnecessary dynamic import and its surrounding
promise and try/catch handling in the redirect logic. Update the affected method
in ApiClient so it directly assigns window.location.href to SIGN_IN_PATH,
preserving the existing redirect behavior without the asynchronous hop.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 5543be2b-b095-499e-a581-c3cbce54313f
📒 Files selected for processing (10)
packages/api/src/index.tspackages/api/src/middleware/auth.tspackages/api/src/routes/auth/cookie-options.tspackages/api/src/routes/auth/sign-in.route.tspackages/api/src/routes/auth/sign-out.route.tspackages/api/src/routes/auth/sign-up.route.tspackages/ui/src/app/signin/page.tsxpackages/ui/src/app/signup/page.tsxpackages/ui/src/hooks/useAuth.tspackages/ui/src/lib/api/ApiClient.ts
There was a problem hiding this comment.
Note
Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.
🟡 Other comments (1)
packages/ui/src/components/AppBar.test.tsx-37-37 (1)
37-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSeed a valid profile for the logout test.
useAuth()clears persisted profiles without anidduring initialization, so this fixture may already be removed before the logout click. Add anidto ensure the assertion actually verifies logout cleanup.Proposed fix
- localStorage.setItem('pocket_pixel_profile', JSON.stringify({ avatar: '/avatar.png' })); + localStorage.setItem( + 'pocket_pixel_profile', + JSON.stringify({ id: 'test-user', avatar: '/avatar.png' }), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/components/AppBar.test.tsx` at line 37, Update the profile fixture in the logout test to include a valid id alongside the avatar before storing it in localStorage. Keep the existing logout interaction and cleanup assertions unchanged so the test verifies removal of a profile that useAuth() preserves during initialization.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Other comments:
In `@packages/ui/src/components/AppBar.test.tsx`:
- Line 37: Update the profile fixture in the logout test to include a valid id
alongside the avatar before storing it in localStorage. Keep the existing logout
interaction and cleanup assertions unchanged so the test verifies removal of a
profile that useAuth() preserves during initialization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: QUIET
Plan: Pro Plus
Run ID: 8d4d2771-c856-44dd-9ebd-6740d6383e75
📒 Files selected for processing (1)
packages/ui/src/components/AppBar.test.tsx
…ge token no longer used)
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/ui/src/components/AuthGuard.tsx`:
- Around line 18-27: Validate the persisted profile in AuthGuard before setting
hasProfile: parse the stored value, require a truthy parsed.id, and treat parse
failures or missing IDs as unauthenticated so the existing cleanup path runs. In
packages/ui/src/components/AuthGuard.test.tsx lines 41, 64, and 87, add an id to
authenticated fixtures; at lines 52-58, retain the malformed fixture and rename
the test to cover invalid persisted profiles.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 611bd65c-0efd-4c24-8567-14e330163a74
📒 Files selected for processing (4)
packages/ui/e2e/login.spec.tspackages/ui/e2e/signup.spec.tspackages/ui/src/components/AuthGuard.test.tsxpackages/ui/src/components/AuthGuard.tsx
…okie-auth # Conflicts: # packages/api/src/middleware/auth.ts
…cookie auth - Resolved conflict in packages/api/src/middleware/auth.ts: TokenPayload now imports from @expense-tracker/shared (per upstream), kept the httpOnly cookie fallback in authenticate() - Fixed packages/ui/src/app/auth/google/callback/page.tsx to match the new single-argument setSession(profile) signature (this page appears orphaned since the Google sign-in button/route were removed upstream -- flagging for a maintainer decision on whether to delete it)
|
Hi @ali-ahnaf ! Rebased on latest While fixing this I noticed |
Hi, yes that leftover file is something I would want gone. Appreciate it if you could clean it up, or create an issue at least. |
The old Google sign-in route and button were already removed upstream in develop; this callback page and its AuthGuard public-path entry were leftover dead code with no remaining references. Removing both per maintainer request on PR review.
|
Done, removed the orphaned |
|
Hark, @Diyaaa-12! Thy branch hath drifted from the main path and the runes no longer align — a merge conflict bars the way. Thou must rebase the base branch and mend the tangled incantations before this quest may be sealed. The gates of merge remain shut until the conflict is banished. |
|
Hi! hope u can still review this |
2 participants
Closes #40
Moved session token storage from localStorage to an httpOnly cookie:
API side:
cookie-parserwired up in index.tssign-in/sign-upnow set an httpOnly, sameSite=lax cookie (secure in production) instead of relying solely on the JSON bodysign-outclears the cookieauthenticatemiddleware now reads the token from the cookie as a fallback when no Bearer header is present (backward compatible)UI side:
ApiClientnow useswithCredentials: true; removed manualAuthorization: Bearerheader and all localStorage reads/writes of the auth tokenuseAuth.setSession()no longer takes a token param — only profile data, since the browser handles the cookie automaticallysignin/page.tsxandsignup/page.tsxneeded no changes — they were already callingsetSessionwithout the tokenPre-existing issues found while testing (unrelated to this PR, flagging for visibility — not fixed here to keep the diff scoped):
npm run dev/npm run build/npm run testforpackages/apicurrently fail to compile because of a type error indebts.service.ts(missingdueDatefield vs theDebtDtotype). This actually blocks the dev server from starting at all, so I couldn't do a full manual sign-in sanity check locally. Happy to open a separate PR for this fix if that's useful — let me know, or feel free to take it yourself if you'd rather.Small typo in
signup/page.tsx:) asany;should be) as any;(missing space).Testing done:
packages/apitest suite: 77/77 relevant tests pass (onlydebts.service.test.tsfails, due to the pre-existing compile error above — not something this PR touches)packages/uiunit tests: no new failures introduced; same 8 pre-existing failures as before this change (localStorage.clear is not a functionin AppBar/AuthGuard/DesktopSidebar tests)Summary by CodeRabbit