Browse Source

fix(sessions): Disallow querying by issue.id (#29810)

The /sessions endpoint reuses a helper function from event search for
parsing some of its query parameters. This helper function produces
conditions and filter_keys to plug into a snuba query. The filter_keys
are not checked against the snuba schema, however, so a query like
issue.id:123 causes a 500 response on this endpoint.

Partially fixes SENTRY-SGT.
Joris Bayer 3 years ago
parent
commit
d8359ac9fb

+ 11 - 1
src/sentry/snuba/sessions_v2.py

@@ -217,6 +217,7 @@ GROUPBY_MAP = {
 }
 
 CONDITION_COLUMNS = ["project", "environment", "release"]
+FILTER_KEY_COLUMNS = ["project_id"]
 
 
 def resolve_column(col):
@@ -225,6 +226,12 @@ def resolve_column(col):
     raise InvalidField(f'Invalid query field: "{col}"')
 
 
+def resolve_filter_key(col):
+    if col in FILTER_KEY_COLUMNS:
+        return col
+    raise InvalidField(f'Invalid query field: "{col}"')
+
+
 class InvalidField(Exception):
     pass
 
@@ -289,10 +296,13 @@ class QueryDefinition:
         # this makes sure that literals in complex queries are properly quoted,
         # and unknown fields are raised as errors
         conditions = [resolve_condition(c, resolve_column) for c in snuba_filter.conditions]
+        filter_keys = {
+            resolve_filter_key(key): value for key, value in snuba_filter.filter_keys.items()
+        }
 
         self.aggregations = snuba_filter.aggregations
         self.conditions = conditions
-        self.filter_keys = snuba_filter.filter_keys
+        self.filter_keys = filter_keys
 
 
 MAX_POINTS = 1000

+ 13 - 0
tests/snuba/api/endpoints/test_organization_sessions.py

@@ -131,6 +131,12 @@ class OrganizationSessionsEndpointTest(APITestCase, SnubaTestCase):
         assert response.status_code == 400, response.content
         assert response.data == {"detail": 'Invalid groupBy: "envriomnent"'}
 
+    def test_illegal_groupby(self):
+        response = self.do_request({"field": ["sum(session)"], "groupBy": ["issue.id"]})
+
+        assert response.status_code == 400, response.content
+        assert response.data == {"detail": 'Invalid groupBy: "issue.id"'}
+
     def test_invalid_query(self):
         response = self.do_request(
             {"statsPeriod": "1d", "field": ["sum(session)"], "query": ["foo:bar"]}
@@ -152,6 +158,13 @@ class OrganizationSessionsEndpointTest(APITestCase, SnubaTestCase):
         # since its not obvious where `message` comes from.
         assert response.data == {"detail": 'Invalid query field: "message"'}
 
+    def test_illegal_query(self):
+        response = self.do_request(
+            {"statsPeriod": "1d", "field": ["sum(session)"], "query": ["issue.id:123"]}
+        )
+        assert response.status_code == 400, response.content
+        assert response.data == {"detail": 'Invalid query field: "group_id"'}
+
     def test_too_many_points(self):
         # default statsPeriod is 90d
         response = self.do_request({"field": ["sum(session)"], "interval": "1h"})