fix: multiple cookies in qwik-auth

[ksjogo]

Overview

#5194 introduced functionality to set multiple cookies.
With fetch setting multiple cookies via the set-cookie key is not supported though.
The testing was limited to the PCKE cookie, which passed as that cookie is last in the list coming from auth.js.
Other cookies like next-auth.callback-url would come earlier and would have been discarded by the later cookie.

I am not sure, if we should handle pre-existing values for set-cookies?

What is it?

• Feature / enhancement
• Bug
• Docs / tests / types / typos

Description

Instead of setting cookies directly, instead collect them and then create a semicolon seperated string.

Checklist:

• My code follows the developer guidelines of this project
• I have performed a self-review of my own code
• I have made corresponding changes to the documentation
• Added new tests to cover the fix / functionality

[netlify]

👷 Deploy request for qwik-insights pending review.

Visit the deploys page to approve it

Name	Link
🔨 Latest commit	1f0d8ad

[Lemon2311]

Overview:
The pull request intends to consolidate multiple Set-Cookie headers into a single header due to limitations in the fetch implementation. The loop correctly identifies Set-Cookie headers and accumulates them in the cookies array. Non-cookie headers are managed appropriately, avoiding potential duplication. Combining cookies into a single header is implemented as intended. This approach could be necessary given the mentioned limitation but requires careful consideration of how combined cookies are parsed on the client side.
Potential Issues:
Cookie Delimiter: Using a comma to separate multiple cookies may cause issues if cookie values themselves contain commas.
Standards Deviation: The standard way to handle multiple cookies is with multiple Set-Cookie headers. This deviation should be documented, explaining the limitation that led to this design choice.
Improvements:
Additional comments may be beneficial to explain the implications of the chosen workaround for future maintenance.
Addition comments:
Some testing and validation are recommended to ensure compatibility and correctness.

[ulic75]

I think this is an issue with the Fetch API. Perhaps their addition of the getSetCookie() method will resolve your use case?

[ksjogo]

I think this is an issue with the Fetch API. Perhaps their addition of the getSetCookie() method will resolve your use case?

Not sure if it is specifically my use case. It seems to be broken with any sitatuation of having a csrf cookie + anything.

Do you know if the getSetCookie array is writable? The doc doesn't seem to say that.

Similar discussion on next: vercel/next.js#54033

I don't have that much experience in diving into cookie stuff, so happy for some guidance on the best fix here.

Afaict the fixCookie code is already expecting seperated cookies and this PR isn't introducing that.

[ksjogo]

next-auth itself seems to be using this to handle multiple: https://github.com/nextauthjs/next-auth/blob/6c958d7e10edabb2915e101096e1dba5e22fff6b/packages/core/src/lib/utils/web.ts#L94

[ksjogo]

Who is leading qwik-auth at the moment/whom could I have a chat with to get this completed? Would prefer to path to be in mainline so it's easier to handle on our end.

[wmertens]

@ksjogo could you rebase? The tests are hanging and I can't restart them.

[ksjogo]

@ksjogo could you rebase? The tests are hanging and I can't restart them.

Done.

[gioboa]

@ksjogo Can you fix the failing tests pls?

[wmertens]

Thanks!