auth.resend() consistent confirmation flow#2401
auth.resend() consistent confirmation flow#2401weilirs wants to merge 1 commit intosupabase:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Cache: Disabled due to Reviews > Disable Cache setting Disabled knowledge base sources:
📒 Files selected for processing (2)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThe changes add PKCE (Proof Key for Code Exchange) support to the resend confirmation API. The Sequence Diagram(s)sequenceDiagram
participant Client
participant ResendHandler as Resend Handler
participant Validator as PKCE Validator
participant FlowState as Flow State Generator
participant MailService as Mail Service
Client->>ResendHandler: POST /resend<br/>(with code_challenge, code_challenge_method)
ResendHandler->>Validator: validatePKCEParams(method, challenge)
alt PKCE params invalid
Validator-->>ResendHandler: Error
ResendHandler-->>Client: HTTP 400
else PKCE params valid
Validator-->>ResendHandler: Valid
ResendHandler->>ResendHandler: Determine flowType from CodeChallenge
alt PKCE flow detected
ResendHandler->>FlowState: Generate flow state
FlowState-->>ResendHandler: Flow state data
end
ResendHandler->>MailService: sendConfirmation/sendEmailChange(flowType)
MailService->>MailService: Create PKCE-prefixed token
MailService-->>Client: Confirmation sent
end
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| return a.sendConfirmation(r, tx, user, models.ImplicitFlow) | ||
| flowType := getFlowFromChallenge(params.CodeChallenge) | ||
| if isPKCEFlow(flowType) { | ||
| if _, terr := generateFlowState(tx, models.EmailSignup.String(), models.EmailSignup, params.CodeChallengeMethod, params.CodeChallenge, &user.ID); terr != nil { |
There was a problem hiding this comment.
🟠 Severity: HIGH
The /resend endpoint allows unauthenticated users to create a new PKCE FlowState for any user. Since verification retrieves the latest FlowState by user_id, an attacker can overwrite a victim's flow. By providing a controlled redirect_to URL, the attacker can intercept the auth_code and hijack the session.
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: The /resend endpoint should not allow unauthenticated users to create or modify FlowState records with PKCE parameters for arbitrary users. The most secure fix is to reject PKCE parameters entirely on the /resend endpoint for signup and email_change verification types. Remove the PKCE flow handling code (lines 130-135 for signup and lines 147-152 for email_change) that calls generateFlowState(). The /resend endpoint is meant for retrying delivery of verification emails, not for initiating new PKCE authentication flows. Since this endpoint is unauthenticated and allows lookups by email, allowing PKCE flow creation creates an inherent authorization vulnerability where attackers can overwrite legitimate users' FlowState records with attacker-controlled code_challenge values.
|
Hi @weilirs , this looks good! Can you rebase the |
weilirs
force-pushed
the
auth.resend-consistent-confirmation-flow
branch
from
284a68a to
7641c89
Compare
@cemalkilic sorry for the late reply, I've rebased the master. |
There was a problem hiding this comment.
Pull request overview
Updates the /resend endpoint to support PKCE-based confirmation for signup and email_change, so resent emails can follow the same code-based confirmation flow as the original signup/email-change request (instead of always using implicit/hash tokens).
Changes:
- Add optional
code_challenge/code_challenge_methodto resend request params for email-based resend types. - Determine flow type from
code_challenge, create a PKCEflow_statewhen needed, and pass the computed flow type into the mail senders. - Add tests covering PKCE param validation and successful PKCE resend behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/api/resend.go |
Adds PKCE request fields + validation and makes resend flow-type aware for signup/email_change (including flow_state creation). |
internal/api/resend_test.go |
Adds PKCE-specific validation and success tests for resend behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| case mail.SignupVerification, mail.EmailChangeVerification: | ||
| if err := validatePKCEParams(p.CodeChallengeMethod, p.CodeChallenge); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
validatePKCEParams only checks that both PKCE params are present and that code_challenge is well-formed; it does not validate that code_challenge_method is one of the supported values. With the current flow, an unsupported method (e.g. "sha512") will pass validation but later fail in models.NewFlowState and be returned as an unhandled error, resulting in a 500. Consider validating/parsing CodeChallengeMethod when CodeChallenge is set and returning a 400 (validation failed) on unsupported methods.
| return a.sendConfirmation(r, tx, user, models.ImplicitFlow) | ||
| flowType := getFlowFromChallenge(params.CodeChallenge) | ||
| if isPKCEFlow(flowType) { | ||
| if _, terr := generateFlowState(tx, models.EmailSignup.String(), models.EmailSignup, params.CodeChallengeMethod, params.CodeChallenge, &user.ID); terr != nil { |
There was a problem hiding this comment.
For signup resends, generateFlowState is being created with ProviderType set to models.EmailSignup.String() ("email/signup"). In the initial signup flow, the flow state is created with ProviderType set to the provider ("email"). This means PKCE token exchange after a resend will report a different provider_type to audit logging/metering than the original signup. Consider using the same provider type as signup.go ("email") for consistency.
| if _, terr := generateFlowState(tx, models.EmailSignup.String(), models.EmailSignup, params.CodeChallengeMethod, params.CodeChallenge, &user.ID); terr != nil { | |
| if _, terr := generateFlowState(tx, "email", models.EmailSignup, params.CodeChallengeMethod, params.CodeChallenge, &user.ID); terr != nil { |
| "code": http.StatusBadRequest, | ||
| "message": InvalidPKCEParamsErrorMessage, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
The PKCE validation tests cover missing/partial parameters, but there is no case for an unsupported code_challenge_method. Given the current implementation, an unsupported method would currently bubble up as an unhandled error (500). Adding a test for an invalid method (and asserting a 400) will help lock in the desired behavior once method validation is added.
| }, | |
| }, | |
| { | |
| desc: "Signup with unsupported code_challenge_method", | |
| params: map[string]interface{}{ | |
| "type": "signup", | |
| "email": "foo@example.com", | |
| "code_challenge": validChallenge, | |
| "code_challenge_method": "unsupported", | |
| }, | |
| expected: map[string]interface{}{ | |
| "code": http.StatusBadRequest, | |
| "message": InvalidPKCEParamsErrorMessage, | |
| }, | |
| }, |
What kind of change does this PR introduce?
Bug fix
What is the current behavior?
The
/resendendpoint hardcodesmodels.ImplicitFlowfor bothsignupandemail_changeverification types (#42527). This means resent confirmation emails always use the implicit flow — redirecting with tokens in the URL hash fragment (#access_token=...) — even when the originalsignUp()used PKCE.This creates an inconsistency where:
https://example.com/auth/confirm?code=xxx(PKCE, works with server routes)https://example.com/auth/confirm#access_token=xxx(implicit, requires client-side handling)Server-side route handlers (e.g., Next.js
route.ts) cannot read hash fragments, forcing developers to implement workarounds with client components and dual flow handling.Closes #42527
What is the new behavior?
The
/resendendpoint now accepts optionalcode_challengeandcode_challenge_methodparameters forsignupandemail_changetypes. When provided, the endpoint:code_challenge(PKCE if present, implicit if absent)FlowStaterecord for PKCE flows (needed by/verifyto issue an auth code)sendConfirmation/sendEmailChangeThis produces confirmation emails with
?code=...query params instead of#access_token=...hash fragments, consistent with the initial signup flow.When
code_challengeis not provided, behavior is unchanged — implicit flow is used, maintaining full backward compatibility.Changes:
internal/api/resend.go: AddedCodeChallengeandCodeChallengeMethodfields toResendConfirmationParams. Added PKCE param validation for email-based types. Replaced hardcodedImplicitFlowwith flow-aware logic forsignupandemail_changecases.internal/api/resend_test.go: AddedTestResendPKCEValidation(invalid PKCE params return 400) andTestResendPKCESuccess(signup and email change tokens getpkce_prefix when PKCE params are provided).Additional context
This is the server-side half of the fix. The JS SDK (
auth-js) needs a corresponding update to sendcode_challenge/code_challenge_methodinresend()calls whenflowType === 'pkce', following the same pattern already used bysignUp()andsignInWithOtp(). See this PRThe implementation mirrors the existing PKCE pattern used across the codebase (
signup.go,user.go,recover.go,magic_link.go):getFlowFromChallenge→ conditionalgenerateFlowState→ passflowTypeto the email sender.