[`flake8-bandit`] Detect patterns from multi line SQL statements (`S608`) #13574

DataEnggNerd · 2024-09-30T17:47:38Z

Summary

The regex written for identifying possible sql injections was not identifying some patterns, mostly multiple lined queries.

Fixes include:

updated regex with the pattern from https://github.com/PyCQA/bandit/blob/4ac55dfaaacf2083497831294b2dcd7e679f8428/bandit/plugins/injection_sql.py
removed concatenated_f_string function, which was escaping \n

Fixes #12044

Test Plan

Added reported patterns from [FALSE NEGATIVE] "S608 Possible SQL injection" #12044 to S608.py test fixtures.

github-actions · 2024-09-30T18:01:13Z

`ruff-ecosystem` results

Linter (stable)

ℹ️ ecosystem check detected linter changes. (+5 -1 violations, +0 -0 fixes in 3 projects; 51 projects unchanged)

apache/airflow (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --no-preview --select ALL

+ providers/tests/system/google/cloud/dataproc_metastore/example_dataproc_metastore_hive_partition_sensor.py:105:35: S608 Possible SQL injection vector through string-based query construction

apache/superset (+4 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --no-preview --select ALL

- superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:871:9: S608 Possible SQL injection vector through string-based query construction
+ tests/unit_tests/databases/filters_test.py:125:12: S608 Possible SQL injection vector through string-based query construction
+ tests/unit_tests/jinja_context_test.py:477:12: S608 Possible SQL injection vector through string-based query construction
+ tests/unit_tests/jinja_context_test.py:485:12: S608 Possible SQL injection vector through string-based query construction
+ tests/unit_tests/jinja_context_test.py:493:12: S608 Possible SQL injection vector through string-based query construction

pandas-dev/pandas (+0 -0 violations, +0 -0 fixes)

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
S608	6	5	1	0	0

Linter (preview)

ℹ️ ecosystem check detected linter changes. (+5 -1 violations, +0 -0 fixes in 2 projects; 52 projects unchanged)

apache/airflow (+1 -0 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

+ providers/tests/system/google/cloud/dataproc_metastore/example_dataproc_metastore_hive_partition_sensor.py:105:35: S608 Possible SQL injection vector through string-based query construction

apache/superset (+4 -1 violations, +0 -0 fixes)

ruff check --no-cache --exit-zero --ignore RUF9 --output-format concise --preview --select ALL

- superset/migrations/versions/2022-04-01_14-38_a9422eeaae74_new_dataset_models_take_2.py:871:9: S608 Possible SQL injection vector through string-based query construction
+ tests/unit_tests/databases/filters_test.py:125:12: S608 Possible SQL injection vector through string-based query construction
+ tests/unit_tests/jinja_context_test.py:477:12: S608 Possible SQL injection vector through string-based query construction
+ tests/unit_tests/jinja_context_test.py:485:12: S608 Possible SQL injection vector through string-based query construction
+ tests/unit_tests/jinja_context_test.py:493:12: S608 Possible SQL injection vector through string-based query construction

Changes by rule (1 rules affected)

code	total	+ violation	- violation	+ fix	- fix
S608	6	5	1	0	0

DataEnggNerd · 2024-09-30T18:06:31Z

@MichaReiser Can you review the changes please?

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

DataEnggNerd · 2024-10-10T03:59:05Z

⚠️ Do not consider previous commit for review. Something messed up and I have accepted wrong snap.

MichaReiser · 2024-10-13T14:04:02Z

@DataEnggNerd is the PR now ready for review again or are there still issues with the snapshots?

DataEnggNerd · 2024-10-14T14:57:24Z

@DataEnggNerd is the PR now ready for review again or are there still issues with the snapshots?

@MichaReiser I am facing trouble in terms of test cases. When I revert to last version in which @dhruvmanila has given reviews, only 37 out of 58 patterns are detected. Not able to understand why such changes are happening.

Additionally, with the changes recommended, 52 test patterns are identified.

MichaReiser · 2024-10-14T16:04:13Z

@DataEnggNerd I suggest reverting the regex changes. Reverting them makes most diagnostic reappear. Could you let me know which diagnostics are missing when using the existing regex? It would help me helping you :)

DataEnggNerd · 2024-10-14T18:12:19Z

@MichaReiser
Below are the few of the patterns not being identified

query10 = f"DELETE FROM table WHERE var = {var}"
query18 = f"UPDATE {table} SET var = {var}"
query23 = f"select * from table where var = {var}"
query28 = f"delete from table where var = {var}"
query32 = f"insert into {table} values var = {var}"
query36 = f"update {table} set var = {var}"
query43 = cursor.execute(f"SELECT * FROM table WHERE var = {var}")

MichaReiser · 2024-10-15T06:16:18Z

@DataEnggNerd would you mind pushing your latest changes? The PR still shows changes to the Regex and it comments out f-string handling.

I did copy paste the examples into Regex101 and it at least matches the first example (I didnt' test th other examples)

DataEnggNerd · 2024-10-15T19:57:44Z

@MichaReiser I have pushed the changes.
This version of code is able to identify 48 patterns only.
Unidentified patterns

def query40():
    return f"""
    SELECT *
    FROM table
    WHERE var = {var}
    """

query46 = "INSERT table VALUES (%s)" % (var,)

# # REPLACE (e.g. MySQL and derivatives, SQLite)
query47 = "REPLACE INTO table VALUES (%s)" % (var,)
query48 = "REPLACE table VALUES (%s)" % (var,)

def query52():
    return f"""
SELECT {var}
    FROM bar
    """

def query53():
    return f"""
    SELECT
        {var}
    FROM bar
    """

def query54():
    return f"""
    SELECT {var}
    FROM
        bar
    """

query56 = f"""SELECT *
FROM {var}.table
"""

query57 = f"""
SELECT *
FROM {var}.table
"""

query58 = f"SELECT\
 * FROM {var}.table"

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

DataEnggNerd · 2024-10-16T19:11:12Z

@MichaReiser @dhruvmanila Sorry to bug you again.
Tests are failing related to salsa crate 😓 . Not able to figure out how to handle this

error[E0463]: can't find crate for `salsa`
 --> crates/ruff_db/src/files.rs:6:5
  |
6 | use salsa::{Durability, Setter};
  |     ^^^^^ can't find crate

DataEnggNerd · 2024-10-17T02:43:16Z

@MichaReiser @dhruvmanila Sorry to bug you again. Tests are failing related to salsa crate 😓 . Not able to figure out how to handle this
error[E0463]: can't find crate for `salsa`
 --> crates/ruff_db/src/files.rs:6:5
  |
6 | use salsa::{Durability, Setter};
  |     ^^^^^ can't find crate

This issue is resolved. Seems to be something irrelevant to the changes in this PR.

dhruvmanila

Thanks for your patience. There are only a few minor changes required but otherwise this looks good to go.

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

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

MichaReiser

Thanks for your persistent work on this.
I went ahead and addressed @dhruvmanila's feedback. Let's merge this PR :)

DataEnggNerd · 2024-10-17T07:13:30Z

@MichaReiser @dhruvmanila
Thank for the detailed guidance throughout this issue.
Looking forward to contribute more, with lesser noob questions 😅.

Santhosh Solomon and others added 10 commits August 24, 2024 01:16

change in regex for S608

ce94a47

verified rege pattern

a565821

test python file

58041ff

implemented regex from bandit with minor modifications

9259cf9

changes suggested in #12044 (comment); failing tests

2573591

Merge branch 'astral-sh:main' into main

2d8e9bf

Merge branch 'astral-sh:main' into main

60c6dea

implemented regex from bandit; resolves #12044

982b80b

linter fix; resolved failed ci pipeline

ef7f033

updated S608 test snapshot for failing CI pipelines

15bffed

dhruvmanila requested changes Oct 1, 2024

View reviewed changes

crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs Outdated Show resolved Hide resolved

crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs Outdated Show resolved Hide resolved

dhruvmanila added the bug Something isn't working label Oct 1, 2024

DataEnggNerd added 2 commits October 10, 2024 08:39

resolves review comments from #13574

3a53e12

resolves review comments from #13574

fce1f53

Merge branch 'astral-sh:main' into main

76e8616

reverting to original changes

91138d8

changes as per review

81fbab3

dhruvmanila reviewed Oct 16, 2024

View reviewed changes

crates/ruff_linter/src/rules/flake8_bandit/rules/hardcoded_sql_expression.rs Outdated Show resolved Hide resolved

dhruvmanila changed the title ~~[flake8-bandit] Detect patterns from multi line SQL statements (S608)~~ [flake8-bandit] Detect patterns from multi line SQL statements (S608) Oct 16, 2024

DataEnggNerd added 2 commits October 17, 2024 00:10

resolves #12044

a555c51

Merge branch 'astral-sh:main' into main

f6b4531

sync with ruff 0.6.9

e3eab03

AlexWaygood closed this Oct 17, 2024

AlexWaygood reopened this Oct 17, 2024

dhruvmanila reviewed Oct 17, 2024

View reviewed changes

MichaReiser added 3 commits October 17, 2024 07:33

Revert some smaller snapshot changes

bc16c72

Add word boundary selector and add test for it

ef6d007

Restore comment

b0e002e

MichaReiser approved these changes Oct 17, 2024

View reviewed changes

MichaReiser enabled auto-merge (squash) October 17, 2024 05:37

MichaReiser merged commit 4ea4bbb into astral-sh:main Oct 17, 2024
18 checks passed

BrewTestBot mentioned this pull request Oct 17, 2024

ruff 0.7.0 Homebrew/homebrew-core#194818

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[`flake8-bandit`] Detect patterns from multi line SQL statements (`S608`) #13574

[`flake8-bandit`] Detect patterns from multi line SQL statements (`S608`) #13574

DataEnggNerd commented Sep 30, 2024 •

edited by MichaReiser

Loading

github-actions bot commented Sep 30, 2024 •

edited

Loading

DataEnggNerd commented Sep 30, 2024

DataEnggNerd commented Oct 10, 2024

MichaReiser commented Oct 13, 2024

DataEnggNerd commented Oct 14, 2024 •

edited

Loading

MichaReiser commented Oct 14, 2024

DataEnggNerd commented Oct 14, 2024

MichaReiser commented Oct 15, 2024

DataEnggNerd commented Oct 15, 2024

DataEnggNerd commented Oct 16, 2024

DataEnggNerd commented Oct 17, 2024

dhruvmanila left a comment

MichaReiser left a comment

DataEnggNerd commented Oct 17, 2024

[flake8-bandit] Detect patterns from multi line SQL statements (S608) #13574

[flake8-bandit] Detect patterns from multi line SQL statements (S608) #13574

Conversation

DataEnggNerd commented Sep 30, 2024 • edited by MichaReiser Loading

Summary

Test Plan

github-actions bot commented Sep 30, 2024 • edited Loading

ruff-ecosystem results

Linter (stable)

Linter (preview)

DataEnggNerd commented Sep 30, 2024

DataEnggNerd commented Oct 10, 2024

MichaReiser commented Oct 13, 2024

DataEnggNerd commented Oct 14, 2024 • edited Loading

MichaReiser commented Oct 14, 2024

DataEnggNerd commented Oct 14, 2024

MichaReiser commented Oct 15, 2024

DataEnggNerd commented Oct 15, 2024

DataEnggNerd commented Oct 16, 2024

DataEnggNerd commented Oct 17, 2024

dhruvmanila left a comment

Choose a reason for hiding this comment

MichaReiser left a comment

Choose a reason for hiding this comment

DataEnggNerd commented Oct 17, 2024

[`flake8-bandit`] Detect patterns from multi line SQL statements (`S608`) #13574

[`flake8-bandit`] Detect patterns from multi line SQL statements (`S608`) #13574

DataEnggNerd commented Sep 30, 2024 •

edited by MichaReiser

Loading

github-actions bot commented Sep 30, 2024 •

edited

Loading

`ruff-ecosystem` results

DataEnggNerd commented Oct 14, 2024 •

edited

Loading