Fix session cookies with proper CORS and credentialed auth requests#254
Open
ChaitanyaChute wants to merge 3 commits into
Open
Fix session cookies with proper CORS and credentialed auth requests#254ChaitanyaChute wants to merge 3 commits into
ChaitanyaChute wants to merge 3 commits into
Conversation
❌ Deploy Preview for github-spy failed.
|
Contributor
📝 WalkthroughWalkthroughBackend CORS now derives allowed origins from FRONTEND_URL (comma-separated) and passes that array to cors with credentials enabled. Frontend Login and Signup POSTs are updated to call axios with { withCredentials: true }; Signup error handling was also changed and a commented mock was added. ChangesSession Authentication with Credentials
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
backend/server.jsParsing error: Unexpected keyword 'const' src/pages/Signup/Signup.tsxParsing error: 'try' expected. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ChaitanyaChute
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/pages/Signup/Signup.tsx (1)
40-56: ⚡ Quick winConsider removing the commented-out mock implementation.
The old mock code and TODO comments are no longer needed since the real backend integration is now working. Removing dead code improves maintainability.
🧹 Proposed cleanup
- - // // Simulate API call (replace with your actual backend integration) - // try { - // // Mock successful signup - // setMessage("Account created successfully! Redirecting to login..."); - - // // In your actual implementation, integrate with your backend here: - // // const response = await fetch(`${backendUrl}/api/auth/signup`, { - // // method: 'POST', - // // headers: { 'Content-Type': 'application/json' }, - // // body: JSON.stringify(formData) - // // }); - - // setTimeout(() => { - // // Navigate to login page in your actual implementation - // console.log("Redirecting to login page..."); - // }, 2000); - } catch (error) { setMessage("Something went wrong. Please try again."); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Signup/Signup.tsx` around lines 40 - 56, Remove the dead commented-out mock signup flow in Signup.tsx: delete the block that simulates the API call and the setTimeout/console.log redirect (the commented lines referencing setMessage, formData, and the mock fetch), leaving only the real backend integration in the signup handler (e.g., the actual fetch/POST logic and subsequent setMessage/navigation). Ensure no leftover TODOs or commented code related to the old mock remain.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/pages/Signup/Signup.tsx`:
- Around line 40-56: Remove the dead commented-out mock signup flow in
Signup.tsx: delete the block that simulates the API call and the
setTimeout/console.log redirect (the commented lines referencing setMessage,
formData, and the mock fetch), leaving only the real backend integration in the
signup handler (e.g., the actual fetch/POST logic and subsequent
setMessage/navigation). Ensure no leftover TODOs or commented code related to
the old mock remain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f369502f-c36b-422e-a280-f53d5dc44797
📒 Files selected for processing (6)
Dockerfile.prodbackend/routes/auth.jsbackend/server.jsspec/auth.routes.spec.cjssrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsx
Collaborator
|
@ChaitanyaChute : resolve conflicts
|
mehul-m-prajapati
added
quality:clean
gssoc:approved
type:bug
level:beginner
and removed
gssoc26
labels
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/pages/Signup/Signup.tsx (1)
72-82:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftCritical: Remove duplicate code that creates syntax errors.
Lines 72-82 contain old code that was not removed during the refactor, creating invalid JavaScript syntax:
- Syntax error: After the catch block at line 70-71, there's unreachable code (line 72-77) followed by another
} catchstatement at line 78, which is invalid.- Missing finally block: The new try-catch (lines 40-71) is missing the finally block that resets
isLoading. The finally block exists only in the old code (lines 80-82) that should be deleted.The correct fix:
- Delete lines 72-79 entirely (duplicate axios call and catch block)
- Move the finally block (lines 80-82) to immediately follow the new catch block at line 71
This code will not compile or run in its current state.
🚨 Proposed fix to remove duplicate code and restore finally block
} catch (error) { setMessage( (error as any).response?.data?.message || "Something went wrong. Please try again." ); - const response = await axios.post(`${backendUrl}/api/auth/signup`, formData); - setMessage(response.data.message); - - if (response.data.message === "User created successfully") { - navigate("/login"); - } - } catch (error: any) { - setMessage(error.response?.data?.message || "Something went wrong. Please try again."); } finally { setIsLoading(false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Signup/Signup.tsx` around lines 72 - 82, Remove the duplicated axios call and stray catch block that remained after refactor (the duplicate axios.post(...) and its associated catch) and ensure the finally block that calls setIsLoading(false) is moved to immediately follow the existing try/catch; specifically, inside the Signup component remove the old duplicate code that repeats the axios.post to `${backendUrl}/api/auth/signup` and the extra `} catch (...)` block, then place the finally block that resets `isLoading` right after the current catch so setIsLoading(false) always runs; keep existing uses of setMessage(response.data.message), navigate("/login") when response.data.message === "User created successfully", and the error handling that reads error.response?.data?.message.
🧹 Nitpick comments (1)
src/pages/Signup/Signup.tsx (1)
49-50: ⚡ Quick winConsider checking response status instead of exact message text.
The current implementation checks for an exact message string match (
'User created successfully'), which tightly couples the frontend to the backend's specific message wording. If the backend message changes, navigation will silently fail.Consider using
response.status === 201or aresponse.data.successboolean flag for more robust success detection.♻️ Proposed refactor using status code
- if (response.data.message === 'User created successfully') { - navigate("/login");} + if (response.status === 201 || response.status === 200) { + navigate("/login"); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Signup/Signup.tsx` around lines 49 - 50, Replace the fragile string check in the Signup component (the if that currently tests response.data.message === 'User created successfully') with a status- or flag-based check: inspect response.status === 201 or response.data.success (whichever the API guarantees) and call navigate("/login") when that condition is true; update the signup submission handler in Signup.tsx to use this new conditional so navigation no longer depends on exact backend message wording.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/pages/Signup/Signup.tsx`:
- Around line 70-71: Restore extraction of the backend-provided error message in
the Signup component's catch block: when catching the error in the signup
handler, read error.response?.data?.message (falling back to error.message or a
generic string) and pass that to setMessage instead of the hardcoded "Something
went wrong..." so users see validation/backend errors; update the catch block
around the signup submission (where setMessage is currently called) to use this
extraction logic.
- Around line 53-69: Remove the dead commented-out mock API block inside the
Signup component (the commented "Simulate API call" section) so the file no
longer contains the old mock fetch/timeout code; specifically delete the
commented lines within the Signup.tsx handleSubmit/submit flow that start with
the "Simulate API call" comment and the subsequent try/Mock successful
signup/setTimeout stubs, leaving only the real implementation and any real
messaging (e.g., setMessage) intact.
---
Outside diff comments:
In `@src/pages/Signup/Signup.tsx`:
- Around line 72-82: Remove the duplicated axios call and stray catch block that
remained after refactor (the duplicate axios.post(...) and its associated catch)
and ensure the finally block that calls setIsLoading(false) is moved to
immediately follow the existing try/catch; specifically, inside the Signup
component remove the old duplicate code that repeats the axios.post to
`${backendUrl}/api/auth/signup` and the extra `} catch (...)` block, then place
the finally block that resets `isLoading` right after the current catch so
setIsLoading(false) always runs; keep existing uses of
setMessage(response.data.message), navigate("/login") when response.data.message
=== "User created successfully", and the error handling that reads
error.response?.data?.message.
---
Nitpick comments:
In `@src/pages/Signup/Signup.tsx`:
- Around line 49-50: Replace the fragile string check in the Signup component
(the if that currently tests response.data.message === 'User created
successfully') with a status- or flag-based check: inspect response.status ===
201 or response.data.success (whichever the API guarantees) and call
navigate("/login") when that condition is true; update the signup submission
handler in Signup.tsx to use this new conditional so navigation no longer
depends on exact backend message wording.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef36cfe8-2f78-4b5e-a019-1cdd11a774e0
📒 Files selected for processing (1)
src/pages/Signup/Signup.tsx
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
backend/server.js (1)
17-35:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Duplicate CORS configuration causes syntax error.
The file contains two overlapping CORS configurations that will cause a syntax error and prevent the server from starting:
- Lines 17-24 declare
allowedOriginsfrom env and startcors({...})with a trailing comma- Lines 25-35 redeclare
allowedOriginsas a hardcoded array and add anothercors()callThe static analysis confirms parse errors at line 25. This appears to be an unresolved merge conflict or incomplete edit. You need to keep only one CORS configuration.
Based on the PR objectives (using env-based origins), keep the first approach and complete it:
🔧 Proposed fix
const allowedOrigins = (process.env.FRONTEND_URL || 'http://localhost:5173') .split(',') .map((origin) => origin.trim()) .filter(Boolean); app.use(cors({ origin: allowedOrigins, - credentials: true, -const allowedOrigins = ['http://localhost:5173', 'https://github-spy.etlify.app']; -app.use(cors({ - origin: function (origin, callback) { - if (!origin || allowedOrigins.indexOf(origin) !== -1) { - callback(null, true); - } else{ - callback(new Error('Blocked by CORS policy')); - } - }, credentials: true }));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/server.js` around lines 17 - 35, Remove the duplicate CORS block and restore a single, valid cors middleware that uses the env-derived allowedOrigins (variable allowedOrigins built from process.env.FRONTEND_URL) and a function-origin validator; specifically delete the hardcoded allowedOrigins array and the second app.use(cors(...)) and complete the first app.use(cors({...})) so it supplies an origin function that checks if (!origin || allowedOrigins.includes(origin)) callback(null,true) else callback(new Error('Blocked by CORS policy')), and keep credentials: true; ensure only the variable allowedOrigins and the single app.use(cors(...)) remain.src/pages/Signup/Signup.tsx (1)
85-132:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: Broken handleSubmit with duplicate code and syntax errors.
The
handleSubmitfunction has severe issues that will prevent compilation:
- Duplicate variable declarations:
const responseis declared multiple times (lines 86, 117, 119)- Multiple catch blocks: There appear to be two
catchblocks (lines 115 and 128) which is invalid syntax- Unguarded API calls in catch block: Lines 117-127 make API calls inside a catch block without try/catch, which will cause unhandled promise rejections
- Commented mock code: Lines 98-114 should be removed (previously flagged)
This appears to be a failed merge. The correct implementation should have a single try block with the axios call including
withCredentials: true, followed by proper error handling.🔧 Proposed fix
setIsLoading(true); try { const response = await axios.post( `${backendUrl}/api/auth/signup`, formData, { withCredentials: true } ); setMessage(response.data.message); if (response.data.message === 'User created successfully') { navigate("/login"); } - - - // // Simulate API call (replace with your actual backend integration) - // try { - // // Mock successful signup - // setMessage("Account created successfully! Redirecting to login..."); - - // // In your actual implementation, integrate with your backend here: - // // const response = await fetch(`${backendUrl}/api/auth/signup`, { - // // method: 'POST', - // // headers: { 'Content-Type': 'application/json' }, - // // body: JSON.stringify(formData) - // // }); - - // setTimeout(() => { - // // Navigate to login page in your actual implementation - // console.log("Redirecting to login page..."); - // }, 2000); - - } catch (error) { - setMessage("Something went wrong. Please try again."); - const response = await axios.post(`${backendUrl}/api/auth/signup`, formData); - setMessage(response.data.message); - const response = await axios.post(`${backendUrl}/api/auth/signup`, - formData // Include cookies for session - ); - setMessage(response.data.message); // Show success message from backend - - // Navigate to login page after successful signup - if (response.data.message === 'User created successfully') { - navigate("/login"); - } - } catch (error: any) { - setMessage(error.response?.data?.message || "Something went wrong. Please try again."); + } catch (error: unknown) { + if (axios.isAxiosError(error)) { + setMessage(error.response?.data?.message || "Something went wrong. Please try again."); + } else { + setMessage("Something went wrong. Please try again."); + } } finally { setIsLoading(false); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/pages/Signup/Signup.tsx` around lines 85 - 132, The handleSubmit function contains duplicated/merged code and invalid syntax; fix it by consolidating into one try/catch/finally: in the try call axios.post(`${backendUrl}/api/auth/signup`, formData, { withCredentials: true }) once, setMessage(response.data.message) and if response.data.message === 'User created successfully' call navigate("/login"); in the catch (error: any) setMessage(error.response?.data?.message || "Something went wrong. Please try again.") and do NOT perform further API calls there, and finally setIsLoading(false); also remove the leftover commented mock block and duplicate const response declarations so handleSubmit is a single well-formed async function.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@backend/server.js`:
- Around line 17-35: Remove the duplicate CORS block and restore a single, valid
cors middleware that uses the env-derived allowedOrigins (variable
allowedOrigins built from process.env.FRONTEND_URL) and a function-origin
validator; specifically delete the hardcoded allowedOrigins array and the second
app.use(cors(...)) and complete the first app.use(cors({...})) so it supplies an
origin function that checks if (!origin || allowedOrigins.includes(origin))
callback(null,true) else callback(new Error('Blocked by CORS policy')), and keep
credentials: true; ensure only the variable allowedOrigins and the single
app.use(cors(...)) remain.
In `@src/pages/Signup/Signup.tsx`:
- Around line 85-132: The handleSubmit function contains duplicated/merged code
and invalid syntax; fix it by consolidating into one try/catch/finally: in the
try call axios.post(`${backendUrl}/api/auth/signup`, formData, {
withCredentials: true }) once, setMessage(response.data.message) and if
response.data.message === 'User created successfully' call navigate("/login");
in the catch (error: any) setMessage(error.response?.data?.message || "Something
went wrong. Please try again.") and do NOT perform further API calls there, and
finally setIsLoading(false); also remove the leftover commented mock block and
duplicate const response declarations so handleSubmit is a single well-formed
async function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 33275f90-04c7-4cde-a479-04caed9dc460
📒 Files selected for processing (3)
backend/server.jssrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsx
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
2 participants
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.
Related Issue
Description
Type of Change
Summary by CodeRabbit
Release Notes