Merge pull request #12246 from edx/robrap/safe-commit

TNL-4317: Add helper for checking git commit is safe

scripts/safe-commit-linter.sh

+#!/usr/bin/env bash
+set -e
+
+###############################################################################
+#
+#   safe-commit-linter.sh
+#
+#   Executes safe_template_linter.py on the set of files in a particular git
+#   commit.
+#
+###############################################################################
+
+show_help() {
+    echo "Usage: safe-commit-linter.sh [OPTION]"
+    echo "Runs the Safe Template Linter against all files in a git commit."
+    echo ""
+    echo "Mandatory arguments to long options are mandatory for short options too."
+    echo "  -m, --main-branch=COMMIT    Run against files changed between the"
+    echo "                              current branch and this commit."
+    echo "                              Defaults to origin/master."
+}
+
+for i in "$@"; do
+    case $i in
+        -m=*|--main-branch=*)
+            MAIN_COMMIT="${i#*=}"
+            shift # past argument=value
+            ;;
+        -h|--help|*)
+            # help or unknown option
+            show_help
+            exit 0
+            ;;
+    esac
+done
+
+current_branch_hash=`git rev-parse HEAD`
+
+if [ -z "${MAIN_COMMIT+x}" ]; then
+    # if commit is not set, get hash of current branch
+    MAIN_COMMIT="origin/master"
+fi
+
+merge_base=`git merge-base "$current_branch_hash" "$MAIN_COMMIT"`
+diff_files=`git diff --name-only "$current_branch_hash" "$merge_base"`
+
+for f in $diff_files; do
+    echo ""
+    echo "Linting $f:"
+    ./scripts/safe_template_linter.py $f
+done

scripts/safe_template_linter.py

@@ -613,7 +613,7 @@ class FileResults(object):
 
         Arguments:
             options: A list of the following options:
-                is_quiet: True to print only file names, and False to print
+                list_files: True to print only file names, and False to print
                     all violations.
             out: output file
 
@@ -623,7 +623,7 @@ class FileResults(object):
 
         """
         num_violations = 0
-        if options['is_quiet']:
+        if options['list_files']:
             if self.violations is not None and 0 < len(self.violations):
                 num_violations += 1
                 print(self.full_path, file=out)
@@ -2326,33 +2326,33 @@ def main():
     parser = argparse.ArgumentParser(
         formatter_class=argparse.RawDescriptionHelpFormatter,
         description='Checks that templates are safe.',
-        epilog=epilog
+        epilog=epilog,
     )
     parser.add_argument(
-        '--quiet', dest='quiet', action='store_true', help='only display the filenames that contain violations'
-    )
-    parser.add_argument('--file', dest='file', nargs=1, default=None, help='a single file to lint')
-    parser.add_argument(
-        '--dir', dest='directory', nargs=1, default=['.'], help='the directory to lint (including sub-directories)'
+        '--list-files', dest='list_files', action='store_true',
+        help='Only display the filenames that contain violations.'
     )
+    parser.add_argument('path', nargs="?", default=None, help='A file to lint or directory to recursively lint.')
 
     args = parser.parse_args()
 
     options = {
-        'is_quiet': args.quiet,
+        'list_files': args.list_files,
     }
-
     template_linters = [MakoTemplateLinter(), UnderscoreTemplateLinter(), JavaScriptLinter(), PythonLinter()]
-    if args.file is not None:
-        if os.path.isfile(args.file[0]) is False:
-            raise ValueError("File [{}] is not a valid file.".format(args.file[0]))
-        num_violations = _process_file(args.file[0], template_linters, options, out=sys.stdout)
+
+    if args.path is not None and os.path.isfile(args.path):
+        num_violations = _process_file(args.path, template_linters, options, out=sys.stdout)
     else:
-        if os.path.exists(args.directory[0]) is False or os.path.isfile(args.directory[0]) is True:
-            raise ValueError("Directory [{}] is not a valid directory.".format(args.directory[0]))
-        num_violations = _process_os_walk(args.directory[0], template_linters, options, out=sys.stdout)
+        directory = "."
+        if args.path is not None:
+            if os.path.exists(args.path):
+                directory = args.path
+            else:
+                raise ValueError("Path [{}] is not a valid file or directory.".format(args.path))
+        num_violations = _process_os_walk(directory, template_linters, options, out=sys.stdout)
 
-    if options['is_quiet'] is False:
+    if options['list_files'] is False:
         # matches output of jshint for simplicity
         print("")
         print("{} violations found".format(num_violations))

scripts/tests/templates/test.py

+message = "<script>alert('XSS');</script>"
+x = "<string>{}</strong>".format(message)

scripts/tests/test_safe_template_linter.py

@@ -82,6 +82,19 @@ class TestSafeTemplateLinter(TestCase):
     Test some top-level linter functions
     """
 
+    def patch_is_valid_directory(self, linter_class):
+        """
+        Creates a mock patch for _is_valid_directory on a Linter to always
+        return true. This avoids nested patch calls.
+
+        Arguments:
+            linter_class: The linter class to be patched
+        """
+        patcher = mock.patch.object(linter_class, '_is_valid_directory', return_value=True)
+        patch_start = patcher.start()
+        self.addCleanup(patcher.stop)
+        return patch_start
+
     def test_process_os_walk(self):
         """
         Tests the top-level processing of template files, including Mako
@@ -90,24 +103,27 @@ class TestSafeTemplateLinter(TestCase):
         out = StringIO()
 
         options = {
-            'is_quiet': False,
+            'list_files': False,
         }
 
-        template_linters = [MakoTemplateLinter(), JavaScriptLinter(), UnderscoreTemplateLinter()]
+        template_linters = [MakoTemplateLinter(), JavaScriptLinter(), UnderscoreTemplateLinter(), PythonLinter()]
+
+        self.patch_is_valid_directory(MakoTemplateLinter)
+        self.patch_is_valid_directory(JavaScriptLinter)
+        self.patch_is_valid_directory(UnderscoreTemplateLinter)
+        self.patch_is_valid_directory(PythonLinter)
 
-        with mock.patch.object(MakoTemplateLinter, '_is_valid_directory', return_value=True):
-            with mock.patch.object(JavaScriptLinter, '_is_valid_directory', return_value=True):
-                with mock.patch.object(UnderscoreTemplateLinter, '_is_valid_directory', return_value=True):
-                    num_violations = _process_os_walk('scripts/tests/templates', template_linters, options, out)
+        num_violations = _process_os_walk('scripts/tests/templates', template_linters, options, out)
 
         output = out.getvalue()
-        self.assertEqual(num_violations, 6)
+        self.assertEqual(num_violations, 7)
         self.assertIsNotNone(re.search('test\.html.*mako-missing-default', output))
         self.assertIsNotNone(re.search('test\.coffee.*javascript-concat-html', output))
         self.assertIsNotNone(re.search('test\.coffee.*underscore-not-escaped', output))
         self.assertIsNotNone(re.search('test\.js.*javascript-concat-html', output))
         self.assertIsNotNone(re.search('test\.js.*underscore-not-escaped', output))
         self.assertIsNotNone(re.search('test\.underscore.*underscore-not-escaped', output))
+        self.assertIsNotNone(re.search('test\.py.*python-interpolate-html', output))
 
 
 @ddt