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

Some set-cookie headers not being returned #171

Open
geoffrich opened this issue Jan 1, 2024 · 8 comments
Open

Some set-cookie headers not being returned #171

geoffrich opened this issue Jan 1, 2024 · 8 comments
Labels
bug Something isn't working

Comments

@geoffrich
Copy link
Owner

Discovered while working on #170

I fixed an issue with cookies in the SK v2 branch by setting path: '/' instead of the default path: 72e5b3a

The SK v1 version of the demo returned path: '/' by default, so we didn't run into this issue. Example of set-cookie header with v1:

sverdle=1631-tooth%20jeeps%20hello%20tests%20%20-___x_%20____c%20_____%20__xx_; domain=polite-desert-00b80111e.2.azurestaticapps.net; path=/; secure; samesite=lax; httponly

The new demo uses path: '', which maps to path=/sverdle. For some reason, even though we return this cookie in the response from the SSR Azure function, the set-cookie header is not present in the response that makes it to the browser. Example of function log:

Response: {"status":200,"body":{ /* truncated */},"headers":{"content-length":"43","content-type":"application/json"},"cookies":[{"name":"sverdle","value":"2260-beets     -_c___","path":"/sverdle","httpOnly":true,"secure":true,"sameSite":"Lax"}],"isRaw":true}

This doesn't happen on the deployed Vercel demo, so it must be Azure-specific.

Need to create a minimal repro, ideally without SK (or even SWA).

@geoffrich geoffrich added the bug Something isn't working label Jan 1, 2024
@geoffrich
Copy link
Owner Author

Theorizing: maybe the rewrite has something to do with it, and Azure preemptively filters out cookies that don't match the current path (which was re-written to /__sk_render).

@idan
Copy link
Contributor

idan commented Jul 9, 2024

I suspect this is related: Azure/static-web-apps#760

@idan
Copy link
Contributor

idan commented Aug 5, 2024

@geoffrich I made a minimal repro here: https://github.com/githubnext/azureswacookies and I'm in discussions with Azure SWA support folks about it, will keep you posted

@idan
Copy link
Contributor

idan commented Aug 27, 2024

@geoffrich so @IvanJobs (from Azure) dug into it, and @lukeed and I both did some investigation as well.

tl;dr we still don't know what's causing this, Azure thinks it's not Azure

Per Ivan, it looks like cookies set in regular +page.server.ts files are being returned just fine. I verified that this is the case in my repro repo.

Image

@lukeed and I ran down the logic in SK, thinking that either the path or the domain attributes didn't match and thus the cookie wasn't even being returned at serialization time (see cookie.js:61-62 or 100-101). I added some headers to check what the server thinks domain and path ought to be, since one of the suspects here is path rewriting in the adapter:

Image

But none of the values here look wrong. And if either path or domain did not match, then I'd expect SK to refuse to enumerate them later. But we're clearly able to enumerate the cookie that isn't getting sent with cookies.getAll() here on L14.

In short, our clues:

  • Cookies do work in individual backend routes, just not in hooks
  • It doesn't seem like some mismatch of domain or path attributes on the cookie that would cause SK to filter the cookie out of the response.
  • I don't know how to tap into the req/res cycle at a later time to try and figure out where the cookie is getting dropped.

@geoffrich hopefully this helps narrow it down, I'm pretty stumped.

@idan
Copy link
Contributor

idan commented Aug 28, 2024

@geoffrich ok we found our specific problem! Azure refuses to honor cookies with super long max-age. In the example, the max-age was set to be 1000 years (31536000000 seconds). When I cut the amount down to one year, Azure sends along the set-cookie header just fine.

I'm talking with the Azure folks about documenting this and maybe changing their behavior here.

@geoffrich
Copy link
Owner Author

Ah, good find! Though when I originally ran into this issue it doesn't appear like I was setting a max-age at all. It's been a bit; I'll try to find some time to see if my original demo issue still repros.

@ranaldsgift
Copy link

ranaldsgift commented Oct 10, 2024

In short, our clues:

  • Cookies do work in individual backend routes, just not in hooks
  • It doesn't seem like some mismatch of domain or path attributes on the cookie that would cause SK to filter the cookie out of the response.
  • I don't know how to tap into the req/res cycle at a later time to try and figure out where the cookie is getting dropped.

This is interesting. I seem to be having problems related to this issue when attempting to authenticate users via supabase auth.

Locally this is working fine, even with the Azure SWA CLI. In my hooks.server.ts I have:

event.locals.supabase = createServerClient(PUBLIC_SUPABASE_URL, PUBLIC_SUPABASE_ANON_KEY, { cookies: { getAll: () => event.cookies.getAll(), setAll: (cookiesToSet) => { cookiesToSet.forEach(({ name, value, options }) => { event.cookies.set(name, value, { ...options, path: "/" }); }); }, }, });

But when I inspect the network traffic, the supabase 'sb' cookies are never set in the Request Headers.

Curious if this is related or not.

@idan
Copy link
Contributor

idan commented Oct 18, 2024

@ranaldsgift this is exactly what we experienced (we also use supabase). The sb-WHATEVER cookies weren't being passed along by SWA because their max-age was too long.

Update your dependencies; Supabase shipped a change that lowered the max-age on their cookies to one year and that sorted us out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants