Code review comment for lp:~abreu-alexandre/oxide/content-script-injection-main-world

Revision history for this message
Chris Coulson (chrisccoulson) wrote :

Thanks for working on this. Some general comments:
- The special URL "oxide-private://main-world-private" would be fine as just "oxide://main-world"

- I'm not sure using greasemonkey metadata for this is a good idea. Scripts that define @inject_in_main_world in their header effectively change the behaviour of the UserScript.context API (it looks like it would be ignored if UserScript.emulateGreasemonkey is true). Also, @inject_in_main_world behaves differently depending on whether UserScript.emulateGreasemonkey is set or not.

Not only that, but the design is inherently unsafe - imagine the webbrowser grows the ability to install and run custom user scripts in the way that Chrome can with extensions. At the moment, the browser can safely isolate scripts based on their source by injecting them in to their own world (like Chrome, which gives each extension ID its own world). Having this additional metadata in the script header means that scripts can bypass this mechanism entirely.

We should drop this additional metadata and probably just rely on the special URL for UserScript.context. That would also mean you wouldn't need the additional accessors on oxide::UserScript. If UserScript.context is set to the special URL, the script should be injected in to the main world regardless of the state of UserScript.emulateGreasemonkey (but obviously, they wouldn't get the privileged API if UserScript.emulateGreasemonkey isn't set).

Note, this would also require a test to verify that scripts injected in to the main world don't have access to oxide.sendMessage if UserScript.emulateGreasemonkey is set to false.

- For tests in qt/tests/qmltests/api, I try to name them as close as possible to the API or class that they are testing. tst_WebView_scriptMainWorld doesn't really make much sense here - it would probably be better off in qt/tests/qmltests/core (and there are already some related tests there).

There are some additional comments inline too.

review: Needs Fixing

« Back to merge proposal