Browse files

Flag dangerous document.write calls (bug 679504)

  • Loading branch information...
1 parent c3a3c9c commit f16604e405a8e51d97714e9d4f35821c684c9729 @mattbasta committed Jan 2, 2013
View
62 tests/test_js_instanceproperties.py
@@ -5,6 +5,12 @@
from js_helper import _do_test_raw, TestCase
+_DANGEROUS_STRINGS = ['"x" + y',
+ '"<div onclick=\\"foo\\"></div>"',
+ '"<a href=\\"javascript:alert();\\">"',
+ '"<script>"']
+
+
def test_innerHTML():
"""Tests that the dev can't define event handlers in innerHTML."""
@@ -13,26 +19,16 @@ def test_innerHTML():
x.innerHTML = "<div></div>";
""").failed()
- assert _do_test_raw("""
- var x = foo();
- x.innerHTML = "<div onclick=\\"foo\\"></div>";
- """).failed()
-
- # Test without declaration
- assert _do_test_raw("""
- x.innerHTML = "<div onclick=\\"foo\\"></div>";
- """).failed()
-
- assert _do_test_raw("""
- var x = foo();
- x.innerHTML = "x" + y;
- """).failed()
+ for pattern in _DANGEROUS_STRINGS:
- assert _do_test_raw("""x.innerHTML = "<script>";""").failed()
+ assert _do_test_raw("""
+ var x = foo();
+ x.innerHTML = %s;
+ """ % pattern).failed(), pattern
- assert _do_test_raw("""
- x.innerHTML = '<a href="javascript:alert();">';
- """).failed()
+ assert _do_test_raw("""
+ x.innerHTML = %s;
+ """ % pattern).failed(), pattern
def test_outerHTML():
@@ -43,20 +39,26 @@ def test_outerHTML():
x.outerHTML = "<div></div>";
""").failed()
- assert _do_test_raw("""
- var x = foo();
- x.outerHTML = "<div onclick=\\"foo\\"></div>";
- """).failed()
+ for pattern in _DANGEROUS_STRINGS:
- # Test without declaration
- assert _do_test_raw("""
- x.outerHTML = "<div onclick=\\"foo\\"></div>";
- """).failed()
+ assert _do_test_raw("""
+ var x = foo();
+ x.outerHTML = %s;
+ """ % pattern).failed(), pattern
- assert _do_test_raw("""
- var x = foo();
- x.outerHTML = "x" + y;
- """).failed()
+ assert _do_test_raw("""
+ x.outerHTML = %s;
+ """ % pattern).failed(), pattern
+
+
+def test_document_write():
+ """Test that the dev can't define event handler in outerHTML."""
+
+ assert not _do_test_raw("""document.write("<div></div>");""").failed()
+
+ for pattern in _DANGEROUS_STRINGS:
+ assert _do_test_raw(
+ """document.write(%s);""" % pattern).failed(), pattern
def _mock_html_error(self, *args, **kwargs):
View
5 validator/constants.py
@@ -2,6 +2,7 @@
import json
import os
+import re
import types
@@ -59,6 +60,10 @@
# package.
MAX_JS_THRESHOLD = 900
+# The pattern that matches event assignments
+# TODO(valcom): Move this to valcom when that's a thing.
+EVENT_ASSIGNMENT = re.compile("<.+ on[a-z]+=")
+
# Graciously provided by @kumar in bug 614574
if (not SPIDERMONKEY_INSTALLATION or
not os.path.exists(SPIDERMONKEY_INSTALLATION)):
View
12 validator/testcases/javascript/entity_values.py
@@ -1,4 +1,5 @@
from call_definitions import open_in_chrome_context
+from instanceproperties import _set_HTML_property
from validator.compat import (FX10_DEFINITION, FX14_DEFINITION, FX16_DEFINITION,
TB14_DEFINITION, TB15_DEFINITION, TB16_DEFINITION)
from validator.constants import BUGZILLA_BUG
@@ -60,6 +61,17 @@ def wrap(traverser):
message=JAVA_MESSAGE, bug=748343)
+@register_entity("document.write")
+def document_write(traverser):
+ def on_write(wrapper, arguments, traverser):
+ if not arguments:
+ return
+ value = traverser._traverse_node(arguments[0])
+ _set_HTML_property('document.write()', value, traverser)
+
+ return {"return": on_write}
+
+
@register_entity("document.xmlEncoding")
def xmlEncoding(traverser):
traverser.err.error(
View
9 validator/testcases/javascript/instanceproperties.py
@@ -2,11 +2,10 @@
import types
from validator.compat import FX10_DEFINITION, FX13_DEFINITION, FX18_DEFINITION
-from validator.constants import BUGZILLA_BUG
+from validator.constants import BUGZILLA_BUG, EVENT_ASSIGNMENT
import jstypes
-EVENT_ASSIGNMENT = re.compile("<.+ on[a-z]+=")
JS_URL = re.compile("href=[\'\"]javascript:")
@@ -20,6 +19,7 @@ def set_outerHTML(new_value, traverser):
return _set_HTML_property("outerHTML", new_value, traverser)
+# TODO(valcom): Make this generic and put it in utils
def _set_HTML_property(function, new_value, traverser):
if not isinstance(new_value, jstypes.JSWrapper):
new_value = jstypes.JSWrapper(new_value, traverser=traverser)
@@ -73,8 +73,9 @@ def _set_HTML_property(function, new_value, traverser):
# Variable assignments
traverser.err.warning(
err_id=("testcases_javascript_instancetypes", "set_%s" % function,
- "variable_assignment"),
- warning="%s should not be set dynamically" % function,
+ "variable_assignment"),
+ warning="Markup should not be passed to `%s` dynamically." %
+ function,
description="Due to both security and performance reasons, "
"%s should not be set using dynamic "
"values. This can lead to security issues or "
View
23 validator/testcases/javascript/predefinedentities.py
@@ -65,13 +65,13 @@
"Authors of bootstrapped add-ons must take care to "
"clean up any added category entries at shutdown.")}}},
u"nsIAbLDAPDirectory":
- {"value":
+ {"value":
{u"replicationFile": entity("nsIAbLDAPDirectory.replicationFile"),
u"databaseFile": entity("nsIAbLDAPDirectory.databaseFile")}},
u"nsIAbManager":
- {"value":
- {u"userProfileDirectory":
- entity("nsIAbManager.userProfileDirectory")}},
+ {"value":
+ {u"userProfileDirectory":
+ entity("nsIAbManager.userProfileDirectory")}},
u"nsIAccessibleRetrieval":
{"dangerous":
"Using the nsIAccessibleRetrieval interface causes significant "
@@ -135,11 +135,11 @@
{"return": call_definitions.nsIMailtoUrl_changed}}},
u"nsIMessenger":
{"value":
- {u"saveAttachmentToFolder":
+ {u"saveAttachmentToFolder":
entity("nsIMessenger.saveAttachmentToFolder")}},
u"nsIMsgAccountManager":
{"value":
- {u"folderUriForPath":
+ {u"folderUriForPath":
entity("nsIMsgAccountManager.folderUriForPath")}},
u"nsIMsgLocalMailFolder":
{"value":
@@ -178,7 +178,7 @@
{"value": call_definitions.nsIMsgFolder_changed},
u"filePath": entity("nsIMsgFolder.filePath")}},
u"nsIMsgIdentity":
- {"value":
+ {"value":
{u"signature": entity("nsIMsgIdentity.signature")}},
u"nsIMsgIncomingServer":
{"value":
@@ -232,7 +232,7 @@
{"return": call_definitions.nsIMsgThread_removed}}},
u"nsINoIncomingServer":
{"value":
- {u"copyDefaultMessages":
+ {u"copyDefaultMessages":
entity("nsINoIncomingServer.copyDefaultMessages")}},
u"nsIObserverService":
{"value":
@@ -254,10 +254,10 @@
"to clean up any added resource substitutions "
"at shutdown."}}},
u"nsIRssIncomingServer":
- {"value":
- {u"subscriptionsDataSourcePath":
+ {"value":
+ {u"subscriptionsDataSourcePath":
entity("nsIRssIncomingServer.subscriptionsDataSourcePath"),
- u"feedItemsDataSourcePath":
+ u"feedItemsDataSourcePath":
entity("nsIRssIncomingServer.feedItemsDataSourcePath")}},
u"nsIStringBundleService":
{"value":
@@ -415,6 +415,7 @@ def build_quick_xpcom(method, interface, traverser):
lambda a, t, e:
not a or not _get_as_str(t(a[0])).lower()
.startswith(("chrome:", "resource:"))},
+ u"write": entity("document.write"),
u"xmlEncoding": entity("document.xmlEncoding"),
u"xmlVersion": entity("document.xmlVersion"),
u"xmlStandalone": entity("document.xmlStandalone")}},

0 comments on commit f16604e

Please sign in to comment.