feat: implement Google & GitHub OAuth and persist session stores via …#343
feat: implement Google & GitHub OAuth and persist session stores via …#343Soumadip-Eagle123 wants to merge 2 commits into
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR integrates OAuth authentication for Google and GitHub into a full-stack application. The backend registers two passport strategies, configures MongoDB session persistence, adds OAuth routes, and updates the User model to handle external provider credentials. The frontend adds OAuth sign-in/sign-up buttons to existing login and signup pages. ChangesOAuth Authentication Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
🚥 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)
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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @Soumadip-Eagle123 for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@backend/config/passportConfig.js`:
- Around line 49-50: The current OAuth username construction using
profile.displayName + "_" + profile.id.substring(0,5) (see the username
assignment in passportConfig.js) can collide with existing users; change it to
produce a deterministic unique base that includes the provider to avoid
collisions (e.g. use provider + "_" + profile.id.substring(0,5) or provider +
"_" + profile.provider + "_" + id suffix) or implement a retry-on-duplicate-key
routine around the user creation logic (catch duplicate key errors when creating
the User and append/increment a suffix until creation succeeds); update both
places where username is built (the existing username assignment and the similar
block later around lines creating new OAuth users) and ensure any error handling
for duplicate-key (E11000) is covered so first OAuth login does not fail.
- Around line 44-50: The Google OAuth callback dereferences
profile.emails[0].value in the User.findOne lookup and when constructing a new
User (username and email) which will throw if profile.emails is missing; update
the Google strategy callback to safely extract the email (e.g., const email =
profile.emails?.[0]?.value) and if email is falsy call done(null, false, {
message: "No email provided by Google" }) to reject authentication, then use
that guarded email value in User.findOne and in the new User({ ... }) and keep
the username generation using profile.displayName but only if present.
In `@backend/models/User.js`:
- Around line 22-24: Replace the magic-string check this.password ===
"OAUTH_USER_EXTERNAL_PROVIDER" with explicit authProvider and providerId fields
on the User model: add authProvider (e.g. 'local' | 'oauth') and providerId,
update the model defaults/migration to include them, and change all logic that
currently branches on that password literal (e.g., the block in User.js that
returns early and the similar check at lines 34-36) to instead check
authProvider !== 'local' (or authProvider === 'oauth') to skip password logic;
also ensure user-creation and update paths set authProvider/providerId for
external users and that any password setters/validators (e.g., validatePassword,
setPassword) only operate when authProvider === 'local'.
In `@backend/routes/auth.js`:
- Around line 54-57: The OAuth callback handlers currently use
passport.authenticate("google", { failureRedirect: "/login" }) which points to a
non-existent backend path; update both passport.authenticate calls to build a
frontend-aware failure URL (e.g., failureRedirect: (process.env.FRONTEND_URL ||
"http://localhost:5173") + "/login") so failures are redirected to the frontend
login page, and keep the existing success res.redirect(process.env.FRONTEND_URL
|| "http://localhost:5173") behavior for parity.
- Line 50: Enable OAuth CSRF protection by adding state: true to the strategy
constructor options in backend/config/passportConfig.js: update the
GoogleStrategy options passed into passport.use(new GoogleStrategy(...)) to
include state: true and likewise update the GitHubStrategy options passed into
passport.use(new GitHubStrategy(...)) to include state: true so the strategy
(not passport.authenticate calls) will validate the state parameter
automatically using the existing express-session.
In `@backend/server.js`:
- Around line 23-29: The session cookie currently only sets maxAge; update the
session middleware configuration (the MongoStore.create + cookie object) to add
cookie.httpOnly: true, cookie.sameSite: 'lax' (or 'strict' if applicable) and
cookie.secure set to true only when running in production (e.g.,
process.env.NODE_ENV === 'production'), and ensure your Express app enables
trust proxy (app.set('trust proxy', 1)) before mounting the session middleware
so secure cookies work behind a proxy/load balancer; keep the store:
MongoStore.create(...) unchanged and apply these cookie and trust-proxy changes
around the session setup.
🪄 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: 43d3531b-8bae-4b29-8cbf-4c22173aef4e
📒 Files selected for processing (8)
backend/.env.samplebackend/config/passportConfig.jsbackend/models/User.jsbackend/package.jsonbackend/routes/auth.jsbackend/server.jssrc/pages/Login/Login.tsxsrc/pages/Signup/Signup.tsx
| let user = await User.findOne({ email: profile.emails[0].value }); | ||
|
|
||
| if (!user) { | ||
| // Create a user placeholder if they are logging in for the first time | ||
| user = new User({ | ||
| username: profile.displayName.replace(/\s+/g, "_").toLowerCase() + "_" + profile.id.substring(0, 5), | ||
| email: profile.emails[0].value, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and read the relevant file
fd -t f "passportConfig.js" backend/config/Repository: GitMetricsLab/github_tracker
Length of output: 105
🏁 Script executed:
# Get the line count first
wc -l backend/config/passportConfig.jsRepository: GitMetricsLab/github_tracker
Length of output: 109
🏁 Script executed:
# Read the relevant section (lines 34-103 for context)
sed -n '34,103p' backend/config/passportConfig.jsRepository: GitMetricsLab/github_tracker
Length of output: 2593
🏁 Script executed:
# Also check the User model to see if username has uniqueness constraint
fd -t f "User" -e "js" -e "ts" | xargs grep -l "username.*unique\|unique.*username" 2>/dev/null | head -5Repository: GitMetricsLab/github_tracker
Length of output: 54
🏁 Script executed:
# Search for email validation patterns in the file
rg "email" backend/config/passportConfig.js -B 2 -A 2Repository: GitMetricsLab/github_tracker
Length of output: 2157
🏁 Script executed:
# Find User model file
fd -t f "User" -e "js" -e "ts" models/ || fd -t f "User.js" || fd -t f "user.js" || fd -t f "*chema*" models/Repository: GitMetricsLab/github_tracker
Length of output: 208
🏁 Script executed:
# Search for User model definition more broadly
rg "new Schema\|export.*User\|module.exports.*User" -A 10 --max-count=3Repository: GitMetricsLab/github_tracker
Length of output: 54
🏁 Script executed:
# Look for username field definition in the codebase
rg "username.*:.*{" --type js --type ts -B 2 -A 2 | grep -A 5 -B 5 uniqueRepository: GitMetricsLab/github_tracker
Length of output: 164
🏁 Script executed:
# Read the User model file
cat -n backend/models/User.jsRepository: GitMetricsLab/github_tracker
Length of output: 1309
🏁 Script executed:
# Search for unique constraints on username
rg "unique\|index" backend/models/User.js -B 2 -A 2Repository: GitMetricsLab/github_tracker
Length of output: 54
Guard email extraction in Google OAuth strategy before dereferencing array.
Line 44 and line 50 directly access profile.emails[0].value without checking if the array exists. If Google returns a profile without the emails field, this throws a TypeError and crashes the callback. The GitHub strategy already handles this correctly with optional chaining. Add null-safe extraction and reject auth when email is missing.
Proposed fix
- let user = await User.findOne({ email: profile.emails[0].value });
+ const googleEmail = profile.emails?.[0]?.value;
+ if (!googleEmail) {
+ return done(null, false, { message: "Google account email is required" });
+ }
+ let user = await User.findOne({ email: googleEmail });
...
- email: profile.emails[0].value,
+ email: googleEmail,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let user = await User.findOne({ email: profile.emails[0].value }); | |
| if (!user) { | |
| // Create a user placeholder if they are logging in for the first time | |
| user = new User({ | |
| username: profile.displayName.replace(/\s+/g, "_").toLowerCase() + "_" + profile.id.substring(0, 5), | |
| email: profile.emails[0].value, | |
| const googleEmail = profile.emails?.[0]?.value; | |
| if (!googleEmail) { | |
| return done(null, false, { message: "Google account email is required" }); | |
| } | |
| let user = await User.findOne({ email: googleEmail }); | |
| if (!user) { | |
| // Create a user placeholder if they are logging in for the first time | |
| user = new User({ | |
| username: profile.displayName.replace(/\s+/g, "_").toLowerCase() + "_" + profile.id.substring(0, 5), | |
| email: googleEmail, |
🤖 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/config/passportConfig.js` around lines 44 - 50, The Google OAuth
callback dereferences profile.emails[0].value in the User.findOne lookup and
when constructing a new User (username and email) which will throw if
profile.emails is missing; update the Google strategy callback to safely extract
the email (e.g., const email = profile.emails?.[0]?.value) and if email is falsy
call done(null, false, { message: "No email provided by Google" }) to reject
authentication, then use that guarded email value in User.findOne and in the new
User({ ... }) and keep the username generation using profile.displayName but
only if present.
| username: profile.displayName.replace(/\s+/g, "_").toLowerCase() + "_" + profile.id.substring(0, 5), | ||
| email: profile.emails[0].value, |
There was a problem hiding this comment.
Handle OAuth username collisions against unique index.
Line 49 and Line 85 can generate usernames that collide with existing local/OAuth users, causing duplicate-key failures on first OAuth login. Derive a deterministic unique base (provider + provider id suffix) or retry on duplicate key.
Also applies to: 85-87
🤖 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/config/passportConfig.js` around lines 49 - 50, The current OAuth
username construction using profile.displayName + "_" +
profile.id.substring(0,5) (see the username assignment in passportConfig.js) can
collide with existing users; change it to produce a deterministic unique base
that includes the provider to avoid collisions (e.g. use provider + "_" +
profile.id.substring(0,5) or provider + "_" + profile.provider + "_" + id
suffix) or implement a retry-on-duplicate-key routine around the user creation
logic (catch duplicate key errors when creating the User and append/increment a
suffix until creation succeeds); update both places where username is built (the
existing username assignment and the similar block later around lines creating
new OAuth users) and ensure any error handling for duplicate-key (E11000) is
covered so first OAuth login does not fail.
| if (this.password === "OAUTH_USER_EXTERNAL_PROVIDER") { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Replace magic-password provider detection with explicit auth provider fields.
Line 22 and Line 34 key OAuth behavior off a literal password value. That makes auth mode user-input-dependent (a local user choosing this exact password gets a broken login path and plaintext persistence). Use explicit fields like authProvider and providerId instead of inspecting password content.
Suggested direction
const UserSchema = new mongoose.Schema({
username: { type: String, required: true, unique: true },
email: { type: String, required: true, unique: true },
password: { type: String, required: true },
+ authProvider: { type: String, enum: ["local", "google", "github"], default: "local" },
+ providerId: { type: String, default: null },
});
UserSchema.pre('save', async function () {
- if (this.password === "OAUTH_USER_EXTERNAL_PROVIDER") return;
+ if (this.authProvider !== "local") return;
if (!this.isModified('password')) return;
const salt = await bcrypt.genSalt(10);
this.password = await bcrypt.hash(this.password, salt);
});
UserSchema.methods.comparePassword = async function (enteredPassword) {
- if (this.password === "OAUTH_USER_EXTERNAL_PROVIDER") return false;
+ if (this.authProvider !== "local") return false;
return await bcrypt.compare(enteredPassword, this.password);
};Also applies to: 34-36
🤖 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/models/User.js` around lines 22 - 24, Replace the magic-string check
this.password === "OAUTH_USER_EXTERNAL_PROVIDER" with explicit authProvider and
providerId fields on the User model: add authProvider (e.g. 'local' | 'oauth')
and providerId, update the model defaults/migration to include them, and change
all logic that currently branches on that password literal (e.g., the block in
User.js that returns early and the similar check at lines 34-36) to instead
check authProvider !== 'local' (or authProvider === 'oauth') to skip password
logic; also ensure user-creation and update paths set authProvider/providerId
for external users and that any password setters/validators (e.g.,
validatePassword, setPassword) only operate when authProvider === 'local'.
| store: MongoStore.create({ | ||
| mongoUrl: process.env.MONGO_URI, | ||
| collectionName: 'sessions' | ||
| }), | ||
| cookie: { | ||
| maxAge: 1000 * 60 * 60 * 24 | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine backend/server.js
find . -name "server.js" -type f | head -20Repository: GitMetricsLab/github_tracker
Length of output: 92
🏁 Script executed:
# Let's check the directory structure to understand the project layout
git ls-files | grep -E "(backend|server)" | head -30Repository: GitMetricsLab/github_tracker
Length of output: 355
🏁 Script executed:
# Search for session configuration in the codebase
rg "session\(" -A 10 --max-count 5Repository: GitMetricsLab/github_tracker
Length of output: 1827
🏁 Script executed:
# Search for trust proxy configuration
rg "trust proxy" -iRepository: GitMetricsLab/github_tracker
Length of output: 54
🏁 Script executed:
# Search for cookie configuration patterns
rg "cookie\s*:" -B 2 -A 5Repository: GitMetricsLab/github_tracker
Length of output: 381
🏁 Script executed:
# Read the full backend/server.js file
cat -n backend/server.jsRepository: GitMetricsLab/github_tracker
Length of output: 1614
🏁 Script executed:
# Check Dockerfile configurations to understand deployment context
cat -n backend/Dockerfile.prodRepository: GitMetricsLab/github_tracker
Length of output: 659
🏁 Script executed:
# Check environment setup in package.json
cat -n backend/package.json | head -30Repository: GitMetricsLab/github_tracker
Length of output: 996
🏁 Script executed:
# Search for any proxy-related configuration in the codebase
rg "proxy" -i -B 2 -A 2Repository: GitMetricsLab/github_tracker
Length of output: 54
🏁 Script executed:
# Check if NODE_ENV is used anywhere in the backend
rg "NODE_ENV" backend/Repository: GitMetricsLab/github_tracker
Length of output: 54
Harden session cookie settings for production.
The session cookie configuration at lines 27-29 sets only maxAge, leaving critical security settings implicit. Add httpOnly, sameSite, secure, and trust proxy settings to prevent XSS session theft, CSRF attacks, and unencrypted transmission.
Proposed fix
+app.set("trust proxy", 1);
app.use(session({
secret: process.env.SESSION_SECRET,
resave: false,
saveUninitialized: false,
store: MongoStore.create({
mongoUrl: process.env.MONGO_URI,
collectionName: 'sessions'
}),
cookie: {
- maxAge: 1000 * 60 * 60 * 24
+ maxAge: 1000 * 60 * 60 * 24,
+ httpOnly: true,
+ sameSite: "lax",
+ secure: process.env.NODE_ENV === "production",
}
}));🤖 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 23 - 29, The session cookie currently only
sets maxAge; update the session middleware configuration (the MongoStore.create
+ cookie object) to add cookie.httpOnly: true, cookie.sameSite: 'lax' (or
'strict' if applicable) and cookie.secure set to true only when running in
production (e.g., process.env.NODE_ENV === 'production'), and ensure your
Express app enables trust proxy (app.set('trust proxy', 1)) before mounting the
session middleware so secure cookies work behind a proxy/load balancer; keep the
store: MongoStore.create(...) unchanged and apply these cookie and trust-proxy
changes around the session setup.
|
not required for now @Soumadip-Eagle123 |
2 participants
…Mongo
Related Issue
Description
This PR introduces secure social authentication via Passport.js and hardens the backend session architecture to support higher concurrent user loads without crashes.
Key changes include:
passport-google-oauth20andpassport-github2strategies into the existing Passport ecosystem.MemoryStoresession implementation withconnect-mongoto persist user sessions seamlessly inside MongoDB..env.samplewith the necessary blank configuration variables so future developers know how to set up social sign-on.How Has This Been Tested?
Screenshots (if applicable)
(Optional: You can drag and drop an image of your updated Login screen here if you have one)
Type of Change
Summary by CodeRabbit
Release Notes