Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -8,24 +8,23 @@

## ActorField vs OwnerActorField

`ActorField` accepts any user or team ID without validating the requesting user's relationship to that actor. `OwnerActorField` validates team membership before allowing assignment.
`ActorField` accepts any user or team ID without validating the requesting user's relationship to that actor. `OwnerActorField` additionally checks that the requesting user is a member of the target team.

**Location:** `src/sentry/api/fields/actor.py`

### When to use which

| Field | Validates Membership | Use For |
| ----------------- | -------------------- | --------------------------------- |
| `ActorField` | No | Read-only display, filtering |
| `OwnerActorField` | Yes | Owner assignment, assignee fields |
Default to `OwnerActorField` for any write-op field accepting a team or user reference (assignment, ownership, delegation). Originally PR #106074.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

these are docs-only edits to clarify this new exception


One known exception: `GroupValidator.assignedTo` uses `ActorField`. Issue assignment is a label — it doesn't grant access, and project-access is validated separately in `validate_assignedTo`. Don't expand this exception without explicit review.

### Real vulnerability: ActorField for ownership (PR #106074)

**Vulnerable code:**

```python
class IssueAlertRuleSerializer(serializers.Serializer):
owner = ActorField(required=False) # BUG: Any actor, no membership check
owner = ActorField(required=False) # BUG: Any team, no membership check
```

**Fixed code:**
Expand All @@ -42,18 +41,12 @@ class IssueAlertRuleSerializer(serializers.Serializer):
3. If user is a member of the target team → allowed
4. Otherwise → `ValidationError("You can only assign teams you are a member of")`

### What to look for

Any serializer field that accepts a team or user reference for **write operations** (assignment, ownership, delegation) should use `OwnerActorField`, not `ActorField`.

Search pattern:
### Finding existing uses

```
grep -rn "ActorField" --include="*.py" src/sentry/api/
```

Flag any `ActorField()` used in a serializer for PUT/POST operations where the field sets an owner or assignee.

## Foreign Key IDs in Request Bodies

When a serializer accepts an ID that references another model, the serializer or view must validate that the referenced object belongs to the same organization.
Expand Down Expand Up @@ -91,7 +84,7 @@ A serializer accepted `condition_group_id` from user input, allowing a user to i
## Checklist

```
□ Owner/assignee fields use OwnerActorField, not ActorField
□ Owner/assignee fields use OwnerActorField (exception: issue assignment uses ActorField)
□ FK ID fields in request body are validated against the org
□ IDs that should be server-set are not exposed in the serializer
□ Serializer context includes organization for validation
Expand Down
22 changes: 12 additions & 10 deletions src/sentry/api/helpers/group_index/validators/group.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
from drf_spectacular.utils import extend_schema_serializer
from rest_framework import serializers

from sentry.api.fields.actor import OwnerActorField
from sentry.api.fields.actor import ActorField
from sentry.api.helpers.group_index.validators.inbox_details import InboxDetailsValidator
from sentry.api.helpers.group_index.validators.status_details import StatusDetailsValidator
from sentry.models.group import STATUS_UPDATE_CHOICES, Group
Expand Down Expand Up @@ -53,7 +53,7 @@ class GroupValidator(serializers.Serializer[Group]):
discard = serializers.BooleanField(
help_text="If true, discards the issues instead of updating them."
)
assignedTo = OwnerActorField(
assignedTo = ActorField(
help_text="The user or team that should be assigned to the issues. Values take the form of `<user_id>`, `user:<user_id>`, `<username>`, `<user_primary_email>`, or `team:<team_id>`."
)
priority = serializers.ChoiceField(
Expand Down Expand Up @@ -82,19 +82,21 @@ class GroupValidator(serializers.Serializer[Group]):
# for the moment, the CLI sends this for any issue update, so allow nulls
snoozeDuration = serializers.IntegerField(allow_null=True)

def validate_assignedTo(self, value: Actor) -> Actor:
def validate_assignedTo(self, value: Actor | None) -> Actor | None:
if not value:
return value

organization = self.context.get("organization")
if organization and organization.flags.allow_joinleave:
return value

if (
value
and value.is_user
value.is_user
and not self.context["project"].member_set.filter(user_id=value.id).exists()
):
raise serializers.ValidationError("Cannot assign to non-team member")

if (
value
and value.is_team
and not self.context["project"].teams.filter(id=value.id).exists()
):
if value.is_team and not self.context["project"].teams.filter(id=value.id).exists():
Comment thread
sentry-warden[bot] marked this conversation as resolved.
Comment thread
cvxluo marked this conversation as resolved.
Comment thread
cursor[bot] marked this conversation as resolved.
raise serializers.ValidationError(
Comment thread
sentry[bot] marked this conversation as resolved.
"Cannot assign to a team without access to the project"
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,8 @@ def test_assign_issue_through_unfurl(self) -> None:
}

def test_assign_issue_where_team_not_in_project(self) -> None:
self.organization.flags.allow_joinleave = False
self.organization.save()
user2 = self.create_user(is_superuser=False)
team2 = self.create_team(
organization=self.organization, members=[self.user], name="Ecosystem"
Expand All @@ -609,6 +611,8 @@ def test_assign_issue_where_team_not_in_project(self) -> None:
assert not GroupAssignee.objects.filter(group=self.group).exists()

def test_assign_issue_where_team_not_in_project_through_unfurl(self) -> None:
self.organization.flags.allow_joinleave = False
self.organization.save()
user2 = self.create_user(is_superuser=False)
team2 = self.create_team(
organization=self.organization, members=[self.user], name="Ecosystem"
Expand Down
2 changes: 2 additions & 0 deletions tests/sentry/issues/endpoints/test_group_details.py
Original file line number Diff line number Diff line change
Expand Up @@ -611,6 +611,8 @@ def test_assign_unavailable_team(self) -> None:
self.login_as(user=self.user)

group = self.create_group()
group.project.organization.flags.allow_joinleave = False
group.project.organization.save()
team = self.create_team(organization=group.project.organization, members=[self.user])

url = f"/api/0/organizations/{group.organization.slug}/issues/{group.id}/"
Expand Down
53 changes: 32 additions & 21 deletions tests/snuba/api/endpoints/test_project_group_index.py
Original file line number Diff line number Diff line change
Expand Up @@ -1400,13 +1400,11 @@ def test_assign_team(self) -> None:
assert response.status_code == 200, response.content
assert response.data["assignedTo"] is None

def test_assign_team_not_member_of_when_open_membership_disabled(self) -> None:
def test_assign_team_when_user_not_on_target_team_in_closed_membership(self) -> None:
"""
Test that a user cannot assign an issue to a team they are not a member of
when Open Membership is disabled. This is a regression test for an authorization
bypass vulnerability.
A user with project access can assign any team that also has project access,
even one they are not a member of.
"""
# Disable Open Membership
self.organization.flags.allow_joinleave = False
self.organization.save()

Expand All @@ -1427,34 +1425,33 @@ def test_assign_team_not_member_of_when_open_membership_disabled(self) -> None:
url = f"{self.path}?id={group.id}"
response = self.client.put(url, data={"assignedTo": f"team:{other_team.id}"})

assert response.status_code == 400, response.content
assert "only assign teams you are a member of" in str(response.data)
assert not GroupAssignee.objects.filter(group=group, team=other_team).exists()
assert response.status_code == 200, response.content
assert response.data["assignedTo"]["id"] == str(other_team.id)
assert GroupAssignee.objects.filter(group=group, team=other_team).exists()

def test_assign_team_not_member_of_with_team_admin_scope(self) -> None:
"""
Test that a user with team:admin scope CAN assign an issue to a team they are
not a member of, even when Open Membership is disabled.
"""
def test_assign_team_without_project_access_when_open_membership_disabled(self) -> None:
self.organization.flags.allow_joinleave = False
self.organization.save()

group = self.create_group()

member_user = self.create_user("member@example.com")
member_team = self.create_team(organization=group.project.organization, name="member-team")
self.create_member(
user=member_user, organization=self.organization, role="member", teams=[member_team]
)
group.project.add_team(member_team)

other_team = self.create_team(organization=group.project.organization, name="other-team")
group.project.add_team(other_team)

admin_user = self.create_user("admin@example.com")
self.create_member(user=admin_user, organization=self.organization, role="owner")
self.login_as(user=admin_user)
self.login_as(user=member_user)

url = f"{self.path}?id={group.id}"
response = self.client.put(url, data={"assignedTo": f"team:{other_team.id}"})

assert response.status_code == 200, response.content
assert response.data["assignedTo"]["id"] == str(other_team.id)
assert response.data["assignedTo"]["type"] == "team"
assert GroupAssignee.objects.filter(group=group, team=other_team).exists()
assert response.status_code == 400, response.content
assert "without access to the project" in str(response.data)
assert not GroupAssignee.objects.filter(group=group, team=other_team).exists()

def test_assign_team_when_open_membership_enabled(self) -> None:
"""
Expand Down Expand Up @@ -1486,6 +1483,20 @@ def test_assign_team_when_open_membership_enabled(self) -> None:
assert response.data["assignedTo"]["type"] == "team"
assert GroupAssignee.objects.filter(group=group, team=other_team).exists()

def test_unassign_in_closed_membership(self) -> None:
self.organization.flags.allow_joinleave = False
self.organization.save()

group = self.create_group()
GroupAssignee.objects.assign(group, self.user)
self.login_as(user=self.user)

url = f"{self.path}?id={group.id}"
response = self.client.put(url, data={"assignedTo": ""})

assert response.status_code == 200, response.content
assert not GroupAssignee.objects.filter(group=group).exists()

def test_discard(self) -> None:
group1 = self.create_group(is_public=True)
group2 = self.create_group(is_public=True)
Expand Down
Loading