Skip to content

Commit d8266d5

Browse files
roggenkemperroggenkemper
committed
add more test
1 parent 8fc6e25 commit d8266d5

File tree

4 files changed

+206
-47
lines changed

4 files changed

+206
-47
lines changed

‎fixtures/events/performance_problems/sql-injection/sql-injection-event-non-vulnerable.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 = ?",
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 = ?",
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+
}

‎fixtures/events/performance_problems/sql-injection-event.json renamed to ‎fixtures/events/performance_problems/sql-injection/sql-injection-event.json

Lines changed: 1 addition & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -18,11 +18,7 @@
1818
"request": {
1919
"url": "http://localhost:3001/vulnerable-login",
2020
"method": "GET",
21-
"query_string": [
22-
["username", "hello"],
23-
["testing", "testing"],
24-
["testing2", "testing2"]
25-
]
21+
"query_string": [["username", "hello"]]
2622
},
2723
"spans": [
2824
{
@@ -156,28 +152,6 @@
156152
"sentry.op": "db",
157153
"sentry.origin": "auto.db.otel.mysql2"
158154
},
159-
"sentry_tags": {
160-
"user": "ip:::1",
161-
"user.ip": "::1",
162-
"environment": "production",
163-
"transaction": "GET /vulnerable-login",
164-
"transaction.method": "GET",
165-
"transaction.op": "http.server",
166-
"browser.name": "Chrome",
167-
"sdk.name": "sentry.javascript.node",
168-
"sdk.version": "9.17.0",
169-
"platform": "node",
170-
"os.name": "macOS",
171-
"action": "SELECT",
172-
"category": "db",
173-
"description": "SELECT * FROM users WHERE username = %s",
174-
"domain": ",users,",
175-
"group": "45330ba0cafa5997",
176-
"op": "db",
177-
"status": "ok",
178-
"system": "mysql",
179-
"trace.status": "ok"
180-
},
181155
"hash": "45330ba0cafa5997"
182156
}
183157
]

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

Lines changed: 42 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
import hashlib
44
from typing import Any
55

6-
from sentry import features
76
from sentry.issues.grouptype import SQLInjectionGroupType
87
from sentry.issues.issue_occurrence import IssueEvidence
98
from sentry.models.organization import Organization
@@ -20,6 +19,34 @@
2019

2120
MAX_EVIDENCE_VALUE_LENGTH = 10_000
2221

22+
SQL_KEYWORDS = [
23+
"SELECT",
24+
"WHERE",
25+
"AND",
26+
"OR",
27+
"NOT",
28+
"IN",
29+
"LIKE",
30+
"LIMIT",
31+
"ORDER BY",
32+
"GROUP BY",
33+
"HAVING",
34+
"DISTINCT",
35+
"JOIN",
36+
"ON",
37+
"AS",
38+
"CASE",
39+
"WHEN",
40+
"THEN",
41+
"ELSE",
42+
"END",
43+
"UNION",
44+
"ALL",
45+
"ANY",
46+
"SOME",
47+
"EXISTS",
48+
]
49+
2350

2451
class SQLInjectionDetector(PerformanceDetector):
2552
__slots__ = "stored_problems"
@@ -36,7 +63,9 @@ def __init__(self, settings: dict[DetectorType, Any], event: dict[str, Any]) ->
3663
if not query_string:
3764
return
3865

39-
self.user_inputs = [query_value[1] for query_value in query_string]
66+
self.user_inputs = [
67+
query_value[1] for query_value in query_string if self.keep_query_value(query_value)
68+
]
4069

4170
def visit_span(self, span: Span) -> None:
4271
if not SQLInjectionDetector.is_span_eligible(span):
@@ -49,7 +78,7 @@ def visit_span(self, span: Span) -> None:
4978
spans_involved = [span["span_id"]]
5079

5180
for user_input in self.user_inputs:
52-
if user_input in description and self.is_sql_injection(user_input):
81+
if user_input in description:
5382
self.stored_problems[fingerprint] = PerformanceProblem(
5483
type=SQLInjectionGroupType,
5584
fingerprint=self._fingerprint(hash),
@@ -86,26 +115,20 @@ def visit_span(self, span: Span) -> None:
86115
)
87116

88117
def is_creation_allowed_for_organization(self, organization: Organization) -> bool:
89-
return features.has("organizations:sql-injection-detector", organization, actor=None)
118+
return True
90119

91120
def is_creation_allowed_for_project(self, project: Project | None) -> bool:
92121
return self.settings["detection_enabled"]
93122

94-
def is_sql_injection(self, user_input: str) -> bool:
95-
if (
96-
"DROP" in user_input
97-
or "DELETE" in user_input
98-
or "UPDATE" in user_input
99-
or "INSERT" in user_input
100-
):
101-
return True
102-
if ";" in user_input:
103-
return True
104-
if "--" in user_input:
105-
return True
106-
if "1=1" in user_input:
107-
return True
108-
return False
123+
def keep_query_value(self, query: tuple[str, str]) -> bool:
124+
query_value = query[1].upper()
125+
query_key = query[0].upper()
126+
127+
if query_key == query_value:
128+
return False
129+
if query_value in SQL_KEYWORDS:
130+
return False
131+
return True
109132

110133
@classmethod
111134
def is_span_eligible(cls, span: Span) -> bool:

‎tests/sentry/utils/performance_issues/test_sql_injection_detector.py

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,10 @@ def find_problems(self, event: dict[str, Any]) -> list[PerformanceProblem]:
2626
return list(detector.stored_problems.values())
2727

2828
def test_sql_injection_detection(self):
29-
injection_event = get_event("sql-injection-event")
29+
injection_event = get_event("sql-injection/sql-injection-event")
3030

3131
assert len(self.find_problems(injection_event)) == 1
32+
33+
def test_sql_injection_on_non_vulnerable_query(self):
34+
injection_event = get_event("sql-injection/sql-injection-event-non-vulnerable")
35+
assert len(self.find_problems(injection_event)) == 0

0 commit comments

Comments
 (0)