-
Notifications
You must be signed in to change notification settings - Fork 0
Issue 82 refactor axios interceptor #86
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?
Conversation
…and refresh token as http only cookie.
… cookie. No test yet.
|
Backend fails as I rebase the main to get the non-public /bookings endpoint, but the test file inside booking failed. |
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.
Pull request overview
This PR refactors the axios interceptor to implement JWT token-based authentication using browser-accessible cookies. The implementation adds automatic token refresh logic when access tokens expire (401 responses), with a queuing mechanism to handle concurrent requests during token refresh.
Key changes include:
- Implementation of cookie-based JWT token storage with in-memory caching
- Automatic token refresh interceptor with request queuing for concurrent API calls
- Helper functions for token and cookie management
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| client/src/lib/api.ts | Complete refactor implementing JWT authentication with axios interceptors, cookie management, and automatic token refresh logic |
| client/package.json | Added devDependencies: axios-cookiejar-support, dotenv, tough-cookie, and tsx |
| client/package-lock.json | Updated lockfile with new dependencies and their transitive dependencies |
Files not reviewed (1)
- client/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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.
Pull request overview
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
Files not reviewed (1)
- client/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
client/src/lib/api.ts
Outdated
|
|
||
| // Prevent infinite loop | ||
| if (error.config?.url?.includes("/users/refresh")) | ||
| return handleEarlyLogout("Token refreshed failed", error); |
Copilot
AI
Jan 10, 2026
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.
The error message contains a typo: "Token refreshed failed" should be "Token refresh failed" for grammatical correctness.
| return handleEarlyLogout("Token refreshed failed", error); | |
| return handleEarlyLogout("Token refresh failed", error); |
client/src/lib/api.ts
Outdated
| // console.log("Access token refreshed", newAccessToken); | ||
| if (!newAccessToken) { | ||
| console.warn("No access token returned, logging out"); | ||
| return handleEarlyLogout("No access token returned"); | ||
| } | ||
|
|
||
| // console.log("Setting new access token:", newAccessToken); |
Copilot
AI
Jan 10, 2026
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.
Commented-out console.log statements at lines 197 and 203 should be removed. If debug logging is needed, consider using a proper logging library or environment-based debug flags instead of leaving commented code.
| // console.log("Access token refreshed", newAccessToken); | |
| if (!newAccessToken) { | |
| console.warn("No access token returned, logging out"); | |
| return handleEarlyLogout("No access token returned"); | |
| } | |
| // console.log("Setting new access token:", newAccessToken); | |
| if (!newAccessToken) { | |
| console.warn("No access token returned, logging out"); | |
| return handleEarlyLogout("No access token returned"); | |
| } |
client/src/lib/api.ts
Outdated
| // test only | ||
| if (typeof window !== "undefined") { | ||
| // @ts-ignore | ||
| window.api = api; |
Copilot
AI
Jan 10, 2026
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.
The test-only code that exposes the api instance on the window object should not be included in production code. This creates a security risk by making the API instance globally accessible. Consider using environment checks to ensure this code only runs in development/test environments, or better yet, use proper testing utilities that don't require modifying production code.
| // test only | |
| if (typeof window !== "undefined") { | |
| // @ts-ignore | |
| window.api = api; | |
| // test only: expose api on window in non-production environments | |
| if (process.env.NODE_ENV !== "production" && typeof window !== "undefined") { | |
| // @ts-ignore | |
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | |
| (window as any).api = api; |
client/src/lib/api.ts
Outdated
| // let inMemoryAccessToken: string | undefined = undefined; | ||
|
|
||
| // Helper function to get cookie by name | ||
| // const getCookie = (name: string) => { | ||
| // const cookies = document.cookie.split("; "); | ||
| // // Only split on the first '=' to allow '=' in the value | ||
| // const prefix = name + "="; | ||
| // const match = cookies.find((row) => row.startsWith(prefix)); | ||
| // if (!match) { | ||
| // return ""; | ||
| // } | ||
| // const value = match.substring(prefix.length); | ||
| // return decodeURIComponent(value); | ||
| // }; | ||
|
|
||
| // const getAccessToken = () => { | ||
| // // Ensure this function is called only on the client side | ||
| // if (typeof window === "undefined") { | ||
| // throw new Error("getAccessToken can only be called on the client side"); | ||
| // } | ||
| // if (inMemoryAccessToken) return inMemoryAccessToken; | ||
| // // Check if the token returned is an empty string before caching it | ||
| // const tokenFromCookie = getCookie("accessToken"); | ||
| // if (!tokenFromCookie) return; | ||
| // inMemoryAccessToken = tokenFromCookie; | ||
| // return inMemoryAccessToken; | ||
| // }; | ||
|
|
||
| // const setAccessToken = (accessToken: string) => { | ||
| // // Ensure this function is called only on the client side | ||
| // if (typeof window === "undefined") { | ||
| // throw new Error("setAccessToken can only be called on the client side"); | ||
| // } | ||
| // // Cookies with Secure flag only be set over HTTPS (allow non-secure in development) | ||
| // const isSecure = window.location.protocol === "https:"; | ||
| // inMemoryAccessToken = accessToken; | ||
| // const newCookie = [ | ||
| // `accessToken=${encodeURIComponent(accessToken)}`, | ||
| // "path=/", | ||
| // "SameSite=Lax", | ||
| // isSecure ? "Secure" : "", | ||
| // ] | ||
| // .filter(Boolean) | ||
| // .join("; "); | ||
| // document.cookie = newCookie; | ||
| // }; | ||
|
|
||
| // const getRefreshToken = () => { | ||
| // // Ensure this function is called only on the client side | ||
| // if (typeof window === "undefined") { | ||
| // throw new Error("getRefreshToken can only be called on the client side"); | ||
| // } | ||
| // return getCookie("refreshToken"); | ||
| // }; | ||
|
|
||
| // const clearTokens = () => { | ||
| // if (typeof window === "undefined") { | ||
| // throw new Error("clearTokens can only be called on the client side"); | ||
| // } | ||
| // // Cookies with Secure flag only be set over HTTPS (allow non-secure in development) | ||
| // const isSecure = window.location.protocol === "https:"; | ||
| // const base = ["path=/", "SameSite=Lax", isSecure ? "Secure" : ""] | ||
| // .filter(Boolean) | ||
| // .join("; "); | ||
| // document.cookie = `accessToken=; Max-Age=0; ${base}`; | ||
| // document.cookie = `refreshToken=; Max-Age=0; ${base}`; | ||
| // inMemoryAccessToken = undefined; | ||
| // }; | ||
|
|
||
| const getAccessToken = () => | ||
| typeof window !== "undefined" ? localStorage.getItem("accessToken") : null; | ||
|
|
||
| const setAccessToken = (accessToken: string) => | ||
| typeof window !== "undefined" | ||
| ? localStorage.setItem("accessToken", accessToken) | ||
| : null; | ||
|
|
||
| const getRefreshToken = () => | ||
| typeof window !== "undefined" ? localStorage.getItem("refreshToken") : null; | ||
|
|
||
| const clearTokens = () => { | ||
| if (typeof window === "undefined") return; | ||
| localStorage.removeItem("accessToken"); | ||
| localStorage.removeItem("refreshToken"); |
Copilot
AI
Jan 10, 2026
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.
Using localStorage for JWT tokens is less secure than httpOnly cookies as it's vulnerable to XSS attacks. Tokens stored in localStorage can be accessed by any JavaScript code running on the page, including malicious scripts. Consider using httpOnly cookies for storing sensitive tokens, or at a minimum, document the security implications of this choice.
| // let inMemoryAccessToken: string | undefined = undefined; | |
| // Helper function to get cookie by name | |
| // const getCookie = (name: string) => { | |
| // const cookies = document.cookie.split("; "); | |
| // // Only split on the first '=' to allow '=' in the value | |
| // const prefix = name + "="; | |
| // const match = cookies.find((row) => row.startsWith(prefix)); | |
| // if (!match) { | |
| // return ""; | |
| // } | |
| // const value = match.substring(prefix.length); | |
| // return decodeURIComponent(value); | |
| // }; | |
| // const getAccessToken = () => { | |
| // // Ensure this function is called only on the client side | |
| // if (typeof window === "undefined") { | |
| // throw new Error("getAccessToken can only be called on the client side"); | |
| // } | |
| // if (inMemoryAccessToken) return inMemoryAccessToken; | |
| // // Check if the token returned is an empty string before caching it | |
| // const tokenFromCookie = getCookie("accessToken"); | |
| // if (!tokenFromCookie) return; | |
| // inMemoryAccessToken = tokenFromCookie; | |
| // return inMemoryAccessToken; | |
| // }; | |
| // const setAccessToken = (accessToken: string) => { | |
| // // Ensure this function is called only on the client side | |
| // if (typeof window === "undefined") { | |
| // throw new Error("setAccessToken can only be called on the client side"); | |
| // } | |
| // // Cookies with Secure flag only be set over HTTPS (allow non-secure in development) | |
| // const isSecure = window.location.protocol === "https:"; | |
| // inMemoryAccessToken = accessToken; | |
| // const newCookie = [ | |
| // `accessToken=${encodeURIComponent(accessToken)}`, | |
| // "path=/", | |
| // "SameSite=Lax", | |
| // isSecure ? "Secure" : "", | |
| // ] | |
| // .filter(Boolean) | |
| // .join("; "); | |
| // document.cookie = newCookie; | |
| // }; | |
| // const getRefreshToken = () => { | |
| // // Ensure this function is called only on the client side | |
| // if (typeof window === "undefined") { | |
| // throw new Error("getRefreshToken can only be called on the client side"); | |
| // } | |
| // return getCookie("refreshToken"); | |
| // }; | |
| // const clearTokens = () => { | |
| // if (typeof window === "undefined") { | |
| // throw new Error("clearTokens can only be called on the client side"); | |
| // } | |
| // // Cookies with Secure flag only be set over HTTPS (allow non-secure in development) | |
| // const isSecure = window.location.protocol === "https:"; | |
| // const base = ["path=/", "SameSite=Lax", isSecure ? "Secure" : ""] | |
| // .filter(Boolean) | |
| // .join("; "); | |
| // document.cookie = `accessToken=; Max-Age=0; ${base}`; | |
| // document.cookie = `refreshToken=; Max-Age=0; ${base}`; | |
| // inMemoryAccessToken = undefined; | |
| // }; | |
| const getAccessToken = () => | |
| typeof window !== "undefined" ? localStorage.getItem("accessToken") : null; | |
| const setAccessToken = (accessToken: string) => | |
| typeof window !== "undefined" | |
| ? localStorage.setItem("accessToken", accessToken) | |
| : null; | |
| const getRefreshToken = () => | |
| typeof window !== "undefined" ? localStorage.getItem("refreshToken") : null; | |
| const clearTokens = () => { | |
| if (typeof window === "undefined") return; | |
| localStorage.removeItem("accessToken"); | |
| localStorage.removeItem("refreshToken"); | |
| let inMemoryAccessToken: string | undefined = undefined; | |
| // Helper function to get cookie by name. | |
| // Note: For maximal security, tokens should ideally be stored in httpOnly cookies | |
| // set by the server so that they are not accessible to JavaScript at all. | |
| // This helper assumes non-httpOnly cookies that are readable on the client. | |
| const getCookie = (name: string) => { | |
| const cookies = document.cookie.split("; "); | |
| // Only split on the first '=' to allow '=' in the value | |
| const prefix = name + "="; | |
| const match = cookies.find((row) => row.startsWith(prefix)); | |
| if (!match) { | |
| return ""; | |
| } | |
| const value = match.substring(prefix.length); | |
| return decodeURIComponent(value); | |
| }; | |
| const getAccessToken = () => { | |
| // Ensure this function is called only on the client side | |
| if (typeof window === "undefined") { | |
| throw new Error("getAccessToken can only be called on the client side"); | |
| } | |
| if (inMemoryAccessToken) return inMemoryAccessToken; | |
| // Check if the token returned is an empty string before caching it | |
| const tokenFromCookie = getCookie("accessToken"); | |
| if (!tokenFromCookie) return; | |
| inMemoryAccessToken = tokenFromCookie; | |
| return inMemoryAccessToken; | |
| }; | |
| const setAccessToken = (accessToken: string) => { | |
| // Ensure this function is called only on the client side | |
| if (typeof window === "undefined") { | |
| throw new Error("setAccessToken can only be called on the client side"); | |
| } | |
| // Cookies with Secure flag only be set over HTTPS (allow non-secure in development) | |
| const isSecure = window.location.protocol === "https:"; | |
| inMemoryAccessToken = accessToken; | |
| const newCookie = [ | |
| `accessToken=${encodeURIComponent(accessToken)}`, | |
| "path=/", | |
| "SameSite=Lax", | |
| isSecure ? "Secure" : "", | |
| ] | |
| .filter(Boolean) | |
| .join("; "); | |
| document.cookie = newCookie; | |
| }; | |
| const getRefreshToken = () => { | |
| // Ensure this function is called only on the client side | |
| if (typeof window === "undefined") { | |
| throw new Error("getRefreshToken can only be called on the client side"); | |
| } | |
| return getCookie("refreshToken"); | |
| }; | |
| const clearTokens = () => { | |
| if (typeof window === "undefined") { | |
| throw new Error("clearTokens can only be called on the client side"); | |
| } | |
| // Cookies with Secure flag only be set over HTTPS (allow non-secure in development) | |
| const isSecure = window.location.protocol === "https:"; | |
| const base = ["path=/", "SameSite=Lax", isSecure ? "Secure" : ""] | |
| .filter(Boolean) | |
| .join("; "); | |
| document.cookie = `accessToken=; Max-Age=0; ${base}`; | |
| document.cookie = `refreshToken=; Max-Age=0; ${base}`; | |
| inMemoryAccessToken = undefined; |
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.
Nothing to change here. LocalStorage is used based on agreement.
client/src/lib/api.ts
Outdated
| // Module-level in-memory cache for the access token. | ||
| // Used only to avoid repeated cookie parsing on the client. | ||
| // This module is loaded once per browser session, so this value persists across all renders on the client. | ||
| // Warning: Don't use functions involving this variable for Server Side Rendering (SSR). | ||
| // let inMemoryAccessToken: string | undefined = undefined; | ||
|
|
||
| // Helper function to get cookie by name | ||
| // const getCookie = (name: string) => { | ||
| // const cookies = document.cookie.split("; "); | ||
| // // Only split on the first '=' to allow '=' in the value | ||
| // const prefix = name + "="; | ||
| // const match = cookies.find((row) => row.startsWith(prefix)); | ||
| // if (!match) { | ||
| // return ""; | ||
| // } | ||
| // const value = match.substring(prefix.length); | ||
| // return decodeURIComponent(value); | ||
| // }; | ||
|
|
||
| // const getAccessToken = () => { | ||
| // // Ensure this function is called only on the client side | ||
| // if (typeof window === "undefined") { | ||
| // throw new Error("getAccessToken can only be called on the client side"); | ||
| // } | ||
| // if (inMemoryAccessToken) return inMemoryAccessToken; | ||
| // // Check if the token returned is an empty string before caching it | ||
| // const tokenFromCookie = getCookie("accessToken"); | ||
| // if (!tokenFromCookie) return; | ||
| // inMemoryAccessToken = tokenFromCookie; | ||
| // return inMemoryAccessToken; | ||
| // }; | ||
|
|
||
| // const setAccessToken = (accessToken: string) => { | ||
| // // Ensure this function is called only on the client side | ||
| // if (typeof window === "undefined") { | ||
| // throw new Error("setAccessToken can only be called on the client side"); | ||
| // } | ||
| // // Cookies with Secure flag only be set over HTTPS (allow non-secure in development) | ||
| // const isSecure = window.location.protocol === "https:"; | ||
| // inMemoryAccessToken = accessToken; | ||
| // const newCookie = [ | ||
| // `accessToken=${encodeURIComponent(accessToken)}`, | ||
| // "path=/", | ||
| // "SameSite=Lax", | ||
| // isSecure ? "Secure" : "", | ||
| // ] | ||
| // .filter(Boolean) | ||
| // .join("; "); | ||
| // document.cookie = newCookie; | ||
| // }; | ||
|
|
||
| // const getRefreshToken = () => { | ||
| // // Ensure this function is called only on the client side | ||
| // if (typeof window === "undefined") { | ||
| // throw new Error("getRefreshToken can only be called on the client side"); | ||
| // } | ||
| // return getCookie("refreshToken"); | ||
| // }; | ||
|
|
||
| // const clearTokens = () => { | ||
| // if (typeof window === "undefined") { | ||
| // throw new Error("clearTokens can only be called on the client side"); | ||
| // } | ||
| // // Cookies with Secure flag only be set over HTTPS (allow non-secure in development) | ||
| // const isSecure = window.location.protocol === "https:"; | ||
| // const base = ["path=/", "SameSite=Lax", isSecure ? "Secure" : ""] | ||
| // .filter(Boolean) | ||
| // .join("; "); | ||
| // document.cookie = `accessToken=; Max-Age=0; ${base}`; | ||
| // document.cookie = `refreshToken=; Max-Age=0; ${base}`; | ||
| // inMemoryAccessToken = undefined; | ||
| // }; | ||
|
|
||
| const getAccessToken = () => | ||
| typeof window !== "undefined" ? localStorage.getItem("accessToken") : null; | ||
|
|
||
| const setAccessToken = (accessToken: string) => | ||
| typeof window !== "undefined" | ||
| ? localStorage.setItem("accessToken", accessToken) | ||
| : null; | ||
|
|
||
| const getRefreshToken = () => | ||
| typeof window !== "undefined" ? localStorage.getItem("refreshToken") : null; | ||
|
|
||
| const clearTokens = () => { | ||
| if (typeof window === "undefined") return; | ||
| localStorage.removeItem("accessToken"); | ||
| localStorage.removeItem("refreshToken"); | ||
| }; |
Copilot
AI
Jan 10, 2026
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.
The PR description mentions refactoring the axios interceptor to use browser-accessible cookies for JWT tokens. However, the implementation uses localStorage instead of cookies (lines 87-102). This is inconsistent with the stated purpose. Additionally, the large block of commented-out cookie-based implementation (lines 14-85) should be removed to keep the codebase clean and avoid confusion.
Change Summary
Refactor axios interceptor to bear JWT tokens (stored in localStorage)
Change Form
Fill this up (NA if not available). If a certain criteria is not met, can you please give a reason.
Other Information
https://github.com/user-attachments/assets/6612d961-ccd0-43f3-a1a6-53733373d51e
Open as draft PR.
Related issue