feat: Add secure email change flow with verification to OLD email (#407) - #446
feat: Add secure email change flow with verification to OLD email (#407)#446Adil2009700 wants to merge 5 commits into
Conversation
…#410) - Add questSubmissions include to getAssignments API to fetch latest submission data - Create ReworkFeedbackPanel component to display criteria results, quality score, and reviewer notes - Show feedback panel when assignment status is 'needs_rework' - Handle graceful fallback when no feedback data is available
… Wrap handleApprove and handleReject in try/catch blocks - Check HTTP response status and JSON success flag - Display error toast on failure, success toast on success - Use finally block to ensure loading state is reset
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
LarytheLord
left a comment
LarytheLord
left a comment
There was a problem hiding this comment.
Good direction on #407 — password re-verification, old-email notification concept, expiring single-use tokens, and invalidating other pending requests on confirm are all the right building blocks. Three things below need attention before this is production-ready, plus the same stale-branch issue flagged on #445 (this PR appears built on top of that branch).
| // In production, this would integrate with an email service like SendGrid, Resend, etc. | ||
| const verificationLink = `${process.env.NEXTAUTH_URL}/api/user/confirm-email-change?token=${token}`; | ||
|
|
||
| console.log(`Email change verification link for user ${userId}: ${verificationLink}`); |
There was a problem hiding this comment.
This is the core issue: the verification link is only console.log'd, never actually emailed. The whole security premise of #407 ("sends confirmation to OLD email, prevents account takeover") depends on the account's real owner receiving and reviewing this link — right now nobody does. As shipped, this flow can't actually be completed by a real user in production (the token never reaches anywhere user-accessible outside dev mode), so functionally this isn't done yet, not just insecure. Needs wiring to an actual email service before merge — the codebase already has lib/emailService.ts per an earlier (unmerged) attempt at this same fix, worth reusing.
| import crypto from 'crypto'; | ||
|
|
||
| // Rate limiting store (in-memory for simplicity, could use Redis in production) | ||
| const rateLimitStore = new Map<string, { count: number; resetTime: number }>(); |
There was a problem hiding this comment.
In-memory Map for rate limiting won't work correctly once deployed — Next.js API routes on Vercel run as isolated serverless function instances, so this Map isn't shared across concurrent invocations/regions and resets on every cold start/redeploy. The "3 requests/hour" limit is easy to bypass simply by hitting different instances, and resets for free on redeploy. This repo already has Upstash Redis wired up for rate limiting elsewhere (per prior security fixes) — that's the right tool to reuse here instead of a per-instance Map.
| import { prisma } from '@/lib/db'; | ||
| import crypto from 'crypto'; | ||
|
|
||
| export async function GET(req: Request) { |
There was a problem hiding this comment.
This is a GET handler that mutates state (changes the user's email, marks tokens confirmed/expired). Once real email delivery is wired up (see the other comment), email link-scanners, antivirus products, and browser prefetching all commonly auto-visit GET links found in emails — which would silently trigger this confirmation without the user ever clicking. Recommend making the actual state change a POST from a confirmation page the GET renders (GET shows "click to confirm", POST does the mutation), which is the standard pattern for exactly this kind of link-triggered mutation.
| }, | ||
| }, | ||
| // Include latest submission for rework feedback display | ||
| questSubmissions: { |
There was a problem hiding this comment.
Same stale-duplicate issue as #445 — this questSubmissions field name (and the matching changes elsewhere in this diff) duplicates the already-merged #417 rework-view feature, including the same bug fixed there (the real Prisma relation is named submissions). Worth rebasing so this PR only carries the email-change work.
|
Following up on the inline review — the direction here (password re-verification, old-email notification, expiring single-use tokens) is right, it just needs the 3 fixes noted inline (actually send the email, fix the rate limiter for serverless, make confirm a POST) plus a rebase onto current main alongside #445/#447, since all three share the same stale-branch baggage right now. |
2 participants
Fixes #407