From 4ea4bbb155237d593785090f00ebdc1a5cd9b6a1 Mon Sep 17 00:00:00 2001 From: Santhosh Solomon Date: Thu, 17 Oct 2024 11:12:03 +0530 Subject: [PATCH] [`flake8-bandit`] Detect patterns from multi line SQL statements (`S608`) (#13574) Co-authored-by: Santhosh Solomon Co-authored-by: Micha Reiser --- .../test/fixtures/flake8_bandit/S608.py | 56 ++++++++- .../rules/hardcoded_sql_expression.rs | 12 +- ...s__flake8_bandit__tests__S608_S608.py.snap | 115 +++++++++++++++--- 3 files changed, 154 insertions(+), 29 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 68296f7182..5377eb264c 100644 --- a/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S608.py +++ b/crates/ruff_linter/resources/test/fixtures/flake8_bandit/S608.py @@ -95,18 +95,62 @@ cursor.execute('SELECT * FROM table WHERE id = 1') cursor.executemany('SELECT * FROM table WHERE id = %s', [var, var2]) # # INSERT without INTO (e.g. MySQL and derivatives) -query = "INSERT table VALUES (%s)" % (var,) +query46 = "INSERT table VALUES (%s)" % (var,) # # REPLACE (e.g. MySQL and derivatives, SQLite) -query = "REPLACE INTO table VALUES (%s)" % (var,) -query = "REPLACE table VALUES (%s)" % (var,) +query47 = "REPLACE INTO table VALUES (%s)" % (var,) +query48 = "REPLACE table VALUES (%s)" % (var,) -query = "Deselect something that is not SQL even though it has a ' from ' somewhere in %s." % "there" +query49 = "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"]) +"SELECT * FROM " + ("table1" if x > 0 else "table2") # query50 +"SELECT * FROM " + ("table1" if x > 0 else ["table2"]) # query51 + +# test cases from #12044 + +def query52(): + return f""" +SELECT {var} + FROM bar + """ + +def query53(): + return f""" + SELECT + {var} + FROM bar + """ + +def query54(): + return f""" + SELECT {var} + FROM + bar + """ + +query55 = f"""SELECT * FROM +{var}.table +""" + +query56 = f"""SELECT * +FROM {var}.table +""" + +query57 = f""" +SELECT * +FROM {var}.table +""" + +query57 = f""" +PRESELECT * +FROM {var}.table +""" + +# to be handled seperately +# query58 = f"SELECT\ +# * FROM {var}.table" 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 375f295baa..670c7d406f 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 @@ -1,18 +1,17 @@ use once_cell::sync::Lazy; use regex::Regex; -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::str::raw_contents; +use ruff_python_ast::{self as ast, Expr, Operator}; use ruff_source_file::Locator; use ruff_text_size::Ranged; use crate::checkers::ast::Checker; 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)") - .unwrap() + Regex::new(r"(?i)\b(select\s+.*\s+from\s|delete\s+from\s|(insert|replace)\s+.*\s+values\s|update\s+.*\s+set\s)") + .unwrap() }); /// ## What it does @@ -88,6 +87,7 @@ pub(crate) fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) { }; string.value.to_str().escape_default().to_string() } + // f"select * from table where val = {val}" Expr::FString(f_string) => concatenated_f_string(f_string, checker.locator()), _ => return, @@ -113,9 +113,7 @@ pub(crate) fn hardcoded_sql_expression(checker: &mut Checker, expr: &Expr) { 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()) - }) + .filter_map(|part| raw_contents(locator.slice(part))) .collect() } 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 3228178f1d..b44cc8f786 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 @@ -452,45 +452,128 @@ S608.py:86:30: S608 Possible SQL injection vector through string-based query con 88 | # # pass | -S608.py:98:9: S608 Possible SQL injection vector through string-based query construction +S608.py:98:11: S608 Possible SQL injection vector through string-based query construction | 97 | # # INSERT without INTO (e.g. MySQL and derivatives) - 98 | query = "INSERT table VALUES (%s)" % (var,) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608 + 98 | query46 = "INSERT table VALUES (%s)" % (var,) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608 99 | 100 | # # REPLACE (e.g. MySQL and derivatives, SQLite) | -S608.py:101:9: S608 Possible SQL injection vector through string-based query construction +S608.py:101:11: S608 Possible SQL injection vector through string-based query construction | 100 | # # REPLACE (e.g. MySQL and derivatives, SQLite) -101 | query = "REPLACE INTO table VALUES (%s)" % (var,) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608 -102 | query = "REPLACE table VALUES (%s)" % (var,) +101 | query47 = "REPLACE INTO table VALUES (%s)" % (var,) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608 +102 | query48 = "REPLACE table VALUES (%s)" % (var,) | -S608.py:102:9: S608 Possible SQL injection vector through string-based query construction +S608.py:102:11: S608 Possible SQL injection vector through string-based query construction | 100 | # # REPLACE (e.g. MySQL and derivatives, SQLite) -101 | query = "REPLACE INTO table VALUES (%s)" % (var,) -102 | query = "REPLACE table VALUES (%s)" % (var,) - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608 +101 | query47 = "REPLACE INTO table VALUES (%s)" % (var,) +102 | query48 = "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 | query49 = "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") +111 | "SELECT * FROM " + ("table1" if x > 0 else "table2") # query50 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608 -112 | "SELECT * FROM " + ("table1" if x > 0 else ["table2"]) +112 | "SELECT * FROM " + ("table1" if x > 0 else ["table2"]) # query51 | 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"]) +111 | "SELECT * FROM " + ("table1" if x > 0 else "table2") # query50 +112 | "SELECT * FROM " + ("table1" if x > 0 else ["table2"]) # query51 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ S608 +113 | +114 | # test cases from #12044 + | + +S608.py:117:12: S608 Possible SQL injection vector through string-based query construction + | +116 | def query52(): +117 | return f""" + | ____________^ +118 | | SELECT {var} +119 | | FROM bar +120 | | """ + | |_______^ S608 +121 | +122 | def query53(): + | + +S608.py:123:12: S608 Possible SQL injection vector through string-based query construction + | +122 | def query53(): +123 | return f""" + | ____________^ +124 | | SELECT +125 | | {var} +126 | | FROM bar +127 | | """ + | |_______^ S608 +128 | +129 | def query54(): + | + +S608.py:130:12: S608 Possible SQL injection vector through string-based query construction + | +129 | def query54(): +130 | return f""" + | ____________^ +131 | | SELECT {var} +132 | | FROM +133 | | bar +134 | | """ + | |_______^ S608 +135 | +136 | query55 = f"""SELECT * FROM + | + +S608.py:136:11: S608 Possible SQL injection vector through string-based query construction + | +134 | """ +135 | +136 | query55 = f"""SELECT * FROM + | ___________^ +137 | | {var}.table +138 | | """ + | |___^ S608 +139 | +140 | query56 = f"""SELECT * + | + +S608.py:140:11: S608 Possible SQL injection vector through string-based query construction + | +138 | """ +139 | +140 | query56 = f"""SELECT * + | ___________^ +141 | | FROM {var}.table +142 | | """ + | |___^ S608 +143 | +144 | query57 = f""" + | + +S608.py:144:11: S608 Possible SQL injection vector through string-based query construction + | +142 | """ +143 | +144 | query57 = f""" + | ___________^ +145 | | SELECT * +146 | | FROM {var}.table +147 | | """ + | |___^ S608 +148 | +149 | query57 = f""" |