From d5f85745bb33e4325e459ae60cf0190de124100c Mon Sep 17 00:00:00 2001 From: Josh Callender <1569818+saponifi3d@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:08:17 -0700 Subject: [PATCH 1/2] Fixes SENTRY-5KVG --- .../incidents/serializers/alert_rule.py | 20 +++++--------- .../serializers/alert_rule_trigger.py | 12 ++++++++- src/sentry/incidents/serializers/utils.py | 27 +++++++++++++++++++ .../test_organization_alert_rule_details.py | 21 +++++++++++++++ 4 files changed, 66 insertions(+), 14 deletions(-) create mode 100644 src/sentry/incidents/serializers/utils.py diff --git a/src/sentry/incidents/serializers/alert_rule.py b/src/sentry/incidents/serializers/alert_rule.py index 20b65449ce9c..ee6117a1b199 100644 --- a/src/sentry/incidents/serializers/alert_rule.py +++ b/src/sentry/incidents/serializers/alert_rule.py @@ -24,7 +24,6 @@ from sentry.api.serializers.rest_framework.base import CamelSnakeModelSerializer from sentry.api.serializers.rest_framework.environment import EnvironmentField from sentry.api.serializers.rest_framework.project import ProjectField -from sentry.api.utils import to_valid_int_id_list from sentry.incidents.logic import ( CRITICAL_TRIGGER_LABEL, WARNING_TRIGGER_LABEL, @@ -39,6 +38,7 @@ AlertRuleThresholdType, AlertRuleTrigger, ) +from sentry.incidents.serializers.utils import validate_object_ids_belong from sentry.incidents.utils.subscription_limits import get_max_metric_alert_subscriptions from sentry.search.eap.trace_metrics.validator import validate_trace_metrics_aggregate from sentry.snuba.dataset import Dataset @@ -422,18 +422,12 @@ def _handle_triggers(self, alert_rule: AlertRule, triggers: list[dict[str, Any]] if triggers is not None: # Delete triggers we don't have present in the incoming data raw_trigger_ids = [x["id"] for x in triggers if "id" in x] - trigger_ids = to_valid_int_id_list("triggers", raw_trigger_ids) - if trigger_ids: - existing_trigger_ids = set( - AlertRuleTrigger.objects.filter( - alert_rule=alert_rule, id__in=trigger_ids - ).values_list("id", flat=True) - ) - missing_ids = [tid for tid in trigger_ids if tid not in existing_trigger_ids] - if missing_ids: - raise serializers.ValidationError( - f"Trigger IDs do not belong to this alert rule: {missing_ids}" - ) + trigger_ids = validate_object_ids_belong( + "triggers", + raw_trigger_ids, + AlertRuleTrigger.objects.filter(alert_rule=alert_rule), + "Trigger IDs do not belong to this alert rule, this alert rule may be incompatible with the legacy API.", + ) triggers_to_delete = AlertRuleTrigger.objects.filter(alert_rule=alert_rule).exclude( id__in=trigger_ids ) diff --git a/src/sentry/incidents/serializers/alert_rule_trigger.py b/src/sentry/incidents/serializers/alert_rule_trigger.py index 574a8c7ad353..5d3f9871e882 100644 --- a/src/sentry/incidents/serializers/alert_rule_trigger.py +++ b/src/sentry/incidents/serializers/alert_rule_trigger.py @@ -17,6 +17,7 @@ update_alert_rule_trigger, ) from sentry.incidents.models.alert_rule import AlertRuleTrigger, AlertRuleTriggerAction +from sentry.incidents.serializers.utils import validate_object_ids_belong from sentry.workflow_engine.migration_helpers.alert_rule import ( dual_delete_migrated_alert_rule_trigger_action, ) @@ -85,8 +86,17 @@ def _handle_actions( ) -> None: channel_lookup_timeout_error = None if actions is not None: + # Validate that any supplied action ids actually belong to this trigger + # before we attempt to load them, so a stale/foreign id yields a 400 + # rather than an unhandled DoesNotExist. + raw_action_ids = [x["id"] for x in actions if "id" in x] + action_ids = validate_object_ids_belong( + "actions", + raw_action_ids, + AlertRuleTriggerAction.objects.filter(alert_rule_trigger=alert_rule_trigger), + "Action IDs do not belong to this trigger", + ) # Delete actions we don't have present in the updated data. - action_ids = [x["id"] for x in actions if "id" in x] actions_to_delete = AlertRuleTriggerAction.objects.filter( alert_rule_trigger=alert_rule_trigger ).exclude(id__in=action_ids) diff --git a/src/sentry/incidents/serializers/utils.py b/src/sentry/incidents/serializers/utils.py new file mode 100644 index 000000000000..a837751936b6 --- /dev/null +++ b/src/sentry/incidents/serializers/utils.py @@ -0,0 +1,27 @@ +from collections.abc import Sequence +from typing import Any + +from django.db.models import QuerySet +from rest_framework import serializers + +from sentry.api.utils import to_valid_int_id_list + + +def validate_object_ids_belong( + name: str, + raw_ids: Sequence[str | int], + queryset: QuerySet[Any], + error_message: str, +) -> list[int]: + """ + Validate that all supplied ids are valid integers and belong to the given + (parent-scoped) queryset. Returns the validated ints. Raises a + serializers.ValidationError (-> HTTP 400) listing any ids that don't belong. + """ + ids = to_valid_int_id_list(name, raw_ids) + if ids: + existing = set(queryset.filter(id__in=ids).values_list("id", flat=True)) + missing = [i for i in ids if i not in existing] + if missing: + raise serializers.ValidationError(f"{error_message}: {missing}") + return ids diff --git a/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py b/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py index a8666ec55804..96cd2a896796 100644 --- a/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py +++ b/tests/sentry/incidents/endpoints/test_organization_alert_rule_details.py @@ -1131,6 +1131,27 @@ def test_update_trigger_id_not_belonging_to_alert_rule(self) -> None: ) assert "do not belong to this alert rule" in str(resp.data) + def test_update_action_id_not_belonging_to_trigger(self) -> None: + self.create_member( + user=self.user, organization=self.organization, role="owner", teams=[self.team] + ) + self.login_as(self.user) + + alert_rule = self.alert_rule + + serialized_alert_rule = self.get_serialized_alert_rule() + # Point the critical trigger's action at an action that belongs to the + # warning trigger. The trigger ids are still valid for this alert rule, so + # the (foreign-to-this-trigger) action id is what should be rejected. + foreign_action_id = serialized_alert_rule["triggers"][1]["actions"][0]["id"] + serialized_alert_rule["triggers"][0]["actions"][0]["id"] = foreign_action_id + + with self.feature("organizations:incidents"): + resp = self.get_error_response( + self.organization.slug, alert_rule.id, status_code=400, **serialized_alert_rule + ) + assert "do not belong to this trigger" in str(resp.data) + def test_update_trigger_alert_threshold(self) -> None: self.create_member( user=self.user, organization=self.organization, role="owner", teams=[self.team] From 8bda5c4bfa46a631f2d9e78182b39bf7d246ebf8 Mon Sep 17 00:00:00 2001 From: Josh Callender <1569818+saponifi3d@users.noreply.github.com> Date: Wed, 24 Jun 2026 12:13:33 -0700 Subject: [PATCH 2/2] remove extra var declaration so we don't need the comment either, this way it all reads a little more clearly. --- src/sentry/incidents/serializers/alert_rule_trigger.py | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/sentry/incidents/serializers/alert_rule_trigger.py b/src/sentry/incidents/serializers/alert_rule_trigger.py index 5d3f9871e882..3109a33c2eca 100644 --- a/src/sentry/incidents/serializers/alert_rule_trigger.py +++ b/src/sentry/incidents/serializers/alert_rule_trigger.py @@ -86,20 +86,18 @@ def _handle_actions( ) -> None: channel_lookup_timeout_error = None if actions is not None: - # Validate that any supplied action ids actually belong to this trigger - # before we attempt to load them, so a stale/foreign id yields a 400 - # rather than an unhandled DoesNotExist. - raw_action_ids = [x["id"] for x in actions if "id" in x] action_ids = validate_object_ids_belong( "actions", - raw_action_ids, + [x["id"] for x in actions if "id" in x], AlertRuleTriggerAction.objects.filter(alert_rule_trigger=alert_rule_trigger), "Action IDs do not belong to this trigger", ) + # Delete actions we don't have present in the updated data. actions_to_delete = AlertRuleTriggerAction.objects.filter( alert_rule_trigger=alert_rule_trigger ).exclude(id__in=action_ids) + for action in actions_to_delete: with transaction.atomic(router.db_for_write(AlertRuleTriggerAction)): dual_delete_migrated_alert_rule_trigger_action(action)