From a19e98a2c7cb24e60f2d8bdd28ac05ec3fb622cb Mon Sep 17 00:00:00 2001 From: Zanie Date: Thu, 16 Nov 2023 09:49:13 -0600 Subject: [PATCH] Fix false positive for `S608` where SQL-like expression follows other text --- .../resources/test/fixtures/flake8_bandit/S608.py | 3 ++- .../flake8_bandit/rules/hardcoded_sql_expression.rs | 12 ++++++++++-- ...r__rules__flake8_bandit__tests__S608_S608.py.snap | 2 +- 3 files changed, 13 insertions(+), 4 deletions(-) diff --git a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S608.py b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S608.py index b4c2aba044..9420427994 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S608.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S608.py @@ -101,4 +101,5 @@ query = "INSERT table VALUES (%s)" % (var,) query = "REPLACE INTO table VALUES (%s)" % (var,) query = "REPLACE table VALUES (%s)" % (var,) -query = "Deselect something that is not SQL even though it has a ' from ' somewhere in %s." % "there" +not_a_query = "Deselect something that is not SQL even though it has a ' from ' somewhere in %s." % "there" +not_a_query = f"Please select a value from the list of possible variants: {variants}." diff --git a/crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs b/crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs index d54e0a0b23..f26e3607f4 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs +++ b/crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs @@ -13,7 +13,15 @@ use crate::checkers::ast::Checker; use super::super::helpers::string_literal; static SQL_REGEX: Lazy = Lazy::new(|| { - Regex::new(r"(?i)\b(select\s.+\sfrom\s|delete\s+from\s|(insert|replace)\s.+\svalues\s|update\s.+\sset\s)") + // We pass this generated expression strings like: + // "SELECT " + val + " FROM " + table + // f'delete from table where var = {var}' + // "\n SELECT *\n FROM table\n WHERE var = {}\n ".format(var) + // + // To avoid false positives, we: + // - Require the SQL to be at the start of the expression, allowing for tokens that are not a part of the string + // - Require whole-word matches for SQL keywords + Regex::new(r#"(?i)\A(\"|f\"|\'|f\'|\\|\\n|\s)*\b(select\s.+\sfrom\s|delete\s+from\s|(insert|replace)\s.+\svalues\s|update\s.+\sset\s)"#) .unwrap() }); @@ -51,7 +59,7 @@ fn has_string_literal(expr: &Expr) -> bool { } fn matches_sql_statement(string: &str) -> bool { - SQL_REGEX.is_match(string) + SQL_REGEX.is_match(string.trim_start()) } fn matches_string_format_expression(expr: &Expr, semantic: &SemanticModel) -> bool { diff --git a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S608_S608.py.snap b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S608_S608.py.snap index 82e67171d8..8d3e8f5a08 100644 --- a/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S608_S608.py.snap +++ b/crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S608_S608.py.snap @@ -476,7 +476,7 @@ S608.py:102:9: S608 Possible SQL injection vector through string-based query con 102 | query = "REPLACE table VALUES (%s)" % (var,) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608 103 | -104 | query = "Deselect something that is not SQL even though it has a ' from ' somewhere in %s." % "there" +104 | not_a_query = "Deselect something that is not SQL even though it has a ' from ' somewhere in %s." % "there" |