@vitejs/plugin-legacy CSP for blankDynamicImport does not work

[tjk]

Describe the bug

This code is rendered as part of a chunk rather than a standalone script tag so the sha CSP does not work for it.

Reproduction

Can supply if not obvious.

System Info

System:
    OS: Linux 5.10 Ubuntu 20.04.2 LTS (Focal Fossa)
    CPU: (16) x64 11th Gen Intel(R) Core(TM) i9-11900KF @ 3.50GHz
    Memory: 6.01 GB / 12.38 GB
    Container: Yes
    Shell: 5.0.17 - /bin/bash
  Binaries:
    Node: 14.17.0 - ~/.asdf/installs/nodejs/14.17.0/bin/node
    Yarn: 1.22.5 - ~/.asdf/shims/yarn
    npm: 6.14.13 - ~/.asdf/installs/nodejs/14.17.0/bin/npm
  npmPackages:
    @vitejs/plugin-vue: ^1.4.0 => 1.4.0
    vite: ^2.5.0-beta.2 => 2.5.0-beta.2

Used Package Manager

yarn

Logs

No response

Validations

• Follow our Code of Conduct
• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Make sure this is a Vite issue and not a framework-specific issue. For example, if it's a Vue SFC related bug, it should likely be reported to https://github.com/vuejs/vue-next instead.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Discord Chat Server.
• The provided reproduction is a minimal reproducible example of the bug.

[tjk]

Bump! For anyone with a strict CSP policy, it is not possible to upgrade @vitejs/plugin-legacy without fixing this issue.

/cc @nulladdict since it broke in this commit: fc6d8f1

[nulladdict]

Hi, I'm not an expert on CSP usage, so this completely escaped me.
I've reproduced the issue, but I'm not quite sure how to fix it.

First of all, when CSP blocks the dynamic import from loading, the main bundle still works, since import is there to only force syntax errors in certain browsers. We do still get the CSP error and network error logged into the console, which is not ideal though.

I've tried providing the SHA hash for the import, but I couldn't make it work. I tried hashing different combinations of '', '\n' and base64 encoded code, but wasn't successful. I also tried changing import to just import('data:text/javascript,'), which works as a banner, but still doesn't pass the CSP check.

Adding data: to the CSP kind of defeats the purpose, so it's not the solution either.

I did manage to make it work by using 'strict-dynamic' and providing the nonce for the index script, but it might not be usable for all cases.

Overall I see 2 ways of dealing with this:

• Removing the blankDynamicImport banner and hoping that the main bundle errors out on its own
• Changing banner code to something like "import('data:text/javascript,').catch(function(){});" to silence the network error and ignore the CSP violation

[tjk]

Would it be possible to move the blankDynamicImport check to a separate script tag like the other checks so that it could be nonced? I'm not too familiar with how import interacts with CSP... I think there is some discussion about it? (w3c/webappsec-csp#506) -- but I think that all might be irrelevant if in a nonced <script>? That said I have an error from a dynamic import in dev from inside code that should be allowed by CSP... so a bit confused and haven't fully tracked it down yet.

[nulladdict]

The import is there to prevent double executing of the bundle in a specific case where the bundled app doesn't use dynamic import and the browser doesn't support them but supports module tags.

For this case blankDynamicImport is injected into the modern bundle to force it to have a dynamic import inside, therefore moving this code to a separate script would be the same as just removing it entirely, since the syntax error would not propagate to the modern bundle.

I guess there's no need to perform an import, and blankDynamicImport can be changed to something like new Function("m","return import(m)");. This would still produce an error if there's no dynamic import support, although in this case, the error would happen during execution and not during the parsing. I think it's fine since the described case is rather specific and parsing overhead can be neglected.

[patak-dev]

• Changing banner code to something like "import('data:text/javascript,').catch(function(){});" to silence the network error and ignore the CSP violation

Is this approach valid? If this still raises the syntax error (so parsing is stopped) and avoids the CSP issue, this seems goods to me

[nulladdict]

Is this approach valid? If this still raises the syntax error (so parsing is stopped) and avoids the CSP issue, this seems goods to me

It only silences the network error after the import is blocked by the CSP, so it doesn't really avoid the issue, only reduces the amount of noise in the console

[patak-dev]

And what is the issue with the import being blocked if we only need it for the syntax error, is this error surfaced in tools like web vitals or security audits?
If this is a problem, I think using new Function("m","return import(m)"); would be the next option (not ideal, but as this only applies to a very small percentage of users I think it is fine)

[patak-dev]

@nulladdict would wrapping the blank import in a function still trigger the syntax error for these browsers?

function(){import('data:text/javascript,')}

I wonder if we could trigger the syntax error without the actual call to import here. If this doesn't work, a less ideal option may be needed (so runtime error instead of syntax error). @tjk if you have a better idea, feel free to draft a PR also to fix this.

[tjk]

I'm not too sure unfortunately... @nulladdict could the the script tag be injected with an onerror handler so bundle script can get callback while still whitelisting the injected script?

[nulladdict]

@patak-js that's a good idea. I tried it inside Edge 18, and it failed with Expected identifier
The message is a little confusing but it's still a parse-time syntax error, so I think it should work

Nevermind, the message was due to function statement requiring the function name. If I change it to expression like this void function(){import('data:text/javascript,')} Edge gives proper SyntaxError

I've made a draft PR with changes (#4767)

[tjk]

Oh I hadn't noticed @patak-js's suggestion... great idea! Nice work both of you, thanks.