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

feat: implement prototype of safe server stores #12215

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AlbertMarashi
Copy link

@AlbertMarashi AlbertMarashi commented May 13, 2024

Context

The unsafe behavior of global variables during SSR (particularly global stores) has been one of the most persistently complained about issues with SvelteKit (#4339).

Screenshot 2024-05-13 at 2 58 04 pm

The way stores are currently implemented result in state being leaked across requests which presents a very high security risk especially for users that are unaware of this difference of behavior in client-side vs server-side apps.

Solution

Using AsyncLocalStorage allows us to isolate the stores between server-side requests in Svelte.

Docs: https://nodejs.org/api/async_context.html

Clarification: This is only a server-side feature, and is not necessary on the browser side of things as the client is naturally isolated from other clients - web support for AsyncLocalStorage is not required

Benefits

  • Ability to use declare and use globally accessible stores in normal JS/TS files, including within hooks
  • No more Cannot access 'x' before initialization
  • Request-level isolation
  • No security worries with data leakage between requests

My thoughts

  • I have temporarily implemented a custom unshared_writable function inside of the Kit repository, but this behavior should be implemented in the Svelte repository itself, with a way for the server to replace the getStore function with another function which uses AsyncLocalStorage.getStore() from the server index.js code
  • Currently, the initialValue is reused in a shallow way (so only primitive types are isolated). we could implement behavior to do a deep clone with structuredClone, alternatively, the syntax of writable could be writable(() => initial_value) such that it is a function that returns a fresh object, but this would change the syntax and be a breaking change with the existing store functions in svelte/store
  • Currently, the global stores aren't transferred to the client-side, so we'll need a way to serialize and serialize the existing state onto the client-side which shouldn't be too difficult to implement

Support

This appears to be supported by all major server runtimes:

I believe they all use the same import module

Copy link

changeset-bot bot commented May 13, 2024

⚠️ No Changeset found

Latest commit: e0f7736

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

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

@AlbertMarashi
Copy link
Author

Posting this early example as a prototype explaining how it works, but the code needs to be updated in the Svelte code so that we can implement this behavior in svelte/store (no need to create new stores)

@Conduitry
Copy link
Member

A web platform equivalent of AsyncLocalStorage is hopefully coming eventually. I'd much rather use that once it exists (and is reasonably well supported) than to introduce a reliance on Node.js-specific things. Not all of the official adapters are for environments where Node.js APIs are available.

@saturnonearth
Copy link

A web platform equivalent of AsyncLocalStorage is hopefully coming eventually. I'd much rather use that once it exists (and is reasonably well supported) than to introduce a reliance on Node.js-specific things. Not all of the official adapters are for environments where Node.js APIs are available.

Agreed with this. As much as I would love to an easy to solution to this so I can stop using Context lol

@AlbertMarashi
Copy link
Author

AlbertMarashi commented May 13, 2024

A web platform equivalent of AsyncLocalStorage is hopefully coming eventually. I'd much rather use that once it exists (and is reasonably well supported) than to introduce a reliance on Node.js-specific things. Not all of the official adapters are for environments where Node.js APIs are available.

We don't need AsyncLocalStorage on the browser, this is only a server-side problem so its not necessary for browser support in order to finally resolve this issue @Conduitry. To clarify, this feature is only used during SSR so it doesn't create any issues with client-side.

I think there's no logical reason to not add this feature as it's practically the most complained about feature in SvelteKit

@AlbertMarashi
Copy link
Author

AlbertMarashi commented May 13, 2024

A web platform equivalent of AsyncLocalStorage is hopefully coming eventually. I'd much rather use that once it exists (and is reasonably well supported) than to introduce a reliance on Node.js-specific things. Not all of the official adapters are for environments where Node.js APIs are available.

Agreed with this. As much as I would love to an easy to solution to this so I can stop using Context lol

You don't need to use getContext and setContext anymore after this feature, except if you specifically want state propagated downwards to child components. This feature is specifically for when you want to have some global state that is also accessible/declared within your typescript/javascript files.

This feature brings the svelte docs more inline with how svelte works as a SPA, and will hopefully reduce confusion, and improve the learning curve with svelte while simultaneously reducing the divide between svelte behaviour and svelte/kit behaviour

@saturnonearth
Copy link

A web platform equivalent of AsyncLocalStorage is hopefully coming eventually. I'd much rather use that once it exists (and is reasonably well supported) than to introduce a reliance on Node.js-specific things. Not all of the official adapters are for environments where Node.js APIs are available.

Agreed with this. As much as I would love to an easy to solution to this so I can stop using Context lol

You don't need to use getContext and setContext anymore after this feature, unless you specifically want state propagated downwards to child components. This feature is specifically for when you want to have some global state that is also accessible/declared within your typescript/javascript files

Ah yea I meant that the solution needs to be platform agnostic

@AlbertMarashi
Copy link
Author

Ah yea I meant that the solution needs to be platform agnostic

Yes, it is imo, is there any platforms that don't have support for AsyncLocalStorage?

@eltigerchino eltigerchino changed the title Implement prototype of safe server stores feat: implement prototype of safe server stores May 16, 2024
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