-
Notifications
You must be signed in to change notification settings - Fork 1
Secure JWT storage with HttpOnly cookies (fixes #85) #91
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -119,6 +119,8 @@ If you prefer running Node processes on the host machine: | |
| 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. | ||
|
|
||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| By default the Admin UI expects the server at `http://localhost:3000`. | ||
| Change `REACT_APP_SERVER_URL` in `apps/hotel-management-service-admin/.env` if needed. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,8 @@ | |
| <meta name="viewport" content="width=device-width, initial-scale=1" /> | ||
| <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';" /> | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🔐 Security / Correctness |
||
|
|
||
| <title>Hotel Management Service</title> | ||
| </head> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| import { render } from "@testing-library/react"; | ||
| import LoginPage from "../pages/LoginPage"; | ||
|
|
||
| // Spy on localStorage methods | ||
| const setItemSpy = jest.spyOn(window.localStorage.__proto__, "setItem"); | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧪 Test Reliability const setItemSpy = jest.spyOn(Storage.prototype, 'setItem');
...
afterEach(() => {
jest.restoreAllMocks();
});Please update the spy target and add teardown logic to ensure isolation. |
||
| const getItemSpy = jest.spyOn(window.localStorage.__proto__, "getItem"); | ||
|
|
||
| // Ensure spies don't affect actual implementation | ||
| setItemSpy.mockImplementation(() => undefined); | ||
| getItemSpy.mockImplementation(() => null); | ||
|
|
||
| describe("No localStorage usage", () => { | ||
| it("renders login page without touching localStorage", () => { | ||
| render(<LoginPage />); | ||
| expect(setItemSpy).not.toHaveBeenCalled(); | ||
| expect(getItemSpy).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,6 @@ | ||
| import { gql } from "@apollo/client/core"; | ||
| import { AuthProvider } from "react-admin"; | ||
| import { | ||
| 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. | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧼 Maintainability
At minimum, please align comments and implementation to prevent future developers from relying on outdated behavior. |
||
| import { Credentials, LoginMutateResult } from "../types"; | ||
| import { apolloClient } from "../data-provider/graphqlDataProvider"; | ||
|
|
||
|
|
@@ -26,48 +23,21 @@ export const httpAuthProvider: AuthProvider = { | |
| }); | ||
|
|
||
| if (userData && userData.data?.login.username) { | ||
| localStorage.setItem( | ||
| CREDENTIALS_LOCAL_STORAGE_ITEM, | ||
| createBasicAuthorizationHeader( | ||
| credentials.username, | ||
| credentials.password | ||
| ) | ||
| ); | ||
| localStorage.setItem( | ||
| USER_DATA_LOCAL_STORAGE_ITEM, | ||
| JSON.stringify(userData.data) | ||
| ); | ||
| // Basic auth header will be carried only within this runtime, not persisted. | ||
| return Promise.resolve(); | ||
| } | ||
| return Promise.reject(); | ||
| }, | ||
| logout: () => { | ||
| localStorage.removeItem(CREDENTIALS_LOCAL_STORAGE_ITEM); | ||
| return Promise.resolve(); | ||
| }, | ||
| logout: () => Promise.resolve(), | ||
| checkError: ({ status }: any) => { | ||
| if (status === 401 || status === 403) { | ||
| localStorage.removeItem(CREDENTIALS_LOCAL_STORAGE_ITEM); | ||
| return Promise.reject(); | ||
| } | ||
| return Promise.resolve(); | ||
| }, | ||
| checkAuth: () => { | ||
| return localStorage.getItem(CREDENTIALS_LOCAL_STORAGE_ITEM) | ||
| ? Promise.resolve() | ||
| : Promise.reject(); | ||
| }, | ||
| checkAuth: () => Promise.resolve(), | ||
| getPermissions: () => Promise.reject("Unknown method"), | ||
| getIdentity: () => { | ||
| const str = localStorage.getItem(USER_DATA_LOCAL_STORAGE_ITEM); | ||
| const userData: LoginMutateResult = JSON.parse(str || ""); | ||
|
|
||
| return Promise.resolve({ | ||
| id: userData.login.username, | ||
| fullName: userData.login.username, | ||
| avatar: undefined, | ||
| }); | ||
| }, | ||
| getIdentity: () => Promise.resolve({ id: undefined, fullName: undefined, avatar: undefined }), | ||
| }; | ||
|
|
||
| function createBasicAuthorizationHeader( | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,9 +1,6 @@ | ||
| import { gql } from "@apollo/client/core"; | ||
| import { AuthProvider } from "react-admin"; | ||
| import { | ||
| CREDENTIALS_LOCAL_STORAGE_ITEM, | ||
| USER_DATA_LOCAL_STORAGE_ITEM, | ||
| } from "../constants"; | ||
| // Note: We no longer persist credentials in localStorage. | ||
| import { Credentials, LoginMutateResult } from "../types"; | ||
| import { apolloClient } from "../data-provider/graphqlDataProvider"; | ||
|
|
||
|
|
@@ -26,47 +23,52 @@ export const jwtAuthProvider: AuthProvider = { | |
| }); | ||
|
|
||
| if (userData && userData.data?.login.username) { | ||
| localStorage.setItem( | ||
| CREDENTIALS_LOCAL_STORAGE_ITEM, | ||
| createBearerAuthorizationHeader(userData.data.login?.accessToken) | ||
| ); | ||
| localStorage.setItem( | ||
| USER_DATA_LOCAL_STORAGE_ITEM, | ||
| JSON.stringify(userData.data) | ||
| ); | ||
| // Cookie is automatically set by the server; nothing to persist client-side | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧼 Maintainability / Security |
||
| return Promise.resolve(); | ||
| } | ||
| return Promise.reject(); | ||
| }, | ||
| logout: () => { | ||
| localStorage.removeItem(CREDENTIALS_LOCAL_STORAGE_ITEM); | ||
| return Promise.resolve(); | ||
| logout: async () => { | ||
| // Invoke backend mutation to clear HttpOnly cookie | ||
| const LOGOUT = gql` | ||
| mutation logout { | ||
| logout | ||
| } | ||
| `; | ||
| try { | ||
| await apolloClient.mutate({ mutation: LOGOUT }); | ||
| // Apollo will include cookies automatically (credentials: include) | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧪 Correctness / UX |
||
| return Promise.resolve(); | ||
| } catch (e) { | ||
| return Promise.reject(e); | ||
| } | ||
| }, | ||
| checkError: ({ status }: any) => { | ||
| if (status === 401 || status === 403) { | ||
| localStorage.removeItem(CREDENTIALS_LOCAL_STORAGE_ITEM); | ||
| return Promise.reject(); | ||
| } | ||
| return Promise.resolve(); | ||
| }, | ||
| checkAuth: () => { | ||
| return localStorage.getItem(CREDENTIALS_LOCAL_STORAGE_ITEM) | ||
| ? Promise.resolve() | ||
| : Promise.reject(); | ||
| checkAuth: async () => { | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠 Correctness |
||
| // Query backend `me` (lightweight) to confirm cookie validity | ||
| const ME = gql` | ||
| query me { | ||
| me { | ||
| id | ||
| } | ||
| } | ||
| `; | ||
| try { | ||
| await apolloClient.query({ query: ME, fetchPolicy: "network-only" }); | ||
| return Promise.resolve(); | ||
| } catch (error) { | ||
| // If request fails due to 401/403 or network error, treat as unauthenticated | ||
| return Promise.reject(error); | ||
| } | ||
| }, | ||
| getPermissions: () => Promise.reject("Unknown method"), | ||
| getIdentity: () => { | ||
| const str = localStorage.getItem(USER_DATA_LOCAL_STORAGE_ITEM); | ||
| const userData: LoginMutateResult = JSON.parse(str || ""); | ||
|
|
||
| return Promise.resolve({ | ||
| id: userData.login.username, | ||
| fullName: userData.login.username, | ||
| avatar: undefined, | ||
| }); | ||
| // TODO: Optionally fetch current user via a `me` query. For now, return empty. | ||
| return Promise.resolve({ id: undefined, fullName: undefined, avatar: undefined }); | ||
| }, | ||
| }; | ||
|
|
||
| export function createBearerAuthorizationHeader(accessToken: string) { | ||
| return `Bearer ${accessToken}`; | ||
| } | ||
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,26 +1,28 @@ | ||
| import buildGraphQLProvider from "ra-data-graphql-amplication"; | ||
| 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. | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠 Correctness
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Completely removing the |
||
|
|
||
| const httpLink = createHttpLink({ | ||
| uri: `${import.meta.env.VITE_REACT_APP_SERVER_URL}/graphql`, | ||
| credentials: "include", // send cookies with each request | ||
| }); | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/naming-convention | ||
| const authLink = setContext((_, { headers }) => { | ||
| const token = localStorage.getItem(CREDENTIALS_LOCAL_STORAGE_ITEM); | ||
| return { | ||
| headers: { | ||
| ...headers, | ||
| authorization: token ? token : "", | ||
| }, | ||
| }; | ||
| }); | ||
|
|
||
| export const apolloClient = new ApolloClient({ | ||
| cache: new InMemoryCache(), | ||
| link: authLink.concat(httpLink), | ||
| link: httpLink, | ||
| // Always include credentials (cookies) in requests | ||
| defaultOptions: { | ||
| watchQuery: { | ||
| fetchPolicy: "cache-and-network", | ||
| }, | ||
| query: { | ||
| fetchPolicy: "network-only", | ||
| }, | ||
| mutate: { | ||
| fetchPolicy: "no-cache", | ||
| }, | ||
| }, | ||
| }); | ||
|
|
||
| export default buildGraphQLProvider({ | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -45,7 +45,10 @@ | |
| "reflect-metadata": "0.1.13", | ||
| "ts-node": "10.9.2", | ||
| "type-fest": "2.19.0", | ||
| "validator": "13.11.0" | ||
| "validator": "13.11.0", | ||
| "cookie-parser": "^1.4.6", | ||
| "@nestjs/helmet": "^11.0.1", | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| "helmet": "^7.0.0" | ||
| }, | ||
| "devDependencies": { | ||
| "@nestjs/cli": "^10.1.18", | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -4,6 +4,7 @@ import { RoomModule } from "./room/room.module"; | |
| import { ReservationModule } from "./reservation/reservation.module"; | ||
| import { CustomerModule } from "./customer/customer.module"; | ||
| import { HealthModule } from "./health/health.module"; | ||
| import { AuthModule } from "./auth/auth.module"; | ||
| import { PrismaModule } from "./prisma/prisma.module"; | ||
| import { SecretsManagerModule } from "./providers/secrets/secretsManager.module"; | ||
| import { ServeStaticModule } from "@nestjs/serve-static"; | ||
|
|
@@ -15,6 +16,7 @@ import { ApolloDriver, ApolloDriverConfig } from "@nestjs/apollo"; | |
| @Module({ | ||
| controllers: [], | ||
| imports: [ | ||
| AuthModule, | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| HotelModule, | ||
| RoomModule, | ||
| ReservationModule, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import { Module } from "@nestjs/common"; | ||
| import { JwtModule } from "@nestjs/jwt"; | ||
| import { ConfigModule, ConfigService } from "@nestjs/config"; | ||
| import { AuthService } from "./auth.service"; | ||
| import { AuthResolver } from "./auth.resolver"; | ||
|
|
||
| @Module({ | ||
| imports: [ | ||
| ConfigModule, | ||
| JwtModule.registerAsync({ | ||
| imports: [ConfigModule], | ||
| useFactory: (config: ConfigService) => ({ | ||
| secret: config.get<string>("JWT_SECRET", "change-me"), | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Using a hard-coded fallback secret ( |
||
| signOptions: { | ||
| expiresIn: config.get<string | number>( | ||
| "JWT_EXPIRES_IN", | ||
| "24h" | ||
| ), | ||
| }, | ||
| }), | ||
| inject: [ConfigService], | ||
| }), | ||
| ], | ||
| providers: [AuthService, AuthResolver], | ||
| exports: [AuthService], | ||
| }) | ||
| export class AuthModule {} | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,37 @@ | ||
| import { Resolver, Mutation, Args } from "@nestjs/graphql"; | ||
| import { CredentialsInput } from "./credentials.input"; | ||
| import { LoginResponse } from "./login-response.object"; | ||
| import { AuthService } from "./auth.service"; | ||
| import { Response } from "express"; | ||
| import { Res } from "@nestjs/common"; | ||
|
|
||
| @Resolver() | ||
| export class AuthResolver { | ||
| constructor(private readonly authService: AuthService) {} | ||
|
|
||
| @Mutation(() => LoginResponse) | ||
| async login( | ||
| @Args("credentials") credentials: CredentialsInput, | ||
| @Res({ passthrough: true }) res: Response | ||
| ): Promise<LoginResponse> { | ||
| const { username, password } = credentials; | ||
| const accessToken = await this.authService.validateUser(username, password).then(() => this.authService.login(username)); | ||
|
|
||
| // Set HttpOnly cookie for access token | ||
| res.cookie("accessToken", accessToken, { | ||
| httpOnly: true, | ||
| sameSite: "strict", | ||
| secure: process.env.NODE_ENV !== "development", | ||
| path: "/", | ||
| maxAge: Number(process.env.JWT_COOKIE_MAX_AGE ?? 1000 * 60 * 60 * 24), // default 1 day | ||
| }); | ||
|
|
||
| return { accessToken }; | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Returning the |
||
| } | ||
|
|
||
| @Mutation(() => Boolean) | ||
| async logout(@Res({ passthrough: true }) res: Response): Promise<boolean> { | ||
| res.clearCookie("accessToken", { path: "/" }); | ||
| return true; | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠 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 namedVITE_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.