-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
fix(detectors): Add comment filtering to SQL Injection detector #95006
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅ ✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## master #95006 +/- ##
==========================================
+ Coverage 77.03% 84.27% +7.23%
==========================================
Files 10478 10462 -16
Lines 605927 605273 -654
Branches 23671 23569 -102
==========================================
+ Hits 466759 510067 +43308
+ Misses 136783 94682 -42101
+ Partials 2385 524 -1861 |
fixtures/events/performance_problems/sql-injection/sql-injection-test-comment.json
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bug: SQL Injection Detector Fails String Literal Handling
The SQL injection detector incorrectly identifies SQL comments due to its simple find("--") method. This fails to account for -- appearing within SQL string literals (e.g., 'value--text'), where it is part of the string, not a comment. Consequently, the detector treats the string literal as a comment boundary, excluding the subsequent query portion from analysis. This leads to false negatives, missing actual SQL injection vulnerabilities.
src/sentry/performance_issues/detectors/sql_injection_detector.py#L130-L131
sentry/src/sentry/performance_issues/detectors/sql_injection_detector.py
Lines 130 to 131 in 052a444
| description_after_where = description[where_index:] | |
| comment_index = description_after_where.find("--") |
Was this report helpful? Give feedback by reacting with 👍 or 👎
sometimes there's a comment at the end of the SQL statement. if an org puts the url with the query parameters for example in the comment, then that would show up as a false positive because right now it only looks after `WHERE`. Now, if a comment exists, we ignore that during detection.
sometimes there's a comment at the end of the SQL statement. if an org puts the url with the query parameters for example in the comment, then that would show up as a false positive because right now it only looks after
WHERE. Now, if a comment exists, we ignore that during detection.