From 0f65e868ab2c71ca8d904ed1803a71216c2aaa98 Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Tue, 9 Dec 2014 16:10:53 -0800 Subject: [PATCH 1/7] Bug 1109369 - Treat `Cu.waiveXrays` the same as `XPCNativeWrapper.unwrap`. --- tests/test_js_wrappedjsobject.py | 12 ++++++++++++ validator/testcases/javascript/predefinedentities.py | 5 ++++- 2 files changed, 16 insertions(+), 1 deletion(-) diff --git a/tests/test_js_wrappedjsobject.py b/tests/test_js_wrappedjsobject.py index 6c1955723..706a99152 100644 --- a/tests/test_js_wrappedjsobject.py +++ b/tests/test_js_wrappedjsobject.py @@ -13,6 +13,7 @@ def test_pass(self): self.run_script(""" var x = foo.wrappedJSObject; var y = XPCNativeWrapper.unwrap(foo); + var z = Cu.waiveXrays(foo); """) self.assert_silent() @@ -60,6 +61,17 @@ def test_cant_assign_unwrap(self): """) self.assert_wrappedjs_failure() + def test_cant_assign_waive(self): + """ + Test that properties can't be assigned to JS objects that were + unwrapped via XPCNativeWrapper.unwrap(). + """ + self.run_script(""" + var x = Cu.waiveXrays(foo); + x.foo = "asdf"; + """) + self.assert_wrappedjs_failure() + def test_recursive_assign_unwrap(self): """ Test that properties can't be assigned to the members of objects that diff --git a/validator/testcases/javascript/predefinedentities.py b/validator/testcases/javascript/predefinedentities.py index 9ffdea59f..d20f91483 100644 --- a/validator/testcases/javascript/predefinedentities.py +++ b/validator/testcases/javascript/predefinedentities.py @@ -780,7 +780,10 @@ def build_quick_xpcom(method, interface, traverser): u"import": {"dangerous": lambda a, t, e: - a and "ctypes.jsm" in _get_as_str(t(a[0]))}}}, + a and "ctypes.jsm" in _get_as_str(t(a[0]))}, + + u"waiveXrays": + {"return": call_definitions.js_unwrap}}}, u"interfaces": {"value": INTERFACE_ENTITIES}}}, u"extensions": {"dangerous": True}, u"xpcnativewrappers": {"dangerous": True}, From 4769037e2fabecf738f47d55abdc0cdd3b9e9c68 Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Wed, 10 Dec 2014 12:19:05 -0800 Subject: [PATCH 2/7] Bug 1099269 - Flag uses of old extension manager API --- tests/test_regex.py | 14 ++++++++++++++ .../testcases/javascript/predefinedentities.py | 16 ++++++++++++++-- validator/testcases/regex.py | 17 +++++++++++++++++ 3 files changed, 45 insertions(+), 2 deletions(-) diff --git a/tests/test_regex.py b/tests/test_regex.py index a7334b19c..fd519cc83 100644 --- a/tests/test_regex.py +++ b/tests/test_regex.py @@ -101,6 +101,20 @@ def test_processNextEvent_banned(): """).failed() +def test_extension_manager_api(): + assert _do_test_raw(""" + Cc["@mozilla.org/extensions/manager;1"].getService(); + """).failed() + + assert _do_test_raw(""" + if (topic == "em-action-requested") true; + """).failed() + + assert _do_test_raw(""" + thing.QueryInterface(Ci.nsIExtensionManager); + """).failed() + + def test_bug_652575(): """Ensure that capability.policy gets flagged.""" assert _do_test_raw("var x = 'capability.policy.';").failed() diff --git a/validator/testcases/javascript/predefinedentities.py b/validator/testcases/javascript/predefinedentities.py index d20f91483..b0b92f736 100644 --- a/validator/testcases/javascript/predefinedentities.py +++ b/validator/testcases/javascript/predefinedentities.py @@ -67,7 +67,19 @@ "any added category entries at shutdown.")}} +OBSOLETE_EXTENSION_MANAGER = { + "value": {}, + "dangerous": "This interface is part of the obsolete extension manager " + "interface, which is not available in any remotely modern " + "version of Firefox. It should not be referenced in any " + "code."} + INTERFACES = { + u"nsIExtensionManager": OBSOLETE_EXTENSION_MANAGER, + u"nsIUpdateItem": OBSOLETE_EXTENSION_MANAGER, + u"nsIInstallLocation": OBSOLETE_EXTENSION_MANAGER, + u"nsIAddonInstallListener": OBSOLETE_EXTENSION_MANAGER, + u"nsIAddonUpdateCheckListener": OBSOLETE_EXTENSION_MANAGER, u"imIUserStatusInfo": {"value": {u"setUserIcon": entity("imIUserStatusInfo.setUserIcon")}}, @@ -85,7 +97,8 @@ {"dangerous": "Using the nsIAccessibleRetrieval interface causes significant " "performance degradation in Gecko. It should only be used in " - "accessibility-related add-ons."}, + "accessibility-related add-ons.", + "value": {}}, u"nsIBrowserSearchService": {"value": {u"currentEngine": {"readonly": True}, @@ -573,7 +586,6 @@ def build_quick_xpcom(method, interface, traverser): u"Ci": {"readonly": False, "value": lambda t: GLOBAL_ENTITIES["Components"]["value"]["interfaces"]}, - u"Cu": {"readonly": False, "value": lambda t: GLOBAL_ENTITIES["Components"]["value"]["utils"]}, diff --git a/validator/testcases/regex.py b/validator/testcases/regex.py index 555c78cd7..a446e4153 100644 --- a/validator/testcases/regex.py +++ b/validator/testcases/regex.py @@ -356,6 +356,23 @@ def js_tests(self): "for more information.") +@register_generator +class ExtensionManagerRegexTests(RegexTestGenerator): + """ + Tests for uses of the old extension manager API, which should not be + referenced in new extensions. + """ + + def js_tests(self): + yield self.get_test( + r"@mozilla\.org/extensions/manager;1|" + r"em-action-requested", + "Obsolete Extension Manager API", + "The old Extension Manager API is not available in any " + "remotely modern version of Firefox and should not be " + "referenced in any code.") + + @register_generator class JSPrototypeExtRegexTests(RegexTestGenerator): """ From 866b85e90fdcfa5985e461d57cbb83e564506e8d Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Wed, 10 Dec 2014 12:36:40 -0800 Subject: [PATCH 3/7] Bug 1007912 - Warn about using the observer service directly in SDK add-ons --- tests/test_jetpack.py | 11 +++++++++++ validator/testcases/javascript/predefinedentities.py | 8 +++++++- 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/tests/test_jetpack.py b/tests/test_jetpack.py index adbd221d0..b940ca201 100644 --- a/tests/test_jetpack.py +++ b/tests/test_jetpack.py @@ -400,6 +400,17 @@ def test_absolute_uris_in_js(): assert err.compat_summary["errors"] +def test_observer_service_flagged(): + assert _js_test(""" + var {Ci} = require("chrome"); + thing.QueryInterface(Ci.nsIObserverService); + """, jetpack=True).failed() + + assert not _js_test(""" + thing.QueryInterface(Ci.nsIObserverService); + """).failed() + + def test_absolute_uris_in_markup(): """ Test that a warning is thrown for absolute URIs within markup files. diff --git a/validator/testcases/javascript/predefinedentities.py b/validator/testcases/javascript/predefinedentities.py index b0b92f736..fbc3b352d 100644 --- a/validator/testcases/javascript/predefinedentities.py +++ b/validator/testcases/javascript/predefinedentities.py @@ -280,7 +280,13 @@ e.get_resource("em:bootstrap") and "Authors of bootstrapped add-ons must take care " "to remove any added observers " - "at shutdown."}}}, + "at shutdown."}}, + "dangerous": lambda a, t, e: + lambda t, e: ( + e.metadata.get("is_jetpack") and + "The observer service should not be used directly in SDK " + "add-ons. Please use the 'sdk/system/events' module " + "instead.")}, u"nsIResProtocolHandler": {"value": {u"setSubstitution": From baf738bc731b9f759070df2c9e7730c9c60ff084 Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Wed, 10 Dec 2014 13:28:49 -0800 Subject: [PATCH 4/7] Bug 1079592 - Warn when using `nsIWebBrowserPersist` that `Downloads.jsm` should be used instead. --- tests/test_js_xpcom.py | 35 ++++++++++++++++--- .../testcases/javascript/call_definitions.py | 24 ++++++++++++- .../javascript/predefinedentities.py | 8 +++-- 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/tests/test_js_xpcom.py b/tests/test_js_xpcom.py index 3b2161b23..7422db370 100644 --- a/tests/test_js_xpcom.py +++ b/tests/test_js_xpcom.py @@ -281,20 +281,45 @@ def test_xpcom_nsiwebbrowserpersist(): with a null load context. """ - assert _do_test_raw(""" + def test(js, want_pass): + err = _do_test_raw(js) + if err.warnings: + result = err.warnings[0]["id"][-1] != "webbrowserpersist_saveuri" + eq_(result, want_pass) + else: + assert want_pass + + test(""" var foo = Cc["foo"].getService(Components.interfaces.nsIWebBrowserPersist); foo.saveURI(null, null, null, null, null, null, null); - """).failed() + """, False) - assert _do_test_raw(""" + test(""" var foo = Cc["foo"].getService(Components.interfaces.nsIWebBrowserPersist); var thing = null; foo.saveURI(null, null, null, null, null, null, thing); - """).failed() + """, False) - assert not _do_test_raw(""" + test(""" var foo = Cc["foo"].getService(Components.interfaces.nsIWebBrowserPersist); foo.saveURI(null, null, null, null, null, null, thing); + """, True) + + + +def test_xpcom_nsiwebbrowserpersist_deprecation(): + """Tests that nsIWebBrowserPersist emits deprecation warnings.""" + + assert _do_test_raw(""" + thing.QueryInterface(Ci.nsIWebBrowserPersist).saveChannel() + """).failed() + + assert _do_test_raw(""" + thing.QueryInterface(Ci.nsIWebBrowserPersist).saveURI(1, 2, 3, 4, 5, 6, 7); + """).failed() + + assert _do_test_raw(""" + thing.QueryInterface(Ci.nsIWebBrowserPersist).savePrivacyAwareURI() """).failed() diff --git a/validator/testcases/javascript/call_definitions.py b/validator/testcases/javascript/call_definitions.py index 665b5213a..1963e05ca 100644 --- a/validator/testcases/javascript/call_definitions.py +++ b/validator/testcases/javascript/call_definitions.py @@ -166,6 +166,26 @@ def spellcheck_updatecurrentdictionary(wrapper, arguments, traverser): tier=5) +def webbrowserpersist(wrapper, arguments, traverser): + """ + Most nsIWebBrowserPersist should no longer be used, in favor of the new + Downloads.jsm interfaces. + """ + traverser.err.warning( + err_id=("testcases_javascript_call_definititions", + "webbrowserpersist"), + warning="nsIWebBrowserPersist should no longer be used", + description=("Most nsIWebBrowserPersist methods have been " + "superseded by simpler methods in Downloads.jsm, namely " + "`Downloads.fetch` and `Downloads.createDownload`. See " + "http://mzl.la/downloads-jsm for more information."), + filename=traverser.filename, + line=traverser.line, + column=traverser.position, + context=traverser.context, + tier=4) + + def webbrowserpersist_saveuri(wrapper, arguments, traverser): """ nsIWebBrowserPersist.saveURI requires a valid privacy context as @@ -176,7 +196,7 @@ def webbrowserpersist_saveuri(wrapper, arguments, traverser): if load_context.get_literal_value() is None: traverser.err.warning( err_id=("testcases_javascript_call_definititions", - "webbrowserpersist_saveuri"), + "webbrowserpersist_saveuri"), warning=("saveURI should not be called with a null load " "context"), description=("While nsIWebBrowserPersist.saveURI accepts null " @@ -189,6 +209,8 @@ def webbrowserpersist_saveuri(wrapper, arguments, traverser): context=traverser.context, tier=4) + webbrowserpersist(wrapper, arguments, traverser) + def xpcom_constructor(method, extend=False, mutate=False, pretraversed=False): """Returns a function which wraps an XPCOM class instantiation function.""" diff --git a/validator/testcases/javascript/predefinedentities.py b/validator/testcases/javascript/predefinedentities.py index fbc3b352d..3ca220ffd 100644 --- a/validator/testcases/javascript/predefinedentities.py +++ b/validator/testcases/javascript/predefinedentities.py @@ -389,9 +389,13 @@ u"execCommandShowHelp": entity("nsIDOMHTMLDocument")}}, u"nsIWebBrowserPersist": {"value": - {u"saveURI": + {u"saveChannel": + {"return": call_definitions.webbrowserpersist}, + u"saveURI": {"return": - call_definitions.webbrowserpersist_saveuri}}}, + call_definitions.webbrowserpersist_saveuri}, + u"savePrivacyAwareURI": + {"return": call_definitions.webbrowserpersist}}}, u"prplIAccount": {"value": {u"noNewlines": entity("prplIAccount.noNewlines"), From e828d823e7987f407b1033cc37a25d08581931de Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Fri, 12 Dec 2014 14:12:02 -0800 Subject: [PATCH 5/7] Bug 1084015 - Add warnings for unsafe template escape sequences --- tests/test_js_functions.py | 7 +++++ tests/test_regex.py | 9 ++++++ .../javascript/predefinedentities.py | 20 +++++++++++++ validator/testcases/regex.py | 28 +++++++++++++++++++ 4 files changed, 64 insertions(+) diff --git a/tests/test_js_functions.py b/tests/test_js_functions.py index af27d0e79..cece73664 100644 --- a/tests/test_js_functions.py +++ b/tests/test_js_functions.py @@ -132,3 +132,10 @@ def test_number_global_conversions(): eq_(_get_var(err, "f"), _get_var(err, "nan")) eq_(_get_var(err, "g"), _get_var(err, "nan")) + +def test_unsafe_template_methods(): + """Test that the use of unsafe template methods is flagged.""" + + assert _do_test_raw("""bar = Handlebars.SafeString(foo)""").failed() + assert _do_test_raw("""bar = $sce.trustAsHTML(foo)""").failed() + assert _do_test_raw("""bar = $sce.trustAs("html", foo)""").failed() diff --git a/tests/test_regex.py b/tests/test_regex.py index fd519cc83..348211b26 100644 --- a/tests/test_regex.py +++ b/tests/test_regex.py @@ -169,6 +169,15 @@ def test_preference_extension_regex(): assert _do_test_raw('"foo.extensions.update.bar"').failed() +def test_template_escape(): + """Tests that the use of unsafe template escape sequences is flagged.""" + + assert _do_test_raw("<%= foo %>").failed() + assert _do_test_raw("{{{ foo }}}").failed() + + assert _do_test_raw("ng-bind-html-unsafe='foo'").failed() + + def test_tb11_strings(): """Flag changed or removed strings in add-on code""" diff --git a/validator/testcases/javascript/predefinedentities.py b/validator/testcases/javascript/predefinedentities.py index 3ca220ffd..cda138bfe 100644 --- a/validator/testcases/javascript/predefinedentities.py +++ b/validator/testcases/javascript/predefinedentities.py @@ -586,6 +586,12 @@ def build_quick_xpcom(method, interface, traverser): return obj +UNSAFE_TEMPLATE_METHOD = ( + "The use of `%s` can lead to unsafe " + "remote code execution, and therefore must be done with " + "great care, and only with sanitized data.") + + # GLOBAL_ENTITIES is also representative of the `window` object. GLOBAL_ENTITIES = { u"window": {"value": lambda t: {"value": GLOBAL_ENTITIES}}, @@ -917,6 +923,20 @@ def build_quick_xpcom(method, interface, traverser): u"allowRemoteContentForSite": entity("allowRemoteContentForSite"), u"createNewHeaderView": entity("createNewHeaderView"), u"getShortcutOrURIAndPostData": entity("getShortcutOrURIAndPostData"), + + # Common third-party libraries + "Handlebars": { + "value": { + "SafeString": + {"dangerous": + UNSAFE_TEMPLATE_METHOD % 'Handlebars.SafeString'}}}, + # Angular + "$sce": { + "value": { + "trustAs": {"dangerous": + UNSAFE_TEMPLATE_METHOD % '$sce.trustAs'}, + "trustAsHTML": {"dangerous": + UNSAFE_TEMPLATE_METHOD % '$sce.trustAsHTML'}}}, } CONTENT_DOCUMENT = GLOBAL_ENTITIES[u"content"]["value"][u"document"] diff --git a/validator/testcases/regex.py b/validator/testcases/regex.py index a446e4153..88cccdaee 100644 --- a/validator/testcases/regex.py +++ b/validator/testcases/regex.py @@ -303,6 +303,34 @@ def tests(self): r"""(?:sdk/)?(%s)['"]\s*\)""") +@register_generator +class UnsafeTemplateRegexTests(RegexTestGenerator): + """ + Checks for the use of unsafe template escape sequences. + """ + + @classmethod + def applicable(cls, err, filename, document): + # Perhaps this should just run on all non-binary files, but + # we'll try to be more selective for the moment. + return bool(re.match(r".*\.(js(|m)|handlebars|mustache|" + r"(t|)htm(|l)|tm?pl)", + filename)) + + def tests(self): + for unsafe, safe in (('<%=', '<%-'), + ('{{{', '{{'), + ('ng-bind-html-unsafe=', 'ng-bind-html')): + yield self.get_test( + re.escape(unsafe), + "Potentially unsafe template escape sequence", + "The use of non-HTML-escaping template escape sequences is " + "potentially dangerous and highly discouraged. Non-escaped " + "HTML may only be used when properly sanitized, and in most " + "cases safer escape sequences such as `%s` must be used " + "instead." % safe) + + @register_generator class ChromePatternRegexTests(RegexTestGenerator): """ From b4b8b421df58ff88de1a9a3708bd787d2b67b488 Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Fri, 12 Dec 2014 15:45:34 -0800 Subject: [PATCH 6/7] Bug 996822 - Flag uses of synchronous SQL via the storage service --- tests/test_js_instanceactions.py | 13 +-- tests/test_js_xpcom.py | 15 +++ .../testcases/javascript/instanceactions.py | 109 ++++++++++-------- .../javascript/predefinedentities.py | 11 +- 4 files changed, 92 insertions(+), 56 deletions(-) diff --git a/tests/test_js_instanceactions.py b/tests/test_js_instanceactions.py index d29767a26..9255ecc08 100644 --- a/tests/test_js_instanceactions.py +++ b/tests/test_js_instanceactions.py @@ -65,20 +65,19 @@ def test_sql_methods(): """Tests that warnings on SQL methods are emitted properly""" err = _do_test_raw(""" - x.createStatement("foo"); + x.executeSimpleSQL("foo " + y); """) - eq_(err.warnings[0]["id"][-1], "synchronous_sql") + eq_(err.warnings[0]["id"][-1], "executeSimpleSQL_dynamic") err = _do_test_raw(""" - x.executeSimpleSQL("foo"); + x.createStatement("foo " + y); """) - eq_(err.warnings[0]["id"][-1], "synchronous_sql") + eq_(err.warnings[0]["id"][-1], "executeSimpleSQL_dynamic") err = _do_test_raw(""" - x.executeSimpleSQL("foo " + y); + x.createAsyncStatement("foo " + y); """) - eq_(err.warnings[0]["id"][-1], "synchronous_sql") - eq_(err.warnings[1]["id"][-1], "executeSimpleSQL_dynamic") + eq_(err.warnings[0]["id"][-1], "executeSimpleSQL_dynamic") def test_setAttribute(): """Tests that setAttribute calls are blocked successfully""" diff --git a/tests/test_js_xpcom.py b/tests/test_js_xpcom.py index 7422db370..23610cd3b 100644 --- a/tests/test_js_xpcom.py +++ b/tests/test_js_xpcom.py @@ -247,6 +247,21 @@ def test_xpcom_nsibrowsersearchservice(): """).failed() +def test_synchronous_sql(): + """Test that uses of synchronous SQL are flagged.""" + + assert _do_test_raw("database.executeSimpleSQL('foo');").failed() + + assert not _do_test_raw("database.createStatement();").failed() + + for meth in "execute", "executeStep": + assert _do_test_raw("database.createStatement().%s();" % meth).failed() + + assert not _do_test_raw(""" + database.createStatement().executeAsync() + """).failed() + + def test_nsisound_play(): """Test that nsISound.play is flagged.""" diff --git a/validator/testcases/javascript/instanceactions.py b/validator/testcases/javascript/instanceactions.py index 359632af6..6f53d33eb 100644 --- a/validator/testcases/javascript/instanceactions.py +++ b/validator/testcases/javascript/instanceactions.py @@ -18,21 +18,6 @@ from instanceproperties import _set_HTML_property -def _warn_synchronous_sql(traverser): - traverser.err.warning( - err_id=("js", "instanceactions", "synchronous_sql"), - warning="Synchronous SQL access should be avoided.", - description="The use of synchronous SQL is known to cause significant " - "IO pauses and general responsiveness issues. Please use " - "asynchronous Storage APIs, particularly " - "http://mzl.la/sqlite-jsm or `createAsyncStatement`, where " - "possible.", - filename=traverser.filename, - line=traverser.line, - column=traverser.position, - context=traverser.context) - - def addEventListener(args, traverser, node, wrapper): """ Handle calls to addEventListener and make sure that the fourth argument is @@ -101,37 +86,6 @@ def createEvent(args, traverser, node, wrapper): tier=5) -def createStatement(args, traverser, node, wrapper): - """Handles createStatement calls.""" - - _warn_synchronous_sql(traverser) - - -def executeSimpleSQL(args, traverser, node, wrapper): - """Handles executeSimpleSQL calls.""" - - _warn_synchronous_sql(traverser) - - simple_args = map(traverser._traverse_node, args) - - if len(args) >= 1 and not simple_args[0].is_literal(): - traverser.warning( - err_id=("js", "instanceactions", "executeSimpleSQL_dynamic"), - warning="`executeSimpleSQL` should not be used with dynamic " - "SQL.", - description=["For dynamic SQL statements, especially those with " - "parameters, Sqlite.jsm wrappers, or " - "`createAsyncStatement`, should be used with " - "dynamic parameter binding.", - "See http://mzl.la/sqlite-jsm and " - "https://developer.mozilla.org/en-US/docs" - "/Storage#Binding_parameters for more information"], - filename=traverser.filename, - line=traverser.line, - column=traverser.position, - context=traverser.context) - - def QueryInterface(args, traverser, node, wrapper): """Handles QueryInterface calls""" @@ -318,6 +272,66 @@ def bind(args, traverser, node, wrapper): return obj +SYNCHRONOUS_SQL_DESCRIPTION = ( + "The use of synchronous SQL via the storage system leads to severe " + "responsiveness issues, and should be avoided at all costs. Please " + "use asynchronous SQL via Sqlite.jsm (http://mzl.la/sqlite-jsm) or " + "the `executeAsync` method, or otherwise switch to a simpler database " + "such as JSON files or IndexedDB.") + + +def _check_dynamic_sql(args, traverser, node=None, wrapper=None): + """ + Checks for the use of non-static strings when creating/exeucting SQL + statements. + """ + + simple_args = map(traverser._traverse_node, args) + if len(args) >= 1 and not simple_args[0].is_literal(): + traverser.warning( + err_id=("js", "instanceactions", "executeSimpleSQL_dynamic"), + warning="SQL statements should be static strings", + description=["Dynamic SQL statement should be constucted via " + "static strings, in combination with dynamic " + "parameter binding via Sqlite.jsm wrappers " + "(http://mzl.la/sqlite-jsm) or " + "`createAsyncStatement` " + "(https://developer.mozilla.org/en-US/docs" + "/Storage#Binding_parameters)"], + filename=traverser.filename, + line=traverser.line, + column=traverser.position, + context=traverser.context) + + +def createStatement(args, traverser, node, wrapper): + """ + Handles calls to `createStatement`, returning an object which emits + warnings upon calls to `execute` and `executeStep` rather than + `executeAsync`. + """ + _check_dynamic_sql(args, traverser) + from predefinedentities import build_quick_xpcom + return build_quick_xpcom("createInstance", "mozIStorageBaseStatement", + traverser, wrapper=True) + + +def executeSimpleSQL(args, traverser, node, wrapper): + """ + Handles calls to `executeSimpleSQL`, warning that asynchronous methods + should be used instead. + """ + _check_dynamic_sql(args, traverser) + traverser.err.warning( + err_id=("js", "instanceactions", "executeSimpleSQL"), + warning="Synchronous SQL should not be used", + description=SYNCHRONOUS_SQL_DESCRIPTION, + filename=traverser.filename, + line=traverser.line, + column=traverser.position, + context=traverser.context) + + def livemarkCallback(arguments, traverser, node, wrapper): """ Handle calls to addLivemark, removeLivemark and getLivemark that pass @@ -403,11 +417,12 @@ def was_called(arguments, traverser, node, wrapper): "createElement": createElement, "createElementNS": createElementNS, "createEvent": createEvent, + "createAsyncStatement": _check_dynamic_sql, "createStatement": createStatement, + "executeSimpleSQL": executeSimpleSQL, "getAsBinary": nsIDOMFile_deprec, "getAsDataURL": nsIDOMFile_deprec, "getInterface": getInterface, - "executeSimpleSQL": executeSimpleSQL, "insertAdjacentHTML": insertAdjacentHTML, "isSameNode": isSameNode, "openDialog": openDialog, diff --git a/validator/testcases/javascript/predefinedentities.py b/validator/testcases/javascript/predefinedentities.py index cda138bfe..471531964 100644 --- a/validator/testcases/javascript/predefinedentities.py +++ b/validator/testcases/javascript/predefinedentities.py @@ -5,6 +5,7 @@ import call_definitions from call_definitions import xpcom_constructor as xpcom_const, python_wrap from entity_values import entity +import instanceactions from jstypes import JSWrapper @@ -75,6 +76,12 @@ "code."} INTERFACES = { + u"mozIStorageBaseStatement": + {"value": + {u"execute": + {"dangerous": instanceactions.SYNCHRONOUS_SQL_DESCRIPTION}, + u"executeStep": + {"dangerous": instanceactions.SYNCHRONOUS_SQL_DESCRIPTION}}}, u"nsIExtensionManager": OBSOLETE_EXTENSION_MANAGER, u"nsIUpdateItem": OBSOLETE_EXTENSION_MANAGER, u"nsIInstallLocation": OBSOLETE_EXTENSION_MANAGER, @@ -574,14 +581,14 @@ def wrap(): INTERFACE_ENTITIES[interface] = {"xpcom_map": construct(interface)} -def build_quick_xpcom(method, interface, traverser): +def build_quick_xpcom(method, interface, traverser, wrapper=False): """A shortcut to quickly build XPCOM objects on the fly.""" constructor = xpcom_const(method, pretraversed=True) interface_obj = traverser._build_global( name=method, entity={"xpcom_map": lambda: INTERFACES[interface]}) obj = constructor(None, [interface_obj], traverser) - if isinstance(obj, JSWrapper): + if isinstance(obj, JSWrapper) and not wrapper: obj = obj.value return obj From 3c7ce14b6fb4d90cb48a821c0a431b6adc4d1d00 Mon Sep 17 00:00:00 2001 From: Kris Maglione Date: Fri, 12 Dec 2014 17:09:42 -0800 Subject: [PATCH 7/7] Update pref tests: flag proxy auto-config prefs, and don't flag extension sync prefs (bugs 1034298 and 1095899). --- validator/testcases/javascript/actions.py | 21 +++--- .../javascript/predefinedentities.py | 65 +++++++++++-------- validator/testcases/regex.py | 6 +- 3 files changed, 54 insertions(+), 38 deletions(-) diff --git a/validator/testcases/javascript/actions.py b/validator/testcases/javascript/actions.py index 228eb67f1..6b065726e 100644 --- a/validator/testcases/javascript/actions.py +++ b/validator/testcases/javascript/actions.py @@ -476,22 +476,25 @@ def _call_create_pref(a, t, e): value = str(t(a[0]).get_literal_value()) from predefinedentities import BANNED_PREF_BRANCHES, BANNED_PREF_REGEXPS - for banned in BANNED_PREF_BRANCHES: + for banned, reason in BANNED_PREF_BRANCHES: if value.startswith(banned): - return ("Extensions should not alter preferences in the '%s' " - "preference branch" % banned) + return reason or ("Extensions should not alter preferences in " + "the '%s' preference branch" % banned) for banned in BANNED_PREF_REGEXPS: if re.match(banned, value): return ("Extensions should not alter preferences matching /%s/" % banned) - if not value.startswith("extensions.") or value.rindex(".") < len("extensions."): - return ("Extensions should not alter preferences outside of the " - "'extensions.' preference branch. Please make sure that " - "all of your extension's preferences are prefixed with " - "'extensions.add-on-name.', where 'add-on-name' is a " - "distinct string unique to and indicative of your add-on.") + for branch in "extensions.", "services.sync.prefs.sync.extensions.": + if value.startswith(branch) and value.rindex(".") > len(branch): + return + + return ("Extensions should not alter preferences outside of the " + "'extensions.' preference branch. Please make sure that " + "all of your extension's preferences are prefixed with " + "'extensions.add-on-name.', where 'add-on-name' is a " + "distinct string unique to and indicative of your add-on.") def _readonly_top(t, r, rn): diff --git a/validator/testcases/javascript/predefinedentities.py b/validator/testcases/javascript/predefinedentities.py index 471531964..ae270d4bd 100644 --- a/validator/testcases/javascript/predefinedentities.py +++ b/validator/testcases/javascript/predefinedentities.py @@ -19,32 +19,45 @@ "instead wherever possible", } -BANNED_PREF_BRANCHES = [ - u"browser.newtab.url", - u"browser.newtabpage.enabled", - u"browser.preferences.instantApply", - u"browser.search.defaultenginename", - u"browser.search.searchEnginesURL", - u"browser.startup.homepage", - u"capability.policy.", - u"extensions.alwaysUnpack", - u"extensions.blocklist.", - u"extensions.bootstrappedAddons", - u"extensions.checkCompatibility", - u"extensions.dss.", - u"extensions.getAddons.", - u"extensions.getMoreThemesURL", - u"extensions.installCache", - u"extensions.lastAppVersion", - u"extensions.pendingOperations", - u"extensions.update.", - u"general.useragent.", - u"keyword.URL", - u"keyword.enabled", - u"network.http.", - u"network.websocket.", - u"nglayout.debug.disable_xul_cache", -] +CUSTOMIZATION_PREF_MESSAGE = ( + "Extensions must not alter user preferences such as the current home " + "page, new tab page, or search engine, without explicit user consent, " + "in which a user takes a non-default action. Such changes must also " + "be reverted when the extension is disabled or uninstalled.") + +BANNED_PREF_BRANCHES = ( + (u"browser.newtab.url", CUSTOMIZATION_PREF_MESSAGE), + (u"browser.newtabpage.enabled", CUSTOMIZATION_PREF_MESSAGE), + (u"browser.preferences.instantApply", None), + (u"browser.search.defaultenginename", CUSTOMIZATION_PREF_MESSAGE), + (u"browser.search.searchEnginesURL", CUSTOMIZATION_PREF_MESSAGE), + (u"browser.startup.homepage", CUSTOMIZATION_PREF_MESSAGE), + (u"capability.policy.", None), + (u"extensions.alwaysUnpack", None), + (u"extensions.blocklist.", None), + (u"extensions.bootstrappedAddons", None), + (u"extensions.checkCompatibility", None), + (u"extensions.dss.", None), + (u"extensions.getAddons.", None), + (u"extensions.getMoreThemesURL", None), + (u"extensions.installCache", None), + (u"extensions.lastAppVersion", None), + (u"extensions.pendingOperations", None), + (u"extensions.update.", None), + (u"general.useragent.", None), + (u"keyword.URL", CUSTOMIZATION_PREF_MESSAGE), + (u"keyword.enabled", CUSTOMIZATION_PREF_MESSAGE), + (u"network.proxy.autoconfig_url", + "As many add-ons have reason to change the proxy autoconfig URL, and " + "only one at a time may do so without conflict, extensions must " + "make proxy changes using other mechanisms. Installing a proxy " + "filter is the recommended alternative: " + "https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/" + "Reference/Interface/nsIProtocolProxyService#registerFilter()"), + (u"network.http.", None), + (u"network.websocket.", None), + (u"nglayout.debug.disable_xul_cache", None), +) BANNED_PREF_REGEXPS = [ r"extensions\..*\.update\.(url|enabled|interval)", diff --git a/validator/testcases/regex.py b/validator/testcases/regex.py index 88cccdaee..4fa45d008 100644 --- a/validator/testcases/regex.py +++ b/validator/testcases/regex.py @@ -291,12 +291,12 @@ def tests(self): "Extensions should not alter preferences matching /%s/." % pattern) - for branch in BANNED_PREF_BRANCHES: + for branch, reason in BANNED_PREF_BRANCHES: yield self.get_test( branch.replace(r".", r"\."), "Potentially unsafe preference branch referenced", - "Extensions should not alter preferences in the `%s` " - "preference branch" % branch) + reason or ("Extensions should not alter preferences in " + "the `%s` preference branch" % branch)) REQUIRE_PATTERN = (r"""(?