From 72fa5a7496caa89f21ace353d038754dbddf9a91 Mon Sep 17 00:00:00 2001 From: Kamil Frydel Date: Mon, 27 Feb 2023 14:23:30 +0100 Subject: [PATCH] Improve handling nosec for multi-line strings (#915) 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 --- bandit/core/node_visitor.py | 7 ---- bandit/core/tester.py | 17 ++++---- bandit/core/utils.py | 8 ++++ examples/sql_multiline_statements.py | 59 ++++++++++++++++++++++++++++ tests/functional/test_functional.py | 25 +++++++++++- 5 files changed, 100 insertions(+), 16 deletions(-) diff --git a/bandit/core/node_visitor.py b/bandit/core/node_visitor.py index c2aa39301..e721db64f 100644 --- a/bandit/core/node_visitor.py +++ b/bandit/core/node_visitor.py @@ -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"): diff --git a/bandit/core/tester.py b/bandit/core/tester.py index 448b91786..042b9400d 100644 --- a/bandit/core/tester.py +++ b/bandit/core/tester.py @@ -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 ) @@ -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. diff --git a/bandit/core/utils.py b/bandit/core/utils.py index 3ac78f54f..a7e1c2b60 100644 --- a/bandit/core/utils.py +++ b/bandit/core/utils.py @@ -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 diff --git a/examples/sql_multiline_statements.py b/examples/sql_multiline_statements.py index 1f078090e..663446ec5 100644 --- a/examples/sql_multiline_statements.py +++ b/examples/sql_multiline_statements.py @@ -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 diff --git a/tests/functional/test_functional.py b/tests/functional/test_functional.py index e1d10a956..39399062b 100644 --- a/tests/functional/test_functional.py +++ b/tests/functional/test_functional.py @@ -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."""