-
Notifications
You must be signed in to change notification settings - Fork 28
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
Fix cookie bugs #171
Fix cookie bugs #171
Conversation
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.
Changes look good to me. I added a small suggestion but don't consider it a blocker.
src/authkit-callback-route.ts
Outdated
@@ -73,7 +73,7 @@ export function handleAuth(options: HandleAuthOptions = {}) { | |||
const cookieName = WORKOS_COOKIE_NAME || 'wos-session'; | |||
const nextCookies = await cookies(); | |||
|
|||
nextCookies.set(cookieName, session, getCookieOptions(request.url)); | |||
nextCookies.set(cookieName, session, getCookieOptions(request.url) as CookieOptions); |
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(non-blocking): I think there's a way to improve the DX on getCookieOptions
where the type could be automatically narrowed, but definitely not a blocker here.
export function getCookieOptions( | ||
redirectUri?: string | null, | ||
asString: boolean = false, | ||
expired: boolean = false, | ||
): CookieOptions | string { |
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.
suggestion(non-blocking): Possible to overload the function signature to ensure the type is narrowed here.
export function getCookieOptions( | |
redirectUri?: string | null, | |
asString: boolean = false, | |
expired: boolean = false, | |
): CookieOptions | string { | |
export function getCookieOptions(): CookieOptions; | |
export function getCookieOptions(redirectUri?: string | null): CookieOptions; | |
export function getCookieOptions(redirectUri: string | null | undefined, asString: true, expired?: boolean): string; | |
export function getCookieOptions( | |
redirectUri: string | null | undefined, | |
asString: false, | |
expired?: boolean, | |
): CookieOptions; | |
export function getCookieOptions( | |
redirectUri?: string | null, | |
asString?: boolean, | |
expired?: boolean, | |
): CookieOptions | string; | |
export function getCookieOptions( | |
redirectUri?: string | null, | |
asString: boolean = false, | |
expired: boolean = false, | |
): CookieOptions | string { |
// When we need to delete a cookie, return it as a header as you can't delete cookies from edge middleware | ||
const deleteCookie = `${cookieName}=; Expires=${new Date(0).toUTCString()}; ${getCookieOptions(request.url, true, true)}`; | ||
newRequestHeaders.append('Set-Cookie', deleteCookie); |
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.
I think this attempt to delete a cookie doesn't work when developing locally and testing on localhost
. When the session is invalid and a refresh attempt fails, I'm still seeing the cookie in my browser's dev tools with an encoded value and expiration set over a year in the future.
The error is being logged as follows:
Session invalid. Refreshing access token that ends in hGt09AzLTA
Failed to refresh. Deleting cookie. OauthException: Error: invalid_grant
Error Description: Refresh token already exchanged.
at Generator.throw (<anonymous>) {
status: 400,
requestID: '',
error: 'invalid_grant',
errorDescription: 'Refresh token already exchanged.',
rawData: {
error: 'invalid_grant',
error_description: 'Refresh token already exchanged.'
}
}
I'm using the new authkit()
middleware handler and manually handling the authorizationUrl
redirect. If I don't manually delete the cookie, attempting to navigate the returned authorizationUrl
results in a redirect to https://error.workos.com/sso. Inspecting the returned authorizationUrl
, it appears the redirect_uri
query parameter is not included. Once I delete the cookie, the authorizationUrl
appears to be correct once again.
@bdbergeron I'm experiencing this exact issue. What code did you write to delete the cookie and solve the issue? For some reason my cookies are empty when I log them. Edit: Realized my issue is slightly different. I have a custom callback route handler that redirects to a dashboard based on organization id. Before v1 I was able to access the session via withAuth(), but with the upgrade, withAuth() returns null. I resolved the overall issue by providing a fallback redirect, but now my custom redirect doesn't work. |
Fixes #170
v1 shipped with bugs surrounding the refresh logic.
tl;dr is that we were using Next's
cookies()
helper method, which doesn't work in edge middleware. Instead we now set cookies via the headers.Want to release this as a fix as the general API is unchanged.