fix(Identities): Sanitize identifiers (#6024)

Author: emyller

api/edge_api/identities/serializers.py

@@ -18,6 +18,7 @@
 from rest_framework.exceptions import ValidationError
 
 from environments.dynamodb.types import IdentityOverrideV2
+from environments.identities.constants import identifier_regex_validator
 from environments.models import Environment
 from features.models import Feature, FeatureState, FeatureStateValue
 from features.multivariate.models import MultivariateFeatureOption
@@ -52,7 +53,11 @@ def to_internal_value(self, data: typing.Any) -> str:
 
 class EdgeIdentitySerializer(serializers.Serializer):  # type: ignore[type-arg]
     identity_uuid = serializers.CharField(read_only=True)
-    identifier = serializers.CharField(required=True, max_length=2000)
+    identifier = serializers.CharField(
+        required=True,
+        max_length=2000,
+        validators=[identifier_regex_validator],
+    )
     dashboard_alias = LowerCaseCharField(
         required=False,
         max_length=100,

api/environments/identities/constants.py

@@ -0,0 +1,10 @@
+import re
+
+from django.core.validators import RegexValidator
+
+RE_VALID_IDENTIFIER = re.compile(r"^[\w!#$%&*+/=?^_`{}|~@.\-]+$")
+
+identifier_regex_validator = RegexValidator(
+    regex=RE_VALID_IDENTIFIER,
+    message="Identifier can only contain unicode letters, numbers, and the symbols: ! # $ %% & * + / = ? ^ _ ` { } | ~ @ . -",
+)

api/environments/identities/migrations/0003_sanitized_identifiers.py

@@ -0,0 +1,29 @@
+# Generated by Django 4.2.22 on 2025-09-08 23:30
+
+import re
+
+import django.core.validators
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("identities", "0002_alter_identity_index_together"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="identity",
+            name="identifier",
+            field=models.CharField(
+                max_length=2000,
+                validators=[
+                    django.core.validators.RegexValidator(
+                        message="Identifier can only contain unicode letters, numbers, and the symbols: ! # $ %% & * + / = ? ^ _ ` { } | ~ @ . -",
+                        regex=re.compile("^[\\w!#$%&*+/=?^_`{}|~@.\\-]+$"),
+                    )
+                ],
+            ),
+        ),
+    ]

api/environments/identities/models.py

@@ -5,6 +5,7 @@
 from flag_engine.context.mappers import map_environment_identity_to_context
 from flag_engine.segments.evaluator import is_context_in_segment
 
+from environments.identities.constants import identifier_regex_validator
 from environments.identities.managers import IdentityManager
 from environments.identities.traits.models import Trait
 from environments.models import Environment
@@ -21,7 +22,10 @@
 
 
 class Identity(models.Model):
-    identifier = models.CharField(max_length=2000)
+    identifier = models.CharField(
+        max_length=2000,
+        validators=[identifier_regex_validator],
+    )
     created_date = models.DateTimeField("DateCreated", auto_now_add=True)
     environment = models.ForeignKey(
         Environment, related_name="identities", on_delete=models.CASCADE

api/environments/identities/serializers.py

@@ -64,10 +64,13 @@ class _TraitSerializer(serializers.Serializer):  # type: ignore[type-arg]
     traits = serializers.ListSerializer(child=_TraitSerializer())  # type: ignore[var-annotated]
 
 
-class SDKIdentitiesQuerySerializer(serializers.Serializer):  # type: ignore[type-arg]
-    identifier = serializers.CharField(required=True)
+class SDKIdentitiesQuerySerializer(serializers.ModelSerializer[Identity]):
     transient = serializers.BooleanField(default=False)
 
+    class Meta:
+        model = Identity
+        fields = ("identifier", "transient")
+
 
 class IdentityAllFeatureStatesFeatureSerializer(serializers.Serializer):  # type: ignore[type-arg]
     id = serializers.IntegerField()

api/environments/identities/views.py

@@ -15,6 +15,7 @@
 from drf_yasg.utils import swagger_auto_schema  # type: ignore[import-untyped]
 from rest_framework import status, viewsets
 from rest_framework.permissions import IsAuthenticated
+from rest_framework.request import Request
 from rest_framework.response import Response
 
 from app.pagination import CustomPagination
@@ -149,7 +150,7 @@ class SDKIdentities(SDKAPIView):
     pagination_class = None  # set here to ensure documentation is correct
     throttle_classes = []
 
-    @swagger_auto_schema(
+    @swagger_auto_schema(  # type: ignore[misc]
         responses={200: SDKIdentitiesResponseSerializer()},
         query_serializer=SDKIdentitiesQuerySerializer(),
         operation_id="identify_user",
@@ -161,14 +162,14 @@ class SDKIdentities(SDKAPIView):
             cache=settings.GET_IDENTITIES_ENDPOINT_CACHE_NAME,
         )
     )
-    def get(self, request):  # type: ignore[no-untyped-def]
-        identifier = request.query_params.get("identifier")
-        if not identifier:
-            return Response(
-                {"detail": "Missing identifier"}
-            )  # TODO: add 400 status - will this break the clients?
-
-        if request.query_params.get("transient"):
+    def get(self, request: Request) -> Response:
+        query_serializer = SDKIdentitiesQuerySerializer(data=request.query_params)
+        if not query_serializer.is_valid():
+            return Response(query_serializer.errors, status=status.HTTP_400_BAD_REQUEST)
+
+        identifier = query_serializer.validated_data["identifier"]
+
+        if query_serializer.validated_data["transient"]:
             identity = Identity(
                 created_date=timezone.now(),
                 identifier=identifier,

api/tests/unit/conftest.py

@@ -17,6 +17,41 @@
 from util.mappers import map_environment_to_environment_document
 
 
+def pytest_generate_tests(metafunc: pytest.Metafunc) -> None:
+    if metafunc.definition.get_closest_marker("valid_identity_identifiers"):
+        metafunc.parametrize(
+            "identifier",
+            [
+                "bond...jamesbond",
+                "ゴジラ",
+                "ElChapulínColorado",
+                "dalek#6453@skaro.gov",
+                "agáta={^_^}=",
+                "_ツ_/-handless-shrug",
+                "who+am+i?",
+                "i_100%_dont_know!",
+                "~neo|simulation`0065192*75`",
+                "KacperGustyr$Flagsmat",
+            ],
+        )
+
+    if metafunc.definition.get_closest_marker("invalid_identity_identifiers"):
+        error_message = "Identifier can only contain unicode letters, numbers, and the symbols: ! # $ % & * + / = ? ^ _ ` { } | ~ @ . -"
+        metafunc.parametrize(
+            ["identifier", "identifier_error_message"],
+            [
+                ("", "This field may not be blank."),
+                (" ", "This field may not be blank."),
+                ("or really anything with a whitespace", error_message),
+                ("<script>alert(1)</script>", error_message),
+                ("'; DROP TABLE users;--", error_message),
+                ("'single-quotes'", error_message),
+                ('"double-quotes"', error_message),
+                ("figaro" * 334, "Ensure this field has no more than 2000 characters."),
+            ],
+        )
+
+
 @pytest.fixture()
 def organisation_one(db):  # type: ignore[no-untyped-def]
     return Organisation.objects.create(name="Test organisation 1")

api/tests/unit/edge_api/identities/test_edge_api_identities_views.py

@@ -1,5 +1,7 @@
 import typing
+from unittest.mock import Mock
 
+import pytest
 from common.environments.permissions import (
     MANAGE_IDENTITIES,
     VIEW_ENVIRONMENT,
@@ -213,3 +215,42 @@ def test_user_cannot_delete_identity_from_another_project(
     assert response.status_code == status.HTTP_403_FORBIDDEN
 
     assert flagsmith_identities_table.get_item(Key={"composite_key": composite_key})
+
+
+@pytest.mark.valid_identity_identifiers
+def test_EdgeIdentityViewSet__identifier_sanitization__accepts_valid_identifiers(
+    admin_client: APIClient,
+    edge_identity_dynamo_wrapper_mock: Mock,
+    environment: Environment,
+    identifier: str,
+) -> None:
+    # Given
+    edge_identity_dynamo_wrapper_mock.get_item.return_value = None
+
+    # When
+    response = admin_client.post(
+        f"/api/v1/environments/{environment.api_key}/edge-identities/",
+        data={"identifier": identifier},
+    )
+
+    # Then
+    assert response.status_code == status.HTTP_201_CREATED
+    assert response.json()["identifier"] == identifier
+
+
+@pytest.mark.invalid_identity_identifiers
+def test_EdgeIdentityViewSet__identifier_sanitization__rejects_invalid_identifiers(
+    admin_client: APIClient,
+    environment: Environment,
+    identifier_error_message: str,
+    identifier: str,
+) -> None:
+    # When
+    response = admin_client.post(
+        f"/api/v1/environments/{environment.api_key}/edge-identities/",
+        data={"identifier": identifier},
+    )
+
+    # Then
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert response.json() == {"identifier": [identifier_error_message]}

api/tests/unit/environments/identities/test_unit_identities_views.py

@@ -2,6 +2,7 @@
 import urllib
 from typing import Any
 from unittest import mock
+from urllib.parse import quote
 
 import pytest
 from common.environments.permissions import (
@@ -1431,7 +1432,7 @@ def test_SDKIdentitiesDeprecated__given_identifier__retrieves_identity(
         pytest.param(True, False, True, 4, id="replica_db,old_identity,transient"),
     ],
 )
-def test_SDKIdentities_retrieves_identity_feature_states(
+def test_SDKIdentities__retrieves_identity_feature_states(
     api_client: APIClient,
     django_assert_num_queries: DjangoAssertNumQueries,
     environment: Environment,
@@ -1458,3 +1459,105 @@ def test_SDKIdentities_retrieves_identity_feature_states(
 
     # Then
     assert response.status_code == status.HTTP_200_OK
+
+
+@pytest.mark.parametrize(
+    ["transient_value", "expected_status"],
+    [
+        ("true", status.HTTP_200_OK),
+        ("false", status.HTTP_200_OK),
+        ("1", status.HTTP_200_OK),
+        ("0", status.HTTP_200_OK),
+        ("yes", status.HTTP_200_OK),
+        ("no", status.HTTP_200_OK),
+        ("foo", status.HTTP_400_BAD_REQUEST),
+        ("123", status.HTTP_400_BAD_REQUEST),
+    ],
+)
+def test_SDKIdentities__given_transient_value__responds_accordingly(
+    api_client: APIClient,
+    environment: Environment,
+    transient_value: str,
+    expected_status: int,
+) -> None:
+    # Given
+    api_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment.api_key)
+
+    # When
+    response = api_client.get(
+        f"/api/v1/identities/?identifier=jamesbond&transient={transient_value}"
+    )
+
+    # Then
+    assert response.status_code == expected_status
+
+
+@pytest.mark.valid_identity_identifiers
+def test_SDKIdentities__identifier_sanitization__accepts_valid_identifiers(
+    api_client: APIClient,
+    environment: Environment,
+    identifier: str,
+) -> None:
+    # Given
+    api_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment.api_key)
+
+    # When
+    response = api_client.get(f"/api/v1/identities/?identifier={quote(identifier)}")
+
+    # Then
+    assert response.status_code == status.HTTP_200_OK
+    assert response.json()["identifier"] == identifier
+
+
+@pytest.mark.invalid_identity_identifiers
+def test_SDKIdentities__identifier_sanitization__rejects_invalid_identifiers(
+    api_client: APIClient,
+    environment: Environment,
+    identifier: str,
+    identifier_error_message: str,
+) -> None:
+    # Given
+    api_client.credentials(HTTP_X_ENVIRONMENT_KEY=environment.api_key)
+
+    # When
+    response = api_client.get(f"/api/v1/identities/?identifier={quote(identifier)}")
+
+    # Then
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert response.json() == {"identifier": [identifier_error_message]}
+
+
+@pytest.mark.valid_identity_identifiers
+def test_IdentityViewSet_create__accepts_valid_identifiers(
+    admin_client: APIClient,
+    environment: Environment,
+    identifier: str,
+) -> None:
+    # When
+    response = admin_client.post(
+        f"/api/v1/environments/{environment.api_key}/identities/",
+        data={"identifier": identifier},
+    )
+
+    # Then
+    assert response.status_code == status.HTTP_201_CREATED
+    assert response.json()["identifier"] == identifier
+    assert Identity.objects.filter(identifier=identifier).exists()
+
+
+@pytest.mark.invalid_identity_identifiers
+def test_IdentityViewSet_create__rejects_invalid_identifiers(
+    admin_client: APIClient,
+    environment: Environment,
+    identifier: str,
+    identifier_error_message: str,
+) -> None:
+    # When
+    response = admin_client.post(
+        f"/api/v1/environments/{environment.api_key}/identities/",
+        data={"identifier": identifier},
+    )
+
+    # Then
+    assert response.status_code == status.HTTP_400_BAD_REQUEST
+    assert response.json() == {"identifier": [identifier_error_message]}