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

Allow "wasm-unsafe-eval" in CSP #38082

Merged
merged 1 commit into from Aug 11, 2023
Merged

Conversation

danxuliu
Copy link
Member

@danxuliu danxuliu commented May 4, 2023

If a page has a Content Security Policy header and the script-src (or default-src) directive does not contain neither wasm-unsafe-eval nor unsafe-eval loading and executing WebAssembly is blocked in the page (although it is still possible to load and execute WebAssembly in a worker thread).

Although the Nextcloud classes to manage the CSP already supported allowing unsafe-eval this affects not only WebAssembly, but also the eval operation in JavaScript.

To make possible to allow WebAssembly execution without allowing JavaScript eval this commit adds support for allowing wasm-unsafe-eval (which is supported since some versions already by browsers).

Nevertheless, please keep in mind that running heavy WebAssembly code in the main thread can lead to unresponsiveness in the rest of the UI; it might be better to run WebAssembly code in a worker thread and do it in the main thread only for light (so it does not block the rest of the code) real time (which could lead to issues if run in a worker thread due to the synchronization of the data between the threads) calculations.

@danxuliu danxuliu added the 3. to review Waiting for reviews label May 4, 2023
@danxuliu danxuliu added this to the Nextcloud 27 milestone May 4, 2023
@danxuliu danxuliu requested review from nickvergessen, a team, ArtificialOwl, Fenn-CS and come-nc and removed request for a team May 4, 2023 15:01
@nickvergessen nickvergessen added the pending documentation This pull request needs an associated documentation update label May 4, 2023
@nickvergessen
Copy link
Member

As per https://help.nextcloud.com/t/changes-to-documentation-handling/161436 this needs to be documented

@danxuliu danxuliu force-pushed the allow-wasm-unsafe-eval-in-csp branch from f1b3c8f to 0530815 Compare May 5, 2023 11:01
This was referenced May 9, 2023
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not see where null and false mean something different, why is the property nullable? Could it be a bool initialized to false or is it ever useful to know if it was set to false or never touched?

@danxuliu
Copy link
Member Author

danxuliu commented May 9, 2023

I do not see where null and false mean something different, why is the property nullable? Could it be a bool initialized to false or is it ever useful to know if it was set to false or never touched?

To be honest I do not really know, I also wondered the same, but I just followed the existing code 🤷

@come-nc
Copy link
Contributor

come-nc commented May 11, 2023

To be honest I do not really know, I also wondered the same, but I just followed the existing code shrug

Don’t do that, fix the existing code!
But at least for this PR use bool instead of ?bool in all places, it’s clearer.

@danxuliu
Copy link
Member Author

To be honest I do not really know, I also wondered the same, but I just followed the existing code shrug

Don’t do that, fix the existing code!

Well, of course, that is what I usually try to do, but not in this case ;-) As you may understand I am not going to change the default value of protected attributes in a public API class and risk breaking any app that may have extended them and rely in the values being null, even if the documentation says that it is a boolean and does not mention it being nullable. Specially when the default values have been null instead of just false since they were introduced, so there might have been a good reason for that, and I am not going to touch that code unless I am fully confident that the change is correct.

But at least for this PR use bool instead of ?bool in all places, it’s clearer.

Given that we do not know whether it is relevant or not to be able to differentiate if a value was ever set or not I would favour consistency with the existing code instead of clearness.

@nickvergessen
Copy link
Member

Given that we do not know whether it is relevant or not to be able to differentiate if a value was ever set or not I would favour consistency with the existing code instead of clearness.

Same

@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz removed this from the Nextcloud 27 milestone May 23, 2023
@blizzz blizzz added this to the Nextcloud 28 milestone May 23, 2023
@raimund-schluessler
Copy link
Member

Is there something that blocks this still? I have an app that uses https://github.com/gruhn/vue-qrcode-reader, which loads a wasm file and the only way to get it to work is by setting $csp->allowEvalScript(true); at the moment which of course is not so nice. Having this PR in would allow to limit it to wasm files only.

@nickvergessen
Copy link
Member

Just missing a rebase and a second review :)

If a page has a Content Security Policy header and the `script-src` (or
`default-src`) directive does not contain neither `wasm-unsafe-eval` nor
`unsafe-eval` loading and executing WebAssembly is blocked in the page
(although it is still possible to load and execute WebAssembly in a
worker thread).

Although the Nextcloud classes to manage the CSP already supported
allowing `unsafe-eval` this affects not only WebAssembly, but also the
`eval` operation in JavaScript.

To make possible to allow WebAssembly execution without allowing
JavaScript `eval` this commit adds support for allowing
`wasm-unsafe-eval`.

Signed-off-by: Daniel Calviño Sánchez <danxuliu@gmail.com>
@danxuliu danxuliu force-pushed the allow-wasm-unsafe-eval-in-csp branch from 0530815 to 41f2d91 Compare August 10, 2023 02:58
@danxuliu
Copy link
Member Author

Rebased :-)

Copy link
Member

@raimund-schluessler raimund-schluessler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works 👍

@nickvergessen nickvergessen merged commit 9fb08f5 into master Aug 11, 2023
37 of 38 checks passed
@nickvergessen nickvergessen deleted the allow-wasm-unsafe-eval-in-csp branch August 11, 2023 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews pending documentation This pull request needs an associated documentation update
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants