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

extension: Change unsafe-eval to wasm-eval in CSP #6026

Merged
merged 1 commit into from
Jan 12, 2022

Conversation

Herschel
Copy link
Member

@Herschel Herschel commented Jan 11, 2022

In Chrome, unsafe-eval was needed in the extension Content Security Policy for Wasm compilation. Other browsers seemed to work regardless of this setting.

Unfortunately, this CSP setting causes the extension to get flagged in the Mozilla Add-On Marketplace, which discourages the use of unsafe-eval. Chrome specifically has a wasm-eval for allowing Wasm in extensions, so let's switch to that instead.

The CSP spec has added a wasm-unsafe-eval setting, and Chrome will deprecate wasm-eval eventually; we may need this for all browsers as they start to support it.
w3c/webappsec-csp#293

@adrian17
Copy link
Collaborator

adrian17 commented Jan 11, 2022

What's the status in Firefox (edit: and Safari)? On MDN, it's not mentioned at all.

@adrian17
Copy link
Collaborator

adrian17 commented Jan 11, 2022

Also, does this affect #2201 in any way? (I guess not, since it's more about code injection?)

@Herschel
Copy link
Member Author

Herschel commented Jan 11, 2022

Confusingly this was renamed to wasm-unsafe-eval (w3c/webappsec-csp#293), but only wasm-eval seems to work in Chrome extensions currently? If that's the case, maybe we should add both wasm-eval and wasm-unsafe-eval in anticipation, with the idea of removing wasm-eval when it's no longer necessary?

Firefox allowed the WebAssembly instantiation regardless of unsafe-eval. It doesn't implement wasm-unsafe-eval yet so there should be no change, and would hopefully "just work" when Firefox actually implements it. (https://bugzilla.mozilla.org/show_bug.cgi?id=1740263).

Not sure about Safari and #2201 -- I expect there to be no changes here. Can someone test?

@Herschel
Copy link
Member Author

Herschel commented Jan 11, 2022

Did some testing. Here's the behavior of the Ruffle extension, which is the same both before this PR ('unsafe-eval'), and after this PR ('wasm-unsafe-eval' 'wasm-eval'):

SWF URL SWF Embed SWF Embed w/ CSP Error (#2201)
Chrome ✔️ ✔️ ❌ (Panics with CSP Error1)
Firefox ✔️ ✔️ ✔️ (Opens in new tab)
Safari ❌ (downloads SWF) ✔️ ❌ (failed to load .wasm component)

I'm still not totally sure why the SWF URL case in Chrome works with wasm-eval but not wasm-unsafe-eval, so I added both values. wasm-unsafe-eval is what's in the standard, and Chrome will supposedly deprecate wasm-eval at some point. We can go ahead preemptively add these, and they'll be gracefully ignored by other browsers for now.

Footnotes

  1. I think this should work with the "Open in new tab" logic that Firefox uses, but Chrome hits a different error than FF and ends up panicking.

@Herschel Herschel changed the title extension: Change unsafe-eval CSP to wasm-eval extension: Change unsafe-eval CSP to wasm-unsafe-eval Jan 11, 2022
@relrelb
Copy link
Contributor

relrelb commented Jan 11, 2022

content_security_policy can be set dynamically in webpack.config.json, as currently done for browser_specific_settings and version_name. However, this would be applied for both Chrome and Safari (the "generic" variant), and I don't see a good cause for this.
Also, I remember that Firefox complains about manifest fields it doesn't know (#5053), so minimum_chrome_version probably causes a warning (didn't verify). It can also be set dynamically in webpack.config.js, but again would be applied for both Chrome and Safari...

@danielhjacobs
Copy link
Contributor

danielhjacobs commented Jan 11, 2022

Confusingly this was renamed to wasm-unsafe-eval (w3c/webappsec-csp#293), but only wasm-eval seems to work in Chrome extensions currently? If that's the case, maybe we should add both wasm-eval and wasm-unsafe-eval in anticipation, with the idea of removing wasm-eval when it's no longer necessary?

That does seem to match the commit message. Presumably it was necessary for backwards compatibly I'm guessing (so preexisting extensions using wasm-eval would still work): https://chromium.googlesource.com/chromium/src/+/791c8335079bbe7abe7a1b7b6251317ec62b2563

Edit: See also https://groups.google.com/a/chromium.org/g/blink-dev/c/5U_SgZ3r8QI/m/oRyGAsZOBgAJ:

currently, extensions (and only extensions and chrome apps), can use the 'wasm-eval' policy source keyword. There was a lot of discussion about using wasm-eval for normal web pages but the community eventually decided against doing that.

wasm-eval will continue to work for extensions. But, IMO, that sets up a problem for the future. Chrome extensions will be able to use wasm-eval; but noone else will (they will have to use wasm-unsafe-eval, or, in the future, a wasm-src policy). I can see there being some pressure to normalize this down the road.

@danielhjacobs
Copy link
Contributor

Also, I remember that Firefox complains about manifest fields it doesn't know (#5053), so minimum_chrome_version probably causes a warning (didn't verify).

It should be ignored according to this, but I didn't verify: https://bugzilla.mozilla.org/show_bug.cgi?id=1282978

@Herschel
Copy link
Member Author

Herschel commented Jan 11, 2022

It looks like Chrome has supported wasm-eval for extensions since Chrome 64(!), so we should have been using this instead of unsafe-eval all along, and we don't need minimum_chrome_version. I don't think it's necessary to add wasm-eval conditionally via webpack.config.json either, assuming browsers/web stores don't complain about an unknown identifier in the CSP -- I'll test this.

I am still considering adding wasm-unsafe-eval too, so that everything naturally works when other browsers implement the Wasm CSP restrictions without us releasing a new version. But maybe this is overkill -- sounds like everything may change in the future anyway.

@Herschel Herschel changed the title extension: Change unsafe-eval CSP to wasm-unsafe-eval extension: Change unsafe-eval to wasm-eval in CSP Jan 12, 2022
@Herschel
Copy link
Member Author

Herschel commented Jan 12, 2022

Firefox does display a warning on unknown CSP directives, so indeed I decided to inject wasm-eval via the webpack config when building for Chrome. Let's keep it simple and hold off on any wasm-unsafe-eval stuff until it's actually needed.

`unsafe-eval` was needed in the extension Content Security
Policy to Wasm compilation in Chrome.

This CSP setting causes the extension to get flagged in the
Mozilla Add-On Marketplace, which discourages the use of
`unsafe-eval`.

However, Chrome has a `wasm-eval` CSP setting which also allows
extensions to compile Wasm without requiring `unsafe-eval`.
Inject this into the extension manifest when building the Chrome
extension.

Eventually this may change to `wasm-unsafe-eval` as drafted by
the CSP spec and be required by all browsers.
@Herschel Herschel merged commit 70bb5fa into ruffle-rs:master Jan 12, 2022
@Herschel Herschel deleted the csp-wasm-eval branch January 12, 2022 19:29
@fosskers
Copy link

Hi there, my team is also paying close attention to the developments with wasm-eval. I'll echo that in my tests today, Chrome 97 complains about wasm-unsafe-eval and accepts wasm-eval without issue.

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.

5 participants