fix: don't flag parameterized cursor.execute() as SQL injection

[mvanhorn]

Summary

The python-sql-raw-001 glob *cursor.execute(*%* treated any % in the call text as string interpolation, so canonical DB-API parameter binding (cursor.execute("... WHERE firm_id = %s", [firm_id])) was reported as Critical SQL Injection. The reporter measured ~60% of this rule's findings on a ~5k-file production codebase as this false positive (#35).

This replaces the glob with a regex: pattern (Pattern Type 4 in the rule guide) that only fires when % is a formatting operator after a complete query expression, a closing quote or a bare identifier, instead of matching %s/%(name)s placeholders inside the string literal. Placeholders bound via execute()'s second argument now pass clean; every interpolation form still flags.

Regression coverage both ways, wired into the acceptance harness:

• false_positive_floor.feature: parameterized %s, %(name)s, and a bound f"%{name}%" LIKE argument produce zero findings
• python_injection.feature: spaced "..." % x, unspaced "..."%x, variable-held q % x, f-string, concatenation, and .format() all still report SQL Injection

One part of #35 is intentionally not covered here: the rule's *cursor.execute(*.format(* pattern still flags psycopg's safe sql.SQL(...).format(...) composition API. Search rules have no pattern-level exclude mechanism, so that case needs its own treatment rather than being bundled into this fix.

Verified with a rebuilt binary against the issue's exact repro: Found 0 vulnerabilities. cargo test --test acceptance passes 15/15 scenarios.

Related issue

Closes #35

Checklist

• cargo build --release succeeds
• Ran any test harness relevant to this change (note results; suite under repair) - cargo test --test acceptance: 15 scenarios / 52 steps pass, including the 7 new scenarios covering this rule
• Updated docs/rules where relevant - the change is to rules/python/sql_injection.ron itself, with an inline comment explaining what the pattern matches

[asadeddin]

Thank you for the second PR. The rule change makes sense. I've tagged some of our reviews to chime in.

[yhoztak]

Can we add these to the same sql test file for readability and coverages?

like:

def percent_spaced(cursor, user_id):
    cursor.execute("SELECT * FROM users WHERE id = %s" % user_id)


def percent_unspaced(cursor, user_id):
    cursor.execute("SELECT * FROM users WHERE id = %s"%user_id)


def percent_variable(cursor, query, user_id):
    cursor.execute(query % user_id)


def fstring_interpolation(cursor, user_id):
    cursor.execute(f"SELECT * FROM users WHERE id = {user_id}")


def string_concatenation(cursor, user_id):
    cursor.execute("SELECT * FROM users WHERE id = " + user_id)


def format_interpolation(cursor, user_id):
    cursor.execute("SELECT * FROM users WHERE id = {}".format(user_id))

[yhoztak]

We are cleaning up test folder structures starting from python and PR is up here fyi #43
My apology for not having a clean structure to follow

[yhoztak]

#43 is now merged. If there's a new fixture files for test cases, can you move them under https://github.com/Corgea/Sighthound/tree/main/tests/test_files/python/fixtures ?
As an example for SQL Injection, add please add new functions to https://github.com/Corgea/Sighthound/blob/main/tests/test_files/python/fixtures/sql_injection_variants.py

[yhoztak]

We try to avoid very complex regex like this as it may become too specific and hard to understand it.
We could have separate AST rule here instead.

As an example:

(
    rules: [
        // Covers `"..." % x` and `"..." + x` passed to cursor.execute.
        // Parameterized execute("...%s", params) has a string as arg0, not
        // a binary_operator, so it is not matched.
        (
            id: Some("python-sql-raw-binop"),
            name: Some("Raw SQL Execution (binary-built query)"),
            category: Some("database"),
            mode: "search",
            patterns: Some([
                "cursor.execute",
            ]),
            conditions: Some([
                (
                    field: "argument",
                    operator: "matches",
                    value: "",
                    condition_type: Some("has_argument"),
                    argument_position: Some(0),
                    node_type: Some("binary_operator"),
                ),
            ]),
            finding_type: Some("SQL Injection"),
            severity: Some("Critical"),
            confidence: Some("Medium"),
            cwe_id: Some("cwe-89"),
            description: Some("Raw SQL query executed from a dynamically-built string (string formatting/concatenation)"),
            file_types: Some((
                extensions: Some([".py"])
            )),
            tags: Some(["sql", "injection", "database"])
        ),

        // Remaining inline string-building shapes the binary-operator rule
        // does not cover (f-strings, str.format, raw-query helpers).
        (
            id: Some("python-sql-raw-001"),
            name: Some("Raw SQL Execution"),
            category: Some("database"),
            mode: "search",
            patterns: Some([
                "execute_raw_query(",
                "*cursor.execute(*.format(*",
                "*cursor.execute(f\"*",
                "*cursor.execute(f'*",
                "*.executescript(*",
            ]),
            finding_type: Some("SQL Injection"),
            severity: Some("Critical"),
            confidence: Some("Medium"),
            cwe_id: Some("cwe-89"),
            description: Some("Raw SQL query executed from a dynamically-built string (string formatting/concatenation)"),
            file_types: Some((
                extensions: Some([".py"])
            )),
            tags: Some(["sql", "injection", "database"])
        ),

        // Taint: user input flowing into a SQL execute sink (catches variable-built
        // queries that aren't visible as a literal at the call site).
        (
            id: Some("python-sql-taint-001"),
            name: Some("SQL Injection Taint Flow"),
            category: Some("database"),
            mode: "taint",
            sources: Some([
                "request.",
                "input(",
                "raw_input(",
                "flask.request",
            ]),
            sinks: Some([
                "execute(",
                "executemany(",
                "executescript(",
            ]),
            sanitizers: Some([
                "parameterized",
            ]),
            finding_type: Some("SQL Injection"),
            severity: Some("Critical"),
            confidence: Some("High"),
            cwe_id: Some("cwe-89"),
            description: Some("User input flows to SQL execution without parameterization"),
            file_types: Some((
                extensions: Some([".py"])
            )),
            tags: Some(["sql", "injection", "taint", "database"])
        ),
    ]
)

[juangaitanv]

The trailing %\s*[^\s,)] excludes (, so tuple formatting such as cursor.execute("SELECT ... %s" % (user_id,)) no longer matches. This leaves a common SQL-injection pattern undetected; allow ( and add a tuple-RHS fixture.