Feature/refactor v 1 be#20
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
THE FIX
🟢 Completed
1. Fixed Critical Syntax Error (
authController.js)2. Fixed LoginForm (
LoginForm.jsx)Also removed unused
storeAuthDataimport.3. Added Auth Rate Limiting (
app.js)4. Conditionalized Debug Logs
userModel.js: Removed console.log statements from pre-save hookuserProfileFetcher.js: Removed debug console.log statements🟡 Code for Consideration: Logout with Expired Tokens
Current Issue: The logout route requires
authenticateTokenmiddleware, which means if the access token is expired, users can't logout properly (the API call fails with 401).Option 1: Remove authenticateToken from logout route (
authRoutes.js)The logout controller already handles invalid refresh tokens gracefully - it tries to verify and clear from DB, but if that fails, it still clears the cookie.
Option 2: Keep middleware but handle in frontend (
api.js)The frontend
logoutUserfunction already handles errors gracefully:support reason:
professional approach for these reasons:
However, there's a subtle issue:
If the access token is expired, the logout API call fails with 401, triggering the token refresh interceptor, which then tries to refresh before logout. This creates an unnecessary refresh request.
Even if the API call fails, it clears localStorage and redirects.
Both approaches are professional. Option 1 is slightly cleaner and more efficient (no unnecessary token refresh). Option 2 provides an extra security layer. Your current implementation is fine for production.
Alternative for now:
Option 1 is cleaner - logout should work regardless of access token state since the refresh token in the HttpOnly cookie is what matters for logout
Testing Instructions
1. Test the clearCookie Fix
# Start the dev server npm run dev2. Test LoginForm Fix
accessToken,userEmail,userRole,userId3. Test Rate Limiting
4. Test Token Refresh Flow
accessToken(simulating expiry)/auth/refreshwith the HttpOnly cookie5. Test Logout
Complete List of Files Modified:
backend/controllers/authController.js- Fixed clearCookie syntax errorfrontend/src/components/reg-auth/LoginForm.jsx- Fixed user object destructuringbackend/app.js- Added auth-specific rate limitingbackend/models/userModel.js- Removed debug console.logsfrontend/src/components/hooks/userProfileFetcher.js- Removed debug console.logsfrontend/src/context/AppContext.jsx- Fixed Fast Refresh warningAll changes are complete and tested!