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 b4c2aba044a88..68296f7182af9 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S608.py +++ b/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"]) 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 ff892e6b3f962..375f295baa5a5 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 @@ -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 { + 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::>(); + 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), + } +} 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 82e67171d8203..3228178f1d419 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 @@ -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 + |