diff --git a/pavelib/quality.py b/pavelib/quality.py index aaaf7b809a9d..be4f1b0e6478 100644 --- a/pavelib/quality.py +++ b/pavelib/quality.py @@ -434,10 +434,11 @@ def run_xsslint(options): _prepare_report_dir(xsslint_report_dir) sh( - "{repo_root}/scripts/xsslint/{xsslint_script} --rule-totals >> {xsslint_report}".format( + "{repo_root}/scripts/xsslint/{xsslint_script} --rule-totals --config={cfg_module} >> {xsslint_report}".format( repo_root=Env.REPO_ROOT, xsslint_script=xsslint_script, xsslint_report=xsslint_report, + cfg_module='scripts.xsslint_config' ), ignore_error=True ) diff --git a/scripts/xss-commit-linter.sh b/scripts/xss-commit-linter.sh index 8941760b577f..4c0e204c4b7d 100755 --- a/scripts/xss-commit-linter.sh +++ b/scripts/xss-commit-linter.sh @@ -82,6 +82,6 @@ else for f in $diff_files; do echo "" echo "Linting $f:" - ./scripts/xsslint/xss_linter.py $f + ./scripts/xsslint/xss_linter.py --config=scripts.xsslint_config $f done fi diff --git a/scripts/xsslint/tests/test_linters.py b/scripts/xsslint/tests/test_linters.py index ccc4b813295c..bbe6937babf4 100644 --- a/scripts/xsslint/tests/test_linters.py +++ b/scripts/xsslint/tests/test_linters.py @@ -15,6 +15,19 @@ from xsslint.utils import ParseString +def _build_javascript_linter(): + return JavaScriptLinter( + underscore_linter=UnderscoreTemplateLinter() + ) + + +def _build_mako_linter(): + return MakoTemplateLinter( + javascript_linter=_build_javascript_linter(), + python_linter=PythonLinter(), + ) + + class TestLinter(TestCase): """ Test Linter base class @@ -219,7 +232,7 @@ def test_concat_with_html(self, data): """ Test check_javascript_file_is_safe with concatenating strings and HTML """ - linter = JavaScriptLinter() + linter = _build_javascript_linter() results = FileResults('') linter.check_javascript_file_is_safe(data['template'], results) @@ -249,7 +262,7 @@ def test_jquery_append(self, data): """ Test check_javascript_file_is_safe with JQuery append() """ - linter = JavaScriptLinter() + linter = _build_javascript_linter() results = FileResults('') linter.check_javascript_file_is_safe(data['template'], results) @@ -277,7 +290,7 @@ def test_jquery_prepend(self, data): """ Test check_javascript_file_is_safe with JQuery prepend() """ - linter = JavaScriptLinter() + linter = _build_javascript_linter() results = FileResults('') linter.check_javascript_file_is_safe(data['template'], results) @@ -309,7 +322,7 @@ def test_jquery_insertion(self, data): other than append(), prepend() and html() that take content as an argument (e.g. before(), after()). """ - linter = JavaScriptLinter() + linter = _build_javascript_linter() results = FileResults('') linter.check_javascript_file_is_safe(data['template'], results) @@ -340,7 +353,7 @@ def test_jquery_insert_to_target(self, data): functions that take a target as an argument, like appendTo() and prependTo(). """ - linter = JavaScriptLinter() + linter = _build_javascript_linter() results = FileResults('') linter.check_javascript_file_is_safe(data['template'], results) @@ -364,7 +377,7 @@ def test_jquery_html(self, data): """ Test check_javascript_file_is_safe with JQuery html() """ - linter = JavaScriptLinter() + linter = _build_javascript_linter() results = FileResults('') linter.check_javascript_file_is_safe(data['template'], results) @@ -379,7 +392,7 @@ def test_javascript_interpolate(self, data): """ Test check_javascript_file_is_safe with interpolate() """ - linter = JavaScriptLinter() + linter = _build_javascript_linter() results = FileResults('') linter.check_javascript_file_is_safe(data['template'], results) @@ -394,7 +407,7 @@ def test_javascript_interpolate(self, data): """ Test check_javascript_file_is_safe with interpolate() """ - linter = JavaScriptLinter() + linter = _build_javascript_linter() results = FileResults('') linter.check_javascript_file_is_safe(data['template'], results) @@ -737,7 +750,8 @@ def test_is_valid_directory(self, data): """ Test _is_valid_directory correctly determines mako directories """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() + linter._skip_mako_dirs = ('test_root',) self.assertEqual(linter._is_valid_directory(data['directory']), data['expected']) @@ -785,7 +799,7 @@ def test_check_page_default(self, data): """ Test _check_mako_file_is_safe with different page defaults """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') linter._check_mako_file_is_safe(data['template'], results) @@ -808,7 +822,7 @@ def test_check_mako_expressions_in_html(self, data): """ Test _check_mako_file_is_safe in html context provides appropriate violations """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -825,7 +839,7 @@ def test_check_mako_expression_display_name(self): Test _check_mako_file_is_safe with display_name_with_default_escaped fails. """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -971,7 +985,7 @@ def test_check_mako_with_text_and_html(self, data): """ Test _check_mako_file_is_safe tests for proper use of Text() and Html(). """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -988,7 +1002,7 @@ def test_check_mako_entity_with_no_default(self): Test _check_mako_file_is_safe does not fail on entities when safe-by-default is not set. """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = "${'Rock & Roll'}" @@ -1003,7 +1017,7 @@ def test_check_mako_expression_default_disabled(self): Test _check_mako_file_is_safe with disable pragma for safe-by-default works to designate that this is not a Mako file """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1023,7 +1037,7 @@ def test_check_mako_expression_disabled(self): Test _check_mako_file_is_safe with disable pragma results in no violation """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1047,7 +1061,7 @@ def test_check_mako_on_django_template(self, data): Test _check_mako_file_is_safe with disable pragma results in no violation """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') linter._check_mako_file_is_safe(data['template'], results) @@ -1059,7 +1073,7 @@ def test_check_mako_expressions_in_html_with_escape_filter(self): Test _check_mako_file_is_safe results in no violations, when strip_all_tags_but_br filter is applied in html context """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1075,7 +1089,7 @@ def test_check_mako_expressions_in_html_without_default(self): Test _check_mako_file_is_safe in html context without the page level default h filter suppresses expression level violation """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1100,7 +1114,7 @@ def test_check_mako_expressions_in_javascript(self, data): Test _check_mako_file_is_safe in JavaScript script context provides appropriate violations """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1126,7 +1140,7 @@ 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() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1152,7 +1166,7 @@ def test_check_mako_expressions_in_require_js(self, data): Test _check_mako_file_is_safe in JavaScript require js context provides appropriate violations """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1183,7 +1197,7 @@ def test_check_mako_expressions_in_script_type(self, data): """ Test _check_mako_file_is_safe in script tag with different media types """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1205,7 +1219,7 @@ def test_check_mako_expressions_in_mixed_contexts(self): Test _check_mako_file_is_safe in mixed contexts provides appropriate violations """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1242,7 +1256,7 @@ def test_check_mako_expressions_javascript_strings(self): - mako_js_missing_quotes - mako_js_html_string """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1277,7 +1291,7 @@ def test_check_javascript_in_mako_javascript_context(self): Test _check_mako_file_is_safe with JavaScript error in JavaScript context. """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') mako_template = textwrap.dedent(""" @@ -1311,7 +1325,7 @@ def test_expression_detailed_results(self, data): Test _check_mako_file_is_safe provides detailed results, including line numbers, columns, and line """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() results = FileResults('') linter._check_mako_file_is_safe(data['template'], results) @@ -1346,7 +1360,7 @@ def test_find_mako_expressions(self, data): """ Test _find_mako_expressions for parseable expressions """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() expressions = linter._find_mako_expressions(data['template']) @@ -1364,7 +1378,7 @@ def test_find_unparseable_mako_expressions(self, data): """ Test _find_mako_expressions for unparseable expressions """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() expressions = linter._find_mako_expressions(data['template']) self.assertTrue(2 <= len(expressions)) @@ -1401,7 +1415,7 @@ def test_parse_string(self, data): """ Test _parse_string helper """ - linter = MakoTemplateLinter() + linter = _build_mako_linter() parse_string = ParseString(data['template'], data['result']['start_index'], len(data['template'])) string_dict = { diff --git a/scripts/xsslint/tests/test_main.py b/scripts/xsslint/tests/test_main.py index dc99ddb7ca04..2d7b735562be 100644 --- a/scripts/xsslint/tests/test_main.py +++ b/scripts/xsslint/tests/test_main.py @@ -46,6 +46,13 @@ def patch_is_valid_directory(self, linter_class): self.addCleanup(patcher.stop) return patch_start + def _build_linters(self): + underscore_linter = UnderscoreTemplateLinter() + python_linter = PythonLinter() + javascript_linter = JavaScriptLinter(underscore_linter=underscore_linter) + mako_linter = MakoTemplateLinter(javascript_linter=javascript_linter, python_linter=python_linter) + return [mako_linter, underscore_linter, javascript_linter, python_linter] + def test_lint_defaults(self): """ Tests the top-level linting with default options. @@ -55,11 +62,12 @@ def test_lint_defaults(self): _lint( 'scripts/xsslint/tests/templates', - template_linters=[MakoTemplateLinter(), UnderscoreTemplateLinter(), JavaScriptLinter(), PythonLinter()], + template_linters=self._build_linters(), options={ 'list_files': False, 'verbose': False, 'rule_totals': False, + 'skip_dirs': () }, summary_results=summary_results, out=out, @@ -97,11 +105,12 @@ def test_lint_with_verbose(self): _lint( 'scripts/xsslint/tests/templates', - template_linters=[MakoTemplateLinter(), UnderscoreTemplateLinter(), JavaScriptLinter(), PythonLinter()], + template_linters=self._build_linters(), options={ 'list_files': False, 'verbose': True, 'rule_totals': False, + 'skip_dirs': () }, summary_results=summary_results, out=out, @@ -131,11 +140,12 @@ def test_lint_with_rule_totals(self): _lint( 'scripts/xsslint/tests/templates', - template_linters=[MakoTemplateLinter(), UnderscoreTemplateLinter(), JavaScriptLinter(), PythonLinter()], + template_linters=self._build_linters(), options={ 'list_files': False, 'verbose': False, 'rule_totals': True, + 'skip_dirs': () }, summary_results=summary_results, out=out, @@ -155,14 +165,14 @@ def test_lint_with_list_files(self): """ out = StringIO() summary_results = SummaryResults() - _lint( 'scripts/xsslint/tests/templates', - template_linters=[MakoTemplateLinter(), UnderscoreTemplateLinter(), JavaScriptLinter(), PythonLinter()], + template_linters=self._build_linters(), options={ 'list_files': True, 'verbose': False, 'rule_totals': False, + 'skip_dirs': () }, summary_results=summary_results, out=out, diff --git a/scripts/xsslint/xsslint/default_config.py b/scripts/xsslint/xsslint/default_config.py new file mode 100644 index 000000000000..c33b032f21c1 --- /dev/null +++ b/scripts/xsslint/xsslint/default_config.py @@ -0,0 +1,51 @@ +# Default xsslint config module. +from xsslint.linters import JavaScriptLinter, MakoTemplateLinter, PythonLinter, UnderscoreTemplateLinter + + +# Define the directories that should be ignored by the script. +SKIP_DIRS = ( + '.git', + '.pycharm_helpers', + 'common/static/xmodule/modules', + 'common/static/bundles', + 'perf_tests', + 'node_modules', + 'reports/diff_quality', + 'scripts/xsslint', + 'spec', + 'test_root', + 'vendor', +) + + +UNDERSCORE_SKIP_DIRS = SKIP_DIRS + ('test',) +UNDERSCORE_LINTER = UnderscoreTemplateLinter( + skip_dirs=UNDERSCORE_SKIP_DIRS +) + + +JAVASCRIPT_SKIP_DIRS = SKIP_DIRS + ('i18n', 'static/coffee') +COFFEESCRIPT_SKIP_DIRS = SKIP_DIRS +JAVASCRIPT_LINTER = JavaScriptLinter( + underscore_linter=UNDERSCORE_LINTER, + javascript_skip_dirs=JAVASCRIPT_SKIP_DIRS, + coffeescript_skip_dirs=COFFEESCRIPT_SKIP_DIRS +) + + +PYTHON_SKIP_DIRS = SKIP_DIRS + ('tests', 'test/acceptance') +PYTHON_LINTER = PythonLinter( + skip_dirs=PYTHON_SKIP_DIRS +) + + +MAKO_SKIP_DIRS = SKIP_DIRS +MAKO_LINTER = MakoTemplateLinter( + javascript_linter=JAVASCRIPT_LINTER, + python_linter=PYTHON_LINTER, + skip_dirs=MAKO_SKIP_DIRS +) + + +# (Required) Define the linters (code-checkers) that should be run by the script. +LINTERS = (MAKO_LINTER, UNDERSCORE_LINTER, JAVASCRIPT_LINTER, PYTHON_LINTER) diff --git a/scripts/xsslint/xsslint/linters.py b/scripts/xsslint/xsslint/linters.py index 6d32cdb24f9d..62c158673a1e 100644 --- a/scripts/xsslint/xsslint/linters.py +++ b/scripts/xsslint/xsslint/linters.py @@ -8,7 +8,7 @@ from xsslint.reporting import ExpressionRuleViolation, FileResults, RuleViolation from xsslint.rules import Rules -from xsslint.utils import SKIP_DIRS, Expression, ParseString, StringLines, is_skip_dir +from xsslint.utils import Expression, ParseString, StringLines, is_skip_dir from xsslint.visitors import AllNodeVisitor, HtmlStringVisitor, OuterFormatVisitor @@ -194,12 +194,12 @@ class UnderscoreTemplateLinter(BaseLinter): """ The linter for Underscore.js template files. """ - def __init__(self): + def __init__(self, skip_dirs=None): """ Init method. """ super(UnderscoreTemplateLinter, self).__init__() - self._skip_underscore_dirs = SKIP_DIRS + ('test',) + self._skip_underscore_dirs = skip_dirs or () def process_file(self, directory, file_name): """ @@ -309,14 +309,14 @@ class JavaScriptLinter(BaseLinter): LINE_COMMENT_DELIM = "//" - def __init__(self): + def __init__(self, underscore_linter, javascript_skip_dirs=None, coffeescript_skip_dirs=None): """ Init method. """ super(JavaScriptLinter, self).__init__() - self._skip_javascript_dirs = SKIP_DIRS + ('i18n', 'static/coffee') - self._skip_coffeescript_dirs = SKIP_DIRS - self.underscore_linter = UnderscoreTemplateLinter() + self.underscore_linter = underscore_linter + self._skip_javascript_dirs = javascript_skip_dirs or () + self._skip_coffeescript_dirs = coffeescript_skip_dirs or () def process_file(self, directory, file_name): """ @@ -697,12 +697,12 @@ class PythonLinter(BaseLinter): LINE_COMMENT_DELIM = "#" - def __init__(self): + def __init__(self, skip_dirs=None): """ Init method. """ super(PythonLinter, self).__init__() - self._skip_python_dirs = SKIP_DIRS + ('tests', 'test/acceptance') + self._skip_python_dirs = skip_dirs or () def process_file(self, directory, file_name): """ @@ -856,13 +856,14 @@ class MakoTemplateLinter(BaseLinter): """ LINE_COMMENT_DELIM = "##" - def __init__(self): + def __init__(self, javascript_linter, python_linter, skip_dirs=None): """ Init method. """ super(MakoTemplateLinter, self).__init__() - self.javascript_linter = JavaScriptLinter() - self.python_linter = PythonLinter() + self.javascript_linter = javascript_linter + self.python_linter = python_linter + self._skip_mako_dirs = skip_dirs or () def process_file(self, directory, file_name): """ @@ -909,7 +910,7 @@ def _is_valid_directory(self, directory): True if this directory should be linted for Mako template violations and False otherwise. """ - if is_skip_dir(SKIP_DIRS, directory): + if is_skip_dir(self._skip_mako_dirs, directory): return False # TODO: This is an imperfect guess concerning the Mako template diff --git a/scripts/xsslint/xsslint/main.py b/scripts/xsslint/xsslint/main.py index fd133759fcd9..40526d7cac4f 100644 --- a/scripts/xsslint/xsslint/main.py +++ b/scripts/xsslint/xsslint/main.py @@ -2,12 +2,20 @@ The main function for the XSS linter. """ import argparse +import importlib import os import sys -from xsslint.linters import JavaScriptLinter, MakoTemplateLinter, PythonLinter, UnderscoreTemplateLinter from xsslint.reporting import SummaryResults -from xsslint.utils import SKIP_DIRS, is_skip_dir +from xsslint.utils import is_skip_dir + + +def _load_config_module(module_path): + cwd = os.getcwd() + if cwd not in sys.path: + # Enable config module to be imported relative to wherever the script was run from. + sys.path.append(cwd) + return importlib.import_module(module_path) def _process_file(full_path, template_linters, options, summary_results, out): @@ -61,8 +69,9 @@ def _process_os_dirs(starting_dir, template_linters, options, summary_results, o out: output file """ + skip_dirs = options.get('skip_dirs', ()) for root, dirs, files in os.walk(starting_dir): - if is_skip_dir(SKIP_DIRS, root): + if is_skip_dir(skip_dirs, root): del dirs continue dirs.sort(key=lambda s: s.lower()) @@ -125,16 +134,23 @@ def main(): '--verbose', dest='verbose', action='store_true', help='Print multiple lines where possible for additional context of violations.' ) + parser.add_argument( + '--config', dest='config', action='store', default='xsslint.default_config', + help='Specifies the config module to use. The config module should be in Python package syntax.' + ) parser.add_argument('path', nargs="?", default=None, help='A file to lint or directory to recursively lint.') args = parser.parse_args() - + config = _load_config_module(args.config) options = { 'list_files': args.list_files, 'rule_totals': args.rule_totals, 'verbose': args.verbose, + 'skip_dirs': getattr(config, 'SKIP_DIRS', ()) } - template_linters = [MakoTemplateLinter(), UnderscoreTemplateLinter(), JavaScriptLinter(), PythonLinter()] summary_results = SummaryResults() + template_linters = getattr(config, 'LINTERS', ()) + if not template_linters: + raise ValueError("LINTERS is empty or undefined in the config module ({}).".format(args.config)) _lint(args.path, template_linters, options, summary_results, out=sys.stdout) diff --git a/scripts/xsslint/xsslint/utils.py b/scripts/xsslint/xsslint/utils.py index f01f7f4de937..325170e95d77 100644 --- a/scripts/xsslint/xsslint/utils.py +++ b/scripts/xsslint/xsslint/utils.py @@ -3,20 +3,6 @@ """ import re -SKIP_DIRS = ( - '.git', - '.pycharm_helpers', - 'common/static/xmodule/modules', - 'common/static/bundles', - 'perf_tests', - 'node_modules', - 'reports/diff_quality', - 'scripts/xsslint', - 'spec', - 'test_root', - 'vendor', -) - def is_skip_dir(skip_dirs, directory): """ diff --git a/scripts/xsslint_config.py b/scripts/xsslint_config.py new file mode 100644 index 000000000000..2dac2680defb --- /dev/null +++ b/scripts/xsslint_config.py @@ -0,0 +1,59 @@ +# xsslint config module for edx-platform +import os +import sys + +# Temporarily add xsslint to sys.path so that we can import from it. This won't be necessary once +# xsslint is moved out of edx-platform. +scripts_dir = os.path.dirname(os.path.abspath(__file__)) +sys.path.append(os.path.join(scripts_dir, 'xsslint')) + +from xsslint.linters import JavaScriptLinter, MakoTemplateLinter, PythonLinter, UnderscoreTemplateLinter + + +# Define the directories that should be ignored by the script. +SKIP_DIRS = ( + '.git', + '.pycharm_helpers', + 'common/static/xmodule/modules', + 'common/static/bundles', + 'perf_tests', + 'node_modules', + 'reports/diff_quality', + 'scripts/xsslint', + 'spec', + 'test_root', + 'vendor', +) + + +UNDERSCORE_SKIP_DIRS = SKIP_DIRS + ('test',) +UNDERSCORE_LINTER = UnderscoreTemplateLinter( + skip_dirs=UNDERSCORE_SKIP_DIRS +) + + +JAVASCRIPT_SKIP_DIRS = SKIP_DIRS + ('i18n', 'static/coffee') +COFFEESCRIPT_SKIP_DIRS = SKIP_DIRS +JAVASCRIPT_LINTER = JavaScriptLinter( + underscore_linter=UNDERSCORE_LINTER, + javascript_skip_dirs=JAVASCRIPT_SKIP_DIRS, + coffeescript_skip_dirs=COFFEESCRIPT_SKIP_DIRS +) + + +PYTHON_SKIP_DIRS = SKIP_DIRS + ('tests', 'test/acceptance') +PYTHON_LINTER = PythonLinter( + skip_dirs=PYTHON_SKIP_DIRS +) + + +MAKO_SKIP_DIRS = SKIP_DIRS +MAKO_LINTER = MakoTemplateLinter( + javascript_linter=JAVASCRIPT_LINTER, + python_linter=PYTHON_LINTER, + skip_dirs=MAKO_SKIP_DIRS +) + + +# (Required) Define the linters (code-checkers) that should be run by the script. +LINTERS = (MAKO_LINTER, UNDERSCORE_LINTER, JAVASCRIPT_LINTER, PYTHON_LINTER)