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
78 changes: 78 additions & 0 deletions src/sentry/api/helpers/projects.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,78 @@
from __future__ import annotations

from collections.abc import Iterable
from typing import NamedTuple, TypeAlias

from drf_spectacular.types import OpenApiTypes
from drf_spectacular.utils import extend_schema_field
from rest_framework import serializers

from sentry.constants import ALL_ACCESS_PROJECT_ID, ALL_ACCESS_PROJECTS_SLUG
from sentry.utils.slug import DEFAULT_SLUG_ERROR_MESSAGE, MIXED_SLUG_REGEX

ProjectIdOrSlug: TypeAlias = int | str


class ParsedProjectIdOrSlugParams(NamedTuple):
ids: set[int]
slugs: set[str]

@property
def has_values(self) -> bool:
return bool(self.ids or self.slugs)

@property
def has_all_projects_sentinel(self) -> bool:
return ALL_ACCESS_PROJECT_ID in self.ids or ALL_ACCESS_PROJECTS_SLUG in self.slugs


def parse_id_or_slug_params(
values: Iterable[ProjectIdOrSlug | None],
) -> ParsedProjectIdOrSlugParams:
"""
Partition project identifier values into numeric IDs and slugs.

All-digit values and the ``-1`` all-access project sigil are treated as IDs.
Everything else is treated as a slug.
"""
ids: set[int] = set()
slugs: set[str] = set()
for value in values:
if value is None or value == "":
continue
if isinstance(value, int) and not isinstance(value, bool):
ids.add(value)
continue

value_str = str(value)
if value_str.isdecimal() or value_str == str(ALL_ACCESS_PROJECT_ID):
ids.add(int(value_str))
else:
slugs.add(value_str)
return ParsedProjectIdOrSlugParams(ids=ids, slugs=slugs)


@extend_schema_field(field=OpenApiTypes.STR)
class ProjectIdOrSlugField(serializers.Field[ProjectIdOrSlug, object, ProjectIdOrSlug, object]):
default_error_messages = {
"invalid": "Expected a project ID or slug.",
"invalid_slug": DEFAULT_SLUG_ERROR_MESSAGE,
}

def to_internal_value(self, data: object) -> ProjectIdOrSlug:
if data is None or isinstance(data, bool):
self.fail("invalid")
if isinstance(data, int):
return data
if not isinstance(data, str) or data == "":
self.fail("invalid")
if data.isdecimal() or data == str(ALL_ACCESS_PROJECT_ID):
return int(data)
if data == ALL_ACCESS_PROJECTS_SLUG:
return data
if MIXED_SLUG_REGEX.match(data) is None:
self.fail("invalid_slug")
return data

def to_representation(self, value: ProjectIdOrSlug) -> ProjectIdOrSlug:
return value
21 changes: 18 additions & 3 deletions src/sentry/api/serializers/rest_framework/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
from drf_spectacular.utils import extend_schema_field
from rest_framework import serializers

from sentry.api.helpers.projects import ProjectIdOrSlugField
from sentry.models.project import Project

ValidationError = serializers.ValidationError
Expand All @@ -18,6 +19,8 @@ def __init__(
The scope parameter specifies which permissions are required to access the project field.
If multiple scopes are provided, the project can be accessed when the user is authenticated with
any of the scopes.

If id_allowed is true, the field accepts a project ID or slug. Otherwise, it accepts slugs only.
"""
self.scope = scope
self.id_allowed = id_allowed
Expand All @@ -26,18 +29,30 @@ def __init__(
def to_representation(self, value):
return value

def to_internal_value(self, data):
def to_internal_value(self, data: object) -> Project:
try:
if self.id_allowed:
project_id_or_slug = ProjectIdOrSlugField().to_internal_value(data)
project = Project.objects.get(
organization=self.context["organization"], slug__id_or_slug=data
organization=self.context["organization"], slug__id_or_slug=project_id_or_slug
)
else:
project = Project.objects.get(organization=self.context["organization"], slug=data)
project_slug = self._validate_slug(data)
project = Project.objects.get(
organization=self.context["organization"], slug=project_slug
)
Comment on lines 39 to +43

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: An invalid project slug raises a generic validation error instead of the intended "Invalid project" error because the specific ValidationError is not caught.
Severity: LOW

Suggested Fix

Modify the except block in ProjectField.to_internal_value to also catch serializers.ValidationError. Inside the handler, raise a new serializers.ValidationError with the intended 'Invalid project' message to ensure consistent error reporting for all invalid project identifiers.

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/serializers/rest_framework/project.py#L39-L43

Potential issue: The `try...except` block in `ProjectField.to_internal_value` is
intended to catch lookup failures and return a `ValidationError` with the message
"Invalid project". However, when an invalid slug format is used (e.g., containing a
space), the underlying `ProjectIdOrSlugField` raises a `serializers.ValidationError`
before the database lookup occurs. This validation error is not caught by the `except
Project.DoesNotExist` handler, causing a generic slug format error to be returned to the
user instead of the intended, more specific "Invalid project" message.

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

except Project.DoesNotExist:
raise ValidationError("Invalid project")

scopes = (self.scope,) if isinstance(self.scope, str) else self.scope
if not self.context["access"].has_any_project_scope(project, scopes):
raise ValidationError("Insufficient access to project")
return project

def _validate_slug(self, data: object) -> str:
if not isinstance(data, str):
raise ValidationError("Invalid project")
project_id_or_slug = ProjectIdOrSlugField().to_internal_value(data)
if not isinstance(project_id_or_slug, str):
raise ValidationError("Invalid project")
return project_id_or_slug
77 changes: 77 additions & 0 deletions tests/sentry/api/helpers/test_projects.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
import pytest
from rest_framework import serializers

from sentry.api.helpers.projects import ProjectIdOrSlugField, parse_id_or_slug_params


class TestParseIdOrSlugParams:
def test_empty_input(self) -> None:
params = parse_id_or_slug_params([])
assert params.ids == set()
assert params.slugs == set()

def test_numeric_values(self) -> None:
params = parse_id_or_slug_params(["1", "2", "3"])
assert params.ids == {1, 2, 3}
assert params.slugs == set()

def test_slug_values(self) -> None:
params = parse_id_or_slug_params(["my-project", "another-proj"])
assert params.ids == set()
assert params.slugs == {"my-project", "another-proj"}

def test_mixed_values(self) -> None:
params = parse_id_or_slug_params(["1", "my-project", 42])
assert params.ids == {1, 42}
assert params.slugs == {"my-project"}

def test_empty_values_are_skipped(self) -> None:
params = parse_id_or_slug_params(["", None, "1", "foo"])
assert params.ids == {1}
assert params.slugs == {"foo"}

def test_all_access_numeric_sentinel_is_id(self) -> None:
params = parse_id_or_slug_params(["-1"])
assert params.ids == {-1}
assert params.slugs == set()

def test_negative_non_sentinel_is_slug(self) -> None:
params = parse_id_or_slug_params(["-2"])
assert params.ids == set()
assert params.slugs == {"-2"}

def test_all_access_sigil_is_slug(self) -> None:
params = parse_id_or_slug_params(["$all"])
assert params.ids == set()
assert params.slugs == {"$all"}

def test_detects_all_access_sentinels(self) -> None:
assert parse_id_or_slug_params(["-1"]).has_all_projects_sentinel
assert parse_id_or_slug_params(["$all"]).has_all_projects_sentinel
assert not parse_id_or_slug_params(["1", "my-project"]).has_all_projects_sentinel

def test_deduplication(self) -> None:
params = parse_id_or_slug_params(["1", "1", "foo", "foo"])
assert params.ids == {1}
assert params.slugs == {"foo"}


class TestProjectIdOrSlugField:
def test_accepts_ids_and_slugs(self) -> None:
field = ProjectIdOrSlugField()
assert field.to_internal_value("1") == 1
assert field.to_internal_value(2) == 2
assert field.to_internal_value("-1") == -1
assert field.to_internal_value("my-project") == "my-project"

def test_negative_non_sentinel_string_is_slug(self) -> None:
field = ProjectIdOrSlugField()
assert field.to_internal_value("-2") == "-2"

def test_rejects_invalid_slug(self) -> None:
field = ProjectIdOrSlugField()

with pytest.raises(serializers.ValidationError) as error:
field.to_internal_value("foo bar")

assert "Enter a valid slug" in str(error.value)
40 changes: 40 additions & 0 deletions tests/sentry/api/serializers/rest_framework/test_project.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
from unittest.mock import Mock

from rest_framework import serializers

from sentry.api.serializers.rest_framework.project import ProjectField
from sentry.testutils.cases import TestCase


class ProjectFieldTest(TestCase):
def test_id_allowed_accepts_project_id(self) -> None:
project = self.create_project()
access = Mock()
access.has_any_project_scope.return_value = True

class ProjectSerializer(serializers.Serializer):
project = ProjectField(scope="project:read", id_allowed=True)

serializer = ProjectSerializer(
data={"project": str(project.id)},
context={"organization": project.organization, "access": access},
)

assert serializer.is_valid(), serializer.errors
assert serializer.validated_data["project"] == project

def test_default_rejects_project_id(self) -> None:
project = self.create_project()
access = Mock()
access.has_any_project_scope.return_value = True

class ProjectSerializer(serializers.Serializer):
project = ProjectField(scope="project:read")

serializer = ProjectSerializer(
data={"project": str(project.id)},
context={"organization": project.organization, "access": access},
)

assert not serializer.is_valid()
assert serializer.errors == {"project": ["Invalid project"]}
11 changes: 11 additions & 0 deletions tests/sentry/models/test_project.py
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
from collections.abc import Iterable
from unittest.mock import MagicMock, patch

import pytest
from django.core.exceptions import ValidationError

from sentry.constants import ObjectStatus
from sentry.db.models.fields.slug import SentrySlugField
from sentry.deletions.models.scheduleddeletion import CellScheduledDeletion
from sentry.deletions.tasks.hybrid_cloud import schedule_hybrid_cloud_foreign_key_jobs_control
from sentry.grouping.grouptype import ErrorGroupType
Expand Down Expand Up @@ -48,6 +52,13 @@


class ProjectTest(APITestCase, TestCase):
def test_slug_rejects_numeric_values(self) -> None:
slug_field = Project._meta.get_field("slug")

assert isinstance(slug_field, SentrySlugField)
with pytest.raises(ValidationError):
slug_field.run_validators("123")

def test_member_set_simple(self) -> None:
user = self.create_user()
org = self.create_organization(owner=user)
Expand Down
Loading