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.

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

callback_url never stored in cookies #23

Closed
dutsik opened this issue Jul 13, 2023 · 10 comments · Fixed by #24, #37 or #38
Closed

callback_url never stored in cookies #23

dutsik opened this issue Jul 13, 2023 · 10 comments · Fixed by #24, #37 or #38
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@dutsik
Copy link

dutsik commented Jul 13, 2023

Environment

node: 18.16.1

Reproduction

I am using signin helper with custom callback_url like this:

  useAuth().signIn('auth0', {callbackUrl:"/activity/hausfuehrung-chesa-laudenbacher", redirect:true}, options)

but this callback never triggered. instead you get to a default home page.

Describe the bug

I can assume that it should be stored in cookie before we do an actual redirect to auth provider page. but it is not... it is basically overwriten while copying cookie headers in utility function respondWithResponse and only the last cookie is stored
packages/authjs-nuxt/src/runtime/utils.ts#L83

i beleive that the straight forward fix would be smth like this:

event.node.res.setHeader(key, value); => event.node.res.appendHeader(key, value);

Additional context

No response

Logs

No response

@Hebilicious Hebilicious added bug Something isn't working good first issue Good for newcomers labels Jul 13, 2023
@Hebilicious
Copy link
Owner

Hebilicious commented Jul 13, 2023

Hi!

This is a bigger issue than I realised and I've opened an issue to track it there unjs/h3#439
Your approach wouldn't work in all runtimes ... So I think to fix it there, we should introduce https://www.npmjs.com/package/set-cookie-parser

Response support has landed in h3 and it will be out in the next release : unjs/h3#436
I think we should change this to appendHeader in h3 as well.

@Hebilicious
Copy link
Owner

@dutsik could you try #24 and let me know if it fixes your issue ?

@dutsik-p
Copy link

0.1.5 worked as expected
0.1.6 introduced a regression. so it doesnot work :(

@Hebilicious
Copy link
Owner

0.1.5 worked as expected 0.1.6 introduced a regression. so it doesnot work :(

Yes this got fixed in 0.1.7, sorry about that.

@dutsik-p
Copy link

no it doesnot fix the issue.
this code that was removed in 0.1.6

  const cookieHeader = response.headers.get("set-cookie")
  if (cookieHeader) {
    const cookieString = setCookieParser.splitCookiesString(cookieHeader)
    event.node.res.setHeader("set-cookie", cookieString)

was solving the issue.

Set-Cookie header is just one header with all the cookie combined in it. you cannot loop through setting this header, you have to combine them and set or if you wish to loop through you need to append.

@Hebilicious
Copy link
Owner

Hebilicious commented Jul 19, 2023

@dutsik this code is still present

if (key === "set-cookie") event.node.res.setHeader(key, splitCookiesString(value))

Are you still experiencing the issue ?
Btw we just landed unjs/h3#445 in h3 so we'll get better support with the next release.

@dutsik
Copy link
Author

dutsik commented Jul 19, 2023

No it doesn’t work

I guess you need to revert back the part that process cookie headers separately.

Or loop through the ‘response.headers.entries()’ instead of ‘response.headers’, but I am not familiar with that part

@Hebilicious
Copy link
Owner

Hebilicious commented Jul 19, 2023

The logic is exactly the same and work on my end, can you please provide me with a reproduction repo/stackblitz? There could be a bug with h3 splitCookieString, which would explain the different behaviour between 0.1.5 and the releases after.

I just pushed 0.1.8, let me know if that fixes it for you @dutsik.

@dutsik
Copy link
Author

dutsik commented Jul 19, 2023

I created the sandbox for you.
expected behavior: when you click on signing the redirect cookie should be set correctly.

Here is what we have:

  • v0.1.8 does not work, i think you have an issues with imports, it just breaks the app
  • v0.1.7 the redirect cookie is not saved
  • v0.1.5 the redirect cookie is stored correctly.

the problem is not with split cookie, but with the loop. output into the console key values, i am getting 'set-cookie' key at least twice with different values and only the last one used...

@Hebilicious
Copy link
Owner

Hebilicious commented Jul 20, 2023

@dutsik Thank you so much, I've been able to reproduce the issue locally. It will be fixed in 0.1.9, and I pushed a similar fix to H3.
Since a Header object can have multiple "set-cookie" headers, and they can also have multiple values, we need to split the header value AND to append it to the h3 headers.

{                                                                                                                                            
  key: 'set-cookie',
  value: 'next-auth.callback-url=http%3A%2F%2Flocalhost%3A3000; Path=/; HttpOnly; SameSite=Lax'
}
{                                                                                                                                             
  key: 'set-cookie',
  value: 'next-auth.session-token=eyJhbGciOiJkaXIiLCJlbmMiOiJBMjU2R0NNIn0..nCX-VfrdV14IYume.pvsRZwx4cWnjNPLk4DjrvBi6jmwGyidtQ-9Uu8FFOv8CmkipG9OU8JonZWKmZgNGVgxAqlRnAziM5xtfDK-licWy8A7Ypre7fcGRwDEKh1Qugp7zM7tPt57Oe3iUsKUn6CjSRvMJuueeUa5aosQVtFWEeSG29TZnfBpnsejIMEmHjCWAAcS-mKqw6NbLKmbhPPb4HB8DFJdyFiNnZOQ6KWBmIn_Q51Ztf68AFklYN1jEwboGfIM1jMJzozSaccr19i4cgukjr3cTqA.958IyzXoMOvoUoUn8cxiNQ; Path=/; Expires=Sat, 19 Aug 2023 06:35:56 GMT; HttpOnly; SameSite=Lax'
}

This is now fixed in 0.1.9

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers
Projects
None yet
3 participants