feat(rules): add P005 sqlalchemy-text-fstring rule#25
Merged
Pawansingh3889 merged 4 commits intoPawansingh3889:mainfrom Apr 25, 2026
Merged
Conversation
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.
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).
Owner
|
Thanks tmchow, this is good work. The P004 There's a small CHANGELOG conflict because #26 just merged a W016 entry into the same Thanks again for the careful PR. |
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #10. Adds rule P005
sqlalchemy-text-fstring, which catchessqlalchemy.text(f"...{var}")-- the same SQL-injection hazard P001 catches forcursor.execute(), but on the sqlalchemy.text() surface.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 oncursor.execute(). "text" was already registered in the scanner'sEXECUTE_METHODSfrozenset, 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: newSqlalchemyTextFstringclass (P005) checkinghit.kind == "fstring" and hit.call_name == "text"; added toPYTHON_RULES. Severityerror, matches P001 + what the issue asked for.sql_guard/rules/python_rules.py: P001'sFStringInExecute.checknow skips whencall_name == "text". This mirrors the existing P004 pattern (BareVariableInExecutealready hascall_name != "text"in its guard) and prevents P001+P005 double-firing on the same line.tests/fixtures/python_hazards.py: newunsafe_sqlalchemy_text_fstringfunction as the fixture for P005.tests/test_python_scanner.py: newtest_p005_sqlalchemy_text_fstring_flagged(severity + message check),test_p001_skips_text_to_avoid_double_firing_with_p005(regression guard), and the end-to-endtest_check_with_include_python_surfaces_p_rulesnow assertsP005is in the surfaced rule ID set.python_rules.pybumped 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
checksignature, same severity). The one extra change beyond a plain clone is the P001 skip-on-textguard, which keeps the findings output clean forsqlalchemy.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).