Improve detecting SQL injections in f-strings #917
Conversation
bandit/plugins/injection_sql.py
Outdated
| @@ -92,8 +92,25 @@ def _evaluate_ast(node): | |||
| elif hasattr(ast, "JoinedStr") and isinstance( | |||
| node._bandit_parent, ast.JoinedStr | |||
| ): | |||
| statement = node.s | |||
| wrapper = node._bandit_parent._bandit_parent | |||
| if hasattr(ast, 'unparse'): | |||
There was a problem hiding this comment.
pyflakes seems to be able to handle JoinedStr https://github.com/PyCQA/pyflakes/blob/e19886e583637a7e2eec428cc036094b9630f2d0/pyflakes/checker.py#L1740-L1753 why can't we do something similar here without unparse?
There was a problem hiding this comment.
Thanks for looking at my pull request!
Do you mean something like this? Now it works for all versions of python.
This commit fixes detecting SQL injection
in statements like:
f"SELECT {column_name} FROM foo WHERE id = 1"
f"INSERT INTO {table_name} VALUES (1)"
f"UPDATE {table_name} SET id = 1"
Before this change, the bandit was analyzing statements
by parts, especially, in the case of:
"SELECT {column_name} FROM foo WHERE id = 1"
it was firstly checking "SELECT " for being
an SQL statement and then " FROM foo WHERE id = 1".
Neither of these parts match to defined
regular expressions:
r"(select\s.*from\s|"
r"delete\s+from\s|"
r"insert\s+into\s.*values\s|"
r"update\s.*set\s)",
Thus SQL injection was not detected.
This commit makes bandit checking the whole SQL
statement for matching the above regexps.
Resolves: PyCQA#916
|
We do not have tests for multiline SQL strings at all in the current code as far as I can see. I added some in my second PR: #915. I can move them here (assuming that you will merge this PR as the first one) and add tests for multiline f-strings. I will get back to you when done. |
|
No i mean implicit concatenation. Think something like Note no commas. This turns into |
|
Yes, I didn't know that name but I googled it. I added tests for multiline strings (as in the second PR but without nosec cases) and some for implicit concatenation: https://github.com/PyCQA/bandit/pull/917/files#diff-a691abc0c51d7e1432ef4486311a653dc950d5f440f71f7093f6f4f0892fe077R89 |
|
@sigmavirus24 Thank you! |
This commit fixes detecting SQL injection
in statements like:
Before this change, the bandit was analyzing statements
by parts, especially, in the case of:
"SELECT {column_name} FROM foo WHERE id = 1"it was firstly checking
"SELECT "for beingan SQL statement and then
" FROM foo WHERE id = 1".Neither of these parts match to defined
regular expressions:
Thus SQL injection was not detected.
This commit makes bandit checking the whole SQL
statement for matching the above regexps.
However, since it uses
ast.unparseit worksonly for python >= 3.9.
Resolves: #916