Skip to content

Commit

Permalink
update comment parsing
Browse files Browse the repository at this point in the history
- 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
  • Loading branch information
mikespallino committed Jan 26, 2022
1 parent d73041c commit abb233d
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 71 deletions.
76 changes: 27 additions & 49 deletions bandit/core/manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -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<tests>[^#]+)?#?")
NOSEC_COMMENT_TESTS = re.compile(r"(?:(B\d+|[a-z_]+),?)+", re.IGNORECASE)


class BanditManager:
Expand Down Expand Up @@ -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
19 changes: 14 additions & 5 deletions bandit/core/metrics.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand All @@ -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):
Expand All @@ -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
"""
Expand Down
25 changes: 15 additions & 10 deletions bandit/core/tester.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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
Expand Down Expand Up @@ -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)

Expand Down
5 changes: 3 additions & 2 deletions examples/nosec.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
4 changes: 2 additions & 2 deletions tests/functional/test_functional.py
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down
9 changes: 6 additions & 3 deletions tests/unit/formatters/test_text.py
Original file line number Diff line number Diff line change
Expand Up @@ -106,9 +106,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
Expand Down

0 comments on commit abb233d

Please sign in to comment.