prevent loading of illegal modules in the browser #7507

Rich-Harris · 2022-11-04T20:50:20Z

Closes #6700. Closes #7288. Right now, we prevent people from importing illegal modules ($env/[type]/private, $lib/server/** and **/*.server.{ext}) by analysing the module graph during SSR. This is a flawed approach for a variety of reasons:

It doesn't account for systems like telefunc, which transform modules differently between SSR and non-SSR modes such that the dangerous import declarations are removed
it means you only get the error when you hit a page directly, not when you soft-navigate to a page. (I was surprised to realise that this morning!)
it applies even to pages that have csr = false

A much simpler approach is to simply prevent the modules in question from being imported in the browser at dev time. We can do this by adding a guard in a load hook that throws if the relevant conditions are met. (At build time we keep the existing graph analysis.)

There's a couple of minor drawbacks: we don't get the import chain, just the module ID, and the error overlay is dismissable. I think these trade-offs are well worth it.

A nice bonus is that we can get rid of the code for traversing the Vite module graph. Since we now only need to worry about the Rollup module graph, this also means we can get rid of a layer of indirection (sorry @tcc-sejohnson, it was good code! but no longer needed). That part is a WIP.

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
This message body should clearly illustrate what problems it solves.
Ideally, include a test that fails without this PR but passes with it.

Tests

Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

changeset-bot · 2022-11-04T20:50:23Z

🦋 Changeset detected

Latest commit: d79cc59

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

elliott-with-the-longest-name-on-github · 2022-11-05T03:57:35Z

As long as we're retaining the same graph analysis in build as we were before I see no problem with this -- I wanted to do this when we originally implemented the feature, but just couldn't figure out how. Please ping me when this is ready-er for merge -- happy to review 😁

Rich-Harris · 2022-11-07T22:56:23Z

Ok I think this is ready now, marking it as such and crossing my fingers that the tests pass. One wrinkle: if the Vite error overlay appears before hydration, it will get nuked if %sveltekit.body% isn't wrapped in an element below the <body>, so I had to make that change to the test app.

I almost wonder if we should enforce that, since it's a bit of a footgun otherwise.

@tcc-sejohnson would be grateful if you do get a chance to review! The build stuff should be basically identical

Rich-Harris · 2022-11-09T18:39:44Z

alright, things finally seem to be working again. @tcc-sejohnson if you have the bandwidth to take another look since you requested changes, that would be great — looking forward to getting this one merged 🙏

lukaszpolowczyk · 2022-11-09T21:30:01Z

Anecdotally, I've seen a good number of people in the Discord that remove the div wrapper for "easier" styling, so... might be a good idea to try to find a way.

@tcc-sejohnson I in one project added to this div a style="display: contents;", and it was enough for me.

I don't know if that can cause a problem, but I haven't noticed any.

elliott-with-the-longest-name-on-github · 2022-11-10T07:58:52Z

packages/kit/src/exports/vite/graph_analysis/index.js

+	function follow(id, chain) {
+		if (seen.has(id)) return;
+		seen.add(id);


nothing is wrong here; I just wanted you to see what the heck GitHub thinks these lines correlate to in the original diff

elliott-with-the-longest-name-on-github

LGTM! This should be a great improvement for the dev-time experience. Thank you!

elliott-with-the-longest-name-on-github · 2022-11-10T08:03:39Z

Anecdotally, I've seen a good number of people in the Discord that remove the div wrapper for "easier" styling, so... might be a good idea to try to find a way.

@tcc-sejohnson I in one project added to this div a style="display: contents;", and it was enough for me.

I don't know if that can cause a problem, but I haven't noticed any.

That won't work for everyone. To be clear, I don't think there's really any logical reason to remove the wrapper div -- it's easy enough to set it up however you want to -- but some people do it and we don't want to break their apps if possible 😁

stephane-vanraes · 2022-11-10T08:12:57Z

That won't work for everyone. To be clear, I don't think there's really any logical reason to remove the wrapper div -- it's easy enough to set it up however you want to -- but some people do it and we don't want to break their apps if possible 😁

I am one of those removing the wrapper div, so maybe shedding some insights on that 🙂

The simple reason is that this div servers no purpose whatsoever in the final product*, and it makes styling harder to a degree (you always have to remember that the body of your page is not body for the css).

As far as I understand this div is a crutch for shortcoming of a tool used in development only and imho that is enough reason to try to find a way to make things work without, because utilities meant for development have no place in a production build.

This div might also clash in some instances where people used external stylesheet or css frameworks (for example an enterprise standard sheet)

*: besides potentially stopping hydration from removing extensions there injected markup, but this should not required adding extra elements to the markup.

Rich-Harris · 2022-11-10T14:39:31Z

besides potentially stopping hydration from removing extensions there injected markup

this is the key — we've had multiple reports of exactly this sort of thing happening. it's very much not about preventing dev-time issues — it's the sort of bug that's likely to manifest in deeply confusing ways in production and that you'll be lucky to even find out about. we added a note to the docs...

...but i'm not sure that's enough, because people will always look at it and assume they can safely remove it. my personal inclination is to require %sveltekit.body% to be inside an element other than <body>, but i'm going to open a discussion to see how large a trade-off that would be

Rich-Harris · 2022-11-10T14:49:54Z

#7585

prevent loading of illegal modules in the browser

5eab07a

update test

204d862

vercel bot had a problem deploying to Preview – kit-svelte-dev November 4, 2022 20:59 Failure

vercel bot deployed to Preview – kit November 4, 2022 20:59 View deployment

Rich-Harris added 2 commits November 4, 2022 17:00

rename function

5261021

fix

b0c2282

vercel bot deployed to Preview – kit November 4, 2022 21:09 View deployment

vercel bot deployed to Preview – kit-svelte-dev November 4, 2022 21:11 View deployment

remove dev test

b6d8a2a

vercel bot deployed to Preview – kit November 4, 2022 21:16 View deployment

remove unused consts

1e3256f

vercel bot deployed to Preview – kit-svelte-dev November 4, 2022 21:18 View deployment

simpler checking code

9132a66

vercel bot deployed to Preview – kit November 4, 2022 22:04 View deployment

vercel bot deployed to Preview – kit-svelte-dev November 4, 2022 22:06 View deployment

move tests (still failing)

b847be2

vercel bot deployed to Preview – kit November 7, 2022 14:27 View deployment

vercel bot deployed to Preview – kit-svelte-dev November 7, 2022 14:29 View deployment

merge

057a688

vercel bot deployed to Preview – kit November 7, 2022 15:26 View deployment

vercel bot deployed to Preview – kit-svelte-dev November 7, 2022 15:27 View deployment

Rich-Harris added 3 commits November 7, 2022 17:44

fix tests

dde14a1

remove only

9737b04

fix

bca015d

vercel bot deployed to Preview – kit November 7, 2022 22:54 View deployment

Rich-Harris marked this pull request as ready for review November 7, 2022 22:56

vercel bot deployed to Preview – kit-svelte-dev November 7, 2022 22:56 View deployment

vercel bot deployed to Preview – kit November 9, 2022 11:02 View deployment

vercel bot deployed to Preview – kit-svelte-dev November 9, 2022 11:04 View deployment

Rich-Harris added 2 commits November 9, 2022 10:55

use consistent terminology between dev and build

6223832

ugh. print error messages so we can see why windows is being a dick

b7f65cc

vercel bot deployed to Preview – kit November 9, 2022 16:14 View deployment

vercel bot deployed to Preview – kit-svelte-dev November 9, 2022 16:16 View deployment

fml. can everyone just stop using fucking windows

c537d21

vercel bot deployed to Preview – kit November 9, 2022 16:39 View deployment

vercel bot deployed to Preview – kit-svelte-dev November 9, 2022 16:42 View deployment

try this

269c456

vercel bot deployed to Preview – kit November 9, 2022 17:02 View deployment

vercel bot deployed to Preview – kit-svelte-dev November 9, 2022 17:04 View deployment

ok turn everything back on

2cea023

vercel bot deployed to Preview – kit November 9, 2022 17:07 View deployment

vercel bot deployed to Preview – kit-svelte-dev November 9, 2022 17:10 View deployment

elliott-with-the-longest-name-on-github reviewed Nov 10, 2022

View reviewed changes

elliott-with-the-longest-name-on-github approved these changes Nov 10, 2022

View reviewed changes

Create soft-gorillas-hear.md

d79cc59

vercel bot deployed to Preview – kit November 10, 2022 14:52 View deployment

Rich-Harris merged commit 6ba57ab into master Nov 10, 2022

Rich-Harris deleted the client-side-illegal-module-guard branch November 10, 2022 14:52

vercel bot deployed to Preview – kit-svelte-dev November 10, 2022 14:54 View deployment

github-actions bot mentioned this pull request Nov 10, 2022

Version Packages (next) #7552

Merged

benmccann mentioned this pull request Jan 20, 2023

chore: reduce windows coverage #8632

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent loading of illegal modules in the browser #7507

prevent loading of illegal modules in the browser #7507

Rich-Harris commented Nov 4, 2022 •

edited by benmccann

Loading

changeset-bot bot commented Nov 4, 2022 •

edited

Loading

elliott-with-the-longest-name-on-github commented Nov 5, 2022

Rich-Harris commented Nov 7, 2022

Rich-Harris commented Nov 9, 2022

lukaszpolowczyk commented Nov 9, 2022

elliott-with-the-longest-name-on-github Nov 10, 2022

Rich-Harris Nov 10, 2022 •

edited

Loading

elliott-with-the-longest-name-on-github left a comment

elliott-with-the-longest-name-on-github commented Nov 10, 2022

stephane-vanraes commented Nov 10, 2022 •

edited

Loading

Rich-Harris commented Nov 10, 2022

Rich-Harris commented Nov 10, 2022

prevent loading of illegal modules in the browser #7507

prevent loading of illegal modules in the browser #7507

Conversation

Rich-Harris commented Nov 4, 2022 • edited by benmccann Loading

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

Tests

Changesets

changeset-bot bot commented Nov 4, 2022 • edited Loading

🦋 Changeset detected

elliott-with-the-longest-name-on-github commented Nov 5, 2022

Rich-Harris commented Nov 7, 2022

Rich-Harris commented Nov 9, 2022

lukaszpolowczyk commented Nov 9, 2022

elliott-with-the-longest-name-on-github Nov 10, 2022

Choose a reason for hiding this comment

Rich-Harris Nov 10, 2022 • edited Loading

Choose a reason for hiding this comment

elliott-with-the-longest-name-on-github left a comment

Choose a reason for hiding this comment

elliott-with-the-longest-name-on-github commented Nov 10, 2022

stephane-vanraes commented Nov 10, 2022 • edited Loading

Rich-Harris commented Nov 10, 2022

Rich-Harris commented Nov 10, 2022

Rich-Harris commented Nov 4, 2022 •

edited by benmccann

Loading

changeset-bot bot commented Nov 4, 2022 •

edited

Loading

Rich-Harris Nov 10, 2022 •

edited

Loading

stephane-vanraes commented Nov 10, 2022 •

edited

Loading