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

docs: Elaborate on credentialed fetch behaviour #10421

Merged
merged 7 commits into from Jul 29, 2023
Merged

docs: Elaborate on credentialed fetch behaviour #10421

merged 7 commits into from Jul 29, 2023

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)

.changeset/funny-fishes-try.md Outdated Show resolved Hide resolved
documentation/docs/20-core-concepts/20-load.md Outdated Show resolved Hide resolved
packages/kit/src/runtime/server/fetch.js Outdated Show resolved Hide resolved
packages/kit/src/runtime/server/fetch.js Outdated Show resolved Hide resolved
@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.

@dummdidumm dummdidumm merged commit 6f36aef into sveltejs:master Jul 29, 2023
12 of 13 checks passed
@github-actions github-actions bot mentioned this pull request Jul 28, 2023
@lachlancollins lachlancollins deleted the docs-fetch-cookies branch July 29, 2023 21:31
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.

None yet

3 participants