Skip to content
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.

Sign up for GitHub

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

Missing PKCE is success in email verification flow #1145

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Missing PKCE is success in email verification flow #1145

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
72f5e13
Missing PKCE is success in email verification flow
scotttrinh Dec 10, 2024
d6c1053
Update the built-in UI callback as well
scotttrinh Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions packages/auth-express/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -413,7 +413,11 @@ export class ExpressAuth {
}
const verifier = req.cookies[this.options.pkceVerifierCookieName];
if (!verifier) {
throw new PKCEError("no pkce verifier cookie found");
// End user verified email from a different user agent than sign-up.
// This is fine, but the application will need to detect this and
// inform the end user that they will need to initiate a new sign up
// attempt to complete the flow.
return next();
}
const isSignUp = searchParams.get("isSignUp") === "true";
const tokenData = await (await this.core).getToken(code, verifier);
Expand Down Expand Up @@ -498,8 +502,13 @@ export class ExpressAuth {
throw new PKCEError("no verification_token in response");
}
if (!verifier) {
throw new PKCEError("no pkce verifier cookie found");
// End user verified email from a different user agent than sign-up.
// This is fine, but the application will need to detect this and
// inform the end user that they will need to initiate a new sign up
// attempt to complete the flow.
return next();
}

const tokenData = await (
await this.core
).verifyEmailPasswordSignup(verificationToken, verifier);
Expand Down
19 changes: 15 additions & 4 deletions packages/auth-nextjs/src/shared.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export interface CreateAuthRouteHandlers {
): Promise<Response>;
onEmailVerify(
params: ParamsOrError<
{ tokenData: TokenData },
{ tokenData: TokenData | null },
{ verificationToken?: string }
>,
req: NextRequest,
Expand Down Expand Up @@ -357,10 +357,14 @@ export abstract class NextAuth extends NextAuthHelpers {
);
}
if (!verifier) {
// End user verified email from a different user agent than
// sign-up. This is fine, but the application will need to detect
// this and inform the end user that they will need to initiate a
// new sign up attempt to complete the flow.
return onEmailVerify(
{
error: new PKCEError("no pkce verifier cookie found"),
verificationToken,
error: null,
tokenData: null,
},
req,
);
Expand Down Expand Up @@ -560,9 +564,16 @@ export abstract class NextAuth extends NextAuthHelpers {
this.options.pkceVerifierCookieName,
)?.value;
if (!verifier) {
// End user verified email from a different user agent than
// sign-up. This is fine, but the application will need to detect
// this and inform the end user that they will need to initiate a
// new sign up attempt to complete the flow.
return onBuiltinUICallback(
{
error: new PKCEError("no pkce verifier cookie found"),
error: null,
tokenData: null,
provider: null,
isSignUp: false,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this probably should be true, as email verification is part of the sign up flow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, good point. I think we should actually use the isSignUp search param here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, actually, I don't think this works as-is: it's impossible to tell the difference between a sign in attempt from an unverified email vs. a successful verification without the PKCE verifier since they have the same shape.

Let me do a little design work here. I think we might need to introduce a discriminant into this callback payload type to make it less ambiguous for the consumer.

},
req,
);
Expand Down
19 changes: 15 additions & 4 deletions packages/auth-remix/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ export interface CreateAuthRouteHandlers {
): Promise<Response>;
onEmailVerify(
params: ParamsOrError<
{ tokenData: TokenData },
{ tokenData: TokenData | null },
{ verificationToken?: string }
>,
): Promise<Response>;
Expand Down Expand Up @@ -358,8 +358,15 @@ export class RemixServerAuth extends RemixClientAuth {
parseCookies(req)[this.options.pkceVerifierCookieName];

if (!verifier) {
// End user verified email from a different user agent than
// sign-up. This is fine, but the application will need to detect
// this and inform the end user that they will need to initiate a
// new sign up attempt to complete the flow.
return cbCall(onBuiltinUICallback, {
error: new PKCEError("no pkce verifier cookie found"),
error: null,
tokenData: null,
provider: null,
isSignUp: false,
});
}
const isSignUp = searchParams.get("isSignUp") === "true";
Expand Down Expand Up @@ -422,9 +429,13 @@ export class RemixServerAuth extends RemixClientAuth {
});
}
if (!verifier) {
// End user verified email from a different user agent than
// sign-up. This is fine, but the application will need to detect
// this and inform the end user that they will need to initiate a
// new sign up attempt to complete the flow.
return cbCall(onEmailVerify, {
error: new PKCEError("no pkce verifier cookie found"),
verificationToken,
error: null,
tokenData: null,
});
}
let tokenData: TokenData;
Expand Down
19 changes: 15 additions & 4 deletions packages/auth-sveltekit/src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ export interface AuthRouteHandlers {
) => Promise<never>;
onEmailVerify?: (
params: ParamsOrError<
{ tokenData: TokenData },
{ tokenData: TokenData | null },
{ verificationToken?: string }
>,
) => Promise<never>;
Expand Down Expand Up @@ -597,8 +597,15 @@ async function handleAuthRoutes(
const verifier = cookies.get(config.pkceVerifierCookieName);

if (!verifier) {
// End user verified email from a different user agent than sign-up.
// This is fine, but the application will need to detect this and inform
// the end user that they will need to initiate a new sign up attempt to
// complete the flow.
return onBuiltinUICallback({
error: new PKCEError("no pkce verifier cookie found"),
error: null,
tokenData: null,
provider: null,
isSignUp: false,
});
}
const isSignUp = searchParams.get("isSignUp") === "true";
Expand Down Expand Up @@ -653,9 +660,13 @@ async function handleAuthRoutes(
});
}
if (!verifier) {
// End user verified email from a different user agent than sign-up.
// This is fine, but the application will need to detect this and inform
// the end user that they will need to initiate a new sign up attempt to
// complete the flow.
return onEmailVerify({
error: new PKCEError("no pkce verifier cookie found"),
verificationToken,
error: null,
tokenData: null,
});
}
let tokenData: TokenData;
Expand Down
Loading