From d29b554263058c4b80d1562d8277a007c465d227 Mon Sep 17 00:00:00 2001 From: Gagan Trivedi Date: Mon, 8 Dec 2025 14:09:42 +0530 Subject: [PATCH] fix: prevent IDOR vulnerability in environment update endpoint Make the `project` field read-only during environment updates to prevent attackers from moving an environment to a different project they don't own. The vulnerability allowed an attacker with access to their own environment to modify the `project` field in the PUT request body, effectively moving their environment into a victim's project. Fix: Override __init__ in CreateUpdateEnvironmentSerializer to set project field as read-only when instance exists (update operation). --- api/environments/serializers.py | 36 +++++++++++-------- api/environments/views.py | 9 +++-- .../test_unit_environments_views.py | 32 +++++++++++++++++ 3 files changed, 60 insertions(+), 17 deletions(-) diff --git a/api/environments/serializers.py b/api/environments/serializers.py index 3448b2b85374..1e575c6a0e67 100644 --- a/api/environments/serializers.py +++ b/api/environments/serializers.py @@ -115,7 +115,7 @@ class Meta(EnvironmentSerializerWithMetadata.Meta): ) -class CreateUpdateEnvironmentSerializer( +class _BaseCreateUpdateEnvironmentSerializer( ReadOnlyIfNotValidPlanMixin, EnvironmentSerializerWithMetadata ): invalid_plans = ("free",) @@ -130,25 +130,33 @@ class Meta(EnvironmentSerializerWithMetadata.Meta): ) ] + +class CreateEnvironmentSerializer(_BaseCreateUpdateEnvironmentSerializer): def get_subscription(self) -> Subscription | None: view = self.context["view"] + # handle `project` not being part of the data + # When request comes from yasg2(as part of schema generation) + project_id = view.request.data.get("project") + if not project_id: + return None - if view.action == "create": - # handle `project` not being part of the data - # When request comes from yasg2(as part of schema generation) - project_id = view.request.data.get("project") - if not project_id: - return None + project = Project.objects.select_related( + "organisation", "organisation__subscription" + ).get(id=project_id) - project = Project.objects.select_related( - "organisation", "organisation__subscription" - ).get(id=project_id) + return getattr(project.organisation, "subscription", None) - return getattr(project.organisation, "subscription", None) - elif view.action in ("update", "partial_update"): - return getattr(self.instance.project.organisation, "subscription", None) # type: ignore[union-attr] - return None +class UpdateEnvironmentSerializer(_BaseCreateUpdateEnvironmentSerializer): + class Meta(_BaseCreateUpdateEnvironmentSerializer.Meta): + read_only_fields = EnvironmentSerializerLight.Meta.read_only_fields + ( # type: ignore[assignment] + "project", + ) + + def get_subscription(self) -> Subscription | None: + if self.instance is None: + return None + return getattr(self.instance.project.organisation, "subscription", None) # type: ignore[union-attr] class CloneEnvironmentSerializer(EnvironmentSerializerLight): diff --git a/api/environments/views.py b/api/environments/views.py index 7e74f7bf25ce..4fd405737be6 100644 --- a/api/environments/views.py +++ b/api/environments/views.py @@ -55,10 +55,11 @@ ) from .serializers import ( CloneEnvironmentSerializer, - CreateUpdateEnvironmentSerializer, + CreateEnvironmentSerializer, EnvironmentAPIKeySerializer, EnvironmentRetrieveSerializerWithMetadata, EnvironmentSerializerWithMetadata, + UpdateEnvironmentSerializer, WebhookSerializer, ) @@ -94,8 +95,10 @@ def get_serializer_class(self): # type: ignore[no-untyped-def] return CloneEnvironmentSerializer if self.action == "retrieve": return EnvironmentRetrieveSerializerWithMetadata - elif self.action in ("create", "update", "partial_update"): - return CreateUpdateEnvironmentSerializer + elif self.action == "create": + return CreateEnvironmentSerializer + elif self.action in ("update", "partial_update"): + return UpdateEnvironmentSerializer return EnvironmentSerializerWithMetadata def get_serializer_context(self): # type: ignore[no-untyped-def] diff --git a/api/tests/unit/environments/test_unit_environments_views.py b/api/tests/unit/environments/test_unit_environments_views.py index f6d90e12d5cb..507219f198d3 100644 --- a/api/tests/unit/environments/test_unit_environments_views.py +++ b/api/tests/unit/environments/test_unit_environments_views.py @@ -1083,6 +1083,38 @@ def test_environment_update_cannot_change_is_creating( assert response.json()["is_creating"] is False +def test_environment_update_cannot_change_project( + environment: Environment, + project: Project, + organisation: Organisation, + admin_client_new: APIClient, +) -> None: + # Given - an environment in the original project + original_project = project + url = reverse("api-v1:environments:environment-detail", args=[environment.api_key]) + + # and a different project + other_project = Project.objects.create( + name="Other Project", organisation=organisation + ) + + data = { + "project": other_project.id, + "name": environment.name, + } + + # When + response = admin_client_new.put( + url, data=json.dumps(data), content_type="application/json" + ) + + # Then - the project should NOT change + assert response.status_code == status.HTTP_200_OK + environment.refresh_from_db() + assert environment.project_id == original_project.id + assert response.json()["project"] == original_project.id + + def test_get_document( environment: Environment, project: Project,