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

@@ -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"):

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.

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

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

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."""