Fix/GitHub oauth signin#8
Conversation
Priyanshu-byte-coder
left a comment
There was a problem hiding this comment.
Thanks for the contribution! The core fix (adding the missing NextAuth route) is correct and needed. But there are a few issues to resolve before this can merge.
Blockers
1. Two competing auth systems
This PR adds NextAuth (client/src/lib/auth.ts) and keeps the Express backend OAuth flow (server/src/controllers/githubController.ts). They both handle GitHub OAuth but with different callback URLs and token formats — they'll conflict.
The sign-in button now uses NextAuth's flow (/api/auth/signin/github?callbackUrl=/dashboard), which calls back to /api/auth/callback/github (NextAuth's route). But AuthTokenHandler looks for a #token= hash, which only comes from the Express backend's redirect in githubCallback. With the current setup, the hash will never appear and the component does nothing.
Please pick one flow and remove the other:
- Option A (recommended): Use NextAuth fully — remove
githubSignIn, removeAuthTokenHandler, add acallbacks.sessioninauth.tsto expose the GitHub access token viauseSession() - Option B: Keep Express JWT flow — remove
auth.ts, remove the NextAuth route, revert the sign-in link to point to the Express backend
2. JWT token passed via URL hash is a security risk
res.redirect(${clientUrl}/dashboard#token=${...}) puts a JWT in the URL hash. While hashes don't go to the server, they:
- Are stored in browser history (visible to anyone with access to the machine)
- Are readable by browser extensions
- May appear in analytics tools
If keeping the Express JWT flow (Option B), exchange the token via a short-lived httpOnly cookie or a one-time code instead.
3. Unused variable
In client/src/app/page.tsx:
const apiUrl = process.env.NEXT_PUBLIC_API_URL ?? "http://localhost:4000";apiUrl is declared but never used. Remove it.
Minor
auth.tsis missing acallbacksblock — without ituseSession()won't expose the GitHub access token, so components can't call the GitHub API on behalf of the user. If going with Option A, add:
callbacks: {
async session({ session, token }) {
session.accessToken = token.accessToken as string;
return session;
},
async jwt({ token, account }) {
if (account) token.accessToken = account.access_token;
return token;
},
},Fix the two blockers and this is close to mergeable. Let me know which auth option you want to go with and I can give more specific guidance.
Changes made: - Removed the Express GitHub OAuth route/controller so there is no competing callback flow. - Removed AuthTokenHandler and the #token= handling path. - Added NextAuth jwt and session callbacks so the GitHub access token is exposed as session.accessToken. - Added SessionProvider and NextAuth session type augmentation for accessToken. - Removed the unused apiUrl variable from client/src/app/page.tsx. - Cleaned the README and env examples so they no longer reference the old backend OAuth/JWT flow.
|
Thanks for the detailed review. I went with Option A and moved the app to the NextAuth-only flow. Changes made:
I also ran type checks for both client and server, and both pass. next lint is currently blocked by the repo not having ESLint initialized yet, since it opens the interactive setup prompt. |
226db15
into
Priyanshu-byte-coder:main
Summary
Fixes GitHub sign-in by adding the missing NextAuth API route for the App Router and sending users to the dashboard after authentication.
Closes #7
Type of Change
Changes Made
client/src/app/api/auth/[...nextauth]/route.ts.client/src/lib/auth.ts./dashboardafter login.How to Test
Steps for the reviewer to verify this works:
NEXTAUTH_URL,NEXTAUTH_SECRET,GITHUB_ID, andGITHUB_SECRETinclient/.env.local.http://localhost:3000/api/auth/callback/github.npm run dev.http://localhost:3000./dashboard.http://localhost:3000/api/auth/sessionand confirm a session object is returned.Screenshots (if UI change)
N/A
Checklist
npm run lintpasses locallynpm run type-check)