Skip to content

Commit 790ac8d

Browse files
authored
chore(detectors): Normalize evidence data for query injection issues (#93760)
since sql injection and query injection detectors create the same issue type, the issue details page should work for both. this pr normalizes the evidence data so the issue details page will more easily show the data for this type of issue. including the value as well, instead of just the key, to provide more evidence to the user.
1 parent bba4d6d commit 790ac8d

File tree

4 files changed

+9
-10
lines changed

4 files changed

+9
-10
lines changed

src/sentry/performance_issues/detectors/query_injection_detector.py

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ def visit_span(self, span: Span) -> None:
5757

5858
unsafe_inputs = []
5959
for input_key, input_value in self.potential_unsafe_inputs:
60+
original_input_value = input_value.copy()
6061
# Replace all operands in filter with "?" since the query description is sanitized
6162
if input_value and isinstance(input_value, dict):
6263
for dict_key, dict_value in input_value.items():
@@ -66,7 +67,7 @@ def visit_span(self, span: Span) -> None:
6667
input_dict = {input_key: input_value}
6768
if json.dumps(input_dict) in description:
6869
description = description.replace(json.dumps(input_value), "?")
69-
unsafe_inputs.append(input_key)
70+
unsafe_inputs.append((input_key, original_input_value))
7071

7172
if len(unsafe_inputs) == 0:
7273
return
@@ -87,7 +88,7 @@ def visit_span(self, span: Span) -> None:
8788
"parent_span_ids": [],
8889
"offender_span_ids": spans_involved,
8990
"transaction_name": self._event.get("transaction", ""),
90-
"unsafe_inputs": unsafe_inputs,
91+
"vulnerable_parameters": unsafe_inputs,
9192
"request_url": self.request_url,
9293
},
9394
evidence_display=[

src/sentry/performance_issues/detectors/sql_injection_detector.py

Lines changed: 3 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -122,9 +122,7 @@ def visit_span(self, span: Span) -> None:
122122
if "WHERE" not in description.upper():
123123
return
124124

125-
for parameter in self.request_parameters:
126-
value = parameter[1]
127-
key = parameter[0]
125+
for key, value in self.request_parameters:
128126
regex_key = rf'(?<![\w.$])"?{re.escape(key)}"?(?![\w.$"])'
129127
regex_value = rf'(?<![\w.$])"?{re.escape(value)}"?(?![\w.$"])'
130128
where_index = description.upper().find("WHERE")
@@ -134,7 +132,7 @@ def visit_span(self, span: Span) -> None:
134132
description = description[:where_index] + re.sub(
135133
regex_value, "?", description[where_index:]
136134
)
137-
vulnerable_parameters.append(key)
135+
vulnerable_parameters.append((key, value))
138136

139137
if len(vulnerable_parameters) == 0:
140138
return
@@ -155,7 +153,7 @@ def visit_span(self, span: Span) -> None:
155153
"parent_span_ids": [],
156154
"offender_span_ids": spans_involved,
157155
"transaction_name": self._event.get("transaction", ""),
158-
"vulnerable_parameters": list(set(vulnerable_parameters)),
156+
"vulnerable_parameters": vulnerable_parameters,
159157
"request_url": self.request_url,
160158
},
161159
evidence_display=[

tests/sentry/performance_issues/test_query_injection_detector.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -40,5 +40,5 @@ def test_query_injection_detection_in_query_params(self):
4040
== '{"find":"?","filter":{"username":?},"limit":"?","singleBatch":"?","batchSize":"?"}'
4141
)
4242
assert problem.evidence_data is not None
43-
assert problem.evidence_data["unsafe_inputs"] == ["username"]
43+
assert problem.evidence_data["vulnerable_parameters"] == [("username", {"$ne": None})]
4444
assert problem.evidence_data["request_url"] == "http://localhost:3000/login"

tests/sentry/performance_issues/test_sql_injection_detector.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,7 @@ def test_sql_injection_detection_in_query_params(self):
3737
assert problem.op == "db"
3838
assert problem.desc == "SELECT * FROM users WHERE username = '?' ORDER BY username ASC"
3939
assert problem.evidence_data is not None
40-
assert problem.evidence_data["vulnerable_parameters"] == ["username"]
40+
assert problem.evidence_data["vulnerable_parameters"] == [("username", "hello")]
4141
assert problem.evidence_data["request_url"] == "http://localhost:3001/vulnerable-login"
4242

4343
def test_sql_injection_detection_in_body(self):
@@ -51,7 +51,7 @@ def test_sql_injection_detection_in_body(self):
5151
assert problem.op == "db"
5252
assert problem.desc == "SELECT * FROM users WHERE username = '?'"
5353
assert problem.evidence_data is not None
54-
assert problem.evidence_data["vulnerable_parameters"] == ["username"]
54+
assert problem.evidence_data["vulnerable_parameters"] == [("username", "hello")]
5555
assert problem.evidence_data["request_url"] == "http://localhost:3001/vulnerable-login"
5656

5757
def test_sql_injection_regex(self):

0 commit comments

Comments
 (0)