From 163062c1cd020261f4bf35a232b44d640e6121a0 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 12 Feb 2025 19:20:29 -0800 Subject: [PATCH 01/10] Init --- src/sentry/flags/endpoints/secrets.py | 2 +- src/sentry/flags/providers.py | 154 ++++++++++++++++++++++++-- 2 files changed, 147 insertions(+), 9 deletions(-) diff --git a/src/sentry/flags/endpoints/secrets.py b/src/sentry/flags/endpoints/secrets.py index a277e70a103a03..059587e15db45e 100644 --- a/src/sentry/flags/endpoints/secrets.py +++ b/src/sentry/flags/endpoints/secrets.py @@ -43,7 +43,7 @@ def serialize(self, obj, attrs, user, **kwargs) -> FlagWebhookSigningSecretRespo class FlagWebhookSigningSecretValidator(serializers.Serializer): provider = serializers.ChoiceField( - choices=["launchdarkly", "generic", "unleash"], required=True + choices=["launchdarkly", "generic", "unleash", "statsig"], required=True ) secret = serializers.CharField(required=True, max_length=32, min_length=32) diff --git a/src/sentry/flags/providers.py b/src/sentry/flags/providers.py index 312aadef3b619e..6ba2d9842ee86d 100644 --- a/src/sentry/flags/providers.py +++ b/src/sentry/flags/providers.py @@ -15,6 +15,7 @@ FlagWebHookSigningSecretModel, ) from sentry.silo.base import SiloLimit +from sentry.utils.safe import get_path def write(rows: list["FlagAuditLogRow"]) -> None: @@ -54,7 +55,7 @@ class ProviderProtocol(Protocol[T]): provider_name: str signature: str | None - def __init__(self, organization_id: int, signature: str | None) -> None: ... + def __init__(self, organization_id: int, signature: str | None, **kwargs) -> None: ... def handle(self, message: T) -> list[FlagAuditLogRow]: ... def validate(self, message_bytes: bytes) -> bool: ... @@ -82,6 +83,12 @@ def get_provider( return GenericProvider(organization_id, signature=headers.get("X-Sentry-Signature")) case "unleash": return UnleashProvider(organization_id, signature=headers.get("Authorization")) + case "statsig": + return StatsigProvider( + organization_id, + signature=headers.get("X-Statsig-Signature"), + request_timestamp=headers.get("X-Statsig-Request-Timestamp"), + ) case _: return None @@ -355,6 +362,136 @@ def _handle_unleash_actions(action: str) -> int: return ACTION_MAP["updated"] +"""Statsig provider.""" + +SUPPORTED_STATSIG_EVENTS = {"statsig::config_change"} + +# config_change is subclassed by the type of Statsig feature. There's "Gate", +# "Experiment", and more. Feature gates are boolean release flags, but all +# other types are unstructured JSON. To reduce noise, Gate is the only type +# we audit for now. +SUPPORTED_STATSIG_TYPES = { + "Gate", + "gate", # Supporting this just in case. Statsig docs and sample events use capitalization. +} + + +class _StatsigEventSerializer(serializers.Serializer): + eventName = serializers.CharField(required=True) + timestamp = serializers.CharField(required=True) # Custom serializer defined below. + metadata = serializers.DictField(required=True) + + user = serializers.DictField(required=False, child=serializers.CharField()) + userID = serializers.CharField(required=False) + value = serializers.CharField(required=False) + statsigMetadata = serializers.DictField(required=False) + timeUUID = serializers.UUIDField(required=False) + unitID = serializers.CharField(required=False) + + def validate_timestamp(self, value: str): + try: + float(value) + except ValueError: + raise serializers.ValidationError( + '"timestamp" field must be a string number, representing milliseconds since epoch.' + ) + return value + + +class StatsigItemSerializer(serializers.Serializer): + data = _StatsigEventSerializer(many=True, required=True) + + +class StatsigProvider: + provider_name = "statsig" + + def __init__( + self, + organization_id: int, + signature: str | None, + request_timestamp: str | None, + version: str = "v0", + ) -> None: + self.organization_id = organization_id + self.signature = signature + self.request_timestamp = request_timestamp + self.version = version + + # Strip the signature's version prefix. For example, signature format for v0 is "v0+{hash}" + prefix_len = len(version) + 1 + if signature and len(signature) > prefix_len: + self.signature = signature[prefix_len:] + + def handle(self, message: dict[str, Any]) -> list[FlagAuditLogRow]: + serializer = StatsigItemSerializer(data=message) + if not serializer.is_valid(): + raise DeserializationError(serializer.errors) + + events = serializer.validated_data["data"] + audit_logs = [] + for event in events: + event_name = event["eventName"] + + if event_name not in SUPPORTED_STATSIG_EVENTS: + continue + + metadata = event.get("metadata") or {} + flag = metadata.get("name") + statsig_type = metadata.get("type") + action = (metadata.get("action") or "").lower() + + if not flag or statsig_type not in SUPPORTED_STATSIG_TYPES or action not in ACTION_MAP: + continue + + action = ACTION_MAP[action] + + # Prioritize email > id > name for created_by. + if created_by := get_path(event, "user", "email"): + created_by_type = CREATED_BY_TYPE_MAP["email"] + elif created_by := event.get("userID") or get_path(event, "user", "userID"): + created_by_type = CREATED_BY_TYPE_MAP["id"] + elif created_by := get_path(event, "user", "name"): + created_by_type = CREATED_BY_TYPE_MAP["name"] + else: + created_by, created_by_type = None, None + + created_at_ms = float(event["timestamp"]) + created_at = datetime.datetime.fromtimestamp(created_at_ms / 1000.0, datetime.UTC) + + tags = {} + if projectName := metadata.get("projectName"): + tags["projectName"] = projectName + if projectID := metadata.get("projectID"): + tags["projectID"] = projectID + if environments := metadata.get("environments"): + tags["environments"] = environments + + audit_logs.append( + FlagAuditLogRow( + action=action, + created_at=created_at, + created_by=created_by, + created_by_type=created_by_type, + flag=flag, + organization_id=self.organization_id, + tags=tags, + ) + ) + + return audit_logs + + def validate(self, message_bytes: bytes) -> bool: + if self.request_timestamp is None: + return False + + signature_basestring = f"{self.version}:{self.request_timestamp}:".encode() + message_bytes + + validator = PayloadSignatureValidator( + self.organization_id, self.provider_name, signature_basestring, self.signature + ) + return validator.validate() + + """Flagpole provider.""" @@ -389,10 +526,11 @@ def handle_flag_pole_event_internal(items: list[FlagAuditLogItem], organization_ class AuthTokenValidator: - """Abstract payload validator. + """Abstract validator for injecting dependencies in tests. Use this when a + provider does not support signing. - Similar to the SecretValidator class below, except we do not need - to validate the authorization string. + Similar to the PayloadSignatureValidator class below, except we do not + validate the authorization string with the payload. """ def __init__( @@ -419,7 +557,7 @@ def validate(self) -> bool: class PayloadSignatureValidator: - """Abstract payload validator. + """Abstract payload validator. Uses HMAC-SHA256 by default. Allows us to inject dependencies for differing use cases. Specifically the test suite. @@ -429,14 +567,14 @@ def __init__( self, organization_id: int, provider: str, - request_body: bytes, + payload: bytes, signature: str | None, secret_finder: Callable[[int, str], Iterator[str]] | None = None, secret_validator: Callable[[str, bytes], str] | None = None, ) -> None: self.organization_id = organization_id self.provider = provider - self.request_body = request_body + self.payload = payload self.signature = signature self.secret_finder = secret_finder or _query_signing_secrets self.secret_validator = secret_validator or hmac_sha256_hex_digest @@ -446,7 +584,7 @@ def validate(self) -> bool: return False for secret in self.secret_finder(self.organization_id, self.provider): - if self.secret_validator(secret, self.request_body) == self.signature: + if self.secret_validator(secret, self.payload) == self.signature: return True return False From df2ea4c5802ca2d094b33c729951487733c3a5e9 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 12 Feb 2025 20:26:53 -0800 Subject: [PATCH 02/10] Unit tests for handle --- tests/sentry/flags/providers/test_statsig.py | 336 +++++++++++++++++++ 1 file changed, 336 insertions(+) create mode 100644 tests/sentry/flags/providers/test_statsig.py diff --git a/tests/sentry/flags/providers/test_statsig.py b/tests/sentry/flags/providers/test_statsig.py new file mode 100644 index 00000000000000..19da5cf374180c --- /dev/null +++ b/tests/sentry/flags/providers/test_statsig.py @@ -0,0 +1,336 @@ +import datetime + +from sentry.flags.providers import StatsigProvider + + +def test_handle_batched_all_actions(): + org_id = 123 + logs = StatsigProvider(org_id, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "user": {"name": "johndoe", "email": "john@sentry.io"}, + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "projectName": "sentry", + "projectID": "1", + "type": "Gate", + "name": "gate1", + "description": "Updated Config Conditions\n - Added rule Rule 1", + "environments": "development,staging,production", + "action": "updated", + "tags": [], + "targetApps": [], + }, + }, + { + "user": {"email": "victor@ingolstadt.edu"}, + "timestamp": 17, + "eventName": "statsig::config_change", + "metadata": { + "projectName": "frankenstein", + "projectID": "1700", + "type": "Gate", + "name": "life", + "description": "Wretched", + "environments": "production", + "action": "created", + "tags": [], + "targetApps": [], + }, + }, + { + "user": {"name": "johndoe", "email": "john@sentry.io"}, + "timestamp": 1739400185233, + "eventName": "statsig::config_change", + "metadata": { + "projectName": "sentry", + "projectID": "1", + "type": "Gate", + "name": "gate1", + "environments": "development,staging", + "action": "deleted", + "tags": [], + "targetApps": [], + }, + }, + ] + } + ) + + assert len(logs) == 3 + + assert logs[0]["action"] == 2 + assert logs[0]["created_at"] == datetime.datetime( + 2025, 2, 12, 22, 43, 5, 198000, tzinfo=datetime.UTC + ) + assert logs[0]["created_by"] == "john@sentry.io" + assert logs[0]["created_by_type"] == 0 + assert logs[0]["flag"] == "gate1" + assert logs[0]["organization_id"] == org_id + assert logs[0]["tags"] == { + "projectName": "sentry", + "projectID": "1", + "environments": "development,staging,production", + } + + assert logs[1]["action"] == 0 + assert logs[1]["created_at"] == datetime.datetime( + 1970, 1, 1, 0, 0, 0, 17000, tzinfo=datetime.UTC + ) + assert logs[1]["created_by"] == "victor@ingolstadt.edu" + assert logs[1]["created_by_type"] == 0 + assert logs[1]["flag"] == "life" + assert logs[1]["organization_id"] == org_id + assert logs[1]["tags"] == { + "projectName": "frankenstein", + "projectID": "1700", + "environments": "production", + } + + assert logs[2]["action"] == 1 + assert logs[2]["created_at"] == datetime.datetime( + 2025, 2, 12, 22, 43, 5, 233000, tzinfo=datetime.UTC + ) + assert logs[2]["created_by"] == "john@sentry.io" + assert logs[2]["created_by_type"] == 0 + assert logs[2]["flag"] == "gate1" + assert logs[2]["organization_id"] == org_id + assert logs[2]["tags"] == { + "projectName": "sentry", + "projectID": "1", + "environments": "development,staging", + } + + +def test_handle_no_user(): + org_id = 123 + logs = StatsigProvider(org_id, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "updated", + }, + } + ] + } + ) + + assert len(logs) == 1 + assert logs[0]["action"] == 2 + assert logs[0]["flag"] == "gate1" + assert logs[0]["organization_id"] == org_id + assert logs[0]["created_by"] is None + assert logs[0]["created_by_type"] is None + + +def test_handle_no_project_or_environments(): + org_id = 123 + logs = StatsigProvider(org_id, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "updated", + }, + } + ] + } + ) + + assert len(logs) == 1 + assert logs[0]["action"] == 2 + assert logs[0]["flag"] == "gate1" + assert logs[0]["organization_id"] == org_id + + +def test_handle_additional_fields(): + org_id = 123 + logs = StatsigProvider(org_id, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "user": {"userID": "456"}, + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "updated", + "foo": "bar", + }, + # Additional fields (optional) + "value": "some value", + "statsigMetadata": {"something": 53}, + "timeUUID": "123e4567-e89b-12d3-a456-426614174000", + "unitID": "eureka", + } + ] + } + ) + + assert len(logs) == 1 + assert logs[0]["action"] == 2 + assert logs[0]["flag"] == "gate1" + assert logs[0]["organization_id"] == org_id + + +def test_handle_created_by_id(): + logs = StatsigProvider(123, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "user": {"userID": "456"}, + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "updated", + }, + } + ] + } + ) + assert len(logs) == 1 + assert logs[0]["created_by"] == "456" + assert logs[0]["created_by_type"] == 1 + + +def test_handle_created_by_id2(): + logs = StatsigProvider(123, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "userID": "456", + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "updated", + }, + } + ] + } + ) + assert len(logs) == 1 + assert logs[0]["created_by"] == "456" + assert logs[0]["created_by_type"] == 1 + + +def test_handle_created_by_name(): + logs = StatsigProvider(123, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "user": {"name": "johndoe"}, + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "updated", + }, + } + ] + } + ) + + assert len(logs) == 1 + assert logs[0]["created_by"] == "johndoe" + assert logs[0]["created_by_type"] == 2 + + +def test_handle_unsupported_events(): + logs = StatsigProvider(123, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "timestamp": 1739400185198, + "eventName": "statsig::gate_exposure", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "updated", + }, + }, + { + "timestamp": 1739400185199, + "eventName": "statsig::config_exposure", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "updated", + }, + }, + { + "timestamp": 1739400185200, + "eventName": "custom event", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "updated", + }, + }, + ] + } + ) + assert len(logs) == 0 + + +def test_handle_unsupported_config_changes(): + logs = StatsigProvider(123, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "type": "Experiment", + "name": "hello", + "action": "updated", + }, + }, + { + "timestamp": 1739400185199, + "eventName": "statsig::config_change", + "metadata": { + "type": "DynamicConfig", + "name": "world", + "action": "updated", + }, + }, + ] + } + ) + assert len(logs) == 0 + + +def test_handle_unsupported_action(): + logs = StatsigProvider(123, "abcdefgh", request_timestamp="1739400185400").handle( + { + "data": [ + { + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "type": "Gate", + "name": "gate1", + "action": "kickflip", + }, + }, + ] + } + ) + assert len(logs) == 0 From ae52ffde4d834ee9e451165cf29838dd7bc394a5 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 12 Feb 2025 21:08:32 -0800 Subject: [PATCH 03/10] e2e hook endpoint test --- src/sentry/flags/providers.py | 6 +-- tests/sentry/flags/endpoints/test_hooks.py | 50 ++++++++++++++++++++++ 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/src/sentry/flags/providers.py b/src/sentry/flags/providers.py index 6ba2d9842ee86d..9b78cb4f38b9a3 100644 --- a/src/sentry/flags/providers.py +++ b/src/sentry/flags/providers.py @@ -567,14 +567,14 @@ def __init__( self, organization_id: int, provider: str, - payload: bytes, + message: bytes, signature: str | None, secret_finder: Callable[[int, str], Iterator[str]] | None = None, secret_validator: Callable[[str, bytes], str] | None = None, ) -> None: self.organization_id = organization_id self.provider = provider - self.payload = payload + self.message = message self.signature = signature self.secret_finder = secret_finder or _query_signing_secrets self.secret_validator = secret_validator or hmac_sha256_hex_digest @@ -584,7 +584,7 @@ def validate(self) -> bool: return False for secret in self.secret_finder(self.organization_id, self.provider): - if self.secret_validator(secret, self.payload) == self.signature: + if self.secret_validator(secret, self.message) == self.signature: return True return False diff --git a/tests/sentry/flags/endpoints/test_hooks.py b/tests/sentry/flags/endpoints/test_hooks.py index 52f6dc5f7d7de9..9822ec9e6c611f 100644 --- a/tests/sentry/flags/endpoints/test_hooks.py +++ b/tests/sentry/flags/endpoints/test_hooks.py @@ -86,6 +86,56 @@ def test_unleash_post_create(self, mock_incr): ) assert FlagAuditLogModel.objects.count() == 1 + def test_statsig_post_create(self, mock_incr): + request_data = { + "data": [ + { + "user": {"name": "johndoe", "email": "john@sentry.io"}, + "timestamp": 1739400185198, + "eventName": "statsig::config_change", + "metadata": { + "projectName": "sentry", + "projectID": "1", + "type": "Gate", + "name": "gate1", + "description": "Updated Config Conditions\n - Added rule Rule 1", + "environments": "development,staging,production", + "action": "updated", + "tags": [], + "targetApps": [], + }, + }, + ] + } + + secret = "webhook-Xk9pL8NQaR5Ym2cx7vHnWtBj4M3f6qyZdC12mnspk8" + + FlagWebHookSigningSecretModel.objects.create( + organization=self.organization, + provider="statsig", + secret=secret, + ) + + request_timestamp = "1739400185400" # ms timestamp of the webhook request + signature_basestring = f"v0:{request_timestamp}:{json.dumps(request_data)}".encode() + signature = "v0=" + hmac_sha256_hex_digest(key=secret, message=signature_basestring) + headers = { + "X-Statsig-Signature": signature, + "X-Statsig-Request-Timestamp": request_timestamp, + } + + with self.feature(self.features): + response = self.client.post( + reverse(self.endpoint, args=(self.organization.slug, "statsig")), + request_data, + headers=headers, + ) + assert response.status_code == 200, response.content + mock_incr.assert_any_call( + "feature_flags.audit_log_event_posted", tags={"provider": "statsig"} + ) + assert FlagAuditLogModel.objects.count() == 1 + def test_launchdarkly_post_create(self, mock_incr): request_data = LD_REQUEST signature = hmac_sha256_hex_digest(key="456", message=json.dumps(request_data).encode()) From 0ac8f51bed0fbe887fda2ac17c01508209b8b56a Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 12 Feb 2025 21:16:38 -0800 Subject: [PATCH 04/10] Update secret validator and add secrets test --- src/sentry/flags/endpoints/secrets.py | 11 ++++++++++- tests/sentry/flags/endpoints/test_secrets.py | 17 ++++++++++++++++- 2 files changed, 26 insertions(+), 2 deletions(-) diff --git a/src/sentry/flags/endpoints/secrets.py b/src/sentry/flags/endpoints/secrets.py index 059587e15db45e..e08b7d5394f0e8 100644 --- a/src/sentry/flags/endpoints/secrets.py +++ b/src/sentry/flags/endpoints/secrets.py @@ -45,7 +45,16 @@ class FlagWebhookSigningSecretValidator(serializers.Serializer): provider = serializers.ChoiceField( choices=["launchdarkly", "generic", "unleash", "statsig"], required=True ) - secret = serializers.CharField(required=True, max_length=32, min_length=32) + secret = serializers.CharField(required=True, min_length=32, max_length=64) + + def validate_secret(self, value): + if self.initial_data.get("provider") == "statsig": + if not value.startswith("webhook-"): + raise serializers.ValidationError("Secret must be of the format webhook-") + elif len(value) > 32: + raise serializers.ValidationError("Ensure this field has no more than 32 characters.") + + return value @region_silo_endpoint diff --git a/tests/sentry/flags/endpoints/test_secrets.py b/tests/sentry/flags/endpoints/test_secrets.py index c0597fc04d5bb0..9bab4bd4778e89 100644 --- a/tests/sentry/flags/endpoints/test_secrets.py +++ b/tests/sentry/flags/endpoints/test_secrets.py @@ -89,11 +89,26 @@ def test_post_unleash(self): assert len(models) == 1 assert models[0].secret == "41271af8b9804cd99a4c787a28274991" + def test_post_statsig(self): + with self.feature(self.features): + response = self.client.post( + self.url, + data={ + "secret": "webhook-Xk9pL8NQaR5Ym2cx7vHnWtBj4M3f6qyZdC12mnspk8", + "provider": "statsig", + }, + ) + assert response.status_code == 201, response.content + + models = FlagWebHookSigningSecretModel.objects.filter(provider="statsig").all() + assert len(models) == 1 + assert models[0].secret == "webhook-Xk9pL8NQaR5Ym2cx7vHnWtBj4M3f6qyZdC12mnspk8" + def test_post_disabled(self): response = self.client.post(self.url, data={}) assert response.status_code == 404, response.content - def test_post_invalid(self): + def test_post_invalid_provider(self): with self.feature(self.features): url = reverse(self.endpoint, args=(self.organization.id,)) response = self.client.post(url, data={"secret": "123", "provider": "other"}) From e266340468f5fe1a12903c0fdcf53d4715d31cdd Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 12 Feb 2025 21:33:30 -0800 Subject: [PATCH 05/10] Rewrite validator and update invalid secret tests --- src/sentry/flags/endpoints/secrets.py | 11 ++--- tests/sentry/flags/endpoints/test_secrets.py | 44 ++++++++++++++++++++ 2 files changed, 50 insertions(+), 5 deletions(-) diff --git a/src/sentry/flags/endpoints/secrets.py b/src/sentry/flags/endpoints/secrets.py index e08b7d5394f0e8..11f2d511d39fcd 100644 --- a/src/sentry/flags/endpoints/secrets.py +++ b/src/sentry/flags/endpoints/secrets.py @@ -45,16 +45,17 @@ class FlagWebhookSigningSecretValidator(serializers.Serializer): provider = serializers.ChoiceField( choices=["launchdarkly", "generic", "unleash", "statsig"], required=True ) - secret = serializers.CharField(required=True, min_length=32, max_length=64) + secret = serializers.CharField(required=True) def validate_secret(self, value): if self.initial_data.get("provider") == "statsig": if not value.startswith("webhook-"): - raise serializers.ValidationError("Secret must be of the format webhook-") - elif len(value) > 32: - raise serializers.ValidationError("Ensure this field has no more than 32 characters.") + raise serializers.ValidationError( + "Ensure this field is of the format webhook-" + ) + return serializers.CharField(min_length=32, max_length=64).run_validation(value) - return value + return serializers.CharField(min_length=32, max_length=32).run_validation(value) @region_silo_endpoint diff --git a/tests/sentry/flags/endpoints/test_secrets.py b/tests/sentry/flags/endpoints/test_secrets.py index 9bab4bd4778e89..4b628e5417bfbf 100644 --- a/tests/sentry/flags/endpoints/test_secrets.py +++ b/tests/sentry/flags/endpoints/test_secrets.py @@ -116,6 +116,50 @@ def test_post_invalid_provider(self): assert response.json()["provider"] == ['"other" is not a valid choice.'] assert response.json()["secret"] == ["Ensure this field has at least 32 characters."] + def test_post_invalid_secret(self): + with self.feature(self.features): + for provider in ["launchdarkly", "generic", "unleash"]: + response = self.client.post( + self.url, data={"secret": "a" * 31, "provider": provider} + ) + assert response.status_code == 400, response.content + assert response.json()["secret"] == [ + "Ensure this field has at least 32 characters." + ], provider + + response = self.client.post( + self.url, data={"secret": "a" * 33, "provider": provider} + ) + assert response.status_code == 400, response.content + assert response.json()["secret"] == [ + "Ensure this field has no more than 32 characters." + ], provider + + # Statsig + response = self.client.post(self.url, data={"secret": "a" * 32, "provider": "statsig"}) + assert response.status_code == 400, response.content + assert response.json()["secret"] == [ + "Ensure this field is of the format webhook-" + ], "statsig" + + response = self.client.post( + self.url, + data={"secret": "webhook-" + "a" * (31 - len("webhook-")), "provider": "statsig"}, + ) + assert response.status_code == 400, response.content + assert response.json()["secret"] == [ + "Ensure this field has at least 32 characters." + ], "statsig" + + response = self.client.post( + self.url, + data={"secret": "webhook-" + "a" * (65 - len("webhook-")), "provider": "statsig"}, + ) + assert response.status_code == 400, response.content + assert response.json()["secret"] == [ + "Ensure this field has no more than 64 characters." + ], "statsig" + def test_post_empty_request(self): with self.feature(self.features): response = self.client.post(self.url, data={}) From 1de9ad577f09f36fb960c56f8e5faeb9369a0a87 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Wed, 12 Feb 2025 21:51:51 -0800 Subject: [PATCH 06/10] Use ListField and test empty data --- src/sentry/flags/providers.py | 2 +- tests/sentry/flags/providers/test_statsig.py | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/src/sentry/flags/providers.py b/src/sentry/flags/providers.py index 9b78cb4f38b9a3..5f147a06f58cd8 100644 --- a/src/sentry/flags/providers.py +++ b/src/sentry/flags/providers.py @@ -399,7 +399,7 @@ def validate_timestamp(self, value: str): class StatsigItemSerializer(serializers.Serializer): - data = _StatsigEventSerializer(many=True, required=True) + data = serializers.ListField(child=_StatsigEventSerializer(), required=True) class StatsigProvider: diff --git a/tests/sentry/flags/providers/test_statsig.py b/tests/sentry/flags/providers/test_statsig.py index 19da5cf374180c..5163880b193088 100644 --- a/tests/sentry/flags/providers/test_statsig.py +++ b/tests/sentry/flags/providers/test_statsig.py @@ -334,3 +334,8 @@ def test_handle_unsupported_action(): } ) assert len(logs) == 0 + + +def test_handle_empty_data(): + logs = StatsigProvider(123, "abcdefgh", request_timestamp="1739400185400").handle({"data": []}) + assert len(logs) == 0 From 869f4878ed08ed825abd267b04366770819c241b Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Thu, 13 Feb 2025 08:51:17 -0800 Subject: [PATCH 07/10] Type ignore --- src/sentry/flags/providers.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/sentry/flags/providers.py b/src/sentry/flags/providers.py index 5f147a06f58cd8..c95ea0edccf47c 100644 --- a/src/sentry/flags/providers.py +++ b/src/sentry/flags/providers.py @@ -399,7 +399,7 @@ def validate_timestamp(self, value: str): class StatsigItemSerializer(serializers.Serializer): - data = serializers.ListField(child=_StatsigEventSerializer(), required=True) + data = serializers.ListField(child=_StatsigEventSerializer(), required=True) # type: ignore[assignment] class StatsigProvider: From f846a7586dbc9143437be465e0b3a64131d9822f Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Fri, 14 Feb 2025 13:51:45 -0800 Subject: [PATCH 08/10] Review comments --- src/sentry/flags/providers.py | 39 ++++++++++++++--------------------- 1 file changed, 16 insertions(+), 23 deletions(-) diff --git a/src/sentry/flags/providers.py b/src/sentry/flags/providers.py index c95ea0edccf47c..896c0764e0a77a 100644 --- a/src/sentry/flags/providers.py +++ b/src/sentry/flags/providers.py @@ -366,17 +366,16 @@ def _handle_unleash_actions(action: str) -> int: SUPPORTED_STATSIG_EVENTS = {"statsig::config_change"} -# config_change is subclassed by the type of Statsig feature. There's "Gate", -# "Experiment", and more. Feature gates are boolean release flags, but all -# other types are unstructured JSON. To reduce noise, Gate is the only type -# we audit for now. +# Case-insensitive set. Config_change is subclassed by the type of Statsig +# feature. There's "Gate", "Experiment", and more. Feature gates are boolean +# release flags, but all other types are unstructured JSON. To reduce noise, +# Gate is the only type we audit for now. SUPPORTED_STATSIG_TYPES = { - "Gate", - "gate", # Supporting this just in case. Statsig docs and sample events use capitalization. + "gate", } -class _StatsigEventSerializer(serializers.Serializer): +class StatsigEventSerializer(serializers.Serializer): eventName = serializers.CharField(required=True) timestamp = serializers.CharField(required=True) # Custom serializer defined below. metadata = serializers.DictField(required=True) @@ -388,37 +387,27 @@ class _StatsigEventSerializer(serializers.Serializer): timeUUID = serializers.UUIDField(required=False) unitID = serializers.CharField(required=False) - def validate_timestamp(self, value: str): - try: - float(value) - except ValueError: - raise serializers.ValidationError( - '"timestamp" field must be a string number, representing milliseconds since epoch.' - ) - return value - class StatsigItemSerializer(serializers.Serializer): - data = serializers.ListField(child=_StatsigEventSerializer(), required=True) # type: ignore[assignment] + data = serializers.ListField(child=StatsigEventSerializer(), required=True) # type: ignore[assignment] class StatsigProvider: provider_name = "statsig" + version = "v0" def __init__( self, organization_id: int, - signature: str | None, - request_timestamp: str | None, - version: str = "v0", + signature: str | None = None, + request_timestamp: str | None = None, ) -> None: self.organization_id = organization_id self.signature = signature self.request_timestamp = request_timestamp - self.version = version # Strip the signature's version prefix. For example, signature format for v0 is "v0+{hash}" - prefix_len = len(version) + 1 + prefix_len = len(self.version) + 1 if signature and len(signature) > prefix_len: self.signature = signature[prefix_len:] @@ -440,7 +429,11 @@ def handle(self, message: dict[str, Any]) -> list[FlagAuditLogRow]: statsig_type = metadata.get("type") action = (metadata.get("action") or "").lower() - if not flag or statsig_type not in SUPPORTED_STATSIG_TYPES or action not in ACTION_MAP: + if ( + not flag + or statsig_type.lower() not in SUPPORTED_STATSIG_TYPES + or action not in ACTION_MAP + ): continue action = ACTION_MAP[action] From 16e6e5bb6eb960a24f2f290d1e7bf38043fec145 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Fri, 14 Feb 2025 17:34:13 -0800 Subject: [PATCH 09/10] Fix ProviderProtocol typing --- src/sentry/flags/providers.py | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/src/sentry/flags/providers.py b/src/sentry/flags/providers.py index 896c0764e0a77a..06107e732bf44e 100644 --- a/src/sentry/flags/providers.py +++ b/src/sentry/flags/providers.py @@ -55,7 +55,9 @@ class ProviderProtocol(Protocol[T]): provider_name: str signature: str | None - def __init__(self, organization_id: int, signature: str | None, **kwargs) -> None: ... + def __init__( + self, organization_id: int, signature: str | None, request_timestamp: str | None + ) -> None: ... def handle(self, message: T) -> list[FlagAuditLogRow]: ... def validate(self, message_bytes: bytes) -> bool: ... @@ -128,7 +130,9 @@ class LaunchDarklyItemSerializer(serializers.Serializer): class LaunchDarklyProvider: provider_name = "launchdarkly" - def __init__(self, organization_id: int, signature: str | None) -> None: + def __init__( + self, organization_id: int, signature: str | None, _request_timestamp: str | None = None + ) -> None: self.organization_id = organization_id self.signature = signature @@ -215,7 +219,9 @@ class GenericRequestSerializer(serializers.Serializer): class GenericProvider: provider_name = "generic" - def __init__(self, organization_id: int, signature: str | None) -> None: + def __init__( + self, organization_id: int, signature: str | None, _request_timestamp: str | None = None + ) -> None: self.organization_id = organization_id self.signature = signature @@ -307,7 +313,9 @@ def _get_user(validated_event: dict[str, Any]) -> tuple[str, int]: class UnleashProvider: provider_name = "unleash" - def __init__(self, organization_id: int, signature: str | None) -> None: + def __init__( + self, organization_id: int, signature: str | None, _request_timestamp: str | None = None + ) -> None: self.organization_id = organization_id self.signature = signature @@ -377,7 +385,7 @@ def _handle_unleash_actions(action: str) -> int: class StatsigEventSerializer(serializers.Serializer): eventName = serializers.CharField(required=True) - timestamp = serializers.CharField(required=True) # Custom serializer defined below. + timestamp = serializers.CharField(required=True) metadata = serializers.DictField(required=True) user = serializers.DictField(required=False, child=serializers.CharField()) @@ -399,8 +407,8 @@ class StatsigProvider: def __init__( self, organization_id: int, - signature: str | None = None, - request_timestamp: str | None = None, + signature: str | None, + request_timestamp: str | None, ) -> None: self.organization_id = organization_id self.signature = signature From a3c146e7a1627da337e39b3ddd8ad4ed7b3a3544 Mon Sep 17 00:00:00 2001 From: Andrew Liu <159852527+aliu39@users.noreply.github.com> Date: Fri, 14 Feb 2025 17:39:42 -0800 Subject: [PATCH 10/10] Fix statsig_type typing --- src/sentry/flags/providers.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/sentry/flags/providers.py b/src/sentry/flags/providers.py index 06107e732bf44e..40349afe50ee2b 100644 --- a/src/sentry/flags/providers.py +++ b/src/sentry/flags/providers.py @@ -439,6 +439,7 @@ def handle(self, message: dict[str, Any]) -> list[FlagAuditLogRow]: if ( not flag + or not statsig_type or statsig_type.lower() not in SUPPORTED_STATSIG_TYPES or action not in ACTION_MAP ):