Merge pull request #12048 from edx/robrap/linter-revert-include

TNL-4345: Remove include linting

openedx/core/djangolib/js_utils.py

@@ -74,45 +74,6 @@ def dump_js_escaped_json(obj, cls=EdxJSONEncoder):
     return json_string
 
 
-def dump_html_escaped_json(obj, cls=EdxJSONEncoder):
-    """
-    JSON dumps and escapes objects that are safe to be embedded in HTML.
-
-    Use this for anything but strings (e.g. dicts, tuples, lists, bools, and
-    numbers).  For strings, just used the default html filter.
-
-    Usage:
-        Used as follows in a Mako template inside a HTML, like in
-        a data attribute::
-
-            data-obj='${obj | n, dump_html_escaped_json}'
-
-        If you must use the cls argument, then use as follows::
-
-            data-obj='${dump_html_escaped_json(obj, cls) | n}'
-
-        Use the "n" Mako filter above.  The default filter will include
-        html escaping in the future, and this ensures proper ordering of
-        these calls.
-
-        Ensure ascii in json.dumps (ensure_ascii=True) allows safe skipping of
-        Mako's default filter decode.utf8.
-
-    Arguments:
-        obj: The object soon to become an HTML escaped JSON string.  The object
-            can be anything but strings (e.g. dicts, tuples, lists, bools, and
-            numbers).
-        cls (class): The JSON encoder class (defaults to EdxJSONEncoder).
-
-    Returns:
-        (string) Escaped encoded JSON.
-
-    """
-    json_string = json.dumps(obj, ensure_ascii=True, cls=cls)
-    json_string = escape(json_string)
-    return json_string
-
-
 def js_escaped_string(string_for_js):
     """
     Mako filter that escapes text for use in a JavaScript string.

openedx/core/djangolib/tests/test_js_utils.py

@@ -10,7 +10,7 @@ import HTMLParser
 from mako.template import Template
 
 from openedx.core.djangolib.js_utils import (
-    dump_js_escaped_json, dump_html_escaped_json, js_escaped_string
+    dump_js_escaped_json, js_escaped_string
 )
 
 
@@ -68,38 +68,6 @@ class TestJSUtils(TestCase):
         escaped_json = dump_js_escaped_json(malicious_dict, cls=self.SampleJSONEncoder)
         self.assertEquals(expected_custom_escaped_json, escaped_json)
 
-    def test_dump_html_escaped_json_escapes_unsafe_html(self):
-        """
-        Test dump_html_escaped_json properly escapes &, <, and >.
-        """
-        malicious_dict = {"</script><script>alert('hello, ');</script>": "</script><script>alert('&world!');</script>"}
-        expected_escaped_json = (
-            "{&#34;&lt;/script&gt;&lt;script&gt;alert(&#39;hello, &#39;);&lt;/script&gt;&#34;: "
-            "&#34;&lt;/script&gt;&lt;script&gt;alert(&#39;&amp;world!&#39;);&lt;/script&gt;&#34;}"
-        )
-
-        escaped_json = dump_html_escaped_json(malicious_dict)
-        self.assertEquals(expected_escaped_json, escaped_json)
-
-    def test_dump_html_escaped_json_with_custom_encoder_escapes_unsafe_html(self):
-        """
-        Test dump_html_escaped_json first encodes with custom JSNOEncoder before escaping &, <, and >
-
-        The test encoder class should first perform the replacement of "<script>" with
-        "sample-encoder-was-here", and then should escape the remaining &, <, and >.
-
-        """
-        malicious_dict = {
-            "</script><script>alert('hello, ');</script>":
-            self.NoDefaultEncoding("</script><script>alert('&world!');</script>")
-        }
-        expected_custom_escaped_json = (
-            "{&#34;&lt;/script&gt;&lt;script&gt;alert(&#39;hello, &#39;);&lt;/script&gt;&#34;: "
-            "&#34;&lt;/script&gt;sample-encoder-was-herealert(&#39;&amp;world!&#39;);&lt;/script&gt;&#34;}"
-        )
-        escaped_json = dump_html_escaped_json(malicious_dict, cls=self.SampleJSONEncoder)
-        self.assertEquals(expected_custom_escaped_json, escaped_json)
-
     def test_js_escaped_string_escapes_unsafe_html(self):
         """
         Test js_escaped_string escapes &, <, and >, as well as returns a unicode type
@@ -139,17 +107,18 @@ class TestJSUtils(TestCase):
         template = Template(
             """
                 <%!
+                import json
                 from openedx.core.djangolib.js_utils import (
-                    dump_js_escaped_json, dump_html_escaped_json, js_escaped_string
+                    dump_js_escaped_json, js_escaped_string
                 )
                 %>
                 <body>
                     <div
-                        data-test-dict='${test_dict | n, dump_html_escaped_json}'
+                        data-test-dict='${json.dumps(test_dict)}'
                         data-test-string='${test_dict["test_string"]}'
-                        data-test-tuple='${test_dict["test_tuple"] | n, dump_html_escaped_json}'
-                        data-test-number='${test_dict["test_number"] | n, dump_html_escaped_json}'
-                        data-test-bool='${test_dict["test_bool"] | n, dump_html_escaped_json}'
+                        data-test-tuple='${json.dumps(test_dict["test_tuple"])}'
+                        data-test-number='${json.dumps(test_dict["test_number"])}'
+                        data-test-bool='${json.dumps(test_dict["test_bool"])}'
                     ></div>
 
                     <script>

scripts/safe_template_linter.py

@@ -205,10 +205,6 @@ class Rules(Enum):
         'mako-multiple-page-tags',
         'A Mako template can only have one <%page> tag.'
     )
-    mako_include_with_violations = (
-        'mako-include-with-violations',
-        'Must fix violations in included templates first.'
-    )
     mako_unparsable_expression = (
         'mako-unparsable-expression',
         'The expression could not be properly parsed.'
@@ -446,42 +442,6 @@ class FileResults(object):
         self.directory = os.path.dirname(full_path)
         self.is_file = os.path.isfile(full_path)
         self.violations = []
-        self.includes = []
-
-    def resolve_include(self, include, include_results):
-        """
-        Resolves potential include violations and determines if they are real or
-        not. For real violations, adds the violations into the violation list.
-
-        If the include_results is not a file, it will be considered a violation
-        and will require a disable pragma.
-
-        Arguments:
-            include: The include with the potential violation to be resolved.
-            include_results: The results of processing the include file.
-
-        """
-        include_has_violations = (not include_results.is_file) or (len(include_results.violations) > 0)
-        if include_has_violations:
-            self.violations.append(include['potential_violation'])
-
-    def add_include(self, include_file, potential_violation):
-        """
-        Adds an include which also must have no violations.
-
-        Arguments:
-            include_file: The include file as provided in an include.
-            potential_violation: Represents the violation of the include, if the
-                included file has any violations.
-
-        """
-        include_full_path = os.path.normpath(self.directory + '/' + include_file)
-        self.includes.append({
-            'directory': os.path.dirname(include_full_path),
-            'file_name': os.path.split(include_full_path)[1],
-            'full_path': include_full_path,
-            'potential_violation': potential_violation,
-        })
 
     def prepare_results(self, file_string):
         """
@@ -494,8 +454,6 @@ class FileResults(object):
         string_lines = StringLines(file_string)
         for violation in self.violations:
             violation.prepare_results(self.full_path, string_lines)
-        for include in self.includes:
-            include['potential_violation'].prepare_results(self.full_path, string_lines)
 
     def print_results(self, options, out):
         """
@@ -524,18 +482,6 @@ class MakoTemplateLinter(object):
 
     _skip_mako_dirs = _skip_dirs
 
-    def __init__(self):
-        """
-        Init method.
-        """
-        self.results = {}
-
-    def supports_includes(self):
-        """
-        Mako template linter supports linting includes.
-        """
-        return True
-
     def process_file(self, directory, file_name):
         """
         Process file to determine if it is a Mako template file and
@@ -552,13 +498,6 @@ class MakoTemplateLinter(object):
         mako_file_full_path = os.path.normpath(directory + '/' + file_name)
         results = FileResults(mako_file_full_path)
 
-        # don't process the same file twice. this could happen when we process
-        # files included by another file
-        if mako_file_full_path in self.results:
-            return self.results[mako_file_full_path]
-
-        self.results[mako_file_full_path] = results
-
         if not results.is_file:
             return results
 
@@ -633,7 +572,6 @@ class MakoTemplateLinter(object):
             if not has_page_default:
                 results.violations.append(RuleViolation(Rules.mako_missing_default))
         self._check_mako_expressions(mako_template, has_page_default, results)
-        self._check_include_files(mako_template, results)
         results.prepare_results(mako_template)
 
     def _is_django_template(self, mako_template):
@@ -675,30 +613,6 @@ class MakoTemplateLinter(object):
         page_match = page_h_filter_regex.search(mako_template)
         return page_match
 
-    def _check_include_files(self, mako_template, results):
-        """
-        Checks if the Mako template includes other template files. If so, sets
-        up potential violations that will be checked later.
-
-        Arguments:
-            mako_template: The contents of the Mako template.
-
-        """
-        regex = re.compile('<%include[^>]*file=(?:"|\')(.+?)(?:"|\')[^>]*/>')
-        for match in regex.finditer(mako_template):
-            include_file = match.group(1)
-            expression = {
-                'start_index': match.start(),
-                'end_index': match.end(),
-                'expression': match.group()
-            }
-            results.add_include(
-                include_file,
-                ExpressionRuleViolation(
-                    Rules.mako_include_with_violations, expression
-                )
-            )
-
     def _check_mako_expressions(self, mako_template, has_page_default, results):
         """
         Searches for Mako expressions and then checks if they contain
@@ -775,9 +689,6 @@ class MakoTemplateLinter(object):
                     results.violations.append(ExpressionRuleViolation(
                         Rules.mako_unwanted_html_filter, expression
                     ))
-            elif (len(filters) == 2) and (filters[0] == 'n') and (filters[1] == 'dump_html_escaped_json'):
-                # {x | n, dump_html_escaped_json} is valid
-                pass
             else:
                 results.violations.append(ExpressionRuleViolation(
                     Rules.mako_invalid_html_filter, expression
@@ -948,12 +859,6 @@ class UnderscoreTemplateLinter(object):
 
     _skip_underscore_dirs = _skip_dirs + ('test',)
 
-    def supports_includes(self):
-        """
-        Underscore template linter does not lint includes.
-        """
-        return False
-
     def process_file(self, directory, file_name):
         """
         Process file to determine if it is an Underscore template file and
@@ -1114,13 +1019,6 @@ def _process_current_walk(current_walk, template_linters, options, out):
         walk_file = os.path.normpath(walk_file)
         for template_linter in template_linters:
             results = template_linter.process_file(walk_directory, walk_file)
-            if template_linter.supports_includes():
-                for include in results.includes:
-                    include_results = template_linter.process_file(
-                        include['directory'],
-                        include['file_name']
-                    )
-                    results.resolve_include(include, include_results)
             results.print_results(options, out)
 
 

scripts/tests/templates/includes/include_safe.html

-## Mako template with safe by default page expression
-<%page expression_filter="h"/>

scripts/tests/templates/includes/include_unsafe.html

-## Mako template with no safe by default page expression

scripts/tests/templates/test.html

-<%include file="./includes/include_safe.html" args="message" />
-
-<%include file="./includes/include_unsafe.html" args="message" />
-
-<%include file="bad_file_name.html" />
-
-## safe-lint: disable=mako-include-with-violations
-<%include file="bad_file_name.html" />
+## Mako template with no page directive for safe-by-default

scripts/tests/test_safe_template_linter.py

@@ -36,9 +36,6 @@ class TestSafeTemplateLinter(TestCase):
 
         output = out.getvalue()
         self.assertIsNotNone(re.search('test\.html.*mako-missing-default', out.getvalue()))
-        self.assertIsNotNone(re.search('test\.html.*mako-include-with-violations.*include_unsafe\.html', out.getvalue()))
-        self.assertIsNotNone(re.search('test\.html.*mako-include-with-violations.*bad_file_name\.html', out.getvalue()))
-        self.assertIsNotNone(re.search('include_unsafe\.html.*mako-missing-default', out.getvalue()))
 
 
 @ddt
@@ -125,7 +122,6 @@ class TestMakoTemplateLinter(TestCase):
             ${'{{unbalanced-nested'}
             ${x | n}
             ${x | h}
-            ${x | n, dump_html_escaped_json}
             ${x | n, dump_js_escaped_json}
         """)
 
@@ -247,7 +243,6 @@ class TestMakoTemplateLinter(TestCase):
                 ${'{{unbalanced-nested'}
                 ${x | n}
                 ${x | h}
-                ${x | n, dump_html_escaped_json}
                 ${x | n, dump_js_escaped_json}
                 "${x-with-quotes | n, js_escaped_string}"
             </script>
@@ -255,7 +250,7 @@ class TestMakoTemplateLinter(TestCase):
 
         linter._check_mako_file_is_safe(mako_template, results)
 
-        self.assertEqual(len(results.violations), 5)
+        self.assertEqual(len(results.violations), 4)
         self.assertEqual(results.violations[0].rule, Rules.mako_invalid_js_filter)
         self.assertEqual(results.violations[0].expression['expression'], "${x}")
         self.assertEqual(results.violations[1].rule, Rules.mako_unparsable_expression)
@@ -265,8 +260,6 @@ class TestMakoTemplateLinter(TestCase):
         self.assertEqual(results.violations[2].expression['expression'], "${x | n}")
         self.assertEqual(results.violations[3].rule, Rules.mako_invalid_js_filter)
         self.assertEqual(results.violations[3].expression['expression'], "${x | h}")
-        self.assertEqual(results.violations[4].rule, Rules.mako_invalid_js_filter)
-        self.assertEqual(results.violations[4].expression['expression'], "${x | n, dump_html_escaped_json}")
 
     def test_check_mako_expressions_in_require_js(self):
         """