feat: add pkce-like session binding and remove nonce impl #525
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
zhongliang02
requested review from
Copilot and
karrui
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
This PR implements a PKCE-like (Proof Key for Code Exchange) authentication flow to enhance OTP security, replacing the previous session-based nonce implementation. The new approach protects against both OTP interception and login request interception, particularly beneficial for mobile/native app implementations where secure channels cannot be guaranteed.
Key changes:
- Replaced session nonce with PKCE flow using
codeVerifierandcodeChallengepairs - Modified database operations to use
createinstead ofupsertto prevent code challenge reuse - Updated client-side state management to store verifier/challenge pairs in memory via React context
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| apps/web/src/validators/auth.ts | Added validation for codeChallenge and codeVerifier parameters |
| apps/web/src/server/session.ts | Removed nonce field from session data interface |
| apps/web/src/server/modules/auth/auth.utils.ts | Updated authentication utilities to use codeChallenge instead of nonce |
| apps/web/src/server/modules/auth/auth.service.ts | Modified login/verification logic to use PKCE flow and prevent challenge reuse |
| apps/web/src/server/modules/auth/auth.pkce.ts | New file implementing PKCE verifier/challenge generation functions |
| apps/web/src/server/modules/auth/tests/auth.utils.spec.ts | Updated tests to use codeChallenge terminology and added entropy validation |
| apps/web/src/server/modules/auth/tests/auth.service.spec.ts | Rewrote tests to validate PKCE flow scenarios |
| apps/web/src/server/api/routers/auth/auth.email.router.ts | Updated router to accept codeChallenge/codeVerifier and removed session nonce handling |
| apps/web/src/app/(public)/sign-in/_components/wizard/email/verification-step.tsx | Modified to retrieve and use codeVerifier from context |
| apps/web/src/app/(public)/sign-in/_components/wizard/email/email-step.tsx | Updated to generate and pass codeChallenge on form submission |
| apps/web/src/app/(public)/sign-in/_components/wizard/email/email-flow.tsx | Added codeChallenge to state propagation |
| apps/web/src/app/(public)/sign-in/_components/wizard/context.tsx | Implemented verifier/challenge map storage and management functions |
Comments suppressed due to low confidence (1)
apps/web/src/server/modules/auth/tests/auth.service.spec.ts:1
- [nitpick] Missing spaces in object destructuring and object literal. Should be:
const { token: invalidToken } = createAuthToken({ email: testEmail, codeChallenge: testCodeChallenge })
import '../../mail/__mocks__/mail.service'
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/web/src/app/(public)/sign-in/_components/wizard/email/email-step.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/app/(public)/sign-in/_components/wizard/email/email-step.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/app/(public)/sign-in/_components/wizard/context.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/app/(public)/sign-in/_components/wizard/context.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/app/(public)/sign-in/_components/wizard/context.tsx
Outdated
Show resolved
Hide resolved
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
apps/web/src/server/modules/auth/__tests__/auth.service.spec.ts
Outdated
Show resolved
Hide resolved
apps/web/src/server/modules/auth/__tests__/auth.service.spec.ts
Outdated
Show resolved
Hide resolved
apps/web/src/app/(public)/sign-in/_components/wizard/email/verification-step.tsx
Show resolved
Hide resolved
Contributor
Author
|
@karrui I need help, Logs |
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
karrui
reviewed
Dec 2, 2025
apps/web/src/app/(public)/sign-in/_components/wizard/context.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/app/(public)/sign-in/_components/wizard/context.tsx
Outdated
Show resolved
Hide resolved
apps/web/src/app/(public)/sign-in/_components/wizard/context.tsx
Outdated
Show resolved
Hide resolved
Collaborator
|
build is failing (locally) too. can see associated issue for the fix? (which looks like overriding some dependencies :O) also also might want to rebase |
Contributor
Author
|
lol its problematic because build works locally for me idek why |
zhongliang02
force-pushed
the
feat/add-pkce-like-session-binding
branch
from
dc49633 to
f19c1e9
Compare
Contributor
Author
|
changed nothing and it works |
Contributor
Author
|
ok i lied i changed vercel build settings to node24 |
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.
Context
Previously, the implementation of OTP is susceptible to interception through shoulder surfing or insecure mail channel or other OTP interception scenarios.
To address this, @karrui implemented a session nonce (#518), which binds the OTP to the login session, thereby protecting against OTP interception.
However, a better approach in my opinion would be to replicate RFC 7636 (originally intended for OAuth 2.0) here.
Details
The idea is as follows,
Login request
codeVerifiercodeChallengecodeChallenge+emailin the login requestOTP response
OTPand stores it (PK on[email, codeChallenge])OTPto userOTP verification request
codeVerifier,email, andOTP.codeVerifierto get acodeChallenge[email, codeChallenge]Advantages
Protection against login request interception
The original
nonceimplementation only guards against OTP response interception.The
PKCE-likeimplementation guards against both OTP response interception and Login request interception.The security improvement is negligible under the traditional browser-server-HTTPs assumption. But in mobile/native app implementations, the security improvement is great, since the Login request cannot be assumed to be transmitted over a secure channel.
I think using this implementation does not incur significant additional engineering costs, but can prevent insecure implementations for teams creating mobile/native apps based on
starter-kit.Protection against db dumps
In the event of a DB dump, while the attacker can extract the OTP through bruteforcing the entire keyspace, it will be impossible for the attacker to recover the original
codeVerifier.Disadvantages
Bloats
VerificationTokentableThis greatly bloats the
VerificationTokentable since each new login attempt creates a new[email, codeChallenge]entry which are only cleaned up on successful logins. However, this is no worse than the previousnonceimplementation. Furthermore, the table is already bloated from users submitting wrong/invalid emails (though they are rate limited in this case).A solution to this is to run a cronjob that removes old entries based on
issuedAt.Additional frontend memory usage
Every login attempt generates a new
codeVerifiercodeChallengepair in frontend which are stored until login succeeds.If the user attempts to log in an absurd amount of times without succeeding, it will lead to memory exhaustion.
Serverside implementation details
We modify the VfnIdentifier to be
JSON.stringify([email, codeChallenge])to prevent issues with string concatenation (e.g. if we construct the id as{email}.{codeChallenge}, an email with a.could lead to incorrect keying)We modify all
auth.utils.tsfunctions to require acodeChallengeas input.We also prevent reuse of
codeChallengeby replacing theVerificationTokenupsert with create.Finally, in our router we expect
codeChallengefor the first leg, andcodeVerifierfor the second leg.There are thorough input validations and tests have been rewritten.
Frontend implementation details
We remove nonce from session data.
In the
SignInWizardProvider, we add aMapto keep track of allcodeVerifierandcodeChallengeever generated. We then provide convenience functionsnewChallenge(),getVerifier(challenge: string)and cleanup functionclearVerifierMap().In the
EmailStep, we generate anewChallenge()on form submission, and we store the challenge we used inVfnStepDataof theSignInWizardProvider.In the
VerificationStep, we retrievegetVerifier(vfnStepData.codeChallenge)and send it with the form submission. When the login flow is successful, the map is cleared.Other risks
This depends on storing the
codeChallengeandcodeVerifierin memory, as opposed tolocalStorageorsessionStorage. This means that if browser refresh or navigation occurs or any other context destroying event occurs, the user needs to restart the login flow.