From bfbf2530331fc8bda838624bfc18dee0eee3f264 Mon Sep 17 00:00:00 2001 From: lebaudantoine Date: Mon, 2 Mar 2026 12:22:05 +0100 Subject: [PATCH] =?UTF-8?q?=F0=9F=94=92=EF=B8=8F(backend)=20enhance=20API?= =?UTF-8?q?=20input=20validation=20to=20strengthen=20security?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit During the bug bounty, attempts were made to pass unexpected hidden fields to manipulate room behavior and join as a ghost. Treat these parameters as suspicious. They are not sent by the frontend, so their presence likely indicates tampering. Explicitly allow the parameters but emit warning logs to help detect and investigate suspicious activity. --- CHANGELOG.md | 4 + src/backend/core/api/serializers.py | 66 +++++-- src/backend/core/api/viewsets.py | 4 +- .../test_api_rooms_participants_management.py | 162 +++++++++++++++++- 4 files changed, 219 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 20b6c37d..3f9cddec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,10 @@ and this project adheres to ## [Unreleased] +### Changed + +- 🔒️(backend) enhance API input validation to strengthen security #1053 + ## [1.9.0] - 2026-03-02 ### Added diff --git a/src/backend/core/api/serializers.py b/src/backend/core/api/serializers.py index d4ce5905..5405b1cb 100644 --- a/src/backend/core/api/serializers.py +++ b/src/backend/core/api/serializers.py @@ -2,9 +2,11 @@ # pylint: disable=abstract-method,no-name-in-module +from django.core.exceptions import SuspiciousOperation from django.utils.translation import gettext_lazy as _ -from livekit.api import ParticipantPermission +from django_pydantic_field.rest_framework import SchemaField +from pydantic import BaseModel, Field from rest_framework import serializers from rest_framework.exceptions import PermissionDenied from timezone_field.rest_framework import TimeZoneSerializerField @@ -261,6 +263,28 @@ class MuteParticipantSerializer(BaseParticipantsManagementSerializer): ) +class ParticipantPermission(BaseModel): + """Mirror the LiveKit ParticipantPermission protobuf. + + Control what a participant is allowed to publish, subscribe, and do within a room. + Unknown fields are rejected. + """ + + can_subscribe: bool | None = None + can_publish: bool | None = None + can_publish_data: bool | None = None + can_publish_sources: list[int] = Field( + default_factory=list + ) # TrackSource enum values + hidden: bool | None = None + recorder: bool | None = None + can_update_metadata: bool | None = None + agent: bool | None = None + can_subscribe_metrics: bool | None = None + + model_config = {"extra": "forbid"} + + class UpdateParticipantSerializer(BaseParticipantsManagementSerializer): """Validate participant update data.""" @@ -272,10 +296,11 @@ class UpdateParticipantSerializer(BaseParticipantsManagementSerializer): allow_null=True, help_text="Participant attributes as JSON object", ) - permission = serializers.DictField( + permission = SchemaField( + schema=ParticipantPermission | None, required=False, allow_null=True, - help_text="Participant permission as JSON object", + help_text="Participant permissions", ) name = serializers.CharField( max_length=255, @@ -285,6 +310,33 @@ class UpdateParticipantSerializer(BaseParticipantsManagementSerializer): help_text="Display name for the participant", ) + def validate_permission(self, permission): + """Validate that the given permission does not include forbidden or unimplemented fields.""" + + if permission is None: + return None + + suspicious_fields = [ + field + for field in ("hidden", "recorder", "agent") + if getattr(permission, field) is not None + ] + if suspicious_fields: + raise SuspiciousOperation( + f"Setting the following participant permissions is not allowed: " + f"{', '.join(suspicious_fields)}." + ) + if permission.can_subscribe_metrics is not None: + raise serializers.ValidationError( + { + "permission": { + "can_subscribe_metrics": "This permission is not implemented." + } + } + ) + + return permission + def validate(self, attrs): """Ensure at least one update field is provided.""" update_fields = ["metadata", "attributes", "permission", "name"] @@ -300,12 +352,4 @@ class UpdateParticipantSerializer(BaseParticipantsManagementSerializer): f"{', '.join(update_fields)}." ) - if "permission" in attrs: - try: - ParticipantPermission(**attrs["permission"]) - except ValueError as e: - raise serializers.ValidationError( - {"permission": f"Invalid permission: {str(e)}"} - ) from e - return attrs diff --git a/src/backend/core/api/viewsets.py b/src/backend/core/api/viewsets.py index 15159770..b2f735ea 100644 --- a/src/backend/core/api/viewsets.py +++ b/src/backend/core/api/viewsets.py @@ -607,13 +607,15 @@ class RoomViewSet( serializer = serializers.UpdateParticipantSerializer(data=request.data) serializer.is_valid(raise_exception=True) + permission = serializer.validated_data.get("permission") + try: ParticipantsManagement().update( room_name=str(room.pk), identity=str(serializer.validated_data["participant_identity"]), metadata=serializer.validated_data.get("metadata"), attributes=serializer.validated_data.get("attributes"), - permission=serializer.validated_data.get("permission"), + permission=permission.model_dump() if permission else None, name=serializer.validated_data.get("name"), ) except ParticipantsManagementException: diff --git a/src/backend/core/tests/rooms/test_api_rooms_participants_management.py b/src/backend/core/tests/rooms/test_api_rooms_participants_management.py index 647fe007..6d51f17d 100644 --- a/src/backend/core/tests/rooms/test_api_rooms_participants_management.py +++ b/src/backend/core/tests/rooms/test_api_rooms_participants_management.py @@ -8,6 +8,7 @@ import random from unittest import mock from uuid import uuid4 +from django.core.exceptions import SuspiciousOperation from django.urls import reverse import pytest @@ -132,11 +133,7 @@ def test_update_participant_success(mock_livekit_client): 1, 2, ], # [TrackSource.CAMERA, TrackSource.MICROPHONE] - "hidden": False, - "recorder": False, "can_update_metadata": True, - "agent": False, - "can_subscribe_metrics": False, }, "name": "John Doe", } @@ -151,6 +148,151 @@ def test_update_participant_success(mock_livekit_client): mock_livekit_client.aclose.assert_called_once() +@pytest.mark.parametrize( + "permission_payload", + [ + {}, # empty dict is valid + {"can_subscribe": True}, + {"can_publish": True}, + {"can_publish_data": True}, + {"can_publish_sources": [1, 2]}, + {"can_update_metadata": True}, + ], +) +def test_update_participant_permission_fields_are_optional( + mock_livekit_client, permission_payload +): + """Test that each required permission field can be passed individually.""" + client = APIClient() + room = RoomFactory() + user = UserFactory() + UserResourceAccessFactory( + resource=room, user=user, role=random.choice(["administrator", "owner"]) + ) + client.force_authenticate(user=user) + + payload = { + "participant_identity": str(uuid4()), + "permission": permission_payload, + } + + url = reverse("rooms-update-participant", kwargs={"pk": room.id}) + response = client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_200_OK + assert response.data == {"status": "success"} + + mock_livekit_client.room.update_participant.assert_called_once() + mock_livekit_client.aclose.assert_called_once() + + +@pytest.mark.parametrize( + "value,permission_key", + [ + (False, "hidden"), + (True, "hidden"), + (False, "recorder"), + (True, "recorder"), + (False, "agent"), + (True, "agent"), + ], +) +@mock.patch("core.api.serializers.SuspiciousOperation", side_effect=SuspiciousOperation) +def test_update_participant_suspicious_permission( + mock_suspicious, value, permission_key +): + """Test update participant raises 400 when a restricted permission is set.""" + client = APIClient() + room = RoomFactory() + user = UserFactory() + UserResourceAccessFactory( + resource=room, user=user, role=random.choice(["administrator", "owner"]) + ) + client.force_authenticate(user=user) + + payload = { + "participant_identity": str(uuid4()), + "permission": { + "can_subscribe": True, + "can_publish": True, + "can_publish_data": True, + "can_update_metadata": False, + permission_key: value, + }, + } + + url = reverse("rooms-update-participant", kwargs={"pk": room.id}) + response = client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + mock_suspicious.assert_called_once_with( + f"Setting the following participant permissions is not allowed: {permission_key}." + ) + + +@mock.patch("core.api.serializers.SuspiciousOperation", side_effect=SuspiciousOperation) +def test_update_participant_suspicious_permission_multiple(mock_suspicious): + """Test update participant raises 400 when multiple suspicious permissions are set.""" + client = APIClient() + room = RoomFactory() + user = UserFactory() + UserResourceAccessFactory( + resource=room, user=user, role=random.choice(["administrator", "owner"]) + ) + client.force_authenticate(user=user) + + payload = { + "participant_identity": str(uuid4()), + "permission": { + "can_subscribe": True, + "can_publish": True, + "can_publish_data": True, + "hidden": True, + "recorder": False, + "can_update_metadata": False, + "agent": True, + "can_subscribe_metrics": False, + }, + } + + url = reverse("rooms-update-participant", kwargs={"pk": room.id}) + response = client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + mock_suspicious.assert_called_once_with( + "Setting the following participant permissions is not allowed: hidden, recorder, agent." + ) + + +@pytest.mark.parametrize("value", (False, True)) +def test_update_participant_unimplemented_can_subscribe_metrics(value): + """Test update participant raises 400 when can_subscribe_metrics is set.""" + client = APIClient() + room = RoomFactory() + user = UserFactory() + UserResourceAccessFactory( + resource=room, user=user, role=random.choice(["administrator", "owner"]) + ) + client.force_authenticate(user=user) + + payload = { + "participant_identity": str(uuid4()), + "permission": { + "can_subscribe": True, + "can_publish": True, + "can_publish_data": True, + "can_update_metadata": False, + "can_subscribe_metrics": value, + }, + } + + url = reverse("rooms-update-participant", kwargs={"pk": room.id}) + response = client.post(url, payload, format="json") + + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "can_subscribe_metrics" in str(response.data) + + def test_update_participant_forbidden_without_access(): """Test update participant returns 403 when user lacks room privileges.""" client = APIClient() @@ -226,7 +368,17 @@ def test_update_participant_invalid_permission(): response = client.post(url, payload, format="json") assert response.status_code == status.HTTP_400_BAD_REQUEST - assert "Invalid permission" in str(response.data) + assert response.json() == { + "permission": [ + { + "type": "extra_forbidden", + "loc": ["invalid-attributes"], + "msg": "Extra inputs are not permitted", + "input": "True", + "url": "https://errors.pydantic.dev/2.12/v/extra_forbidden", + }, + ] + } def test_update_participant_wrong_metadata_attributes():