-
Notifications
You must be signed in to change notification settings - Fork 10
feat: add nonce to ensure generation and verify OTP is via same session #518
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add nonce to ensure generation and verify OTP is via same session #518
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This pull request significantly enhances the security of the email OTP authentication flow by introducing session-bound nonces. Each OTP is now tied to a unique session nonce (UUID), ensuring that OTPs cannot be reused across different sessions or devices, effectively mitigating DoS and session fixation attacks.
Key changes:
- Introduced per-session nonce generation using
randomUUID()that binds each OTP to a specific authentication session - Updated the verification token identifier from email-only to
nonce:emailformat, ensuring session-specific token storage - Modified token hashing to include the nonce, preventing cross-session token validation
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/server/session.ts | Added optional nonce field to SessionData interface with documentation explaining its purpose for authentication flow verification |
| apps/web/src/server/modules/auth/auth.utils.ts | Introduced createVfnIdentifier function and updated createAuthToken, createTokenHash, and isValidToken to require and use nonce parameter for session-specific token operations |
| apps/web/src/server/modules/auth/auth.service.ts | Updated emailLogin and emailVerifyOtp functions to accept nonce parameter and use session-specific identifiers for verification token operations |
| apps/web/src/server/modules/auth/tests/auth.utils.spec.ts | Added comprehensive test coverage for createVfnIdentifier and updated all existing tests to include nonce parameter, including cross-session validation prevention tests |
| apps/web/src/server/modules/auth/tests/auth.service.spec.ts | Updated all authentication service tests to include nonce parameter and added new tests verifying session-specific OTP behavior and nonce validation |
| apps/web/src/server/api/routers/auth/auth.email.router.ts | Implemented nonce generation and session storage in login endpoint, and added nonce validation in OTP verification endpoint with appropriate error handling |
| apps/web/src/app/(public)/sign-in/_components/wizard/email/verification-step.tsx | Added skipStreaming: true to resend OTP mutation to ensure session data is properly set |
| apps/web/src/app/(public)/sign-in/_components/wizard/email/email-step.tsx | Added skipStreaming: true to login mutation to ensure session data is properly set |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/web/src/app/(public)/sign-in/_components/wizard/email/verification-step.tsx
Show resolved
Hide resolved
apps/web/src/app/(public)/sign-in/_components/wizard/email/email-step.tsx
Show resolved
Hide resolved
| import { env } from '~/env' | ||
|
|
||
| export interface SessionData { | ||
| // Used to verify that the same user that started the authentication flow is the one verifying it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: from code, either nonce is defined or userId is defined so should be xor type but idt it matters much
|
forgot to get security eyes on this, help do post-review haha since this is merged already |
| return db.verificationToken.delete({ | ||
| where: { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@karrui how are unused verification tokens from previous attempts cleaned up?
Seems like they don't get auto-deleted since the identifier now includes the nonce.
|
I understand the attack scenario is a shoulder surfing otp situation or insecure mail delivery or malicious mail provider or other otp interception scenarios. I think this is quite similar to situations in mobile app oauth where in the first leg. I.e. once the user logs in and gets an access code, the access code is usually passed back to the app through a custom app url schema -> app intent, which can be intercepted by the browser or other apps. the solution oauth takes is to do a dance called PKCE, where in the first leg, the client generates a secret and sends over the hash secret. in the second leg of oauth, the access token must be sent together with that secret to the token server to get a token. the server verifies that it's the preimage of the hash sent in the first leg. while the solution of generating a nonce serverside works to address the insecure mail channel, it can be further improved with little implementation cost to address the (although small) possibility of an insecure otp request channel by doing pkce |
There still needs to be a rate limit to prevent email spam, but DoS possibility should be mitigated now.
This pull request introduces a significant security and architecture improvement to the email OTP authentication flow by introducing a per-session nonce. Each OTP is now tied to a unique session nonce, ensuring that OTPs cannot be reused across sessions or devices and that each login attempt is isolated. The changes affect both backend logic and frontend mutation calls, and are thoroughly tested with new and updated unit tests.
Key changes include:
Authentication Flow & Security Improvements:
nonce(usingrandomUUID) for each login attempt, stores it in the session, and requires it for OTP verification. This ensures OTPs are session-bound and cannot be reused across sessions. [1]], [2]])Frontend Integration:
skipStreaming: truein the tRPC context to ensure session data (including nonce) is properly set and available for subsequent verification. [1]], [2]])Testing Enhancements:
Utility Functions:
createVfnIdentifier, updatedcreateAuthToken, andisValidToken) now require bothemailandnonce, ensuring all token operations are session-specific. [1]], [2]], [3]])These changes collectively harden the authentication system, making OTP attacks and session fixation much harder, and improve the clarity and maintainability of the codebase.