Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Pull request overview
This PR enhances the verification flow by adding better handling of identifier (email/phone) management during registration and resend flows. The changes include updating the email sender domain, adding identifier URL parameters for improved resend functionality, adding defensive null checks, and creating a new helper endpoint to retrieve identifiers from tokens.
Key Changes:
- Updated email sender from test domain to production domain (
auth@soulledger.app) - Added identifier parameter passing in verification URLs and improved resend functionality with token-based identifier lookup
- Added defensive validation for userId in verification flow and improved phone verification logging
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/lib/email-service.ts | Updated sender email address from test domain to production domain |
| src/app/auth/verify-request/page.tsx | Added identifier state management and token-based identifier lookup for resend functionality |
| src/app/api/auth/verify/route.ts | Added defensive null check for userId before database operations |
| src/app/api/auth/resend-verification/route.ts | Improved phone verification logging and added token return in development mode |
| src/app/api/auth/register/route.ts | Added identifier URL parameters for verification flow and improved phone verification logging |
| src/app/api/auth/get-identifier-from-token/route.ts | New helper endpoint to retrieve identifier from verification token for resend flows |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/app/auth/verify-request/page.tsx
Outdated
| setType(lookupData.type); | ||
| } | ||
| } | ||
| } catch (err) { |
There was a problem hiding this comment.
[nitpick] The error handling here silently fails and proceeds to show a generic error message. However, the actual API call to /api/auth/get-identifier-from-token might fail for legitimate reasons (network error, server error) that should be distinguished from a missing identifier.
Consider logging the error for debugging purposes:
} catch (err) {
console.error('Failed to fetch identifier from token:', err);
// Silently fail - we'll show error below if identifier is still missing
}| } catch (err) { | |
| } catch (err) { | |
| console.error('Failed to fetch identifier from token:', err); |
src/app/api/auth/register/route.ts
Outdated
| redirectUrl += `?identifier=${encodeURIComponent(identifierParam)}&type=${email ? 'EMAIL_VERIFICATION' : 'PHONE_VERIFICATION'}`; | ||
|
|
||
| if (invitationAccepted && inviterName) { | ||
| redirectUrl = `/welcome?accepted=true&inviterName=${encodeURIComponent(inviterName)}`; |
There was a problem hiding this comment.
When the invitation is accepted, the redirectUrl is overwritten on line 221, losing the identifier and type parameters that were added on line 218. This means that users who register via invitation won't have the identifier available for the resend verification functionality, breaking the resend flow for invited users.
Consider preserving the query parameters when setting the welcome redirect:
if (invitationAccepted && inviterName) {
redirectUrl += `&accepted=true&inviterName=${encodeURIComponent(inviterName)}`;
redirectUrl = redirectUrl.replace('/auth/verify-request', '/welcome');
}| redirectUrl = `/welcome?accepted=true&inviterName=${encodeURIComponent(inviterName)}`; | |
| // Preserve existing query parameters and append accepted/inviterName | |
| redirectUrl = redirectUrl.replace('/auth/verify-request', '/welcome') + `&accepted=true&inviterName=${encodeURIComponent(inviterName)}`; |
src/app/api/auth/register/route.ts
Outdated
| // Build redirect URL if invitation was accepted | ||
| let redirectUrl = '/auth/verify-request'; | ||
| // Pass identifier in URL for resend functionality | ||
| const identifierParam = email || phone!; |
There was a problem hiding this comment.
The non-null assertion operator ! is used here, but it's unsafe since this code path executes when email is falsy. While TypeScript's type narrowing should ensure phone is defined, using the non-null assertion is risky and could lead to runtime errors if the logic changes.
Consider using an explicit check or optional chaining with a fallback:
const identifierParam = email || phone || '';
if (!identifierParam) {
throw new Error('Either email or phone must be provided');
}| import { NextRequest, NextResponse } from "next/server"; | ||
| import { prisma } from "@/lib/prisma"; | ||
|
|
||
| /** | ||
| * Helper endpoint to get identifier (email/phone) from a verification token | ||
| * This is used by the resend verification flow when identifier is not in URL | ||
| */ | ||
| export async function POST(request: NextRequest) { | ||
| try { | ||
| const { token } = await request.json(); | ||
|
|
||
| if (!token) { | ||
| return NextResponse.json( | ||
| { error: "Token is required" }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| // Find verification token | ||
| const verificationToken = await prisma.verificationToken.findUnique({ | ||
| where: { token }, | ||
| include: { user: true }, | ||
| }); | ||
|
|
||
| if (!verificationToken) { | ||
| return NextResponse.json( | ||
| { error: "Invalid verification token" }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| // Check if token is expired | ||
| if (verificationToken.expires < new Date()) { | ||
| return NextResponse.json( | ||
| { error: "Verification token has expired" }, | ||
| { status: 400 } | ||
| ); | ||
| } | ||
|
|
||
| // Return identifier and type | ||
| return NextResponse.json({ | ||
| identifier: verificationToken.identifier, | ||
| type: verificationToken.type, | ||
| }); | ||
| } catch (error) { | ||
| console.error("Get identifier from token error:", error); | ||
| return NextResponse.json( | ||
| { error: "Internal server error" }, | ||
| { status: 500 } | ||
| ); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The new helper endpoint /api/auth/get-identifier-from-token exposes a potential security risk. It allows anyone with a valid (even expired within 24 hours) token to query for user identifiers (email/phone), which could be exploited for user enumeration or information disclosure.
Consider adding rate limiting to this endpoint and/or requiring additional authentication. Additionally, consider whether this endpoint is necessary - the identifier could be stored in localStorage/sessionStorage during registration instead of fetching it from the server.
| if (type === "PHONE_VERIFICATION") { | ||
| // SMS implementation will be added later | ||
| console.log(`📱 SMS verification for phone numbers will be implemented soon`); | ||
| // For now, log the token in development mode | ||
| if (process.env.NODE_ENV === 'development') { | ||
| console.log(`📱 PHONE VERIFICATION TOKEN (RESEND) for ${identifier}:`, token); | ||
| console.log(`📱 SMS verification for phone numbers will be implemented soon`); | ||
| } | ||
| // In production, this should return an error or be handled differently | ||
| // For now, we'll return success but note that SMS wasn't actually sent | ||
| } |
There was a problem hiding this comment.
In production, when NODE_ENV !== 'development', the phone verification flow silently succeeds without actually sending any SMS. This creates a confusing user experience where users receive a success message but never get their verification code.
The code should either:
- Return an error in production indicating SMS is not yet implemented
- Implement the SMS sending functionality
- Clearly document this limitation in the API response
Consider:
if (process.env.NODE_ENV === 'development') {
console.log(`📱 PHONE VERIFICATION TOKEN (RESEND) for ${identifier}:`, token);
} else {
return NextResponse.json(
{ error: "Phone verification is not yet implemented" },
{ status: 501 } // Not Implemented
);
}There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| if (phone) { | ||
| // SMS implementation will be added later | ||
| console.log(`📱 SMS verification for phone numbers will be implemented soon`); | ||
| // For now, log the token in development mode | ||
| if (process.env.NODE_ENV === 'development') { | ||
| console.log(`📱 PHONE VERIFICATION TOKEN for ${phone}:`, token); | ||
| console.log(`📱 SMS verification for phone numbers will be implemented soon`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Same issue as in the resend-verification route: in production, phone verification silently succeeds without sending SMS, creating a confusing user experience where registration appears successful but users never receive their verification code.
Consider returning an error in production or implementing the SMS functionality:
if (process.env.NODE_ENV === 'development') {
console.log(`📱 PHONE VERIFICATION TOKEN for ${phone}:`, token);
} else {
return NextResponse.json(
{ error: "Phone verification is not yet implemented" },
{ status: 501 } // Not Implemented
);
}There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@chemist-god I've opened a new pull request, #62, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@chemist-god I've opened a new pull request, #63, to work on those changes. Once the pull request is ready, I'll request review from you. |
Update: Auth endpoints, verification UI, email service
Fixes recent edge cases in the verification flow (resend/verify) and tightens registration behavior around invite/verification handling.Improves verification email formatting/handling to reduce deliverability and UX issues.Adds a small helper endpoint to extract identifiers from tokens for downstream flows.