From b0d1a137dae4099a79a5779a02bbb61875298b2f Mon Sep 17 00:00:00 2001 From: Jimmy Date: Wed, 25 Apr 2018 16:54:54 +1200 Subject: [PATCH] Greasemonkey: Don't attempt scope isolation on webkit Since the JSCore used by WebKit 602.1 doesn't fully support Proxy and I can't think of a way to provide isolation otherwise just revert to the old behaviour in that case. I am checking for the specific WebKit version because I'm pretty sure that version just happened to be released when Proxy support was only partially done, any later release will presumably have a newer JSCore where it works. There I changed the indentation of a block in the jinja template which will have inflated the diff. I added mocking of `objects.backend` to the `webview` and `webenginewebview` fixtures, I am pretty sure they are mutually exclusive so don't expect any issues from that. Because of the feature detection being at template compile time I had to tweak the test setup to be done via a fixture instead of the setupClass functionality that I was using before. --- qutebrowser/browser/greasemonkey.py | 10 +- .../javascript/greasemonkey_wrapper.js | 108 +++++++++--------- tests/helpers/fixtures.py | 10 +- tests/unit/javascript/test_greasemonkey.py | 31 ++--- tests/unit/javascript/test_js_execution.py | 4 +- 5 files changed, 91 insertions(+), 72 deletions(-) diff --git a/qutebrowser/browser/greasemonkey.py b/qutebrowser/browser/greasemonkey.py index 578830793..db8246bab 100644 --- a/qutebrowser/browser/greasemonkey.py +++ b/qutebrowser/browser/greasemonkey.py @@ -31,9 +31,10 @@ import attr from PyQt5.QtCore import pyqtSignal, QObject, QUrl from qutebrowser.utils import (log, standarddir, jinja, objreg, utils, - javascript, urlmatch) + javascript, urlmatch, version, usertypes) from qutebrowser.commands import cmdutils from qutebrowser.browser import downloads +from qutebrowser.misc import objects def _scripts_dir(): @@ -108,13 +109,18 @@ class GreasemonkeyScript: browser's debugger/inspector will not match up to the line numbers in the source script directly. """ + # Don't use Proxy on this webkit version, the support isn't there. + use_proxy = not ( + objects.backend == usertypes.Backend.QtWebKit and + version.qWebKitVersion() == '602.1') template = jinja.js_environment.get_template('greasemonkey_wrapper.js') return template.render( scriptName=javascript.string_escape( "/".join([self.namespace or '', self.name])), scriptInfo=self._meta_json(), scriptMeta=javascript.string_escape(self.script_meta), - scriptSource=self._code) + scriptSource=self._code, + use_proxy=use_proxy) def _meta_json(self): return json.dumps({ diff --git a/qutebrowser/javascript/greasemonkey_wrapper.js b/qutebrowser/javascript/greasemonkey_wrapper.js index b38aaeec9..546de3c8a 100644 --- a/qutebrowser/javascript/greasemonkey_wrapper.js +++ b/qutebrowser/javascript/greasemonkey_wrapper.js @@ -145,58 +145,64 @@ } }; - /* - * Try to give userscripts an environment that they expect. Which - * seems to be that the global window object should look the same as - * the page's one and that if a script writes to an attribute of - * window it should be able to access that variable in the global - * scope. - * Use a Proxy to stop scripts from actually changing the global - * window (that's what unsafeWindow is for). - * Use the "with" statement to make the proxy provide what looks - * like global scope. - * - * There are other Proxy functions that we may need to override. - * set, get and has are definitely required. - */ - const unsafeWindow = window; - const qute_gm_window_shadow = {}; // stores local changes to window - const qute_gm_windowProxyHandler = { - get: function(obj, prop) { - if (prop in qute_gm_window_shadow) - return qute_gm_window_shadow[prop]; - if (prop in obj) { - if (typeof obj[prop] === 'function' && typeof obj[prop].prototype == 'undefined') - // Getting TypeError: Illegal Execution when callers try to execute - // eg addEventListener from here because they were returned - // unbound - return obj[prop].bind(obj); - return obj[prop]; + {% if use_proxy %} + /* + * Try to give userscripts an environment that they expect. Which + * seems to be that the global window object should look the same as + * the page's one and that if a script writes to an attribute of + * window it should be able to access that variable in the global + * scope. + * Use a Proxy to stop scripts from actually changing the global + * window (that's what unsafeWindow is for). + * Use the "with" statement to make the proxy provide what looks + * like global scope. + * + * There are other Proxy functions that we may need to override. + * set, get and has are definitely required. + */ + const unsafeWindow = window; + const qute_gm_window_shadow = {}; // stores local changes to window + const qute_gm_windowProxyHandler = { + get: function(obj, prop) { + if (prop in qute_gm_window_shadow) + return qute_gm_window_shadow[prop]; + if (prop in obj) { + if (typeof obj[prop] === 'function' && typeof obj[prop].prototype == 'undefined') + // Getting TypeError: Illegal Execution when callers try to execute + // eg addEventListener from here because they were returned + // unbound + return obj[prop].bind(obj); + return obj[prop]; + } + }, + set: function(target, prop, val) { + return qute_gm_window_shadow[prop] = val; + }, + has: function(target, key) { + return key in qute_gm_window_shadow || key in target; } - }, - set: function(target, prop, val) { - return qute_gm_window_shadow[prop] = val; - }, - has: function(target, key) { - return key in qute_gm_window_shadow || key in target; - } - }; - const qute_gm_window_proxy = new Proxy( - unsafeWindow, qute_gm_windowProxyHandler); + }; + const qute_gm_window_proxy = new Proxy( + unsafeWindow, qute_gm_windowProxyHandler); - with (qute_gm_window_proxy) { - // We can't return `this` or `qute_gm_window_proxy` from - // `qute_gm_window_proxy.get('window')` because the Proxy implementation - // does typechecking on read-only things. So we have to shadow `window` - // more conventionally here. Except we can't do it directly within - // with `with` scope because then it would get assigned to the - // proxy and we would get the same problem, so we have to make yet - // another nested scope. - (function() { - let window = qute_gm_window_proxy; - // ====== The actual user script source ====== // + with (qute_gm_window_proxy) { + // We can't return `this` or `qute_gm_window_proxy` from + // `qute_gm_window_proxy.get('window')` because the Proxy implementation + // does typechecking on read-only things. So we have to shadow `window` + // more conventionally here. Except we can't do it directly within + // with `with` scope because then it would get assigned to the + // proxy and we would get the same problem, so we have to make yet + // another nested scope. + (function() { + let window = qute_gm_window_proxy; + // ====== The actual user script source ====== // {{ scriptSource }} - // ====== End User Script ====== // - })(); - }; + // ====== End User Script ====== // + })(); + }; + {% else %} + // ====== The actual user script source ====== // +{{ scriptSource }} + // ====== End User Script ====== // + {% endif %} })(); diff --git a/tests/helpers/fixtures.py b/tests/helpers/fixtures.py index c4756e1f5..cfaba7a55 100644 --- a/tests/helpers/fixtures.py +++ b/tests/helpers/fixtures.py @@ -43,10 +43,10 @@ import helpers.stubs as stubsmod import helpers.utils from qutebrowser.config import (config, configdata, configtypes, configexc, configfiles) -from qutebrowser.utils import objreg, standarddir, utils +from qutebrowser.utils import objreg, standarddir, utils, usertypes from qutebrowser.browser import greasemonkey from qutebrowser.browser.webkit import cookies -from qutebrowser.misc import savemanager, sql +from qutebrowser.misc import savemanager, sql, objects from qutebrowser.keyinput import modeman @@ -360,9 +360,10 @@ def qnam(qapp): @pytest.fixture -def webengineview(qtbot): +def webengineview(qtbot, monkeypatch): """Get a QWebEngineView if QtWebEngine is available.""" QtWebEngineWidgets = pytest.importorskip('PyQt5.QtWebEngineWidgets') + monkeypatch.setattr(objects, 'backend', usertypes.Backend.QtWebEngine) view = QtWebEngineWidgets.QWebEngineView() qtbot.add_widget(view) return view @@ -379,9 +380,10 @@ def webpage(qnam): @pytest.fixture -def webview(qtbot, webpage): +def webview(qtbot, webpage, monkeypatch): """Get a new QWebView object.""" QtWebKitWidgets = pytest.importorskip('PyQt5.QtWebKitWidgets') + monkeypatch.setattr(objects, 'backend', usertypes.Backend.QtWebKit) view = QtWebKitWidgets.QWebView() qtbot.add_widget(view) diff --git a/tests/unit/javascript/test_greasemonkey.py b/tests/unit/javascript/test_greasemonkey.py index 8f780c432..80285ffca 100644 --- a/tests/unit/javascript/test_greasemonkey.py +++ b/tests/unit/javascript/test_greasemonkey.py @@ -163,10 +163,14 @@ def test_required_scripts_are_included(download_stub, tmpdir): class TestWindowIsolation: """Check that greasemonkey scripts get a shadowed global scope.""" - @classmethod - def setup_class(cls): + @pytest.fixture + def setup(self): + class SetupData: + pass + ret = SetupData() + # Change something in the global scope - cls.setup_script = "window.$ = 'global'" + ret.setup_script = "window.$ = 'global'" # Greasemonkey script to report back on its scope. test_script = greasemonkey.GreasemonkeyScript.parse( @@ -188,7 +192,7 @@ class TestWindowIsolation: # The compiled source of that scripts with some additional setup # bookending it. - cls.test_script = "\n".join([ + ret.test_script = "\n".join([ "var result = [];", test_script.code(), """ @@ -201,21 +205,22 @@ class TestWindowIsolation: ]) # What we expect the script to report back. - cls.expected = [ + ret.expected = [ "global", "global", "shadowed", "shadowed", "global", "global"] + return ret - def test_webengine(self, callback_checker, webengineview): + def test_webengine(self, callback_checker, webengineview, setup): page = webengineview.page() - page.runJavaScript(self.setup_script) - page.runJavaScript(self.test_script, callback_checker.callback) - callback_checker.check(self.expected) + page.runJavaScript(setup.setup_script) + page.runJavaScript(setup.test_script, callback_checker.callback) + callback_checker.check(setup.expected) # The JSCore in 602.1 doesn't fully support Proxy. @pytest.mark.qtwebkit6021_skip - def test_webkit(self, webview): + def test_webkit(self, webview, setup): elem = webview.page().mainFrame().documentElement() - elem.evaluateJavaScript(self.setup_script) - result = elem.evaluateJavaScript(self.test_script) - assert result == self.expected + elem.evaluateJavaScript(setup.setup_script) + result = elem.evaluateJavaScript(setup.test_script) + assert result == setup.expected diff --git a/tests/unit/javascript/test_js_execution.py b/tests/unit/javascript/test_js_execution.py index c5b5018d2..81f999962 100644 --- a/tests/unit/javascript/test_js_execution.py +++ b/tests/unit/javascript/test_js_execution.py @@ -26,7 +26,7 @@ import pytest @pytest.mark.parametrize('js_enabled, expected', [(True, 2.0), (False, None)]) def test_simple_js_webkit(webview, js_enabled, expected): """With QtWebKit, evaluateJavaScript works when JS is on.""" - # If we get there (because of the webengineview fixture) we can be certain + # If we get there (because of the webview fixture) we can be certain # QtWebKit is available from PyQt5.QtWebKit import QWebSettings webview.settings().setAttribute(QWebSettings.JavascriptEnabled, js_enabled) @@ -37,7 +37,7 @@ def test_simple_js_webkit(webview, js_enabled, expected): @pytest.mark.parametrize('js_enabled, expected', [(True, 2.0), (False, 2.0)]) def test_element_js_webkit(webview, js_enabled, expected): """With QtWebKit, evaluateJavaScript on an element works with JS off.""" - # If we get there (because of the webengineview fixture) we can be certain + # If we get there (because of the webview fixture) we can be certain # QtWebKit is available from PyQt5.QtWebKit import QWebSettings webview.settings().setAttribute(QWebSettings.JavascriptEnabled, js_enabled)