Skip to content

Conversation

@lachlancollins
Copy link
Contributor

@lachlancollins lachlancollins commented Jul 22, 2023

Changes:

  • Document why credentials: 'include' behaves identically to credentials: 'same-origin'
  • Synced fetch documentation between site and JSDoc
  • Splits Cookies and headers section into Cookies and Headers
  • Copy subdomains example from fetch.js

@changeset-bot
Copy link

changeset-bot bot commented Jul 22, 2023

🦋 Changeset detected

Latest commit: 46faacf

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@benmccann benmccann changed the title docs: Elaborate on fetch credentialled request behaviour docs: elaborate on credentialed fetch behaviour Jul 24, 2023
@benmccann benmccann requested a review from Conduitry July 24, 2023 21:30
@lachlancollins lachlancollins changed the title docs: elaborate on credentialed fetch behaviour fix: Handle cookies correctly when credentials: 'include' is set, and elaborate on credentialed fetch behaviour Jul 26, 2023
@lachlancollins lachlancollins changed the title fix: Handle cookies correctly when credentials: 'include' is set, and elaborate on credentialed fetch behaviour fix: Correctly handle cookies when credentials: 'include' is set, and elaborate on credentialed fetch behaviour Jul 26, 2023
@dummdidumm
Copy link
Member

After speaking to other maintainers, I conclude that this fix won't actually solve your problem, and it would open up security vulnerabilities.

Won't solve the problem: Suppose we have sveltekit at site.com and an API at api.com

  • during client-side render of site.com/, fetch api.com/blah with credentials: include, the browser sends the cookie that belongs to api.com
  • but for SSR, the client is only accessing site.com/, so the browser is only sending site.com cookies. On the server-side, you never got api.com cookies from the browser when attempting to do the SSR fetch to api.com
    --> you can't forward something you don't get in the first place

Security vulnerabilities: If credentials: include was sent, we can't just forward cookies, because we don't know which ones have sameSite: strict and which one have not - it's only safe to send those who do not have sameSite: strict. The browser only gives us the names and values, but no info about sameSite.

To work around your specific problem you a) need to proxy your API request through your own domain and b) use handleFetch to forward the cookie manually.

My suggestion is therefore:

  • revert the includes code change but add a code comment at that position about why we don't handle the includes case
  • adjust the docs to link to handleFetch from the load docs in the cookies section

Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(as pointed out in the comment above)

@lachlancollins
Copy link
Contributor Author

  • during client-side render of site.com/, fetch api.com/blah with credentials: include, the browser sends the cookie that belongs to api.com
  • but for SSR, the client is only accessing site.com/, so the browser is only sending site.com cookies. On the server-side, you never got api.com cookies from the browser when attempting to do the SSR fetch to api.com
    --> you can't forward something you don't get in the first place

@dummdidumm oh yeah I get that, good point. For my use case, I just want my.domain.com to be able to fetch api.domain.com, rather than needing api.my.domain.com. Since the domain is the same, I can set a cookie that works for both. However, I see how behaviour would get very complex if the domain was different.

Would it be possible to add a limited version of credentials: 'include' which only works on the same domain? I believe this would effectively treat all cookies as sameSite: strict, although I'm not entirely sure if sameSite: strict is origin-based or domain-based. If not, I'll revert the last changes and go with your comments :)

@Conduitry
Copy link
Member

If we're rendering an SSR request on foo.example.com and that makes a credentials: 'include' request to bar.example.com, the server doesn't know whether it's safe to forward its cookies, because it doesn't know whether any of them were set on example.com (in which case a browser would send the cookie) or on foo.example.com (in which case it would not). For security, we need to err on the side of not sending the cookies.

This is what's discussed in the 'credentials' subsection in https://kit.svelte.dev/docs/hooks#server-hooks-handlefetch - Is this what you're asking about?

@lachlancollins
Copy link
Contributor Author

This is what's discussed in the 'credentials' subsection in kit.svelte.dev/docs/hooks#server-hooks-handlefetch - Is this what you're asking about?

@Conduitry oh it's literally right there, I should be able to make that work. I'll fix up this PR now - thanks for your help!

@lachlancollins lachlancollins changed the title fix: Correctly handle cookies when credentials: 'include' is set, and elaborate on credentialed fetch behaviour docs: Elaborate on credentialed fetch behaviour Jul 28, 2023
Copy link
Member

@dummdidumm dummdidumm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@dummdidumm
Copy link
Member

Not sure what the preview deployment failure is about, but we probably have to address it before merging or it won't rebuilt the site in production

@Conduitry
Copy link
Member

I imagine it was what I fixed in #10431. I've merged master back into this branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants