Avoid false-positives for list concatenations in SQL construction

crates/ruff_linter/resources/test/fixtures/flake8_bandit/S608.py

@@ -102,3 +102,11 @@ def query41():
 query = "REPLACE table VALUES (%s)" % (var,)
 
 query = "Deselect something that is not SQL even though it has a ' from ' somewhere in %s." % "there"
+
+# # pass
+["select colA from tableA"] + ["select colB from tableB"]
+"SELECT * FROM " + (["table1"] if x > 0 else ["table2"])
+
+# # errors
+"SELECT * FROM " + ("table1" if x > 0 else "table2")
+"SELECT * FROM " + ("table1" if x > 0 else ["table2"])

crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs

@@ -4,7 +4,6 @@ use ruff_python_ast::{self as ast, Expr, Operator};
 
 use ruff_diagnostics::{Diagnostic, Violation};
 use ruff_macros::{derive_message_formats, violation};
-use ruff_python_ast::helpers::any_over_expr;
 use ruff_python_ast::str::raw_contents;
 use ruff_source_file::Locator;
 use ruff_text_size::Ranged;
@@ -45,25 +44,6 @@ impl Violation for HardcodedSQLExpression {
     }
 }
 
-/// Concatenates the contents of an f-string, without the prefix and quotes,
-/// and escapes any special characters.
-///
-/// ## Example
-///
-/// ```python
-/// "foo" f"bar {x}" "baz"
-/// ```
-///
-/// becomes `foobar {x}baz`.
-fn concatenated_f_string(expr: &ast::ExprFString, locator: &Locator) -> String {
-    expr.value
-        .iter()
-        .filter_map(|part| {
-            raw_contents(locator.slice(part)).map(|s| s.escape_default().to_string())
-        })
-        .collect()
-}
-
 /// S608
 pub(crate) fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) {
     let content = match expr {
@@ -79,7 +59,7 @@ pub(crate) fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) {
             {
                 return;
             }
-            if !any_over_expr(expr, &Expr::is_string_literal_expr) {
+            if is_explicit_concatenation(expr) != Some(true) {
                 return;
             }
             checker.generator().expr(expr)
@@ -119,3 +99,98 @@ pub(crate) fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) {
             .push(Diagnostic::new(HardcodedSQLExpression, expr.range()));
     }
 }
+
+/// Concatenates the contents of an f-string, without the prefix and quotes,
+/// and escapes any special characters.
+///
+/// ## Example
+///
+/// ```python
+/// "foo" f"bar {x}" "baz"
+/// ```
+///
+/// becomes `foobar {x}baz`.
+fn concatenated_f_string(expr: &ast::ExprFString, locator: &Locator) -> String {
+    expr.value
+        .iter()
+        .filter_map(|part| {
+            raw_contents(locator.slice(part)).map(|s| s.escape_default().to_string())
+        })
+        .collect()
+}
+
+/// Returns `Some(true)` if an expression appears to be an explicit string concatenation,
+/// `Some(false)` if it's _not_ an explicit concatenation, and `None` if it's ambiguous.
+fn is_explicit_concatenation(expr: &Expr) -> Option<bool> {
+    match expr {
+        Expr::BinOp(ast::ExprBinOp { left, right, .. }) => {
+            let left = is_explicit_concatenation(left);
+            let right = is_explicit_concatenation(right);
+            match (left, right) {
+                // If either side is definitively _not_ a string, neither is the expression.
+                (Some(false), _) | (_, Some(false)) => Some(false),
+                // If either side is definitively a string, the expression is a string.
+                (Some(true), _) | (_, Some(true)) => Some(true),
+                _ => None,
+            }
+        }
+        // Ambiguous (e.g., `x + y`).
+        Expr::Call(_) => None,
+        Expr::Subscript(_) => None,
+        Expr::Attribute(_) => None,
+        Expr::Name(_) => None,
+
+        // Non-strings.
+        Expr::Lambda(_) => Some(false),
+        Expr::List(_) => Some(false),
+        Expr::Tuple(_) => Some(false),
+        Expr::Dict(_) => Some(false),
+        Expr::Set(_) => Some(false),
+        Expr::Generator(_) => Some(false),
+        Expr::Yield(_) => Some(false),
+        Expr::YieldFrom(_) => Some(false),
+        Expr::Await(_) => Some(false),
+        Expr::Starred(_) => Some(false),
+        Expr::Slice(_) => Some(false),
+        Expr::BooleanLiteral(_) => Some(false),
+        Expr::EllipsisLiteral(_) => Some(false),
+        Expr::NumberLiteral(_) => Some(false),
+        Expr::ListComp(_) => Some(false),
+        Expr::SetComp(_) => Some(false),
+        Expr::DictComp(_) => Some(false),
+        Expr::Compare(_) => Some(false),
+        Expr::FString(_) => Some(true),
+        Expr::StringLiteral(_) => Some(true),
+        Expr::BytesLiteral(_) => Some(false),
+        Expr::NoneLiteral(_) => Some(false),
+        Expr::IpyEscapeCommand(_) => Some(false),
+
+        // Conditionally strings.
+        Expr::Named(ast::ExprNamed { value, .. }) => is_explicit_concatenation(value),
+        Expr::If(ast::ExprIf { body, orelse, .. }) => {
+            let body = is_explicit_concatenation(body);
+            let orelse = is_explicit_concatenation(orelse);
+            match (body, orelse) {
+                // If either side is definitively a string, the expression could be a string.
+                (Some(true), _) | (_, Some(true)) => Some(true),
+                // If both sides are definitively _not_ a string, neither is the expression.
+                (Some(false), Some(false)) => Some(false),
+                _ => None,
+            }
+        }
+        Expr::BoolOp(ast::ExprBoolOp { values, .. }) => {
+            let values = values
+                .iter()
+                .map(is_explicit_concatenation)
+                .collect::<Vec<_>>();
+            if values.iter().any(|v| *v == Some(true)) {
+                Some(true)
+            } else if values.iter().all(|v| *v == Some(false)) {
+                Some(false)
+            } else {
+                None
+            }
+        }
+        Expr::UnaryOp(ast::ExprUnaryOp { operand, .. }) => is_explicit_concatenation(operand),
+    }
+}

crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S608_S608.py.snap

@@ -479,4 +479,18 @@ S608.py:102:9: S608 Possible SQL injection vector through string-based query con
 104 | query = "Deselect something that is not SQL even though it has a ' from ' somewhere in %s." % "there"
     |
 
+S608.py:111:1: S608 Possible SQL injection vector through string-based query construction
+    |
+110 | # # errors
+111 | "SELECT * FROM " + ("table1" if x > 0 else "table2")
+    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608
+112 | "SELECT * FROM " + ("table1" if x > 0 else ["table2"])
+    |
 
+S608.py:112:1: S608 Possible SQL injection vector through string-based query construction
+    |
+110 | # # errors
+111 | "SELECT * FROM " + ("table1" if x > 0 else "table2")
+112 | "SELECT * FROM " + ("table1" if x > 0 else ["table2"])
+    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608
+    |