Repository: PyCQA/bandit
Pull request #915 (2022-06-28 07:30:52)

Improve handling nosec for multi-line strings
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, in the case of ast `Constants`
it searches for nosec in the last line of the expression.

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

kfrydel: > So I think using the last line of linerange will fail for Python 3.7. Currently Bandit incorrectly reports the last line number for Py37. See issue #820

Thank you for taking a look at my changes.
I'm not sure if I understand. Do you mean that my changes are not going to work in Python 3.7?
`get_nosec` function returns `context['lineno']` for strings in Python 3.7. Why? Because in the case of Python 3.7 strings are parsed to `ast.Str`. In the case of Python 3.10 they are parsed to `ast.Constant`. It looks like `ast.Constant` is used from 3.8 according to https://docs.python.org/3/library/ast.html:
> Changed in version 3.8: Class [ast.Constant](https://docs.python.org/3/library/ast.html#ast.Constant) is now used for all constants.

I did the following test:
file: `string_examples.py`
```python
a = "string 1"
b = f"f-string {a}"
c = """multi-line
string
1"""
d = f"""multi-line
f-string
{a}"""

```
file: `ast_example.py`
```python
import ast

with open('string_examples.py', 'r') as f:
    ast_tree = ast.parse(f.read())

    for node in ast_tree.body:
        print(ast.dump(node))

```
And the output is:
1. python 3.7:
```
Assign(targets=[Name(id='a', ctx=Store())], value=Str(s='string 1'))
Assign(targets=[Name(id='b', ctx=Store())], value=JoinedStr(values=[Str(s='f-string '), FormattedValue(value=Name(id='a', ctx=Load()), conversion=-1, format_spec=None)]))
Assign(targets=[Name(id='c', ctx=Store())], value=Str(s='multi-line\nstring\n1'))
Assign(targets=[Name(id='d', ctx=Store())], value=JoinedStr(values=[Str(s='multi-line\nf-string\n'), FormattedValue(value=Name(id='a', ctx=Load()), conversion=-1, format_spec=None)]))
```
2. python 3.8:
```
Assign(targets=[Name(id='a', ctx=Store())], value=Constant(value='string 1', kind=None), type_comment=None)
Assign(targets=[Name(id='b', ctx=Store())], value=JoinedStr(values=[Constant(value='f-string ', kind=None), FormattedValue(value=Name(id='a', ctx=Load()), conversion=-1, format_spec=None)]), type_comment=None)
Assign(targets=[Name(id='c', ctx=Store())], value=Constant(value='multi-line\nstring\n1', kind=None), type_comment=None)
Assign(targets=[Name(id='d', ctx=Store())], value=JoinedStr(values=[Constant(value='multi-line\nf-string\n', kind=None), FormattedValue(value=Name(id='a', ctx=Load()), conversion=-1, format_spec=None)]), type_comment=None)
```
3. python 3.10:
```
Assign(targets=[Name(id='a', ctx=Store())], value=Constant(value='string 1'))
Assign(targets=[Name(id='b', ctx=Store())], value=JoinedStr(values=[Constant(value='f-string '), FormattedValue(value=Name(id='a', ctx=Load()), conversion=-1)]))
Assign(targets=[Name(id='c', ctx=Store())], value=Constant(value='multi-line\nstring\n1'))
Assign(targets=[Name(id='d', ctx=Store())], value=JoinedStr(values=[Constant(value='multi-line\nf-string\n'), FormattedValue(value=Name(id='a', ctx=Load()), conversion=-1)]))
```
frenzymadness: Thank you for working on this. This also fixes #658 reported in December 2020.
kfrydel: Hi @ericwb , would you find a while to take a look at this pull request? If you think it should be fixed in another way, do not hesitate to say so. I'm willing to spend some more time on this if needed.
sshishov: Hello guys, can we expect this PR to be merged some time soon?

Patch:
diff --git a/bandit/core/node_visitor.py b/bandit/core/node_visitor.py
index c2aa393..e721db6 100644
--- a/bandit/core/node_visitor.py
+++ b/bandit/core/node_visitor.py
@@ -200,13 +200,6 @@ class BanditNodeVisitor:
         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/utils.py b/bandit/core/utils.py
index 3ac78f5..a7e1c2b 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 1f07809..663446e 100644
--- a/examples/sql_multiline_statements.py
+++ b/examples/sql_multiline_statements.py
@@ -121,3 +121,62 @@ def a():
 
 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


Test patch:
diff --git a/bandit/core/tester.py b/bandit/core/tester.py
index 448b917..042b940 100644
--- a/bandit/core/tester.py
+++ b/bandit/core/tester.py
@@ -76,12 +76,15 @@ class BanditTester:
 
                     # 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 @@ class BanditTester:
             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/tests/functional/test_functional.py b/tests/functional/test_functional.py
index e1d10a9..3939906 100644
--- a/tests/functional/test_functional.py
+++ b/tests/functional/test_functional.py
@@ -457,21 +457,42 @@ class FunctionalTests(testtools.TestCase):
         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."""