Conversation
|
There was a problem hiding this comment.
Pull request overview
This pull request refactors the authentication flow by moving signOut and refreshAccessToken functions from the React context to standalone utility functions. The changes improve separation of concerns and add a callback feature to preserve user navigation state after sign-out.
Changes:
- Refactored authentication functions from React context to utility functions for better modularity
- Added callback parameter support to
signOutto redirect users back to their previous location after signing in - Simplified error handling in token refresh logic and distributed it across interceptors
- Added
COMMON_ERROR_MISSING_COOKIE_IN_TOKENerror constant and corresponding handler
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/src/community/auth/utils/authUtils.ts | Added new signOut function with callback support; simplified error handling in getNewAccessToken and clearCookies |
| frontend/src/community/auth/providers/AuthProvider.tsx | Removed signOut and refreshAccessToken from context; simplified checkAuth function |
| frontend/src/community/auth/types/auth.ts | Removed signOut and refreshAccessToken from AuthContextType interface |
| frontend/src/community/auth/utils/authInterceptor.ts | Added interceptor to handle missing cookie errors and trigger sign-out |
| frontend/src/community/common/constants/errorMessageKeys.ts | Added COMMON_ERROR_MISSING_COOKIE_IN_TOKEN constant |
| frontend/src/community/common/providers/TanStackProvider.tsx | Updated to use imported functions instead of context; moved handleTokenRefresh inside useEffect |
| frontend/src/community/common/components/templates/ContentLayout/ContentLayout.tsx | Updated to import signOut directly from utilities |
| frontend/src/community/common/components/molecules/ProfileMenu/ProfileMenu.tsx | Updated to import signOut directly; removed unused code |
| frontend/pages/index.tsx | Updated to import signOut directly and added missing await |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| await getNewAccessToken(); | ||
| queryClient.invalidateQueries(); |
There was a problem hiding this comment.
The handleTokenRefresh function doesn't handle the case where getNewAccessToken returns null (indicating token refresh failure). If token refresh fails, the queries are still invalidated which could lead to failed requests. Consider checking the return value and only invalidating queries if token refresh was successful, or calling signOut if it fails.
| await getNewAccessToken(); | |
| queryClient.invalidateQueries(); | |
| const newAccessToken = await getNewAccessToken(); | |
| if (newAccessToken) { | |
| queryClient.invalidateQueries(); | |
| } else { | |
| await signOut(); | |
| } |
| export const signOut = async (redirect: boolean = true): Promise<void> => { | ||
| await clearCookies(); | ||
|
|
||
| if (redirect === false) return; | ||
|
|
||
| if (typeof window !== 'undefined') { | ||
| const currentPath = window.location.pathname; | ||
| const urlParams = new URLSearchParams(window.location.search); | ||
| const existingCallback = urlParams.get('callback'); | ||
|
|
||
| const callbackPath = existingCallback || currentPath; | ||
| window.location.href = `${ROUTES.AUTH.SIGNIN}?callback=${callbackPath}`; |
There was a problem hiding this comment.
The callback path is used without validation. If a malicious actor crafts a URL with a callback parameter pointing to an external site (e.g., ?callback=https://evil.com), users could be redirected to an untrusted site after signing out. Consider validating that the callback path is a relative path (starts with /) or is within the allowed domain before using it in the redirect.
| export const signOut = async (redirect: boolean = true): Promise<void> => { | |
| await clearCookies(); | |
| if (redirect === false) return; | |
| if (typeof window !== 'undefined') { | |
| const currentPath = window.location.pathname; | |
| const urlParams = new URLSearchParams(window.location.search); | |
| const existingCallback = urlParams.get('callback'); | |
| const callbackPath = existingCallback || currentPath; | |
| window.location.href = `${ROUTES.AUTH.SIGNIN}?callback=${callbackPath}`; | |
| const isSafeCallbackPath = (callback: string | null): boolean => { | |
| if (!callback) { | |
| return false; | |
| } | |
| // Allow only relative paths that start with a single "/" and are not protocol-relative. | |
| if (!callback.startsWith("/")) { | |
| return false; | |
| } | |
| if (callback.startsWith("//")) { | |
| return false; | |
| } | |
| return true; | |
| }; | |
| export const signOut = async (redirect: boolean = true): Promise<void> => { | |
| await clearCookies(); | |
| if (redirect === false) return; | |
| if (typeof window !== "undefined") { | |
| const currentPath = window.location.pathname; | |
| const urlParams = new URLSearchParams(window.location.search); | |
| const existingCallback = urlParams.get("callback"); | |
| const callbackPath = isSafeCallbackPath(existingCallback) | |
| ? existingCallback! | |
| : currentPath; | |
| const encodedCallback = encodeURIComponent(callbackPath); | |
| window.location.href = `${ROUTES.AUTH.SIGNIN}?callback=${encodedCallback}`; |
| if (typeof window !== 'undefined') { | ||
| const currentPath = window.location.pathname; | ||
| const urlParams = new URLSearchParams(window.location.search); | ||
| const existingCallback = urlParams.get('callback'); |
There was a problem hiding this comment.
The code uses single quotes ('undefined') instead of double quotes. The codebase consistently uses double quotes for strings throughout the authentication utilities (see lines 117, 127, 147 in the same file). This should be changed to maintain consistency with the established convention.
| if (typeof window !== 'undefined') { | |
| const currentPath = window.location.pathname; | |
| const urlParams = new URLSearchParams(window.location.search); | |
| const existingCallback = urlParams.get('callback'); | |
| if (typeof window !== "undefined") { | |
| const currentPath = window.location.pathname; | |
| const urlParams = new URLSearchParams(window.location.search); | |
| const existingCallback = urlParams.get("callback"); |
| if (typeof window !== 'undefined') { | ||
| const currentPath = window.location.pathname; | ||
| const urlParams = new URLSearchParams(window.location.search); | ||
| const existingCallback = urlParams.get('callback'); |
There was a problem hiding this comment.
The code uses single quotes ('callback') instead of double quotes. The codebase consistently uses double quotes for strings throughout the authentication utilities. This should be changed to maintain consistency with the established convention.
| if (typeof window !== 'undefined') { | |
| const currentPath = window.location.pathname; | |
| const urlParams = new URLSearchParams(window.location.search); | |
| const existingCallback = urlParams.get('callback'); | |
| if (typeof window !== "undefined") { | |
| const currentPath = window.location.pathname; | |
| const urlParams = new URLSearchParams(window.location.search); | |
| const existingCallback = urlParams.get("callback"); |
| const existingCallback = urlParams.get('callback'); | ||
|
|
||
| const callbackPath = existingCallback || currentPath; | ||
| window.location.href = `${ROUTES.AUTH.SIGNIN}?callback=${callbackPath}`; |
There was a problem hiding this comment.
The callback path is being directly interpolated into the URL without encoding. If currentPath contains special characters (like query parameters with &, =, etc.), this could break the URL structure or lead to incorrect routing. Consider using encodeURIComponent to properly encode the callback value.
| window.location.href = `${ROUTES.AUTH.SIGNIN}?callback=${callbackPath}`; | |
| window.location.href = `${ROUTES.AUTH.SIGNIN}?callback=${encodeURIComponent(callbackPath)}`; |



PR checklist
TaskId: (https://github.com/SkappHQ/skapp/issues/[id])
Summary
How to test
Project Checklist
npm run formatnpm run check-lintOther
PR Checklist
ready-for-code-review)Additional Information