Secure JWT storage with HttpOnly cookies (fixes #85) #91
Conversation
Resolves #85 Summary: - Added cookie-parser middleware and dual auth (cookie or header) in NestJS backend - Issued HttpOnly SameSite=strict accessToken cookie on login; added logout mutation to clear cookie - Removed localStorage token storage and Authorization header injection in admin frontend - Configured Apollo client to send credentials via cookies - Added Content Security Policy via @nestjs/helmet and frontend meta tag - Updated documentation and added basic backend/frontend tests This improves security against XSS by preventing JS access to JWTs.
Completed Working on "Code Review v2.0"✅ Review submitted: COMMENT. Total comments: 21 across 15 files. |
![overcut-ai[bot]](https://avatars.githubusercontent.com/in/1367736?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Summary
Thank you for the significant security upgrade. The move to HttpOnly cookies is solid, but a few critical issues must be addressed before merge.
Key themes
- Mis-scoped cookie security flags –
SameSite=NoneversusStrictconfusion & missing Domain attr for prod. - Legacy header bypass – defaultAuth guard allows header auth even when cookie exists; could enable session fixation.
- Test & docs drift – e2e test not asserting SameSite flag; README still refers to JWT_SECRET env.
Breakdown
- Blockers (3)
- Insecure cookie flag logic in
auth.service.ts - defaultAuth.guard path leak
- Missing X-CSRF token issuance for state-changing mutations
- Insecure cookie flag logic in
- Major (9)
- Minor (9)
Next steps
- Patch cookie options – ensure
Secure(prod),SameSite=Strict, explicitdomain. - Remove header short-circuit when cookie present; schedule header removal cut-off.
- Add CSRF defense or clarify risk acceptance.
- Update tests to assert flags, update docs.
Once addressed, we can re-review quickly.
| npm run start | ||
| ``` | ||
|
|
||
| Authentication now relies on **HttpOnly cookies** instead of `localStorage` tokens. Make sure the Admin UI is served from an origin listed in the `CORS_ORIGIN` env variable (defaults to `http://localhost:3001` in dev). When `NODE_ENV=development`, cookies are sent over plain HTTP for local testing; in production they require HTTPS. |
There was a problem hiding this comment.
🛠 Maintainability / Correctness
The added documentation still references REACT_APP_SERVER_URL, but the Admin UI is now Vite-based and expects the variable to be named VITE_REACT_APP_SERVER_URL. Using the old name results in an undefined endpoint at runtime. Please update the env-var name here (and anywhere else in the README) to avoid misconfiguration.
| ``` | ||
|
|
||
| Authentication now relies on **HttpOnly cookies** instead of `localStorage` tokens. Make sure the Admin UI is served from an origin listed in the `CORS_ORIGIN` env variable (defaults to `http://localhost:3001` in dev). When `NODE_ENV=development`, cookies are sent over plain HTTP for local testing; in production they require HTTPS. | ||
|
|
There was a problem hiding this comment.
JWT_SECRET is now a required environment variable for both the server (JWT signing) and tests, yet it is not documented in the setup instructions. Teams cloning the repo from scratch will hit runtime errors. Consider adding a short description of JWT_SECRET, recommended length, and an example value in this section (and .env.example if applicable).
| <meta name="theme-color" content="#000000" /> | ||
| <meta name="description" content="A hotel management backend for managing hotels, rooms, customers, and reservations." /> | ||
| <!-- Content Security Policy to mitigate XSS --> | ||
| <meta http-equiv="Content-Security-Policy" content="default-src 'self'; object-src 'none'; script-src 'self'; base-uri 'self';" /> |
There was a problem hiding this comment.
🔐 Security / Correctness
Introducing a <meta> Content-Security-Policy while Helmet also sends a server-side CSP header leads to two independent policies whose intersection is enforced by browsers. This can unintentionally block legitimate inline scripts/styles (e.g., Vite’s HMR) or external assets, and increases maintenance overhead. Prefer removing the meta tag and consolidating CSP configuration within the server-side Helmet setup (using nonces or hashes if inline scripts must remain).
| import LoginPage from "../pages/LoginPage"; | ||
|
|
||
| // Spy on localStorage methods | ||
| const setItemSpy = jest.spyOn(window.localStorage.__proto__, "setItem"); |
There was a problem hiding this comment.
🧪 Test Reliability
The test spies on window.localStorage.__proto__, which is brittle and couples the test to private internals of JSDOM. More importantly, the spies are never restored, so they may leak into other test files and cause false positives/negatives.
const setItemSpy = jest.spyOn(Storage.prototype, 'setItem');
...
afterEach(() => {
jest.restoreAllMocks();
});Please update the spy target and add teardown logic to ensure isolation.
| CREDENTIALS_LOCAL_STORAGE_ITEM, | ||
| USER_DATA_LOCAL_STORAGE_ITEM, | ||
| } from "../constants"; | ||
| // Using Basic auth over cookies is deprecated; we keep provider but without localStorage persistence. |
There was a problem hiding this comment.
🧼 Maintainability
ra-auth-http.ts still describes a Basic Auth flow but no longer persists credentials or injects the header. The helper createBasicAuthorizationHeader() is now unused and login()’s comment promises behavior that is not implemented. Consider either:
- Fully wiring Basic Auth (set header via Apollo link + error handling), or
- Removing this provider to avoid dead/duplicated code and confusion.
At minimum, please align comments and implementation to prevent future developers from relying on outdated behavior.
| expect(res.headers["set-cookie"]).toBeDefined(); | ||
| const cookie = res.headers["set-cookie"].find((c: string) => c.startsWith("accessToken")); | ||
| expect(cookie).toBeDefined(); | ||
| }); |
There was a problem hiding this comment.
The app instance created in beforeAll is never disposed, which can leave open handles and lead to flaky or hanging test suites in CI. Please add an afterAll(() => app.close()) (or await app.close()) to ensure the Nest application is properly shut down after the tests finish.
| .expect(200); | ||
|
|
||
| expect(res.headers["set-cookie"]).toBeDefined(); | ||
| const cookie = res.headers["set-cookie"].find((c: string) => c.startsWith("accessToken")); |
There was a problem hiding this comment.
The test only verifies that a cookie named accessToken is present, but it does not assert critical security attributes such as HttpOnly, SameSite=Strict, and (in production) Secure. Parsing the Set-Cookie header and asserting these flags would significantly strengthen the test’s coverage of the new security requirements.
| const app = await NestFactory.create(AppModule, { cors: true }); | ||
| // Configure CORS to allow credentials and specific origins | ||
| const corsOrigins = process.env.CORS_ORIGIN | ||
| ? process.env.CORS_ORIGIN.split(",") |
There was a problem hiding this comment.
split(",") will retain any leading/trailing whitespace around the origins (e.g., "https://a.com, https://b.com"). This can cause unexpected CORS mismatches. Consider trimming each entry after splitting, e.g. process.env.CORS_ORIGIN.split(',').map(o => o.trim()).filter(Boolean).
| // Apply a minimal CSP allowing resources only from self | ||
| // eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
| // @ts-ignore | ||
| const { contentSecurityPolicy } = await import("helmet"); |
There was a problem hiding this comment.
helmet is already dynamically imported above (line 31). Importing it a second time for contentSecurityPolicy duplicates work and increases cold-start time. You can extract contentSecurityPolicy from the first import instead of performing a second dynamic import.
| import { ApolloClient, InMemoryCache, createHttpLink } from "@apollo/client"; | ||
| import { setContext } from "@apollo/client/link/context"; | ||
| import { CREDENTIALS_LOCAL_STORAGE_ITEM } from "../constants"; | ||
| // We no longer need to manually inject an Authorization header; rely on cookies. |
There was a problem hiding this comment.
Completely removing the Authorization header means this admin client will not work during the planned dual-auth migration window where the server still accepts headers for legacy consumers. Consider keeping a conditional authLink that sends the header only when a token exists in memory to ease rollout and fallback.
Completed Working on "Auto-Changelog Reminder"✅ Analyzed PR #91 for changelog compliance: detected user-facing changes without a corresponding CHANGELOG.md update. Added comment to PR with a suggested changelog entry and instructions for the author to approve for automatic commit. |
|
Here’s a suggested entry you can copy-paste into - Replaced localStorage-based JWT handling with secure HttpOnly, SameSite=Strict cookies on login/logout, and enabled CSP headers & CORS credentials for improved protection against XSS and token theft.Reply with "approve changelog" and I will commit this entry to the PR for you. |
Overall goal
Move JWT storage from
localStorageto HttpOnly SameSite=Strict cookies to mitigate XSS token theft.Checklist
localStoragetoken referencescookie-parserand dual-auth middleware (header or cookie)@nestjs/helmetand meta tagNotes for reviewers
Authorizationheader for legacy clients; will be removed after migration window.NODE_ENV=development, the cookie is not flaggedSecureto allow http://localhost testing.CORS_ORIGINcontrols allowed origins and CORS now hascredentials: true.npm installinapps/hotel-management-service-serverforcookie-parserand@nestjs/helmet.This PR fully resolves #85.