Skip to content

Commit

Permalink
Improve handling nosec for multi-line strings (#915)
Browse files Browse the repository at this point in the history
This commit improves handling nosecs
in multi-line strings, like:

1. nosec_not_working = f"""
2.     SELECT * FROM {table}
3. """  # nosec

Before this change, bandit was checking if there is
a nosec in line 1. Now, it searches for nosec in all
lines of the expression.

In python 3.7, linerange for a multiline expression is sqeezed to
first line. Thus, if nosec is set  in the second or further line
then it is not taken into account by bandit.

This commit also moves detecting nosec without test number
to test phase from "pre-visit" phase.
It may increase the time of performing checks but avoids
counting the same nosec mark multiple times.
"pre-visit" phase is run separately for each part of multi-line
string split by FormattedValue items. Thus for the above example,
it would be run twice, the first time for "\n    SELECT * FROM "
and the second time for "\n" making the nosec being counted twice.

Resolves: #880
  • Loading branch information
kfrydel authored Feb 27, 2023
1 parent 7e6f580 commit 72fa5a7
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 16 deletions.
7 changes: 0 additions & 7 deletions bandit/core/node_visitor.py
Original file line number Diff line number Diff line change
Expand Up @@ -200,13 +200,6 @@ def pre_visit(self, node):
if hasattr(node, "lineno"):
self.context["lineno"] = node.lineno

# 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"):
self.context["col_offset"] = node.col_offset
if hasattr(node, "end_col_offset"):
Expand Down
17 changes: 10 additions & 7 deletions bandit/core/tester.py
Original file line number Diff line number Diff line change
Expand Up @@ -76,12 +76,15 @@ def run_tests(self, raw_context, checktype):

# 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
# count
if not nosec_tests_to_skip or (
result.test_id in nosec_tests_to_skip
):
# If the set is empty then it means that nosec was
# used without test number -> update nosecs counter.
# If 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:
LOG.debug("skipped, nosec without test number")
self.metrics.note_nosec()
continue
elif result.test_id in nosec_tests_to_skip:
LOG.debug(
"skipped, nosec for test %s" % result.test_id
)
Expand Down Expand Up @@ -129,7 +132,7 @@ def _get_nosecs_from_contexts(self, context, test_result=None):
if test_result
else None
)
context_tests = self.nosec_lines.get(context["lineno"], None)
context_tests = utils.get_nosec(self.nosec_lines, context)

# if both are none there were no comments
# this is explicitly different from being empty.
Expand Down
8 changes: 8 additions & 0 deletions bandit/core/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -370,3 +370,11 @@ def check_ast_node(name):
pass

raise TypeError("Error: %s is not a valid node type in AST" % name)


def get_nosec(nosec_lines, context):
for lineno in context["linerange"]:
nosec = nosec_lines.get(lineno, None)
if nosec is not None:
return nosec
return None
59 changes: 59 additions & 0 deletions examples/sql_multiline_statements.py
Original file line number Diff line number Diff line change
Expand Up @@ -121,3 +121,62 @@ def b():

a()("""SELECT %s
FROM foo""" % val)

# skip
query = """SELECT *
FROM foo WHERE id = '%s'""" % identifier # nosec
query = """SELECT *
FROM foo WHERE id = '%s'""" % identifier # nosec B608
query = """
SELECT *
FROM foo
WHERE id = '%s'
""" % identifier # nosec B608

query = f"""
SELECT *
FROM foo
WHERE id = {identifier}
""" # nosec
query = f"""
SELECT *
FROM foo
WHERE id = {identifier}
""" # nosec B608

query = f"""
SELECT *
FROM foo
WHERE id = {identifier}""" # nosec
query = f"""
SELECT *
FROM foo
WHERE id = {identifier}""" # nosec B608

cur.execute("SELECT * " # nosec
"FROM foo "
f"WHERE id = {identifier}")
cur.execute("SELECT * " # nosec B608
"FROM foo "
f"WHERE id = {identifier}")

query = ("SELECT * " # nosec
"FROM foo "
f"WHERE id = {identifier}")
query = ("SELECT * " # nosec B608
"FROM foo "
f"WHERE id = {identifier}")

# nosec is not recognized for the 4 below cases in python 3.7
query = ("SELECT * "
"FROM foo " # nosec
f"WHERE id = {identifier}")
query = ("SELECT * "
"FROM foo " # nosec B608
f"WHERE id = {identifier}")
query = ("SELECT * "
"FROM foo "
f"WHERE id = {identifier}") # nosec
query = ("SELECT * "
"FROM foo "
f"WHERE id = {identifier}") # nosec B608
25 changes: 23 additions & 2 deletions tests/functional/test_functional.py
Original file line number Diff line number Diff line change
Expand Up @@ -457,21 +457,42 @@ def test_multiline_sql_statements(self):
multi-line strings.
"""
example_file = "sql_multiline_statements.py"
confidence_low_tests = 13
severity_medium_tests = 26
nosec_tests = 7
skipped_tests = 8
if sys.version_info[:2] <= (3, 7):
# In the case of implicit concatenation in python 3.7,
# we know only the first line of multi-line string.
# Thus, cases like:
# query = ("SELECT * "
# "FROM foo " # nosec
# f"WHERE id = {identifier}")
# are not skipped but reported as errors.
confidence_low_tests = 17
severity_medium_tests = 30
nosec_tests = 5
skipped_tests = 6
expect = {
"SEVERITY": {
"UNDEFINED": 0,
"LOW": 0,
"MEDIUM": 26,
"MEDIUM": severity_medium_tests,
"HIGH": 0,
},
"CONFIDENCE": {
"UNDEFINED": 0,
"LOW": 13,
"LOW": confidence_low_tests,
"MEDIUM": 13,
"HIGH": 0,
},
}
expect_stats = {
"nosec": nosec_tests,
"skipped_tests": skipped_tests,
}
self.check_example(example_file, expect)
self.check_metrics(example_file, expect_stats)

def test_ssl_insecure_version(self):
"""Test for insecure SSL protocol versions."""
Expand Down

0 comments on commit 72fa5a7

Please sign in to comment.