Skip to content

fix(aci): Forbid unsafe legacy interactions with shared Workflows/Detectors#116532

Merged
kcons merged 4 commits into
masterfrom
kcons/saferdel
May 30, 2026
Merged

fix(aci): Forbid unsafe legacy interactions with shared Workflows/Detectors#116532
kcons merged 4 commits into
masterfrom
kcons/saferdel

Conversation

@kcons
Copy link
Copy Markdown
Member

@kcons kcons commented May 29, 2026

For reasons valid and otherwise, it's possible to have multiple legacy models (Rule, AlertRule) mapped to the same Workflow or Detector.
For read purposes, this can be convenient for preserving the old interface, but mutations in this case are confusing at best.
Moreover, our deletion code had assumed no sharing, which meant that deleting a Rule can result in a workflow used by many other projects being deleted, which is almost certainly not the intended behavior.

To address this, here we:

  1. Remove the ability for Rules to delete Workflows. They only delete their link to them. Rules are vestigial; they should be removable without impacting Workflows.
  2. Serve a 400 when a user attempts to modify a Rule/AlertRule backed by a shared Workflow or Detector, providing an error that indicates that the new API should be used. This is less convenient, as some operations may be totally fine, but allowing changing one Rule to update another Rule with a different id via shared backend is wrong. At this point they're all aliases I think (at least, for Rules) but there's no way via the legacy API to know that. Thus, we ask them to use the new API in this case.

The second part in particular may prove annoying, but we can fix it by un-merging (in some cases) or they can use new APIs.

@kcons kcons requested review from a team as code owners May 29, 2026 21:25
@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label May 29, 2026
Copy link
Copy Markdown
Member

@ceorourke ceorourke left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix makes sense, but what's the valid reason that a rule or alert rule would be mapped to multiple workflows?

@kcons
Copy link
Copy Markdown
Member Author

kcons commented May 29, 2026

Fix makes sense, but what's the valid reason that a rule or alert rule would be mapped to multiple workflows?

We expose an endpoint to dedup workflows, which specifically creates that multiple mapping.
Also, I think we still have a few cases from when we had a project creation bug a month or two ago.. those don't really matter for folks on the new UI, except for this.

@kcons kcons enabled auto-merge (squash) May 29, 2026 22:43
kcons added a commit that referenced this pull request May 29, 2026
…low deletion (#116537)

It's broken for project deletion to result in org-scoped workflow
deletion, and for teams with legacy alerting models, this is a real
risk. It's not too troubling if the project was the only user of the
workflow, and arguably expected, but removing org-scoped data when no
longer used anywhere is a choice we haven't yet made, so best to default
to the safest path.

The legacy Rule endpoint still will delete the Workflow associated,
which is troublesome in sharing cases, but will be addressed elsewhere.


This is spun off of #116532.
@kcons kcons force-pushed the kcons/saferdel branch from 50e9cf9 to 5709508 Compare May 30, 2026 00:26
Comment on lines 78 to 80
workflow__organization=project.organization,
workflow__status=ObjectStatus.ACTIVE,
)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: PUT requests bypass the shared workflow modification check because they don't set the use_workflow_engine flag, falling back to a legacy path without the protection.
Severity: HIGH

Suggested Fix

Update the condition for use_workflow_engine to include PUT requests unconditionally, similar to how GET and DELETE are handled. This will ensure that PUT requests enter the new code path where the shared workflow check is performed. Additionally, add a test case to verify that PUT requests on shared workflows are correctly blocked.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location: src/sentry/api/bases/rule.py#L78-L80

Potential issue: The logic to determine whether to use the new workflow engine path,
controlled by the `use_workflow_engine` variable, is only enabled for `GET` and `DELETE`
requests by default. For `PUT` requests, it depends on a `method_flag` which is not set,
causing these requests to fall back to the legacy code path. The check that prevents
modification of shared workflows is only present in the new `if use_workflow_engine:`
block. This allows users to modify rules backed by a shared workflow via a `PUT`
request, which contradicts the intended behavior of the change.

Did we get this right? 👍 / 👎 to inform future reviews.

@kcons kcons merged commit f05b40a into master May 30, 2026
54 of 55 checks passed
@kcons kcons deleted the kcons/saferdel branch May 30, 2026 00:41
@kcons kcons added the Trigger: Revert Add to a merged PR to revert it (skips CI) label May 30, 2026
@getsentry-bot
Copy link
Copy Markdown
Contributor

PR reverted: aa8dbc0

getsentry-bot added a commit that referenced this pull request May 30, 2026
…lows/Detectors (#116532)"

This reverts commit f05b40a.

Co-authored-by: kcons <6990351+kcons@users.noreply.github.com>
@kcons
Copy link
Copy Markdown
Member Author

kcons commented May 30, 2026

This was reverted due to it being unintentionally merged; it was marked auto-merge hours ago accidentally, then updated to no longer be failing without awareness of that, causing it to merge.
We still want to do some version of this, and it isn't unreasonable, but I don't want to deal with the implications of this over the weekend.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Scope: Backend Automatically applied to PRs that change backend components Trigger: Revert Add to a merged PR to revert it (skips CI)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants