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

Documentation should clarify that server state is not automatically per-request #1542

Closed
Egnus opened this issue May 24, 2021 · 6 comments · Fixed by #1807
Closed

Documentation should clarify that server state is not automatically per-request #1542

Egnus opened this issue May 24, 2021 · 6 comments · Fixed by #1807
Labels
documentation Improvements or additions to documentation

Comments

@Egnus
Copy link

Egnus commented May 24, 2021

Describe the bug

External variables, stores and other states processed during Load functions will keep in memory references that will remain upon all visitors of the website that is handled by the same server node instance.

This happens with "svelte-kit dev" "svelte-kit preview" and in production, at least tested in Netlify.

I think this issue comes from the assumption than node SSR is running clean on each instance.

To Reproduce
Use this basic demo repo
https://github.com/Egnus/sveltekit-bug-server-XSS-repro

run npm run dev and go to localhost:3000/todos. There is a further red text block explaining what happens there

reload the page several times, and notice how the counter stays with 1 in browser but keeps growing in the Server, which is also accessible by the visitor by viewing source code or disabling Javascript so the hydratation doesn't fix the XSS issue

Use several browsers and incognito mode to find the same values shared for all users. An experienced hacker could get access to others data by accesing those values on memory.

Expected behavior
SSR runs clean in every call. (ideal safe option) forced by the sveltekit boot.
OR
Documentation could clearly states this issues and how to protect from XSS during Load.

Information about your SvelteKit Installation:
This is happening with the latest version of SvelteKit

Severity
Very severe since this is not mentioned in the documentation: https://kit.svelte.dev/docs#loading-input-context
and also there is no clear mention of this memory between calls anywhere.

@Conduitry Conduitry added the documentation Improvements or additions to documentation label May 24, 2021
@Conduitry
Copy link
Member

This is the intended behavior. There's no reasonable way for the server to automatically know what state you want to keep separated by request, nor a reasonable way to automatically keep it separated. This is what the request.locals and the session are for. You generally shouldn't use other shared state on the server for this very reason.

I'm labeling this documentation because this could probably be explained better.

@Conduitry Conduitry changed the title Possible XSS with server scripts and Load method Documentation should clarify that server state is not automatically per-request May 24, 2021
@Egnus
Copy link
Author

Egnus commented May 24, 2021

I totally understand that conclusion.
I have some ideas to solve this from sveltekit core.
But due to how singleton works. A full clean of references might introduce extremely higher latencies, issues depending on the adapter and it might still not cover all the options.

At this point. I am redoing all the portions of the app with load assuming that load only runs once when first load in the server and all the rest are exclusively only on browser.
Still, this was worth to mention in a beta stage

@benmccann
Copy link
Member

In reviewing #1807 that @ignatiusmb so kindly put together, I'm having a really hard time coming up with how I would ideally want this to be documented. It seems to me like this is just how global variables work in JavaScript. I think I'd lean towards just saying that this is too fundamental of a concept to document the same way that we don't document any other JavaScript basics

@Egnus
Copy link
Author

Egnus commented Jul 7, 2021

Well I have to say that Svelte has one of the best documentations I have ever seen (together with three.js) having at least those warnings I think is the bare minimun for the mainly focused frontend developers who are facing for the first time a Fullstack experience with more control over SSR rules.
Many people, me included, until not a long ago, thought that each call to a server was clean as there were no reason to believe that scoped variables would be accesible by different users and different times.
And somehow I feel like this is good to mention so the people organice their code earlier knowing this surprising behaviour for pure FE devs.

@benmccann
Copy link
Member

there were no reason to believe that scoped variables would be accesible by different users and different times

To clarify the issue here, scoped variables aren't accessible by different users. The issue is that there's an unscoped global variable:

Egnus/sveltekit-bug-server-XSS-repro@05fe07d#diff-276a0044b7db537e1835eb8b2c20368b8a7437a3fde350198bff4db2b9e418fe

@Egnus
Copy link
Author

Egnus commented Jul 8, 2021

Or a store, or a singleton which are the ones I got problems with, but yeah. Not simple scoped variables

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants