Skip to content

Commit c68c43f

Browse files
roggenkemperroggenkemper
authored
fix(detectors): Add comment filtering to SQL Injection detector (#95006)
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.
1 parent 42c7662 commit c68c43f

File tree

3 files changed

+177
-4
lines changed

3 files changed

+177
-4
lines changed

‎fixtures/events/performance_problems/sql-injection/sql-injection-test-comment.json

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
{
2+
"event_id": "5d6401994d7949d2ac3474f472564370",
3+
"platform": "node",
4+
"message": "",
5+
"datetime": "2025-05-12T22:42:38.642986+00:00",
6+
"breakdowns": {
7+
"span_ops": {
8+
"ops.db": {
9+
"value": 65.715075,
10+
"unit": "millisecond"
11+
},
12+
"total.time": {
13+
"value": 67.105293,
14+
"unit": "millisecond"
15+
}
16+
}
17+
},
18+
"request": {
19+
"url": "http://localhost:3001/vulnerable-login",
20+
"method": "GET",
21+
"query_string": [["username", "hello"]]
22+
},
23+
"spans": [
24+
{
25+
"timestamp": 1747089758.567536,
26+
"start_timestamp": 1747089758.567,
27+
"exclusive_time": 0.536203,
28+
"op": "middleware.express",
29+
"span_id": "4a06692f4abc8dbe",
30+
"parent_span_id": "91fa92ff0205967d",
31+
"trace_id": "375a86eca09a4a4e91903838dd771f50",
32+
"status": "ok",
33+
"description": "corsMiddleware",
34+
"origin": "auto.http.otel.express",
35+
"data": {
36+
"express.name": "corsMiddleware",
37+
"express.type": "middleware",
38+
"sentry.op": "middleware.express",
39+
"sentry.origin": "auto.http.otel.express"
40+
},
41+
"sentry_tags": {
42+
"user": "ip:::1",
43+
"user.ip": "::1",
44+
"environment": "production",
45+
"transaction": "GET /vulnerable-login",
46+
"transaction.method": "GET",
47+
"transaction.op": "http.server",
48+
"browser.name": "Chrome",
49+
"sdk.name": "sentry.javascript.node",
50+
"sdk.version": "9.17.0",
51+
"platform": "node",
52+
"os.name": "macOS",
53+
"category": "middleware",
54+
"op": "middleware.express",
55+
"status": "ok",
56+
"trace.status": "ok"
57+
},
58+
"hash": "e6088cf8b370ed60"
59+
},
60+
{
61+
"timestamp": 1747089758.568761,
62+
"start_timestamp": 1747089758.568,
63+
"exclusive_time": 0.761032,
64+
"op": "middleware.express",
65+
"span_id": "92553d2584d250b8",
66+
"parent_span_id": "91fa92ff0205967d",
67+
"trace_id": "375a86eca09a4a4e91903838dd771f50",
68+
"status": "ok",
69+
"description": "jsonParser",
70+
"origin": "auto.http.otel.express",
71+
"data": {
72+
"express.name": "jsonParser",
73+
"express.type": "middleware",
74+
"sentry.op": "middleware.express",
75+
"sentry.origin": "auto.http.otel.express"
76+
},
77+
"sentry_tags": {
78+
"user": "ip:::1",
79+
"user.ip": "::1",
80+
"environment": "production",
81+
"transaction": "GET /vulnerable-login",
82+
"transaction.method": "GET",
83+
"transaction.op": "http.server",
84+
"browser.name": "Chrome",
85+
"sdk.name": "sentry.javascript.node",
86+
"sdk.version": "9.17.0",
87+
"platform": "node",
88+
"os.name": "macOS",
89+
"category": "middleware",
90+
"op": "middleware.express",
91+
"status": "ok",
92+
"trace.status": "ok"
93+
},
94+
"hash": "c81e963dad9ebc6c"
95+
},
96+
{
97+
"timestamp": 1747089758.569093,
98+
"start_timestamp": 1747089758.569,
99+
"exclusive_time": 0.092983,
100+
"op": "request_handler.express",
101+
"span_id": "435146ab0909419d",
102+
"parent_span_id": "91fa92ff0205967d",
103+
"trace_id": "375a86eca09a4a4e91903838dd771f50",
104+
"status": "ok",
105+
"description": "/vulnerable-login",
106+
"origin": "auto.http.otel.express",
107+
"data": {
108+
"express.name": "/vulnerable-login",
109+
"express.type": "request_handler",
110+
"http.route": "/vulnerable-login",
111+
"sentry.op": "request_handler.express",
112+
"sentry.origin": "auto.http.otel.express"
113+
},
114+
"sentry_tags": {
115+
"user": "ip:::1",
116+
"user.ip": "::1",
117+
"environment": "production",
118+
"transaction": "GET /vulnerable-login",
119+
"transaction.method": "GET",
120+
"transaction.op": "http.server",
121+
"browser.name": "Chrome",
122+
"sdk.name": "sentry.javascript.node",
123+
"sdk.version": "9.17.0",
124+
"platform": "node",
125+
"os.name": "macOS",
126+
"op": "request_handler.express",
127+
"status": "ok",
128+
"trace.status": "ok"
129+
},
130+
"hash": "872b0c84a6f1c590"
131+
},
132+
{
133+
"timestamp": 1747089758.637715,
134+
"start_timestamp": 1747089758.572,
135+
"exclusive_time": 65.715075,
136+
"op": "db",
137+
"span_id": "4703181ac343f71a",
138+
"parent_span_id": "91fa92ff0205967d",
139+
"trace_id": "375a86eca09a4a4e91903838dd771f50",
140+
"status": "ok",
141+
"description": "SELECT * FROM users WHERE username = ? --hello",
142+
"origin": "auto.db.otel.mysql2",
143+
"data": {
144+
"db.system": "mysql",
145+
"db.connection_string": "jdbc:mysql://localhost:3306/injection_test",
146+
"db.name": "injection_test",
147+
"db.statement": "SELECT * FROM users WHERE username = ? --hello",
148+
"db.user": "root",
149+
"net.peer.name": "localhost",
150+
"net.peer.port": 3306,
151+
"otel.kind": "CLIENT",
152+
"sentry.op": "db",
153+
"sentry.origin": "auto.db.otel.mysql2"
154+
},
155+
"hash": "45330ba0cafa5997"
156+
}
157+
]
158+
}

‎src/sentry/performance_issues/detectors/sql_injection_detector.py

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -126,11 +126,22 @@ def visit_span(self, span: Span) -> None:
126126
regex_key = rf'(?<![\w.$])"?{re.escape(key)}"?(?![\w.$"])'
127127
regex_value = rf"(?<![\w.$])(['\"]?){re.escape(value)}\1(?![\w.$'\"])"
128128
where_index = description.upper().find("WHERE")
129-
if re.search(regex_key, description[where_index:]) and re.search(
130-
regex_value, description[where_index:]
129+
# Search for comments only in the portion after WHERE clause
130+
description_after_where = description[where_index:]
131+
comment_index = description_after_where.find("--")
132+
if comment_index != -1:
133+
description_to_search = description_after_where[:comment_index]
134+
description_after_comment = description_after_where[comment_index:]
135+
else:
136+
description_to_search = description_after_where
137+
description_after_comment = ""
138+
if re.search(regex_key, description_to_search) and re.search(
139+
regex_value, description_to_search
131140
):
132-
description = description[:where_index] + re.sub(
133-
regex_value, "[UNTRUSTED_INPUT]", description[where_index:]
141+
description = (
142+
description[:where_index]
143+
+ re.sub(regex_value, "[UNTRUSTED_INPUT]", description_to_search)
144+
+ description_after_comment
134145
)
135146
vulnerable_parameters.append((key, value))
136147

‎tests/sentry/performance_issues/test_sql_injection_detector.py

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,10 @@ def test_sql_injection_not_in_where(self):
6868
injection_event = get_event("sql-injection/sql-injection-not-in-where-event")
6969
assert len(self.find_problems(injection_event)) == 0
7070

71+
def test_sql_injection_with_comment(self):
72+
injection_event = get_event("sql-injection/sql-injection-test-comment")
73+
assert len(self.find_problems(injection_event)) == 0
74+
7175
def test_sql_injection_on_non_vulnerable_query(self):
7276
injection_event = get_event("sql-injection/sql-injection-event-non-vulnerable")
7377
assert len(self.find_problems(injection_event)) == 0

0 commit comments

Comments
 (0)