From 5a04d52dc259e47d722f9fcf7e20f9ef0117e046 Mon Sep 17 00:00:00 2001 From: Joshua Li Date: Sat, 31 Oct 2026 10:59:44 -0700 Subject: [PATCH 1/2] fix(dashboards): validate threshold JSON shape Thresholds are a JSONField, but unit and max_values were cast to their response types without validation. Reject non-object thresholds, unknown or non-integer maximum keys, and non-string units at write time so the serializer's casts are backed by a validated invariant. --- .../serializers/rest_framework/dashboard.py | 69 ++++++++++++------- .../test_organization_dashboard_details.py | 43 ++++++++++++ 2 files changed, 87 insertions(+), 25 deletions(-) diff --git a/src/sentry/api/serializers/rest_framework/dashboard.py b/src/sentry/api/serializers/rest_framework/dashboard.py index 71a35a31e73b..698a6861f446 100644 --- a/src/sentry/api/serializers/rest_framework/dashboard.py +++ b/src/sentry/api/serializers/rest_framework/dashboard.py @@ -897,46 +897,65 @@ def validate(self, data): {"thresholds": "Heatmap widgets do not support thresholds."} ) - if thresholds: + if thresholds is not None: + if not isinstance(thresholds, dict): + raise serializers.ValidationError({"thresholds": "Must be an object."}) + max_values = thresholds.get("max_values") allowed_max_keys = [key.value for key in ThresholdMaxKeys] - if max_values: - for i in range(len(max_values)): - max_key = f"max{i + 1}" - - if max_key not in allowed_max_keys: + if max_values is not None: + if not isinstance(max_values, dict): + raise serializers.ValidationError( + {"thresholds": {"max_values": "Must be an object."}} + ) + if max_values: + invalid_keys = set(max_values) - set(allowed_max_keys) + if invalid_keys: raise serializers.ValidationError( - {"thresholds": f"Invalid maximum key {max_key}"} + {"thresholds": f"Invalid maximum key {sorted(invalid_keys)[0]}"} ) - if max_values.get(max_key): - if max_values.get(max_key) < 0: + for max_key, max_value in max_values.items(): + if not isinstance(max_value, int) or isinstance(max_value, bool): + raise serializers.ValidationError( + {"thresholds": {max_key: "Maximum values must be integers."}} + ) + if max_value < 0: raise serializers.ValidationError( {"thresholds": {max_key: "Maximum values can not be negative"}} ) - elif i > 0: - prev_max_key = f"max{i}" - if max_values.get(prev_max_key) and max_values.get( - prev_max_key - ) >= max_values.get(max_key): - raise serializers.ValidationError( - { - "thresholds": { - max_key: "Maximum value must be greater than minimum." - } - } - ) - if len(max_values) < len(ThresholdMaxKeys): - for key in allowed_max_keys: - if max_values.get(key) is None: + for prev_max_key, max_key in zip( + allowed_max_keys, allowed_max_keys[1:], strict=False + ): + if ( + prev_max_key in max_values + and max_key in max_values + and max_values[prev_max_key] >= max_values[max_key] + ): raise serializers.ValidationError( { "thresholds": { - key: "Must set all threshold maximums or none at all." + max_key: "Maximum value must be greater than minimum." } } ) + + if len(max_values) < len(ThresholdMaxKeys): + for key in allowed_max_keys: + if key not in max_values: + raise serializers.ValidationError( + { + "thresholds": { + key: "Must set all threshold maximums or none at all." + } + } + ) + + unit = thresholds.get("unit") + if unit is not None and not isinstance(unit, str): + raise serializers.ValidationError({"thresholds": {"unit": "Must be a string."}}) + preferred_polarity = thresholds.get("preferred_polarity") if preferred_polarity is not None and preferred_polarity not in ("+", "-", ""): raise serializers.ValidationError( diff --git a/tests/sentry/dashboards/endpoints/test_organization_dashboard_details.py b/tests/sentry/dashboards/endpoints/test_organization_dashboard_details.py index debf9da0e400..80bd78530715 100644 --- a/tests/sentry/dashboards/endpoints/test_organization_dashboard_details.py +++ b/tests/sentry/dashboards/endpoints/test_organization_dashboard_details.py @@ -2118,6 +2118,49 @@ def test_update_widget_with_invalid_preferred_polarity(self) -> None: == "Must be '+', '-', or empty string." ) + def test_update_widget_with_invalid_thresholds(self) -> None: + cases = ( + ([], "Must be an object."), + ({"max_values": []}, "Must be an object."), + ( + {"max_values": {"max1": 100, "max3": 300}}, + "Invalid maximum key max3", + ), + ( + {"max_values": {"max1": "100", "max2": 200}}, + "Maximum values must be integers.", + ), + ( + {"max_values": {"max1": 100, "max2": 200}, "unit": 5}, + "Must be a string.", + ), + ) + for thresholds, message in cases: + with self.subTest(thresholds=thresholds): + data = { + "title": "Dashboard", + "widgets": [ + { + "id": str(self.widget_1.id), + "title": "Big Number with Invalid Thresholds", + "displayType": "big_number", + "thresholds": thresholds, + "queries": [ + { + "name": "", + "fields": ["count()"], + "columns": [], + "aggregates": ["count()"], + "conditions": "", + } + ], + }, + ], + } + response = self.do_request("put", self.url(self.dashboard.id), data=data) + assert response.status_code == 400, response.data + assert message in str(response.data) + def test_update_widget_with_invalid_threshold_time_window(self) -> None: data = { "title": "Dashboard", From bd45f8cc477aad6e4d2ce84cbd440b631e7e8ae0 Mon Sep 17 00:00:00 2001 From: Joshua Li Date: Sat, 31 Oct 2026 11:30:36 -0700 Subject: [PATCH 2/2] test(dashboards): annotate invalid threshold cases Keep the parametrized case table typed so the backend mypy run accepts the new test module. --- .../dashboards/endpoints/test_organization_dashboard_details.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/sentry/dashboards/endpoints/test_organization_dashboard_details.py b/tests/sentry/dashboards/endpoints/test_organization_dashboard_details.py index 80bd78530715..987b05803df3 100644 --- a/tests/sentry/dashboards/endpoints/test_organization_dashboard_details.py +++ b/tests/sentry/dashboards/endpoints/test_organization_dashboard_details.py @@ -2119,7 +2119,7 @@ def test_update_widget_with_invalid_preferred_polarity(self) -> None: ) def test_update_widget_with_invalid_thresholds(self) -> None: - cases = ( + cases: tuple[tuple[object, str], ...] = ( ([], "Must be an object."), ({"max_values": []}, "Must be an object."), (