Python
/
ruff
mirror of https://github.com/astral-sh/ruff
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Avoid false-positives for list concatenations in SQL construction (#12720) 

## Summary

Closes https://github.com/astral-sh/ruff/issues/12710.
This commit is contained in:
Charlie Marsh 2024-08-06 16:26:03 -04:00 committed by GitHub
parent aae9619d3d
commit 90e5bc2bd9
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 118 additions and 21 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

8
crates/ruff_linter/resources/test/fixtures/flake8_bandit/S608.py vendored
View File

@ -102,3 +102,11 @@ 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"
# # 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"])

117
crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs
View File

@ -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),
}
}

14
crates/ruff_linter/src/rules/flake8_bandit/snapshots/ruff_linter__rules__flake8_bandit__tests__S608_S608.py.snap
View File

@ -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
|