-
-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Conversation
As per https://help.nextcloud.com/t/changes-to-documentation-handling/161436 this needs to be documented |
23e9fab
to
f1b3c8f
Compare
f1b3c8f
to
0530815
Compare
There was a problem hiding this 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?
To be honest I do not really know, I also wondered the same, but I just followed the existing code 🤷 |
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.
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 |
Is there something that blocks this still? I have an app that uses https://github.com/gruhn/vue-qrcode-reader, which loads a |
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>
0530815
to
41f2d91
Compare
Rebased :-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works 👍
If a page has a Content Security Policy header and the
script-src
(ordefault-src
) directive does not contain neitherwasm-unsafe-eval
norunsafe-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 theeval
operation in JavaScript.To make possible to allow WebAssembly execution without allowing JavaScript
eval
this commit adds support for allowingwasm-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.