Skip to content

feat(rules): add P005 sqlalchemy-text-fstring rule#25

Merged
Pawansingh3889 merged 4 commits intoPawansingh3889:mainfrom
tmchow:osc/10-p005-sqlalchemy-text-fstring
Apr 25, 2026
Merged

feat(rules): add P005 sqlalchemy-text-fstring rule#25
Pawansingh3889 merged 4 commits intoPawansingh3889:mainfrom
tmchow:osc/10-p005-sqlalchemy-text-fstring

Conversation

@tmchow
Copy link
Copy Markdown
Contributor

@tmchow tmchow commented Apr 23, 2026

Summary

Closes #10. Adds rule P005 sqlalchemy-text-fstring, which catches sqlalchemy.text(f"...{var}") -- the same SQL-injection hazard P001 catches for cursor.execute(), but on the sqlalchemy.text() surface.

from sqlalchemy import text

# Flagged: P005 (error)
conn.execute(text(f"SELECT * FROM users WHERE id = {user_id}"))

# OK:
conn.execute(
    text("SELECT * FROM users WHERE id = :id"),
    {"id": user_id},
)

Why this matters

sqlalchemy.text() is the documented escape hatch for raw SQL in SQLAlchemy. Wrapping an f-string inside it defeats parameter binding and re-introduces the exact SQL-injection risk that P001-P004 catch on cursor.execute(). "text" was already registered in the scanner's EXECUTE_METHODS frozenset, so P001 silently fired on these cases with a generic .text() message; P005 replaces that with a sqlalchemy-specific message and suggestion so the warning lands correctly for the reader.

Changes

  • sql_guard/rules/python_rules.py: new SqlalchemyTextFstring class (P005) checking hit.kind == "fstring" and hit.call_name == "text"; added to PYTHON_RULES. Severity error, matches P001 + what the issue asked for.
  • sql_guard/rules/python_rules.py: P001's FStringInExecute.check now skips when call_name == "text". This mirrors the existing P004 pattern (BareVariableInExecute already has call_name != "text" in its guard) and prevents P001+P005 double-firing on the same line.
  • tests/fixtures/python_hazards.py: new unsafe_sqlalchemy_text_fstring function as the fixture for P005.
  • tests/test_python_scanner.py: new test_p005_sqlalchemy_text_fstring_flagged (severity + message check), test_p001_skips_text_to_avoid_double_firing_with_p005 (regression guard), and the end-to-end test_check_with_include_python_surfaces_p_rules now asserts P005 is in the surfaced rule ID set.
  • Docstring header in python_rules.py bumped from "P001-P004" to "P001-P005".

Testing

  • pytest tests/test_python_scanner.py -v: 23 passed, including the 3 new P005-related tests.
  • pytest -q (full suite): 106 passed.
  • ruff check sql_guard/ tests/: clean (caught an unused local during self-review, now removed).

Implementation notes

The issue said to "Model after P001 fstring-in-execute"; this PR does that structurally (same class shape, same check signature, same severity). The one extra change beyond a plain clone is the P001 skip-on-text guard, which keeps the findings output clean for sqlalchemy.text(f"...") callers -- one P005 finding per site, not a P001+P005 pair.

Estimated LOC in the issue: ~30 code + ~25 test. Actual: ~22 code + ~20 test (matches the estimate closely).


This contribution was developed with AI assistance (Claude Code).

Compound Engineering

tmchow added 2 commits April 23, 2026 03:45
Closes Pawansingh3889#10. Adds P005 to catch sqlalchemy.text(f"...{var}") which
defeats parameter binding just like P001's f-string-in-execute case,
but on the sqlalchemy.text() surface instead of cursor.execute().

P005 fires on hit.kind == 'fstring' AND hit.call_name == 'text',
mirroring the P001 structure the maintainer asked me to model after.
P001 now skips when call_name == 'text' to avoid double-firing with
P005 (mirrors how P004 already excludes 'text' from its bare-variable
check).

Test coverage:
- new fixture in tests/fixtures/python_hazards.py: text(f'...')
- new test: P005 fires with error severity and sqlalchemy.text() in
  the message
- new test: P001 no longer fires on text() calls (regression guard
  against double-firing)
- updated end-to-end test to assert P005 is included in the detected
  rule set when include_python=True

106 tests pass.
Codex review flagged F841 on the unused `text_line_findings` local in
test_p001_skips_text_to_avoid_double_firing_with_p005. The assertion
was already going through the loop below -- the intermediate list just
carried noise. Removed it so the pre-commit ruff hook stays clean.
@tmchow tmchow requested a review from Pawansingh3889 as a code owner April 23, 2026 10:48
Addresses the pr-sop governance check that flagged this PR for a
missing CHANGELOG update (Pawansingh3889#10 added a new rule, and the repo's
pr-sop config requires changelog documentation for rule additions).
@Pawansingh3889
Copy link
Copy Markdown
Owner

Thanks tmchow, this is good work. The P004 call_name != "text" guard precedent is well-spotted, adding the same skip to P001 keeps findings clean instead of double-firing on the same line. Exactly the right move, and the regression test for that double-fire case is the kind of thing that prevents a future maintainer from undoing it accidentally.

There's a small CHANGELOG conflict because #26 just merged a W016 entry into the same [Unreleased] section. Easy to resolve from the web editor (keep both lines), so no need for you to push anything more. I'll handle that and squash-merge after.

Thanks again for the careful PR.

@Pawansingh3889 Pawansingh3889 merged commit 221adb7 into Pawansingh3889:main Apr 25, 2026
6 checks passed
@Pawansingh3889 Pawansingh3889 mentioned this pull request Apr 25, 2026
11 tasks
Pawansingh3889 added a commit that referenced this pull request Apr 27, 2026
Trevin Chow shipped two rules pre-v0.6 that were missed when the
Contributors section first landed:

- W011 union-without-all (PR #12)
- P005 sqlalchemy-text-fstring (PR #25), one of the five Python rules

Reordered the table chronologically by first contribution so the
thank-you reads in joining order: tmchow, mvanhorn, Prabhu-1409.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rule: P005 sqlalchemy-text-fstring - warn on sqlalchemy.text(f"...{var}")

2 participants