geoffrich / svelte-adapter-azure-swa

SvelteKit adapter for Azure Static Web Apps.
MIT License
140 stars 31 forks source link

Some set-cookie headers not being returned #171

Open geoffrich opened 10 months ago

geoffrich commented 10 months ago

Discovered while working on #170

I fixed an issue with cookies in the SK v2 branch by setting path: '/' instead of the default path: https://github.com/geoffrich/svelte-adapter-azure-swa/pull/170/commits/72e5b3a64da70761e8857fc9a6adb572ed05cf40

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 commented 10 months ago

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 commented 3 months ago

I suspect this is related: https://github.com/Azure/static-web-apps/issues/760

idan commented 2 months ago

@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 commented 2 months ago

@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:

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

idan commented 2 months ago

@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 commented 2 months ago

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 commented 3 weeks ago

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 commented 2 weeks ago

@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.