Permalink
Browse files

Finalize remote reference ban for themes (bug 664823)

  • Loading branch information...
mattbasta committed Jan 31, 2013
1 parent 140cef7 commit 35bf0dc769cbd8a248c6a56d79a812411999697a
View
@@ -1,6 +1,8 @@
-import validator.testcases.themes as themes
-from validator.errorbundler import ErrorBundle
from validator.constants import PACKAGE_THEME
+from validator.errorbundler import ErrorBundle
+import validator.testcases.markup.csstester as csstester
+import validator.testcases.themes as themes
+
from helper import _do_test
from js_helper import _do_real_test_raw
@@ -33,3 +35,19 @@ def test_js_banned():
print err.print_summary(verbose=True)
assert err.failed()
+
+def test_remote_css():
+ """Test that remote CSS references are flagged."""
+
+ snippet = """
+ x {foo: url(http://foo.com/bar);}
+ """
+
+ err = ErrorBundle()
+ csstester.test_css_snippet(err, "x.css", snippet, 0)
+ assert not err.failed()
+
+ err = ErrorBundle()
+ err.detected_type = PACKAGE_THEME
+ csstester.test_css_snippet(err, "x.css", snippet, 0)
+ assert err.failed()
@@ -7,8 +7,9 @@
from validator.contextgenerator import ContextGenerator
-BAD_URL_PAT = "url\(['\"]?(?!(chrome:|resource:))(\/\/|(ht|f)tps?:\/\/|data:)[a-z0-9\/\-\.#]*['\"]?\)"
+BAD_URL_PAT = "url\(['\"]?(?!(chrome:|resource:))(\/\/|(ht|f)tps?:\/\/|data:).*['\"]?\)"
BAD_URL = re.compile(BAD_URL_PAT, re.I)
+REM_URL = re.compile("url\(['\"]?(\/\/|ht|f)tps?:\/\/.*['\"]?\)", re.I)
SKIP_TYPES = ("S", "COMMENT")
@@ -27,7 +28,8 @@ def test_css_file(err, filename, data, line_start=1):
tokenizer = cssutils.tokenize2.Tokenizer()
context = ContextGenerator(data)
- data = "".join(c for c in data if 8 < ord(c) < 127)
+ if data:
+ data = "".join(c for c in data if 8 < ord(c) < 127)
token_generator = tokenizer.tokenize(data)
@@ -101,21 +103,29 @@ def _run_css_tests(err, tokens, filename, line_start=0, context=None):
# If we hit a URI after -moz-binding, we may have a
# potential security issue.
- if last_descriptor == "-moz-binding":
+ if last_descriptor == "-moz-binding" and BAD_URL.match(value):
# We need to make sure the URI is not remote.
- if BAD_URL.match(value):
- err.warning(("testcases_markup_csstester",
- "_run_css_tests",
- "-moz-binding_external"),
- "Cannot reference external scripts.",
- "-moz-binding cannot reference external "
+ err.warning(
+ err_id=("css", "_run_css_tests", "-moz-binding_external"),
+ warning="Cannot reference external scripts",
+ description="`-moz-binding` cannot reference external "
"scripts in CSS. This is considered to be a "
"security issue. The script file must be "
"placed in the /content/ directory of the "
"package.",
- filename,
- line=line + line_start,
- context=context.get_context(line))
+ filename=filename,
+ line=line + line_start,
+ context=context.get_context(line))
+ elif err.detected_type == PACKAGE_THEME and REM_URL.match(value):
+ err.warning(
+ err_id=("css", "_run_css_tests", "remote_url"),
+ warning="Themes may not reference remote resources",

This comment has been minimized.

Show comment Hide comment
@cvan

cvan Jan 31, 2013

Member

you mean "complete themes"?

@cvan

cvan Jan 31, 2013

Member

you mean "complete themes"?

This comment has been minimized.

Show comment Hide comment
@mattbasta

mattbasta Jan 31, 2013

Contributor

Both

@mattbasta

mattbasta Jan 31, 2013

Contributor

Both

+ description="Themes may not reference resources that are "
+ "stored on remote servers. All resources must "
+ "be stored locally within the theme.",
+ filename=filename,
+ line=line + line_start,
+ context=context.get_context(line))
elif tok_type == "HASH":
# Search for interference with the identity box.
@@ -34,6 +34,10 @@
GENERIC_IDS = ("string-bundle", "strings", )
+REMOTE_URL_PATTERN = re.compile("(ht|f)tps?://")
+_markedsectionclose = re.compile(r']\s*]\s*>')
+
+
class MarkupParser(htmlparser.HTMLParser):
"""Parse and analyze the versious components of markup files."""
@@ -136,23 +140,22 @@ def _feed_parser(self, line):
self.debug and "testscript" in self.xml_state):
if "script_comments" in self.reported or not self.strict:
return
- self.err.notice(("testcases_markup_markuptester",
- "_feed",
- "missing_script_comments"),
- "Missing comments in <script> tag",
- "Markup parsing errors occurred while trying "
+ self.err.notice(
+ err_id=("markup", "_feed", "missing_script_comments"),
+ notice="Missing comments in <script> tag",
+ description="Markup parsing errors occurred while trying "
"to parse the file. This would likely be "
"mitigated by wrapping <script> tag contents "
"in HTML comment tags (<!-- -->)",
- self.filename,
- line=self.line,
- context=self.context,
- tier=2)
+ filename=self.filename,
+ line=self.line,
+ context=self.context,
+ tier=2)
self.reported.add("script_comments")
return
if self.strict:
- self.err.warning(("testcases_markup_markuptester",
+ self.err.warning(("markup",
"_feed",
"parse_error"),
"Markup parsing error",
@@ -188,9 +191,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
# A fictional tag for testing purposes.
if tag == "xbannedxtestx":
self.err.error(
- err_id=("testcases_markup_markuptester",
- "handle_starttag",
- "banned_element"),
+ err_id=("markup", "starttag", "banned_element"),
error="Banned markup element",
description="A banned markup element was found.",
filename=self.filename,
@@ -207,8 +208,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
if self.xbl:
if tag in UNSAFE_THEME_XBL:
self.err.warning(
- err_id=("testcases_markup_markuptester",
- "handle_starttag",
+ err_id=("markup", "starttag",
"unsafe_theme_xbl_element"),
warning="Banned XBL element in theme.",
description=["Certain XBL elements are disallowed in "
@@ -221,8 +221,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
elif (tag == "property" and
any(a[0] in (u"onset", u"onget") for a in attrs)):
self.err.warning(
- err_id=("testcases_markup_markuptester",
- "handle_starttag",
+ err_id=("markup", "starttag",
"theme_xbl_property"),
warning="Themes are not allowed to use XBL properties",
description="XBL properties cannot be used in themes.",
@@ -236,9 +235,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
(self.err.detected_type == PACKAGE_THEME and
tag in UNSAFE_THEME_TAGS)):
self.err.warning(
- err_id=("testcases_markup_markuptester",
- "handle_starttag",
- "unsafe_langpack_theme"),
+ err_id=("markup", "starttag", "unsafe_langpack_theme"),
warning="Unsafe tag for add-on type",
description=["A tag in your markup has been marked as "
"being potentially unsafe. Consider "
@@ -248,22 +245,18 @@ def handle_starttag(self, tag, attrs, self_closing=False):
filename=self.filename,
line=self.line,
context=self.context)
- if DEBUG: # pragma: no cover
- print "Unsafe Tag ------"
# Make sure all src/href attributes are local
- for attr in attrs:
- if attr[0].lower() in ("src", "href") and \
- not self._is_url_local(attr[1].lower()):
- self.err.warning(("testcases_markup_markuptester",
- "handle_starttag",
- "remote_src_href"),
- "src/href attributes must be local.",
- "Language packs require that all src and "
- "href attributes are relative URLs.",
- self.filename,
- line=self.line,
- context=self.context)
+ if any(not self._is_url_local(attr[1]) for attr in attrs if
+ attr[0] in ("src", "href")):
+ self.err.warning(
+ err_id=("markup", "starttag", "remote_src_href"),
+ warning="`src`/`href` attributes must be local.",
+ description="Themes and language packs may not reference "
+ "remote resources.",
+ filename=self.filename,
+ line=self.line,
+ context=self.context)
if tag == "prefwindow":
# Flag <prefwindow> elements without IDs.
@@ -302,9 +295,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
if (type_ and
not (type_ in SAFE_IFRAME_TYPES or
not remote_src)):
- self.err.warning(("testcases_markup_markuptester",
- "handle_starttag",
- "iframe_type_unsafe"),
+ self.err.warning(("markup", "starttag", "iframe_type_unsafe"),
"iframe/browser missing 'type' attribute",
"All iframe and browser elements must have "
"either a valid `type` attribute or a `src` "
@@ -315,9 +306,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
elif ((not type_ or
type_ not in SAFE_IFRAME_TYPES) and
remote_src):
- self.err.warning(("testcases_markup_markuptester",
- "handle_starttag",
- "iframe_type_unsafe"),
+ self.err.warning(("markup", "starttag", "iframe_type_unsafe"),
"Typeless iframes/browsers must be local.",
"iframe and browser elements that lack a type "
"attribute must always have src attributes "
@@ -339,8 +328,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
if src:
if not self._is_url_local(src):
self.err.warning(
- err_id=("testcases_markup_markuptester",
- "handle_starttag",
+ err_id=("markup", "starttag",
"banned_remote_scripts"),
warning="Scripts must not be remote in XUL",
description="In XUL, <script> tags must not be "
@@ -370,8 +358,8 @@ def handle_starttag(self, tag, attrs, self_closing=False):
attr_value.startswith("resource://") and
"-data/" in attr_value):
self.err.warning(
- err_id=("testcases_markup_markuptester",
- "handle_starttag", "jetpack_abs_uri"),
+ err_id=("markup", "starttag",
+ "jetpack_abs_uri"),
warning="Absolute URI referenced in Jetpack 1.4",
description=["As of Jetpack 1.4, absolute URIs are no "
"longer allowed within add-ons.",
@@ -386,8 +374,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
attr_value.startswith(("data:", "javascript:"))):
self.err.warning(
- err_id=("testcases_markup_markuptester",
- "handle_starttag",
+ err_id=("markup", "starttag",
"theme_attr_prefix"),
warning="Attribute contains banned prefix",
description=["A mark element's attribute contains a "
@@ -406,8 +393,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
# Warn about DOM mutation event handlers.
if attr_name in DOM_MUTATION_HANDLERS:
self.err.warning(
- err_id=("testcases_markup_markuptester",
- "handle_starttag",
+ err_id=("markup", "starttag",
"dom_manipulation_handler"),
warning="DOM Mutation Events Prohibited",
description="DOM mutation events are flagged because "
@@ -418,11 +404,10 @@ def handle_starttag(self, tag, attrs, self_closing=False):
line=self.line,
context=self.context)
- scripting.test_js_snippet(err=self.err,
- data=attr_value,
- filename=self.filename,
- line=self.line,
- context=self.context)
+ scripting.test_js_snippet(
+ err=self.err, data=attr_value,
+ filename=self.filename, line=self.line,
+ context=self.context)
elif (self.extension == "xul" and
attr_name in ("insertbefore", "insertafter") and
@@ -431,8 +416,7 @@ def handle_starttag(self, tag, attrs, self_closing=False):
"javascriptConsole",
"webConsole"))):
self.err.notice(
- err_id=("testcases_markup_markuptester",
- "handle_starttag",
+ err_id=("markup", "starttag",
"incompatible_menu_items"),
notice="Menu item has been moved",
description="Your add-on has an overlay that uses the "
@@ -453,8 +437,8 @@ def handle_starttag(self, tag, attrs, self_closing=False):
# Test for generic IDs
if attr_name == "id" and attr_value in GENERIC_IDS:
self.err.warning(
- err_id=("testcases_markup_markuptester",
- "handle_starttag", "generic_ids"),
+ err_id=("markup",
+ "starttag", "generic_ids"),
warning="Overlay contains generically-named IDs",
description="An overlay is using a generically-named ID "
"that could cause compatibility problems with "
@@ -489,8 +473,8 @@ def handle_endtag(self, tag):
if DEBUG:
print "Unstrict; extra closing tags ------"
return
- self.err.warning(("testcases_markup_markuptester",
- "handle_endtag",
+ self.err.warning(("markup",
+ "endtag",
"extra_closing_tags"),
"Markup parsing error",
"The markup file has more closing tags than it "
@@ -516,8 +500,8 @@ def handle_endtag(self, tag):
elif tag not in self.xml_state:
# If the tag we're processing isn't on the stack, then
# something is wrong.
- self.err.warning(("testcases_markup_markuptester",
- "handle_endtag",
+ self.err.warning(("markup",
+ "endtag",
"extra_closing_tags"),
"Parse error: tag closed before opened",
["Markup tags cannot be closed before they are "
@@ -550,8 +534,8 @@ def handle_endtag(self, tag):
self.extension[0] == 'x' and
not self.strict):
- self.err.warning(("testcases_markup_markuptester",
- "handle_endtag",
+ self.err.warning(("markup",
+ "endtag",
"invalid_nesting"),
"Markup invalidly nested",
"It has been determined that the document "
@@ -584,7 +568,6 @@ def handle_comment(self, data):
def parse_marked_section(self, i, report=0):
rawdata = self.rawdata
- _markedsectionclose = re.compile(r']\s*]\s*>')
assert rawdata[i:i + 3] == '<![', \
"unexpected call to parse_marked_section()"
@@ -630,13 +613,10 @@ def _format_args(self, args):
return " " + " ".join(output)
def _is_url_local(self, url):
-
+ url = url.lower()
if url.startswith("chrome://"):
return True
-
- pattern = re.compile("(ht|f)tps?://")
-
- return not pattern.match(url)
+ return not REMOTE_URL_PATTERN.match(url)
# Code to fix for Python issue 670664
@@ -15,17 +15,16 @@ def test_theme_manifest(err, xpi_package=None):
for triple in chrome.triples:
subject = triple["subject"]
# Test to make sure that the triple's subject is valid
- if subject not in ("skin",
- "style"):
- err.warning(("testcases_themes",
- "test_theme_manifest",
- "invalid_chrome_manifest_subject"),
- "Invalid chrome.manifest subject.",
- ["chrome.manifest files for themes are only allowed to "
- "have 'skin' and 'style' items. Other types of items "
- "are disallowed for security reasons.",
- "Invalid subject: %s" % subject],
- filename=triple["filename"],
- line=triple["line"],
- context=triple["context"])
-
+ if subject not in ("skin", "style"):
+ err.warning(
+ err_id=("themes", "test_theme_manifest",
+ "invalid_chrome_manifest_subject"),
+ warning="Invalid chrome.manifest subject",
+ description=["chrome.manifest files for themes are only "
+ "allowed to have 'skin' and 'style' items. "
+ "Other types of items are disallowed for "
+ "security reasons.",
+ "Invalid subject: %s" % subject],
+ filename=triple["filename"],
+ line=triple["line"],
+ context=triple["context"])

0 comments on commit 35bf0dc

Please sign in to comment.