From bfa9288b91cd3bc6167356196c34ce97fc91e3f1 Mon Sep 17 00:00:00 2001 From: Michael Spallino Date: Mon, 6 Apr 2020 19:55:43 -0400 Subject: [PATCH 1/3] support for disabling individual tests - allow disabling tests by id and by name (e.g. B602,assert_used) - update nosec_lines to be a dict keyed on line number to a set of tests to ignore - use an empty set to indicate a blanket nosec (i.e. just #nosec, no individual tests) - use None to indicate that there was no nosec comment on the line in question - track and report metrics on the number of tests skipped by specific tests and the number of tests that fail because they were not included in the list of specific tests to ignore Resolves #211 See also #418 --- bandit/core/manager.py | 92 ++++++++++++++++++++++++----- bandit/core/metrics.py | 26 ++++++-- bandit/core/node_visitor.py | 12 +++- bandit/core/tester.py | 49 ++++++++++++--- bandit/formatters/text.py | 8 +++ doc/source/config.rst | 17 ++++++ examples/nosec.py | 7 +++ tests/functional/test_functional.py | 4 +- tests/unit/formatters/test_text.py | 8 ++- 9 files changed, 190 insertions(+), 33 deletions(-) diff --git a/bandit/core/manager.py b/bandit/core/manager.py index 0febb8b53..c2376a7e6 100644 --- a/bandit/core/manager.py +++ b/bandit/core/manager.py @@ -7,6 +7,7 @@ import json import logging import os +import re import sys import tokenize import traceback @@ -21,6 +22,8 @@ LOG = logging.getLogger(__name__) +NOSEC_SPECIFIC_IDS_IGNORE = re.compile(r"([bB][\d]+),?[ ]?") +NOSEC_SPECIFIC_NAMES_IGNORE = re.compile(r"([a-z_]+),?[ ]?") class BanditManager: @@ -308,21 +311,20 @@ def _parse_file(self, fname, fdata, new_files_list): lines = data.splitlines() self.metrics.begin(fname) self.metrics.count_locs(lines) - if self.ignore_nosec: - nosec_lines = set() - else: - try: - fdata.seek(0) - tokens = tokenize.tokenize(fdata.readline) - nosec_lines = { - lineno - for toktype, tokval, (lineno, _), _, _ in tokens - if toktype == tokenize.COMMENT - and "#nosec" in tokval - or "# nosec" in tokval - } - except tokenize.TokenError: - nosec_lines = set() + # nosec_lines is a dict of line number -> set of tests to ignore + # for the line + nosec_lines = dict() + try: + fdata.seek(0) + tokens = tokenize.tokenize(fdata.readline) + + if not self.ignore_nosec: + for toktype, tokval, (lineno, _), _, _ in tokens: + if toktype == tokenize.COMMENT: + nosec_lines[lineno] = _parse_nosec_comment(tokval) + + except tokenize.TokenError: + pass score = self._execute_ast_visitor(fname, data, nosec_lines) self.scores.append(score) self.metrics.count_issues( @@ -460,3 +462,63 @@ def _find_candidate_matches(unmatched_issues, results_list): ] return issue_candidates + + +def _get_tests_from_nosec_comment(comment): + nosec_no_space = '#nosec' + nosec_space = '# nosec' + nospace = comment.find(nosec_no_space) + space = comment.find(nosec_space) + if nospace > -1: + start_comment = nospace + nosec_length = len(nosec_no_space) + elif space > -1: + start_comment = space + nosec_length = len(nosec_space) + else: + # None is explicitly different to empty set, None indicates no + # nosec comment on a line + return None + + # find the next # separator + end = comment.find('#', start_comment + 1) + # if we don't find one, set end index to the length + if end == -1: + end = len(comment) + + # extract text after #[ ]?nosec and the end index, this is just the + # list of potential test ids or names + return comment[start_comment + nosec_length:end] + + +def _parse_nosec_comment(comment): + nosec_tests = _get_tests_from_nosec_comment(comment) + if nosec_tests is None: + return None + # empty set indicates that there was a nosec comment without specific + # test ids or names + test_ids = set() + if nosec_tests: + extman = extension_loader.MANAGER + # check for IDS to ignore (e.g. B602) + for test in re.finditer(NOSEC_SPECIFIC_IDS_IGNORE, + nosec_tests): + test_id_match = test.group(1) + plugin_id = extman.check_id(test_id_match) + if plugin_id: + test_ids.add(test_id_match) + else: + LOG.warning("Test in comment: %s is not a test id", + test_id_match) + # check for NAMES to ignore (e.g. assert_used) + for test in re.finditer(NOSEC_SPECIFIC_NAMES_IGNORE, + nosec_tests): + plugin_id = extman.get_plugin_id(test.group(1)) + if plugin_id: + test_ids.add(plugin_id) + else: + LOG.warning( + "Test in comment: %s is not a test name, ignoring", + test.group(1)) + + return test_ids diff --git a/bandit/core/metrics.py b/bandit/core/metrics.py index 14d50d27b..b65644cf0 100644 --- a/bandit/core/metrics.py +++ b/bandit/core/metrics.py @@ -19,7 +19,8 @@ class Metrics: def __init__(self): self.data = dict() - self.data["_totals"] = {"loc": 0, "nosec": 0} + self.data["_totals"] = {"loc": 0, "nosec": 0, "nosec_by_test": 0, + "failed_nosec_by_test": 0} # initialize 0 totals for criteria and rank; this will be reset later for rank in constants.RANKING: @@ -30,21 +31,36 @@ def begin(self, fname): """Begin a new metric block. This starts a new metric collection name "fname" and makes is active. - :param fname: the metrics unique name, normally the file name. """ - self.data[fname] = {"loc": 0, "nosec": 0} + self.data[fname] = {"loc": 0, "nosec": 0, "nosec_by_test": 0, + "failed_nosec_by_test": 0} self.current = self.data[fname] def note_nosec(self, num=1): - """Note a "nosec" commnet. + """Note a "nosec" comment. Increment the currently active metrics nosec count. - :param num: number of nosecs seen, defaults to 1 """ self.current["nosec"] += num + def note_nosec_by_test(self, num=1): + """Note a "nosec BXXX, BYYY, ..." comment. + + Increment the currently active metrics nosec_by_test count. + :param num: number of nosecs seen, defaults to 1 + """ + self.current["nosec_by_test"] += num + + def note_failed_nosec_by_test(self, num=1): + """Note a test failed not caught when specific tests in comment used + + Increment the currently active metrics failed_nosec_by_test count. + :param num: number of failed nosecs seen, defaults to 1 + """ + self.current["failed_nosec_by_test"] += num + def count_locs(self, lines): """Count lines of code. diff --git a/bandit/core/node_visitor.py b/bandit/core/node_visitor.py index 6acf587e4..33603be6d 100644 --- a/bandit/core/node_visitor.py +++ b/bandit/core/node_visitor.py @@ -201,8 +201,10 @@ def pre_visit(self, node): if hasattr(node, "lineno"): self.context["lineno"] = node.lineno - if node.lineno in self.nosec_lines: - LOG.debug("skipped, nosec") + # explicitly check for empty set to skip all tests for a line + nosec_tests = self.nosec_lines.get(node.lineno) + if nosec_tests is not None and not len(nosec_tests): + LOG.debug("skipped, nosec without test number") self.metrics.note_nosec() return False if hasattr(node, "col_offset"): @@ -273,6 +275,12 @@ def update_scores(self, scores): severity, this is needed to update the stored list. :param score: The score list to update our scores with """ + # scores has extra nosec information about nosecs with specific tests + # so pop those out first and track the metrics for them + self.metrics.note_nosec_by_test(scores.pop("nosecs_by_tests")) + self.metrics.note_failed_nosec_by_test( + scores.pop("failed_nosecs_by_test") + ) # we'll end up with something like: # SEVERITY: {0, 0, 0, 10} where 10 is weighted by finding and level for score_type in self.scores: diff --git a/bandit/core/tester.py b/bandit/core/tester.py index 322320919..4a2761434 100644 --- a/bandit/core/tester.py +++ b/bandit/core/tester.py @@ -30,13 +30,15 @@ def run_tests(self, raw_context, checktype): :param raw_context: Raw context dictionary :param checktype: The type of checks to run - :param nosec_lines: Lines which should be skipped because of nosec - :return: a score based on the number and type of test results + :return: a score based on the number and type of test results with + extra metrics about nosec comments """ scores = { "SEVERITY": [0] * len(constants.RANKING), "CONFIDENCE": [0] * len(constants.RANKING), + "nosecs_by_tests": 0, + "failed_nosecs_by_test": 0 } tests = self.testset.get_tests(checktype) @@ -51,12 +53,24 @@ def run_tests(self, raw_context, checktype): else: result = test(context) - # if we have a result, record it and update scores - if ( - result is not None - and result.lineno not in self.nosec_lines - and temp_context["lineno"] not in self.nosec_lines - ): + if result is not None: + nosec_tests_to_skip = set() + base_tests = self.nosec_lines.get(result.lineno, None) + context_tests = self.nosec_lines.get( + temp_context["lineno"], None) + + # if both are non there are were no comments + # this is explicitly different than being empty + # empty set indicates blanket nosec comment without + # individual test names or ids + if base_tests is None and context_tests is None: + nosec_tests_to_skip = None + + # combine tests from current line and context line + if base_tests is not None: + nosec_tests_to_skip.update(base_tests) + if context_tests is not None: + nosec_tests_to_skip.update(context_tests) if isinstance(temp_context["filename"], bytes): result.fname = temp_context["filename"].decode("utf-8") @@ -71,6 +85,25 @@ def run_tests(self, raw_context, checktype): if result.test_id == "": result.test_id = test._test_id + # don't skip a the test if there was no nosec comment + if nosec_tests_to_skip is not None: + # if the set is empty or the test id is in the set of + # tests to skip, log and increment the skip by test + # count + if not nosec_tests_to_skip or \ + (result.test_id in nosec_tests_to_skip): + LOG.debug("skipped, nosec for test %s" + % result.test_id) + scores['nosecs_by_tests'] += 1 + continue + # otherwise this test was not called out explicitly by + # a nosec BXX type comment and should fail. Log and + # increment the failed test count + else: + LOG.debug("uncaught test %s in nosec comment" + % result.test_id) + scores['failed_nosecs_by_test'] += 1 + self.results.append(result) LOG.debug("Issue identified by %s: %s", name, result) diff --git a/bandit/formatters/text.py b/bandit/formatters/text.py index 3e821d1d6..29c470a04 100644 --- a/bandit/formatters/text.py +++ b/bandit/formatters/text.py @@ -171,6 +171,14 @@ def report(manager, fileobj, sev_level, conf_level, lines=-1): "\tTotal lines skipped (#nosec): %i" % (manager.metrics.data["_totals"]["nosec"]) ) + bits.append( + "\tTotal lines skipped (#nosec BXXX,BYYY,...): %i" + % (manager.metrics.data["_totals"]["nosec_by_test"]) + ) + bits.append( + "\tTotal other nosec test caught (#nosec BXXX,BYYY but BZZZ): %i" + % (manager.metrics.data["_totals"]["failed_nosec_by_test"]) + ) skipped = manager.get_skipped() bits.append(get_metrics(manager)) diff --git a/doc/source/config.rst b/doc/source/config.rst index b3b4af5df..c28247975 100644 --- a/doc/source/config.rst +++ b/doc/source/config.rst @@ -41,6 +41,23 @@ security issue, it will not be reported:: self.process = subprocess.Popen('/bin/echo', shell=True) # nosec +Because multiple issues can be reported for the same line, specific tests may +be provided to suppress those reports. This will cause other issues not +included to be reported. This can be useful in preventing situations where a +nosec comment is used, but a separate vulnerability may be added to the line +later causing the new vulnerability to be ignored. + +For example, this will suppress the report of B602 and B607:: + + self.process = subprocess.Popen('/bin/ls *', shell=True) #nosec B602, B607 + +Full test names rather than the test ID may also be used. + +For example, this will suppress the report of B101 and continue to report B506 +as an issue.:: + + assert yaml.load("{}") == [] # nosec assert_used + ----------------- Scanning Behavior ----------------- diff --git a/examples/nosec.py b/examples/nosec.py index 6bc2cc296..030ef1959 100644 --- a/examples/nosec.py +++ b/examples/nosec.py @@ -5,3 +5,10 @@ shell=True) #nosec (on the specific kwarg line) subprocess.Popen('#nosec', shell=True) subprocess.Popen('/bin/ls *', shell=True) # type: … # nosec # noqa: E501 ; pylint: disable=line-too-long +subprocess.Popen('/bin/ls *', shell=True) #nosec subprocess_popen_with_shell_equals_true (on the line) +subprocess.Popen('#nosec', shell=True) # nosec B607, B602 +subprocess.Popen('#nosec', shell=True) # nosec B607 B602 +subprocess.Popen('/bin/ls *', shell=True) # nosec subprocess_popen_with_shell_equals_true start_process_with_partial_path +subprocess.Popen('/bin/ls *', shell=True) # type: … # noqa: E501 ; pylint: disable=line-too-long # nosec +subprocess.Popen('#nosec', shell=True) # nosec B607, B101 +subprocess.Popen('#nosec', shell=True) # nosec B602, subprocess_popen_with_shell_equals_true diff --git a/tests/functional/test_functional.py b/tests/functional/test_functional.py index ab2ada95e..1dbc75914 100644 --- a/tests/functional/test_functional.py +++ b/tests/functional/test_functional.py @@ -747,8 +747,8 @@ def test_flask_debug_true(self): def test_nosec(self): expect = { - "SEVERITY": {"UNDEFINED": 0, "LOW": 2, "MEDIUM": 0, "HIGH": 0}, - "CONFIDENCE": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 0, "HIGH": 2}, + "SEVERITY": {"UNDEFINED": 0, "LOW": 4, "MEDIUM": 0, "HIGH": 0}, + "CONFIDENCE": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 0, "HIGH": 4}, } self.check_example("nosec.py", expect) diff --git a/tests/unit/formatters/test_text.py b/tests/unit/formatters/test_text.py index 2ce80d499..97e5d87b0 100644 --- a/tests/unit/formatters/test_text.py +++ b/tests/unit/formatters/test_text.py @@ -107,7 +107,9 @@ def test_report_nobaseline(self, get_issue_list): get_issue_list.return_value = [issue_a, issue_b] - self.manager.metrics.data["_totals"] = {"loc": 1000, "nosec": 50} + self.manager.metrics.data["_totals"] = {"loc": 1000, "nosec": 50, + "nosec_by_test": 0, + "failed_nosec_by_test": 0} for category in ["SEVERITY", "CONFIDENCE"]: for level in ["UNDEFINED", "LOW", "MEDIUM", "HIGH"]: self.manager.metrics.data["_totals"][f"{category}.{level}"] = 1 @@ -153,6 +155,10 @@ def test_report_nobaseline(self, get_issue_list): "High: 1", "Total lines skipped ", "(#nosec): 50", + "Total lines skipped ", + "(#nosec BXXX,BYYY,...): 0", + "Total other nosec test caught ", + "(#nosec BXXX,BYYY but BZZZ): 0", "Total issues (by severity)", "Total issues (by confidence)", "Files skipped (1)", From 7fe51bb464e25271f4c60ce8910b3a607bf36ef2 Mon Sep 17 00:00:00 2001 From: Michael Spallino Date: Mon, 10 Jan 2022 19:37:39 -0500 Subject: [PATCH 2/3] update comment parsing - consolidating the test regex to limit the number of passes we need to make - use a regex to capture the nosec comment anchor rather than multiple find calls - remove unicode ellipses in example file - update docstring for clarity --- bandit/core/manager.py | 76 ++++++++++------------------- bandit/core/metrics.py | 19 ++++++-- bandit/core/tester.py | 25 ++++++---- examples/nosec.py | 5 +- tests/functional/test_functional.py | 4 +- tests/unit/formatters/test_text.py | 9 ++-- 6 files changed, 67 insertions(+), 71 deletions(-) diff --git a/bandit/core/manager.py b/bandit/core/manager.py index c2376a7e6..f43ef2a32 100644 --- a/bandit/core/manager.py +++ b/bandit/core/manager.py @@ -22,8 +22,8 @@ LOG = logging.getLogger(__name__) -NOSEC_SPECIFIC_IDS_IGNORE = re.compile(r"([bB][\d]+),?[ ]?") -NOSEC_SPECIFIC_NAMES_IGNORE = re.compile(r"([a-z_]+),?[ ]?") +NOSEC_COMMENT = re.compile(r"#\s*nosec:?\s*(?P[^#]+)?#?") +NOSEC_COMMENT_TESTS = re.compile(r"(?:(B\d+|[a-z_]+),?)+", re.IGNORECASE) class BanditManager: @@ -464,61 +464,39 @@ def _find_candidate_matches(unmatched_issues, results_list): return issue_candidates -def _get_tests_from_nosec_comment(comment): - nosec_no_space = '#nosec' - nosec_space = '# nosec' - nospace = comment.find(nosec_no_space) - space = comment.find(nosec_space) - if nospace > -1: - start_comment = nospace - nosec_length = len(nosec_no_space) - elif space > -1: - start_comment = space - nosec_length = len(nosec_space) - else: - # None is explicitly different to empty set, None indicates no - # nosec comment on a line - return None - - # find the next # separator - end = comment.find('#', start_comment + 1) - # if we don't find one, set end index to the length - if end == -1: - end = len(comment) - - # extract text after #[ ]?nosec and the end index, this is just the - # list of potential test ids or names - return comment[start_comment + nosec_length:end] +def _find_test_id_from_nosec_string(extman, match): + plugin_id = extman.check_id(match) + if plugin_id: + return match + # Finding by short_id didn't work, let's check the plugin name + plugin_id = extman.get_plugin_id(match) + if not plugin_id: + # Name and short id didn't work: + LOG.warning( + "Test in comment: %s is not a test name or id, ignoring", match + ) + return plugin_id # We want to return None or the string here regardless def _parse_nosec_comment(comment): - nosec_tests = _get_tests_from_nosec_comment(comment) - if nosec_tests is None: + found_no_sec_comment = NOSEC_COMMENT.search(comment) + if not found_no_sec_comment: + # there was no nosec comment return None + + matches = found_no_sec_comment.groupdict() + nosec_tests = matches.get("tests", set()) + # empty set indicates that there was a nosec comment without specific # test ids or names test_ids = set() if nosec_tests: extman = extension_loader.MANAGER - # check for IDS to ignore (e.g. B602) - for test in re.finditer(NOSEC_SPECIFIC_IDS_IGNORE, - nosec_tests): - test_id_match = test.group(1) - plugin_id = extman.check_id(test_id_match) - if plugin_id: - test_ids.add(test_id_match) - else: - LOG.warning("Test in comment: %s is not a test id", - test_id_match) - # check for NAMES to ignore (e.g. assert_used) - for test in re.finditer(NOSEC_SPECIFIC_NAMES_IGNORE, - nosec_tests): - plugin_id = extman.get_plugin_id(test.group(1)) - if plugin_id: - test_ids.add(plugin_id) - else: - LOG.warning( - "Test in comment: %s is not a test name, ignoring", - test.group(1)) + # lookup tests by short code or name + for test in re.finditer(NOSEC_COMMENT_TESTS, nosec_tests): + test_match = test.group(1) + test_id = _find_test_id_from_nosec_string(extman, test_match) + if test_id: + test_ids.add(test_id) return test_ids diff --git a/bandit/core/metrics.py b/bandit/core/metrics.py index b65644cf0..92bd3bab5 100644 --- a/bandit/core/metrics.py +++ b/bandit/core/metrics.py @@ -19,8 +19,12 @@ class Metrics: def __init__(self): self.data = dict() - self.data["_totals"] = {"loc": 0, "nosec": 0, "nosec_by_test": 0, - "failed_nosec_by_test": 0} + self.data["_totals"] = { + "loc": 0, + "nosec": 0, + "nosec_by_test": 0, + "failed_nosec_by_test": 0, + } # initialize 0 totals for criteria and rank; this will be reset later for rank in constants.RANKING: @@ -33,8 +37,12 @@ def begin(self, fname): This starts a new metric collection name "fname" and makes is active. :param fname: the metrics unique name, normally the file name. """ - self.data[fname] = {"loc": 0, "nosec": 0, "nosec_by_test": 0, - "failed_nosec_by_test": 0} + self.data[fname] = { + "loc": 0, + "nosec": 0, + "nosec_by_test": 0, + "failed_nosec_by_test": 0, + } self.current = self.data[fname] def note_nosec(self, num=1): @@ -54,8 +62,9 @@ def note_nosec_by_test(self, num=1): self.current["nosec_by_test"] += num def note_failed_nosec_by_test(self, num=1): - """Note a test failed not caught when specific tests in comment used + """Note a test failed when using a nosec comment with specific tests. + e.g. # nosec B102, B607, but B602 failed. Increment the currently active metrics failed_nosec_by_test count. :param num: number of failed nosecs seen, defaults to 1 """ diff --git a/bandit/core/tester.py b/bandit/core/tester.py index 4a2761434..cb0de5511 100644 --- a/bandit/core/tester.py +++ b/bandit/core/tester.py @@ -38,7 +38,7 @@ def run_tests(self, raw_context, checktype): "SEVERITY": [0] * len(constants.RANKING), "CONFIDENCE": [0] * len(constants.RANKING), "nosecs_by_tests": 0, - "failed_nosecs_by_test": 0 + "failed_nosecs_by_test": 0, } tests = self.testset.get_tests(checktype) @@ -57,7 +57,8 @@ def run_tests(self, raw_context, checktype): nosec_tests_to_skip = set() base_tests = self.nosec_lines.get(result.lineno, None) context_tests = self.nosec_lines.get( - temp_context["lineno"], None) + temp_context["lineno"], None + ) # if both are non there are were no comments # this is explicitly different than being empty @@ -90,19 +91,23 @@ def run_tests(self, raw_context, checktype): # if the set is empty or the test id is in the set of # tests to skip, log and increment the skip by test # count - if not nosec_tests_to_skip or \ - (result.test_id in nosec_tests_to_skip): - LOG.debug("skipped, nosec for test %s" - % result.test_id) - scores['nosecs_by_tests'] += 1 + if not nosec_tests_to_skip or ( + result.test_id in nosec_tests_to_skip + ): + LOG.debug( + "skipped, nosec for test %s" % result.test_id + ) + scores["nosecs_by_tests"] += 1 continue # otherwise this test was not called out explicitly by # a nosec BXX type comment and should fail. Log and # increment the failed test count else: - LOG.debug("uncaught test %s in nosec comment" - % result.test_id) - scores['failed_nosecs_by_test'] += 1 + LOG.debug( + "uncaught test %s in nosec comment" + % result.test_id + ) + scores["failed_nosecs_by_test"] += 1 self.results.append(result) diff --git a/examples/nosec.py b/examples/nosec.py index 030ef1959..c69979114 100644 --- a/examples/nosec.py +++ b/examples/nosec.py @@ -4,11 +4,12 @@ subprocess.Popen('/bin/ls *', shell=True) #nosec (on the specific kwarg line) subprocess.Popen('#nosec', shell=True) -subprocess.Popen('/bin/ls *', shell=True) # type: … # nosec # noqa: E501 ; pylint: disable=line-too-long +subprocess.Popen('/bin/ls *', shell=True) # type: ... # nosec # noqa: E501 ; pylint: disable=line-too-long +subprocess.Popen('/bin/ls *', shell=True) # type: ... # nosec B607 # noqa: E501 ; pylint: disable=line-too-long subprocess.Popen('/bin/ls *', shell=True) #nosec subprocess_popen_with_shell_equals_true (on the line) subprocess.Popen('#nosec', shell=True) # nosec B607, B602 subprocess.Popen('#nosec', shell=True) # nosec B607 B602 subprocess.Popen('/bin/ls *', shell=True) # nosec subprocess_popen_with_shell_equals_true start_process_with_partial_path -subprocess.Popen('/bin/ls *', shell=True) # type: … # noqa: E501 ; pylint: disable=line-too-long # nosec +subprocess.Popen('/bin/ls *', shell=True) # type: ... # noqa: E501 ; pylint: disable=line-too-long # nosec subprocess.Popen('#nosec', shell=True) # nosec B607, B101 subprocess.Popen('#nosec', shell=True) # nosec B602, subprocess_popen_with_shell_equals_true diff --git a/tests/functional/test_functional.py b/tests/functional/test_functional.py index 1dbc75914..2ea2a56f3 100644 --- a/tests/functional/test_functional.py +++ b/tests/functional/test_functional.py @@ -747,8 +747,8 @@ def test_flask_debug_true(self): def test_nosec(self): expect = { - "SEVERITY": {"UNDEFINED": 0, "LOW": 4, "MEDIUM": 0, "HIGH": 0}, - "CONFIDENCE": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 0, "HIGH": 4}, + "SEVERITY": {"UNDEFINED": 0, "LOW": 5, "MEDIUM": 0, "HIGH": 0}, + "CONFIDENCE": {"UNDEFINED": 0, "LOW": 0, "MEDIUM": 0, "HIGH": 5}, } self.check_example("nosec.py", expect) diff --git a/tests/unit/formatters/test_text.py b/tests/unit/formatters/test_text.py index 97e5d87b0..cb4ae6735 100644 --- a/tests/unit/formatters/test_text.py +++ b/tests/unit/formatters/test_text.py @@ -107,9 +107,12 @@ def test_report_nobaseline(self, get_issue_list): get_issue_list.return_value = [issue_a, issue_b] - self.manager.metrics.data["_totals"] = {"loc": 1000, "nosec": 50, - "nosec_by_test": 0, - "failed_nosec_by_test": 0} + self.manager.metrics.data["_totals"] = { + "loc": 1000, + "nosec": 50, + "nosec_by_test": 0, + "failed_nosec_by_test": 0, + } for category in ["SEVERITY", "CONFIDENCE"]: for level in ["UNDEFINED", "LOW", "MEDIUM", "HIGH"]: self.manager.metrics.data["_totals"][f"{category}.{level}"] = 1 From 5747e306262d71aca46542f71c6fda116de98b6c Mon Sep 17 00:00:00 2001 From: Michael Spallino Date: Fri, 28 Jan 2022 00:48:45 -0500 Subject: [PATCH 3/3] feedback - re.finditer -> compiled_obj.finditer - remove metric when using comment with specific tests to ingore and a different test fails - pass metrics from node_visitor to constructor of tester so we can just call the methods directly and not alter calls signatures or return types --- bandit/core/manager.py | 2 +- bandit/core/metrics.py | 23 +++------ bandit/core/node_visitor.py | 9 +--- bandit/core/tester.py | 78 ++++++++++++++++++------------ bandit/formatters/text.py | 9 ++-- tests/unit/formatters/test_text.py | 9 ++-- 6 files changed, 62 insertions(+), 68 deletions(-) diff --git a/bandit/core/manager.py b/bandit/core/manager.py index f43ef2a32..fc6a1eddb 100644 --- a/bandit/core/manager.py +++ b/bandit/core/manager.py @@ -493,7 +493,7 @@ def _parse_nosec_comment(comment): if nosec_tests: extman = extension_loader.MANAGER # lookup tests by short code or name - for test in re.finditer(NOSEC_COMMENT_TESTS, nosec_tests): + for test in NOSEC_COMMENT_TESTS.finditer(nosec_tests): test_match = test.group(1) test_id = _find_test_id_from_nosec_string(extman, test_match) if test_id: diff --git a/bandit/core/metrics.py b/bandit/core/metrics.py index 92bd3bab5..045e544b8 100644 --- a/bandit/core/metrics.py +++ b/bandit/core/metrics.py @@ -22,8 +22,7 @@ def __init__(self): self.data["_totals"] = { "loc": 0, "nosec": 0, - "nosec_by_test": 0, - "failed_nosec_by_test": 0, + "skipped_tests": 0, } # initialize 0 totals for criteria and rank; this will be reset later @@ -40,8 +39,7 @@ def begin(self, fname): self.data[fname] = { "loc": 0, "nosec": 0, - "nosec_by_test": 0, - "failed_nosec_by_test": 0, + "skipped_tests": 0, } self.current = self.data[fname] @@ -53,22 +51,13 @@ def note_nosec(self, num=1): """ self.current["nosec"] += num - def note_nosec_by_test(self, num=1): + def note_skipped_test(self, num=1): """Note a "nosec BXXX, BYYY, ..." comment. - Increment the currently active metrics nosec_by_test count. - :param num: number of nosecs seen, defaults to 1 - """ - self.current["nosec_by_test"] += num - - def note_failed_nosec_by_test(self, num=1): - """Note a test failed when using a nosec comment with specific tests. - - e.g. # nosec B102, B607, but B602 failed. - Increment the currently active metrics failed_nosec_by_test count. - :param num: number of failed nosecs seen, defaults to 1 + Increment the currently active metrics skipped_tests count. + :param num: number of skipped_tests seen, defaults to 1 """ - self.current["failed_nosec_by_test"] += num + self.current["skipped_tests"] += num def count_locs(self, lines): """Count lines of code. diff --git a/bandit/core/node_visitor.py b/bandit/core/node_visitor.py index 33603be6d..293f18d49 100644 --- a/bandit/core/node_visitor.py +++ b/bandit/core/node_visitor.py @@ -30,7 +30,7 @@ def __init__(self, fname, metaast, testset, debug, nosec_lines, metrics): self.imports = set() self.import_aliases = {} self.tester = b_tester.BanditTester( - self.testset, self.debug, nosec_lines + self.testset, self.debug, nosec_lines, metrics ) # in some cases we can't determine a qualified name @@ -207,6 +207,7 @@ def pre_visit(self, node): LOG.debug("skipped, nosec without test number") self.metrics.note_nosec() return False + if hasattr(node, "col_offset"): self.context["col_offset"] = node.col_offset @@ -275,12 +276,6 @@ def update_scores(self, scores): severity, this is needed to update the stored list. :param score: The score list to update our scores with """ - # scores has extra nosec information about nosecs with specific tests - # so pop those out first and track the metrics for them - self.metrics.note_nosec_by_test(scores.pop("nosecs_by_tests")) - self.metrics.note_failed_nosec_by_test( - scores.pop("failed_nosecs_by_test") - ) # we'll end up with something like: # SEVERITY: {0, 0, 0, 10} where 10 is weighted by finding and level for score_type in self.scores: diff --git a/bandit/core/tester.py b/bandit/core/tester.py index cb0de5511..6ae22d4ad 100644 --- a/bandit/core/tester.py +++ b/bandit/core/tester.py @@ -15,12 +15,13 @@ class BanditTester: - def __init__(self, testset, debug, nosec_lines): + def __init__(self, testset, debug, nosec_lines, metrics): self.results = [] self.testset = testset self.last_result = None self.debug = debug self.nosec_lines = nosec_lines + self.metrics = metrics def run_tests(self, raw_context, checktype): """Runs all tests for a certain type of check, for example @@ -37,8 +38,6 @@ def run_tests(self, raw_context, checktype): scores = { "SEVERITY": [0] * len(constants.RANKING), "CONFIDENCE": [0] * len(constants.RANKING), - "nosecs_by_tests": 0, - "failed_nosecs_by_test": 0, } tests = self.testset.get_tests(checktype) @@ -54,25 +53,10 @@ def run_tests(self, raw_context, checktype): result = test(context) if result is not None: - nosec_tests_to_skip = set() - base_tests = self.nosec_lines.get(result.lineno, None) - context_tests = self.nosec_lines.get( - temp_context["lineno"], None + nosec_tests_to_skip = self._get_nosecs_from_contexts( + temp_context, test_result=result ) - # if both are non there are were no comments - # this is explicitly different than being empty - # empty set indicates blanket nosec comment without - # individual test names or ids - if base_tests is None and context_tests is None: - nosec_tests_to_skip = None - - # combine tests from current line and context line - if base_tests is not None: - nosec_tests_to_skip.update(base_tests) - if context_tests is not None: - nosec_tests_to_skip.update(context_tests) - if isinstance(temp_context["filename"], bytes): result.fname = temp_context["filename"].decode("utf-8") else: @@ -86,7 +70,7 @@ def run_tests(self, raw_context, checktype): if result.test_id == "": result.test_id = test._test_id - # don't skip a the test if there was no nosec comment + # don't skip the test if there was no nosec comment if nosec_tests_to_skip is not None: # if the set is empty or the test id is in the set of # tests to skip, log and increment the skip by test @@ -97,17 +81,8 @@ def run_tests(self, raw_context, checktype): LOG.debug( "skipped, nosec for test %s" % result.test_id ) - scores["nosecs_by_tests"] += 1 + self.metrics.note_skipped_test() continue - # otherwise this test was not called out explicitly by - # a nosec BXX type comment and should fail. Log and - # increment the failed test count - else: - LOG.debug( - "uncaught test %s in nosec comment" - % result.test_id - ) - scores["failed_nosecs_by_test"] += 1 self.results.append(result) @@ -118,6 +93,18 @@ def run_tests(self, raw_context, checktype): con = constants.RANKING.index(result.confidence) val = constants.RANKING_VALUES[result.confidence] scores["CONFIDENCE"][con] += val + else: + nosec_tests_to_skip = self._get_nosecs_from_contexts( + temp_context + ) + if ( + nosec_tests_to_skip + and test._test_id in nosec_tests_to_skip + ): + LOG.warning( + f"nosec encountered ({test._test_id}), but no " + f"failed test on line {temp_context['lineno']}" + ) except Exception as e: self.report_error(name, context, e) @@ -126,6 +113,35 @@ def run_tests(self, raw_context, checktype): LOG.debug("Returning scores: %s", scores) return scores + def _get_nosecs_from_contexts(self, context, test_result=None): + """Use context and optional test result to get set of tests to skip. + :param context: temp context + :param test_result: optional test result + :return: set of tests to skip for the line based on contexts + """ + nosec_tests_to_skip = set() + base_tests = ( + self.nosec_lines.get(test_result.lineno, None) + if test_result + else None + ) + context_tests = self.nosec_lines.get(context["lineno"], None) + + # if both are none there were no comments + # this is explicitly different from being empty. + # empty set indicates blanket nosec comment without + # individual test names or ids + if base_tests is None and context_tests is None: + nosec_tests_to_skip = None + + # combine tests from current line and context line + if base_tests is not None: + nosec_tests_to_skip.update(base_tests) + if context_tests is not None: + nosec_tests_to_skip.update(context_tests) + + return nosec_tests_to_skip + @staticmethod def report_error(test, context, error): what = "Bandit internal error running: " diff --git a/bandit/formatters/text.py b/bandit/formatters/text.py index 29c470a04..54d208bd9 100644 --- a/bandit/formatters/text.py +++ b/bandit/formatters/text.py @@ -172,12 +172,9 @@ def report(manager, fileobj, sev_level, conf_level, lines=-1): % (manager.metrics.data["_totals"]["nosec"]) ) bits.append( - "\tTotal lines skipped (#nosec BXXX,BYYY,...): %i" - % (manager.metrics.data["_totals"]["nosec_by_test"]) - ) - bits.append( - "\tTotal other nosec test caught (#nosec BXXX,BYYY but BZZZ): %i" - % (manager.metrics.data["_totals"]["failed_nosec_by_test"]) + "\tTotal potential issues skipped due to specifically being " + "disabled (e.g., #nosec BXXX): %i" + % (manager.metrics.data["_totals"]["skipped_tests"]) ) skipped = manager.get_skipped() diff --git a/tests/unit/formatters/test_text.py b/tests/unit/formatters/test_text.py index cb4ae6735..689fe1ade 100644 --- a/tests/unit/formatters/test_text.py +++ b/tests/unit/formatters/test_text.py @@ -110,8 +110,7 @@ def test_report_nobaseline(self, get_issue_list): self.manager.metrics.data["_totals"] = { "loc": 1000, "nosec": 50, - "nosec_by_test": 0, - "failed_nosec_by_test": 0, + "skipped_tests": 0, } for category in ["SEVERITY", "CONFIDENCE"]: for level in ["UNDEFINED", "LOW", "MEDIUM", "HIGH"]: @@ -158,10 +157,8 @@ def test_report_nobaseline(self, get_issue_list): "High: 1", "Total lines skipped ", "(#nosec): 50", - "Total lines skipped ", - "(#nosec BXXX,BYYY,...): 0", - "Total other nosec test caught ", - "(#nosec BXXX,BYYY but BZZZ): 0", + "Total potential issues skipped due to specifically being ", + "disabled (e.g., #nosec BXXX): 0", "Total issues (by severity)", "Total issues (by confidence)", "Files skipped (1)",