Remove use of eval in non-legacy rollup builds

seanlail commented 3 years ago

Description

As @maximedupre reported in https://github.com/sveltejs/sapper/issues/1622:

This file contains new Function (injected in the browser), which is a form of eval, which causes a CSP error, unless adding unsafe-eval

Adding unsafe-eval to CSP would then defeat the purpose of using CSP.

It was using new Function to do feature detection for imports, but it looks like that is now supported by all non-legacy browsers anyway.

This PR removes the try/catch when building a non-legacy rollup bundle, thereby removing the use of shimport. This will allow users to remove "unsafe-eval" from any CSP configuration.

I realise Sapper is in maintenance but I feel like this is a blocking issue for production usage.

Closes https://github.com/sveltejs/sapper/issues/1622 Relates to https://github.com/sveltejs/sapper/issues/343

Test

I came across this issue originally because I added CSP + blocked unsafe-eval and noticed that reactivity did not work. So I've added a test that does just that. Binds a text input, types in a value and then checks that the updated value is shown. This test now passes, before the change it failed.

Questions

[ ] Should I add the nonce_attr in?
[ ] Should I just use the webpack line? (i.e. </script><script${nonce_attr} src="${main}" defer>

Conduitry commented 3 years ago

I'd be wary about making this without it being a breaking change. Are there, say, any old version of Chromium on old web TVs that support async/await but do not support ESM? I would imagine so, and I know there are people who use Sapper for these environments.

seanlail commented 3 years ago

I'd be wary about making this without it being a breaking change. Are there, say, any old version of Chromium on old web TVs that support async/await but do not support ESM? I would imagine so, and I know there are people who use Sapper for these environments.

@Conduitry Good point, this would be a breaking change. If those people have not targetted legacy then shimport could be saving them at the moment.

Could we rewrite that try/catch then to be CSP safe? 🤔

Kapsonfire-DE commented 3 years ago

'noModule' in HTMLScriptElement.prototype can be used without try catch to detect module functionality

Conduitry commented 3 years ago

Does that safely check for dynamic import support as well? There was a while where Firefox supported import/export by not import(). We still need to bring in Shimport if the browser supports import ... from ... but not import(...).

seanlail commented 3 years ago

Unfortunately I think 'new Function' is the way to test dynamic.

Could we solve this another way?

Default to using shimport and then expose a flag for people to skip/disable it?

It's like the opposite of legacy though.

Conduitry commented 3 years ago

If there's not a reasonable way to test for dynamic imports, then I think at this point in Sapper's life, the answer for how to do this in a CSP-compatible way is to use a fork of Sapper. There's not a good way to expose options like this without there being an overbearing number of CLI options.

I would encourage you to follow along with SvelteKit and, when we get to a point where we begin to think about differential legacy builds there, to advocate for whatever features you think are necessary to keep CSP happy.

Kapsonfire-DE commented 3 years ago

But whats wrong with try catch?

Kapsonfire-DE commented 3 years ago

try { import('').catch(ex => {}); } catch(ex) { console.log('dynamic export not supported'); } I mean this works perfectly without any console output

Conduitry commented 3 years ago

I haven't checked, but won't import('') be a parsing error on some browsers? If it won't even parse, this won't work, and none of the code will run. The eval was to get around stuff not parsing.

Kapsonfire-DE commented 3 years ago

either dynamic import exists, then it returns a promise and its error is catched with .catch(ex), or import doesnt exists and its throws a default error catched by try-catch... if import('') really does a parsing error, then you can use any valid string here import('notExistant.js') still same result

Kapsonfire-DE commented 3 years ago

oh you are right - giving syntax error on IE

seanlail commented 3 years ago

@Kapsonfire-DE @Conduitry Thanks for digging in to this.

I have added a comment to 2 SvelteKit issues and will follow the progress.

I'm going to fork Sapper I think to get around the issue for now, as like you said there won't be many changes to Sapper anyway and hopefully we can move to Kit soon.

Thanks for looking at this anyway 👍

sveltejs / sapper