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).

Author: gagantrivedi

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):

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]

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,