-
-
Notifications
You must be signed in to change notification settings - Fork 732
Improve handling nosec for multi-line strings#915
Conversation
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
ericwb
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
bandit/core/utils.py
Outdated
| import logging | ||
| import os.path | ||
| import sys | ||
| from _ast import Constant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not use ast.Constant?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are right, it should be ast. Fixed.
06b7c6a to
80b337d
Compare
Thank you for taking a look at my changes.
I did the following test: a = "string 1"
b = f"f-string {a}" c = """multi-line string 1""" d = f"""multi-line f-string {a}""" file: 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:
Assign(targets=[Name(id='a', ctx=Store())], value=Str(s='string 1'))
Assign(targets=[Name(id='a', ctx=Store())], value=Constant(value='string 1', kind=None), type_comment=None)
Assign(targets=[Name(id='a', ctx=Store())], value=Constant(value='string 1'))
|
|
Thank you for working on this. This also fixes #658 reported in December 2020. |
|
Hello guys, can we expect this PR to be merged some time soon? |
sigmavirus24
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we include a test for implicit concatenation over multiple lines and the behavior for those cases? I'm not sure how python will report the lineno on 3.7 or the range on 3.8+. Also, uncertain where on those we should expect to see a nosec
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: PyCQA#880
|
I added tests for implicit concatenation cases and it turned out that my solution didn't work. I changed the approach and now I search for In the case of a multiline string, as shown below, f"""independently from the python version. |
|
@sigmavirus24 Thank you! |
|
Not sure but this change appears to have broken our Python 3.7 runs. We had tests getting skipped and now |
|
Definitely and I think I'm eating my words... we had a reduction in "skipped" tests, but the new ones were not previously skipped, they were two of the new rules that were added. I am able to correlate those, I just need to dig up why the skipped tests count dropped. I'll let you know if that has any relationship to this change. Apologies for the noise! |