Feat/GitHub signin oauth#336
Open
pratyushranjn wants to merge 5 commits into
Open
Conversation
✅ Deploy Preview for github-spy ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thank you @pratyushranjn for your contribution. Please make sure your PR follows https://github.com/GitMetricsLab/github_tracker/blob/main/CONTRIBUTING.md#-pull-request-guidelines
Author
|
Note: The Netlify deploy preview currently shows The GitHub OAuth flow has been tested locally and works correctly with the required environment variables configured. |
Contributor
|
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. 📝 WalkthroughWalkthroughThis PR implements GitHub OAuth sign-in alongside existing email/password authentication. The backend adds environment configuration, updates the User model to support GitHub-only accounts, implements Passport GitHub strategy and local strategy guards, and provides auth routes with configuration checks. The frontend adds a GitHub sign-in button, handles OAuth callback status via query parameters, and displays status toasts. ChangesGitHub OAuth Sign-In Implementation
Sequence DiagramsequenceDiagram
participant User
participant LoginUI as Login UI
participant Backend as Backend Auth
participant GitHub as GitHub OAuth
participant Database as User DB
User->>LoginUI: Click "Sign In with GitHub"
LoginUI->>Backend: GET /api/auth/github
alt GitHub OAuth Configured
Backend->>GitHub: Initiate OAuth (scope: user:email)
GitHub->>User: Redirect to GitHub login
User->>GitHub: Authenticate & authorize
GitHub->>Backend: Redirect to /github/callback with code
Backend->>GitHub: Exchange code for profile
GitHub->>Backend: Return email, avatar, githubId
Backend->>Database: Lookup/create user by githubId
Database->>Backend: Return/save user
Backend->>LoginUI: Redirect with githubAuth=success
LoginUI->>User: Show success toast, navigate to /track
else GitHub OAuth Not Configured
Backend->>LoginUI: Redirect with githubAuth=not_configured
LoginUI->>User: Show configuration error toast
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/config/passportConfig.js (1)
12-23:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid account enumeration in login failures.
Returning different messages for unknown email vs wrong password leaks account existence.
Suggested fix
const user = await User.findOne({ email }); - if (!user) { - return done(null, false, { message: 'Email is invalid ' }); - } - - if (!user.password) { - return done(null, false, { message: 'Use GitHub sign in for this account' }); - } + if (!user || !user.password) { + return done(null, false, { message: 'Invalid credentials' }); + } @@ const isMatch = await user.comparePassword(password); if (!isMatch) { - return done(null, false, { message: 'Invalid password' }); + return done(null, false, { message: 'Invalid credentials' }); }🤖 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 12 - 23, The current passport local verify callback leaks account existence by returning different messages for missing user, accounts without a password, and wrong passwords; change the logic in the verify function (the callback that calls done) so that any authentication failure (no user found, user.password falsy, or comparePassword returns false) returns the same generic error via done(null, false, { message: 'Invalid email or password' }) instead of distinct strings; keep the call to user.comparePassword but ensure its result is masked by the unified error message to avoid account enumeration.
🤖 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 48-64: The code is persisting email: null which breaks the sparse
unique index; update the User creation and update logic in passportConfig.js so
you only set the email field when primaryEmail is truthy (e.g., omit the email
key or set it to undefined when primaryEmail is falsy) — change the new
User({... email: primaryEmail ...}) to conditionally include email, and
similarly change any later assignment like user.email = primaryEmail to only run
when primaryEmail is present; reference the User model, the code in
passportConfig.js that constructs new User and the code path that updates
user.email.
- Around line 39-45: The GitHubStrategy instantiation in passportConfig.js
currently omits OAuth2 CSRF state handling; update the new GitHubStrategy(...)
options to enable state by adding the OAuth2 state option (e.g., state: true or
provide a state store) so Passport generates and validates the state parameter
during the GitHub login/callback flow, and ensure your session middleware is
used so the state store can persist between request and callback (refer to the
GitHubStrategy constructor and the surrounding passport/session setup).
In `@backend/server.js`:
- Line 26: The session middleware is using a fallback 'dev-session-secret' which
should not be used in production; before calling the express-session setup (the
session({...}) call and its secret property), check NODE_ENV === 'production'
and if process.env.SESSION_SECRET is missing, throw an error or call
process.exit(1) to fail fast; update the session configuration to use
process.env.SESSION_SECRET (no default) for the secret field and ensure any
environment-dependent sameSite/secure options are set appropriately for
production deployments.
In `@src/pages/Login/Login.tsx`:
- Around line 59-83: The useEffect in Login.tsx schedules a setTimeout for
navigate("/track") but never clears it on unmount; capture the timer id (e.g.,
const timer = window.setTimeout(...) or let redirectTimer) when githubAuthStatus
=== "success" and return a cleanup function from the React.useEffect that calls
clearTimeout(redirectTimer) to cancel the pending navigation if the component
unmounts or deps change; keep the existing window.history.replaceState and toast
calls intact.
- Around line 54-57: handleGitHubSignIn currently unconditionally redirects
using backendUrl which can be undefined; update the handler to check that
backendUrl (the VITE_BACKEND_URL-derived value) is truthy before setting loading
and assigning window.location.href. If backendUrl is falsy, do not
setGithubLoading(true) and instead route the user to a safe fallback (e.g.,
navigate to a local "/not_configured" page or show an error/toast) so the app
doesn't send the browser to "undefined/api/auth/github". Modify the
handleGitHubSignIn function and related use of setGithubLoading to implement
this guard.
---
Outside diff comments:
In `@backend/config/passportConfig.js`:
- Around line 12-23: The current passport local verify callback leaks account
existence by returning different messages for missing user, accounts without a
password, and wrong passwords; change the logic in the verify function (the
callback that calls done) so that any authentication failure (no user found,
user.password falsy, or comparePassword returns false) returns the same generic
error via done(null, false, { message: 'Invalid email or password' }) instead of
distinct strings; keep the call to user.comparePassword but ensure its result is
masked by the unified error message to avoid account enumeration.
🪄 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: a1723c7c-5212-44e4-8b19-07ab9ba8ae99
📒 Files selected for processing (7)
backend/.env.samplebackend/config/passportConfig.jsbackend/models/User.jsbackend/package.jsonbackend/routes/auth.jsbackend/server.jssrc/pages/Login/Login.tsx
| app.use(bodyParser.json()); | ||
| app.use(session({ | ||
| secret: process.env.SESSION_SECRET, | ||
| secret: process.env.SESSION_SECRET || 'dev-session-secret', |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate and inspect backend/server.js around the referenced lines
if [ -f backend/server.js ]; then
echo "=== backend/server.js (head) ==="
sed -n '1,120p' backend/server.js | cat -n
echo
echo "=== Search for session config and SESSION_SECRET ==="
rg -n "SESSION_SECRET|express-session|app\\.use\\(session\\(|cookie:|sameSite|trust proxy|cors\\(" backend/server.js || true
else
echo "backend/server.js not found"
ls -la backend || true
fi
# Check for any other session middleware usage elsewhere
echo
echo "=== Search repo for express-session usage ==="
rg -n "express-session|app\\.use\\(session\\(" . || true
# Check for CORS usage and FRONTEND_URL usage
echo
echo "=== Search repo for CORS/front-end URL config ==="
rg -n "cors\\(|FRONTEND_URL|frontendUrl" . || trueRepository: GitMetricsLab/github_tracker
Length of output: 3224
Fail fast if SESSION_SECRET is missing in production (backend/server.js:26)
express-session currently falls back to 'dev-session-secret', making session cookie signing predictable when prod env config is incomplete. (Also, with sameSite: 'lax', cross-site credentialed requests may fail if the frontend is on a different site.)
Suggested fix
const app = express();
const frontendUrl = process.env.FRONTEND_URL || 'http://localhost:5173';
+const isProd = process.env.NODE_ENV === 'production';
+if (isProd && !process.env.SESSION_SECRET) {
+ throw new Error('SESSION_SECRET must be set in production');
+}
@@
app.use(session({
- secret: process.env.SESSION_SECRET || 'dev-session-secret',
+ secret: process.env.SESSION_SECRET || 'dev-session-secret',🤖 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` at line 26, The session middleware is using a fallback
'dev-session-secret' which should not be used in production; before calling the
express-session setup (the session({...}) call and its secret property), check
NODE_ENV === 'production' and if process.env.SESSION_SECRET is missing, throw an
error or call process.exit(1) to fail fast; update the session configuration to
use process.env.SESSION_SECRET (no default) for the secret field and ensure any
environment-dependent sameSite/secure options are set appropriately for
production deployments.
Comment on lines
+54
to
+57
| const handleGitHubSignIn = () => { | ||
| setGithubLoading(true); | ||
| window.location.href = `${backendUrl}/api/auth/github`; | ||
| }; |
Contributor
There was a problem hiding this comment.
Guard the GitHub redirect when VITE_BACKEND_URL is missing.
If backendUrl is unset, this sends the browser to undefined/api/auth/github, so the backend's not_configured fallback is never reached and the new sign-in path hard-fails.
Suggested fix
const handleGitHubSignIn = () => {
+ if (!backendUrl) {
+ toast.error("GitHub sign in is not configured for this deployment.");
+ return;
+ }
+
setGithubLoading(true);
- window.location.href = `${backendUrl}/api/auth/github`;
+ window.location.href = `${backendUrl}/api/auth/github`;
};🤖 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/Login/Login.tsx` around lines 54 - 57, handleGitHubSignIn currently
unconditionally redirects using backendUrl which can be undefined; update the
handler to check that backendUrl (the VITE_BACKEND_URL-derived value) is truthy
before setting loading and assigning window.location.href. If backendUrl is
falsy, do not setGithubLoading(true) and instead route the user to a safe
fallback (e.g., navigate to a local "/not_configured" page or show an
error/toast) so the app doesn't send the browser to "undefined/api/auth/github".
Modify the handleGitHubSignIn function and related use of setGithubLoading to
implement this guard.
Comment on lines
+59
to
+83
| React.useEffect(() => { | ||
| const githubAuthStatus = new URLSearchParams(window.location.search).get("githubAuth"); | ||
|
|
||
| if (githubAuthStatus === "success") { | ||
| toast.success("GitHub login successful"); | ||
|
|
||
| window.history.replaceState({}, document.title, window.location.pathname); | ||
|
|
||
| setTimeout(() => { | ||
| navigate("/track"); | ||
| }, 1000); | ||
| } | ||
|
|
||
| if (githubAuthStatus === "failed") { | ||
| toast.error("GitHub sign in failed. Please try again."); | ||
|
|
||
| window.history.replaceState({}, document.title, window.location.pathname); | ||
| } | ||
|
|
||
| if (githubAuthStatus === "not_configured") { | ||
| toast.error("GitHub sign in is not configured on server."); | ||
|
|
||
| window.history.replaceState({}, document.title, window.location.pathname); | ||
| } | ||
| }, [navigate]); |
Contributor
There was a problem hiding this comment.
Clear the delayed redirect timer on cleanup.
Line 67 schedules a navigation that still fires if the user leaves /login within that 1-second window, which can unexpectedly pull them back to /track.
Suggested fix
React.useEffect(() => {
+ let redirectTimer: number | undefined;
const githubAuthStatus = new URLSearchParams(window.location.search).get("githubAuth");
if (githubAuthStatus === "success") {
toast.success("GitHub login successful");
window.history.replaceState({}, document.title, window.location.pathname);
- setTimeout(() => {
+ redirectTimer = window.setTimeout(() => {
navigate("/track");
}, 1000);
}
if (githubAuthStatus === "failed") {
toast.error("GitHub sign in failed. Please try again.");
window.history.replaceState({}, document.title, window.location.pathname);
}
if (githubAuthStatus === "not_configured") {
toast.error("GitHub sign in is not configured on server.");
window.history.replaceState({}, document.title, window.location.pathname);
}
+
+ return () => {
+ if (redirectTimer) {
+ window.clearTimeout(redirectTimer);
+ }
+ };
}, [navigate]);📝 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.
Suggested change
| React.useEffect(() => { | |
| const githubAuthStatus = new URLSearchParams(window.location.search).get("githubAuth"); | |
| if (githubAuthStatus === "success") { | |
| toast.success("GitHub login successful"); | |
| window.history.replaceState({}, document.title, window.location.pathname); | |
| setTimeout(() => { | |
| navigate("/track"); | |
| }, 1000); | |
| } | |
| if (githubAuthStatus === "failed") { | |
| toast.error("GitHub sign in failed. Please try again."); | |
| window.history.replaceState({}, document.title, window.location.pathname); | |
| } | |
| if (githubAuthStatus === "not_configured") { | |
| toast.error("GitHub sign in is not configured on server."); | |
| window.history.replaceState({}, document.title, window.location.pathname); | |
| } | |
| }, [navigate]); | |
| React.useEffect(() => { | |
| let redirectTimer: number | undefined; | |
| const githubAuthStatus = new URLSearchParams(window.location.search).get("githubAuth"); | |
| if (githubAuthStatus === "success") { | |
| toast.success("GitHub login successful"); | |
| window.history.replaceState({}, document.title, window.location.pathname); | |
| redirectTimer = window.setTimeout(() => { | |
| navigate("/track"); | |
| }, 1000); | |
| } | |
| if (githubAuthStatus === "failed") { | |
| toast.error("GitHub sign in failed. Please try again."); | |
| window.history.replaceState({}, document.title, window.location.pathname); | |
| } | |
| if (githubAuthStatus === "not_configured") { | |
| toast.error("GitHub sign in is not configured on server."); | |
| window.history.replaceState({}, document.title, window.location.pathname); | |
| } | |
| return () => { | |
| if (redirectTimer) { | |
| window.clearTimeout(redirectTimer); | |
| } | |
| }; | |
| }, [navigate]); |
🤖 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/Login/Login.tsx` around lines 59 - 83, The useEffect in Login.tsx
schedules a setTimeout for navigate("/track") but never clears it on unmount;
capture the timer id (e.g., const timer = window.setTimeout(...) or let
redirectTimer) when githubAuthStatus === "success" and return a cleanup function
from the React.useEffect that calls clearTimeout(redirectTimer) to cancel the
pending navigation if the component unmounts or deps change; keep the existing
window.history.replaceState and toast calls intact.
Collaborator
|
@pratyushranjn : oauth will be done after we have proper backend, can't merge this PR for now |
mehul-m-prajapati
added
onhold
and removed
level:intermediate
quality:clean
gssoc:approved
labels
Author
|
Understood @mehul-m-prajapati, thanks for the clarification. |
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
Closes #29
Description
Implemented GitHub OAuth Sign-In using Passport GitHub Strategy.
Features added:
Also updated
.env.samplewith GitHub OAuth environment variables.How Has This Been Tested?
Screenshots
Type of Change
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores