Skip to content
Draft
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
8 changes: 8 additions & 0 deletions sdk/src/firetower_sdk/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,14 @@ def get_incident(self, incident_id: str) -> dict[str, Any]:
"""Get an incident by ID."""
return self._request("GET", f"/api/incidents/{incident_id}/")

def get_incident_status(self, incident_id: str) -> dict[str, Any]:
"""Get the status of an incident by ID.

Returns only the incident's id and status. Requires the
`incidents.view_all_incident_statuses` permission on the server.
"""
return self._request("GET", f"/api/incidents/{incident_id}/status/")

def list_incidents(
self,
statuses: list[IncidentStatus] | None = None,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# Generated by Django 5.2.14 on 2026-05-15 19:22

from django.db import migrations


class Migration(migrations.Migration):
dependencies = [
("incidents", "0016_schedule_demo"),
]

operations = [
migrations.AlterModelOptions(
name="incident",
options={
"ordering": ["-created_at"],
"permissions": [
(
"view_all_incident_statuses",
"Can view status of all incidents, including private",
)
],
},
),
]
6 changes: 6 additions & 0 deletions src/firetower/incidents/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -248,6 +248,12 @@ class Meta:
models.Index(fields=["status", "-created_at"]),
models.Index(fields=["severity", "-created_at"]),
]
permissions = [
(
"view_all_incident_statuses",
"Can view status of all incidents, including private",
),
]

@property
def incident_number(self) -> str:
Expand Down
14 changes: 14 additions & 0 deletions src/firetower/incidents/permissions.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,20 @@
return request.user and request.user.is_authenticated


class IncidentStatusPermission(permissions.BasePermission):
"""
Permission for the incident status endpoint.

Requires the django-admin `incidents.view_all_incident_statuses` permission,
which grants access to the status of any incident (including private ones).
"""

def has_permission(self, request: Request, view: "APIView") -> bool:
if not (request.user and request.user.is_authenticated):

Check warning on line 34 in src/firetower/incidents/permissions.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[XR6-53F] Private incident existence leaked via 403 vs 404 in IncidentStatusRetrieveAPIView (additional location)

`IncidentStatusRetrieveAPIView.get_object()` fetches from `Incident.objects.all()` before checking permissions, so an unauthorized user who requests a private incident they cannot see receives 403 (incident exists) rather than 404 — revealing its existence. Fix in `views.py`: apply `filter_visible_to_user` to the queryset when the user lacks `incidents.view_all_incident_statuses`, mirroring the pattern in `IncidentRetrieveUpdateAPIView`.
return False
return request.user.has_perm("incidents.view_all_incident_statuses")


class IncidentPermission(permissions.BasePermission):
"""
Permission class for incident CRUD operations.
Expand Down
11 changes: 11 additions & 0 deletions src/firetower/incidents/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,17 @@ def get_participants(self, obj: Incident) -> list[ParticipantData]:
return participants_list


class IncidentStatusSerializer(serializers.ModelSerializer):
"""Serializer that exposes only the incident's status."""

id = serializers.CharField(source="incident_number", read_only=True)

class Meta:
model = Incident
fields = ["id", "status"]
read_only_fields = ["id", "status"]


class IncidentReadSerializer(serializers.ModelSerializer):
"""
Serializer for reading incidents via the service API.
Expand Down
153 changes: 152 additions & 1 deletion src/firetower/incidents/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

import pytest
from django.conf import settings
from django.contrib.auth.models import User
from django.contrib.auth.models import Permission, User

Check failure on line 7 in src/firetower/incidents/tests/test_views.py

View check run for this annotation

@sentry/warden / warden: code-review

IncidentStatusPermission missing has_object_permission causes OR composition to bypass visibility checks

Because `IncidentStatusPermission` never overrides `has_object_permission`, DRF's `BasePermission` default returns `True` for it — so `IncidentPermission | IncidentStatusPermission` always resolves `has_object_permission` to `True` for any authenticated user, letting them read the status of private incidents they have no visibility to. The fix is to add `has_object_permission` to `IncidentStatusPermission` (in `permissions.py`) that mirrors its `has_permission` check: `return request.user.has_perm('incidents.view_all_incident_statuses')`.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

IncidentStatusPermission missing has_object_permission causes OR composition to bypass visibility checks

Because IncidentStatusPermission never overrides has_object_permission, DRF's BasePermission default returns True for it — so IncidentPermission | IncidentStatusPermission always resolves has_object_permission to True for any authenticated user, letting them read the status of private incidents they have no visibility to. The fix is to add has_object_permission to IncidentStatusPermission (in permissions.py) that mirrors its has_permission check: return request.user.has_perm('incidents.view_all_incident_statuses').

Verification

Traced DRF's OR operator: OR.has_object_permission calls both operands' has_object_permission independently and returns op1 or op2. IncidentStatusPermission only defines has_permission; BasePermission.has_object_permission returns True unconditionally. In IncidentStatusRetrieveAPIView.get_object, get_queryset() returns Incident.objects.all() (unfiltered), and then check_object_permissions is called. For a regular user targeting a private incident they don't own: IncidentPermission.has_object_permission → False (visibility denied); IncidentStatusPermission.has_object_permission → True (default); combined → True → 200. test_user_without_visibility_or_perm_is_denied asserts 403 but would actually receive 200, confirming the test would fail. Files checked: src/firetower/incidents/permissions.py (both classes), src/firetower/incidents/views.py (IncidentStatusRetrieveAPIView).

Also found at 2 additional locations
  • src/firetower/incidents/urls.py:34
  • src/firetower/incidents/views.py:32-38

Identified by Warden code-review · WS3-MGR

from rest_framework.test import APIClient

from firetower.incidents.models import (
Expand Down Expand Up @@ -1481,3 +1481,154 @@

assert response.status_code == 200
assert response.data == ["UsedTwice", "UsedOnce", "Unused"]


@pytest.mark.django_db
class TestIncidentStatusRetrieveAPIView:
"""Tests for GET /api/incidents/{id}/status/."""

def setup_method(self):
self.client = APIClient()
self.user = User.objects.create_user(
username="test@example.com",
email="test@example.com",
password="testpass123",
)
self.other_user = User.objects.create_user(
username="other@example.com",
email="other@example.com",
password="testpass123",
)

def _grant_view_all_statuses(self, user: User) -> None:
perm = Permission.objects.get(codename="view_all_incident_statuses")
user.user_permissions.add(perm)

def test_returns_only_id_and_status(self):
"""Response exposes only id and status — no title or other fields."""
incident = Incident.objects.create(
title="Secret Title",
description="Sensitive description",
status=IncidentStatus.ACTIVE,
severity=IncidentSeverity.P1,
)

self.client.force_authenticate(user=self.user)
response = self.client.get(f"/api/incidents/{incident.incident_number}/status/")

assert response.status_code == 200
assert response.data == {
"id": incident.incident_number,
"status": IncidentStatus.ACTIVE,
}

def test_visible_user_can_read_public_incident_status(self):
"""User with normal read visibility (public incident) gets status."""
incident = Incident.objects.create(
title="Public",
status=IncidentStatus.MITIGATED,
severity=IncidentSeverity.P2,
is_private=False,
)

self.client.force_authenticate(user=self.user)
response = self.client.get(f"/api/incidents/{incident.incident_number}/status/")

assert response.status_code == 200
assert response.data["status"] == IncidentStatus.MITIGATED

def test_captain_can_read_private_incident_status_via_visibility(self):
"""Captain of a private incident has visibility, so no special perm needed."""
incident = Incident.objects.create(
title="Private",
status=IncidentStatus.ACTIVE,
severity=IncidentSeverity.P1,
is_private=True,
captain=self.user,
)

self.client.force_authenticate(user=self.user)
response = self.client.get(f"/api/incidents/{incident.incident_number}/status/")

assert response.status_code == 200
assert response.data["status"] == IncidentStatus.ACTIVE

def test_user_without_visibility_or_perm_is_denied(self):
"""User who can't see a private incident and lacks the perm is denied."""
incident = Incident.objects.create(
title="Other's Private",
status=IncidentStatus.ACTIVE,
severity=IncidentSeverity.P1,
is_private=True,
captain=self.other_user,
)

self.client.force_authenticate(user=self.user)
response = self.client.get(f"/api/incidents/{incident.incident_number}/status/")

assert response.status_code == 403

Check warning on line 1569 in src/firetower/incidents/tests/test_views.py

View check run for this annotation

@sentry/warden / warden: find-bugs

Private incident existence leaked via 403 vs 404 in IncidentStatusRetrieveAPIView

`IncidentStatusRetrieveAPIView.get_object()` fetches from `Incident.objects.all()` before checking permissions, so an unauthorized user who requests a private incident they cannot see receives 403 (incident exists) rather than 404 — revealing its existence. Fix in `views.py`: apply `filter_visible_to_user` to the queryset when the user lacks `incidents.view_all_incident_statuses`, mirroring the pattern in `IncidentRetrieveUpdateAPIView`.

def test_user_with_view_all_perm_can_read_private_status(self):
"""User without visibility but holding view_all_incident_statuses gets through."""
incident = Incident.objects.create(
title="Other's Private",
status=IncidentStatus.DONE,
severity=IncidentSeverity.P1,
is_private=True,
captain=self.other_user,
)
self._grant_view_all_statuses(self.user)

self.client.force_authenticate(user=self.user)
response = self.client.get(f"/api/incidents/{incident.incident_number}/status/")

assert response.status_code == 200
assert response.data == {
"id": incident.incident_number,
"status": IncidentStatus.DONE,
}

def test_unauthenticated_user_denied(self):
incident = Incident.objects.create(
title="Public",
status=IncidentStatus.ACTIVE,
severity=IncidentSeverity.P1,
)

response = self.client.get(f"/api/incidents/{incident.incident_number}/status/")

assert response.status_code in (401, 403)

def test_superuser_can_read_private_incident_status(self):
superuser = User.objects.create_superuser(
username="admin@example.com",
email="admin@example.com",
password="testpass123",
)
incident = Incident.objects.create(
title="Other's Private",
status=IncidentStatus.ACTIVE,
severity=IncidentSeverity.P1,
is_private=True,
captain=self.other_user,
)

self.client.force_authenticate(user=superuser)
response = self.client.get(f"/api/incidents/{incident.incident_number}/status/")

assert response.status_code == 200
assert response.data["status"] == IncidentStatus.ACTIVE

def test_invalid_format_returns_400(self):
self.client.force_authenticate(user=self.user)
response = self.client.get("/api/incidents/INVALID-123/status/")

assert response.status_code == 400

def test_not_found_returns_404(self):
self.client.force_authenticate(user=self.user)
response = self.client.get(
f"/api/incidents/{settings.PROJECT_KEY}-99999/status/"
)

assert response.status_code == 404
6 changes: 6 additions & 0 deletions src/firetower/incidents/urls.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
AvailabilityView,
IncidentListCreateAPIView,
IncidentRetrieveUpdateAPIView,
IncidentStatusRetrieveAPIView,
TagListCreateAPIView,
incident_detail_ui,
incident_list_ui,
Expand All @@ -28,8 +29,13 @@
path(
"incidents/<str:incident_id>/",
IncidentRetrieveUpdateAPIView.as_view(),
name="incident-retrieve-update",
),
path(

Check failure on line 34 in src/firetower/incidents/urls.py

View check run for this annotation

@sentry/warden / warden: code-review

[WS3-MGR] IncidentStatusPermission missing has_object_permission causes OR composition to bypass visibility checks (additional location)

Because `IncidentStatusPermission` never overrides `has_object_permission`, DRF's `BasePermission` default returns `True` for it — so `IncidentPermission | IncidentStatusPermission` always resolves `has_object_permission` to `True` for any authenticated user, letting them read the status of private incidents they have no visibility to. The fix is to add `has_object_permission` to `IncidentStatusPermission` (in `permissions.py`) that mirrors its `has_permission` check: `return request.user.has_perm('incidents.view_all_incident_statuses')`.
"incidents/<str:incident_id>/status/",
IncidentStatusRetrieveAPIView.as_view(),

Check warning on line 36 in src/firetower/incidents/urls.py

View check run for this annotation

@sentry/warden / warden: find-bugs

[XR6-53F] Private incident existence leaked via 403 vs 404 in IncidentStatusRetrieveAPIView (additional location)

`IncidentStatusRetrieveAPIView.get_object()` fetches from `Incident.objects.all()` before checking permissions, so an unauthorized user who requests a private incident they cannot see receives 403 (incident exists) rather than 404 — revealing its existence. Fix in `views.py`: apply `filter_visible_to_user` to the queryset when the user lacks `incidents.view_all_incident_statuses`, mirroring the pattern in `IncidentRetrieveUpdateAPIView`.
name="incident-status-retrieve",
),
path(
"incidents/<str:incident_id>/sync-participants/",
sync_incident_participants,
Expand Down
43 changes: 42 additions & 1 deletion src/firetower/incidents/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,19 +29,20 @@
Tag,
TagType,
filter_visible_to_user,
)
from .permissions import IncidentPermission
from .permissions import IncidentPermission, IncidentStatusPermission
from .reporting_utils import (
build_incidents_by_tag,
compute_regions,
get_month_periods,
get_quarter_periods,

Check failure on line 38 in src/firetower/incidents/views.py

View check run for this annotation

@sentry/warden / warden: code-review

[WS3-MGR] IncidentStatusPermission missing has_object_permission causes OR composition to bypass visibility checks (additional location)

Because `IncidentStatusPermission` never overrides `has_object_permission`, DRF's `BasePermission` default returns `True` for it — so `IncidentPermission | IncidentStatusPermission` always resolves `has_object_permission` to `True` for any authenticated user, letting them read the status of private incidents they have no visibility to. The fix is to add `has_object_permission` to `IncidentStatusPermission` (in `permissions.py`) that mirrors its `has_permission` check: `return request.user.has_perm('incidents.view_all_incident_statuses')`.
get_year_periods,
)
from .serializers import (
IncidentListUISerializer,
IncidentOrRedirectReadSerializer,
IncidentReadSerializer,
IncidentStatusSerializer,
IncidentWriteSerializer,
TagCreateSerializer,
TagSerializer,
Expand Down Expand Up @@ -265,6 +266,46 @@
return obj


class IncidentStatusRetrieveAPIView(generics.RetrieveAPIView):
"""
Service API for retrieving an incident's status only.

GET: Get incident status

Accepts incident_id in format: INC-2000

Access is granted if either:
- The user has normal read visibility to the incident (IncidentPermission), or
- The user has the `incidents.view_all_incident_statuses` permission
(IncidentStatusPermission), which grants status access to any incident
including private ones.
"""

permission_classes = [IncidentPermission | IncidentStatusPermission]
serializer_class = IncidentStatusSerializer
lookup_field = "id"

def get_queryset(self) -> QuerySet[Incident]:
return Incident.objects.all()

def get_object(self) -> Incident:
incident_id = self.kwargs["incident_id"]
project_key = settings.PROJECT_KEY

incident_pattern = rf"^{re.escape(project_key)}-(\d+)$"
match = re.match(incident_pattern, incident_id, re.IGNORECASE)

if not match:
raise ValidationError(
f"Invalid incident ID format. Expected format: {project_key}-<number> (e.g., {project_key}-123)"
)

numeric_id = int(match.group(1))

Check warning on line 303 in src/firetower/incidents/views.py

View check run for this annotation

@sentry/warden / warden: code-review

Private incident existence disclosed via 403 vs 404 on status endpoint

Unlike every other `get_object()` in this file, this implementation fetches the incident first (returns 404 only if the row is missing) and then checks permissions (returning 403 if denied). A user without visibility and without the special permission receives 403, confirming the incident exists — whereas all other endpoints apply `filter_visible_to_user` before the lookup so non-visible incidents return 404 and reveal nothing. Consider applying `filter_visible_to_user` to the queryset for users who do not hold `incidents.view_all_incident_statuses` before calling `get_object_or_404`.
Comment on lines +301 to +303
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Private incident existence disclosed via 403 vs 404 on status endpoint

Unlike every other get_object() in this file, this implementation fetches the incident first (returns 404 only if the row is missing) and then checks permissions (returning 403 if denied). A user without visibility and without the special permission receives 403, confirming the incident exists — whereas all other endpoints apply filter_visible_to_user before the lookup so non-visible incidents return 404 and reveal nothing. Consider applying filter_visible_to_user to the queryset for users who do not hold incidents.view_all_incident_statuses before calling get_object_or_404.

Verification

Checked IncidentPermission.has_object_permission() (permissions.py:54-58): for SAFE_METHODS it returns obj.is_visible_to_user(user). For a non-captain, non-admin user on a private incident this returns False. IncidentStatusPermission.has_permission() (permissions.py:36-38) also returns False for users without the Django perm. The OR composition therefore denies at the object-permission stage — after the incident has already been fetched — and DRF raises PermissionDenied (HTTP 403). Every other retrieval view (lines 78, 140, 181, 250, 347) applies filter_visible_to_user before get_object_or_404, causing invisible incidents to return 404. The test test_user_without_visibility_or_perm_is_denied (test_views.py ~1567) explicitly asserts 403, confirming the disclosed behaviour is present.

Identified by Warden code-review · 44H-AK7

obj = get_object_or_404(self.get_queryset(), id=numeric_id)
self.check_object_permissions(self.request, obj)
return obj


class SyncIncidentParticipantsView(generics.GenericAPIView):
"""
Force sync incident participants from Slack channel.
Expand Down
Loading