Merge pull request #12254 from edx/robrap/linter-javascript-context

TNL-4427: Add additional JavaScript context tags

scripts/safe_template_linter.py

@@ -235,6 +235,10 @@ class Rules(Enum):
         'mako-html-entities',
         "HTML entities should be plain text or wrapped with HTML()."
     )
+    mako_unknown_context = (
+        'mako-unknown-context',
+        "The context could not be determined."
+    )
     underscore_not_escaped = (
         'underscore-not-escaped',
         'Expressions should be escaped using <%- expression %>.'
@@ -1727,6 +1731,12 @@ class MakoTemplateLinter(BaseLinter):
             results: A list of results into which violations will be added.
 
         """
+        if context == 'unknown':
+            results.violations.append(ExpressionRuleViolation(
+                Rules.mako_unknown_context, expression
+            ))
+            return
+
         # Example: finds "| n, h}" when given "${x | n, h}"
         filters_regex = re.compile(r'\|([.,\w\s]*)\}')
         filters_match = filters_regex.search(expression.expression)
@@ -1851,15 +1861,25 @@ class MakoTemplateLinter(BaseLinter):
                 - index: the index of the context.
                 - type: the context type (e.g. 'html' or 'javascript').
         """
-        contexts_re = re.compile(r"""
-            <script.*?>|  # script tag start
-            </script>|  # script tag end
-            <%static:require_module.*?>|  # require js script tag start
-            </%static:require_module>  # require js script tag end""", re.VERBOSE | re.IGNORECASE)
+        contexts_re = re.compile(
+            r"""
+                <script.*?> |  # script tag start
+                </script> |  # script tag end
+                <%static:require_module.*?> |  # require js script tag start
+                </%static:require_module> | # require js script tag end
+                <%block[ ]*name=['"]requirejs['"]\w*> |  # require js tag start
+                </%block>  # require js tag end
+            """,
+            re.VERBOSE | re.IGNORECASE
+        )
         media_type_re = re.compile(r"""type=['"].*?['"]""", re.IGNORECASE)
 
         contexts = [{'index': 0, 'type': 'html'}]
-        javascript_types = ['text/javascript', 'text/ecmascript', 'application/ecmascript', 'application/javascript']
+        javascript_types = [
+            'text/javascript', 'text/ecmascript', 'application/ecmascript', 'application/javascript',
+            'text/x-mathjax-config', 'json/xblock-args'
+        ]
+        html_types = ['text/template']
         for context in contexts_re.finditer(mako_template):
             match_string = context.group().lower()
             if match_string.startswith("<script"):
@@ -1869,18 +1889,15 @@ class MakoTemplateLinter(BaseLinter):
                     # get media type (e.g. get text/javascript from
                     # type="text/javascript")
                     match_type = match_type.group()[6:-1].lower()
-                    if match_type not in javascript_types:
-                        # TODO: What are other types found, and are these really
-                        # html?  Or do we need to properly handle unknown
-                        # contexts? Only "text/template" is a known alternative.
+                    if match_type in html_types:
                         context_type = 'html'
+                    elif match_type not in javascript_types:
+                        context_type = 'unknown'
                 contexts.append({'index': context.end(), 'type': context_type})
-            elif match_string.startswith("</script"):
+            elif match_string.startswith("</"):
                 contexts.append({'index': context.start(), 'type': 'html'})
-            elif match_string.startswith("<%static:require_module"):
-                contexts.append({'index': context.end(), 'type': 'javascript'})
             else:
-                contexts.append({'index': context.start(), 'type': 'html'})
+                contexts.append({'index': context.end(), 'type': 'javascript'})
 
         return contexts
 

scripts/tests/test_safe_template_linter.py

@@ -464,9 +464,38 @@ class TestMakoTemplateLinter(TestLinter):
 
         mako_template = textwrap.dedent("""
             <%page expression_filter="h"/>
+            ## switch to JavaScript context
             <script>
                 {expression}
             </script>
+            ## switch back to HTML context
+            ${{x}}
+        """.format(expression=data['expression']))
+
+        linter._check_mako_file_is_safe(mako_template, results)
+
+        self._validate_data_rules(data, results)
+
+    @data(
+        {'expression': '${x}', 'rule': Rules.mako_invalid_js_filter},
+        {'expression': '"${x | n, js_escaped_string}"', 'rule': None},
+    )
+    def test_check_mako_expressions_in_require_module(self, data):
+        """
+        Test _check_mako_file_is_safe in JavaScript require context provides
+        appropriate violations
+        """
+        linter = MakoTemplateLinter()
+        results = FileResults('')
+
+        mako_template = textwrap.dedent("""
+            <%page expression_filter="h"/>
+            ## switch to JavaScript context (after next line)
+            <%static:require_module module_name="${{x}}" class_name="TestFactory">
+                {expression}
+            </%static:require_module>
+            ## switch back to HTML context
+            ${{x}}
         """.format(expression=data['expression']))
 
         linter._check_mako_file_is_safe(mako_template, results)
@@ -479,7 +508,7 @@ class TestMakoTemplateLinter(TestLinter):
     )
     def test_check_mako_expressions_in_require_js(self, data):
         """
-        Test _check_mako_file_is_safe in JavaScript require context provides
+        Test _check_mako_file_is_safe in JavaScript require js context provides
         appropriate violations
         """
         linter = MakoTemplateLinter()
@@ -487,9 +516,12 @@ class TestMakoTemplateLinter(TestLinter):
 
         mako_template = textwrap.dedent("""
             <%page expression_filter="h"/>
-            <%static:require_module module_name="${{x}}" class_name="TestFactory">
+            # switch to JavaScript context
+            <%block name="requirejs">
                 {expression}
-            </%static:require_module>
+            </%block>
+            ## switch back to HTML context
+            ${{x}}
         """.format(expression=data['expression']))
 
         linter._check_mako_file_is_safe(mako_template, results)
@@ -497,12 +529,14 @@ class TestMakoTemplateLinter(TestLinter):
         self._validate_data_rules(data, results)
 
     @data(
-        {'media_type': 'text/javascript', 'expected_violations': 0},
-        {'media_type': 'text/ecmascript', 'expected_violations': 0},
-        {'media_type': 'application/ecmascript', 'expected_violations': 0},
-        {'media_type': 'application/javascript', 'expected_violations': 0},
-        {'media_type': 'text/template', 'expected_violations': 1},
-        {'media_type': 'unknown/type', 'expected_violations': 1},
+        {'media_type': 'text/javascript', 'rule': None},
+        {'media_type': 'text/ecmascript', 'rule': None},
+        {'media_type': 'application/ecmascript', 'rule': None},
+        {'media_type': 'application/javascript', 'rule': None},
+        {'media_type': 'text/x-mathjax-config', 'rule': None},
+        {'media_type': 'json/xblock-args', 'rule': None},
+        {'media_type': 'text/template', 'rule': Rules.mako_invalid_html_filter},
+        {'media_type': 'unknown/type', 'rule': Rules.mako_unknown_context},
     )
     def test_check_mako_expressions_in_script_type(self, data):
         """
@@ -513,14 +547,17 @@ class TestMakoTemplateLinter(TestLinter):
 
         mako_template = textwrap.dedent("""
             <%page expression_filter="h"/>
+            # switch to JavaScript context
             <script type="{}">
                 ${{x | n, dump_js_escaped_json}}
             </script>
+            ## switch back to HTML context
+            ${{x}}
         """).format(data['media_type'])
 
         linter._check_mako_file_is_safe(mako_template, results)
 
-        self.assertEqual(len(results.violations), data['expected_violations'])
+        self._validate_data_rules(data, results)
 
     def test_check_mako_expressions_in_mixed_contexts(self):
         """