feat(flags): enforce filters validation one rule at a time

posthog/settings/feature_flags.py

@@ -4,7 +4,7 @@
 
 from posthog.settings.access import SECRET_KEY
 from posthog.settings.base_variables import TEST
-from posthog.settings.utils import get_from_env, get_list, str_to_bool
+from posthog.settings.utils import get_from_env, get_list, get_set
 
 # Used mostly by the hobby install to have some feature flags enabled by default
 # NOTE: This only affects the frontend, the same FFs will still be considered disabled on the backend
@@ -125,19 +125,16 @@
     type_cast=int,  # 512KB
 )
 
-# Staged rollout switch for feature flag filters validation (#50084). When off (the
-# production default for now), the new structural and cross-field filter tiers on the flag
-# API log violations (`feature_flag_filters_enforcement_bypassed`) instead of rejecting
-# them. Not gated by this switch: the pre-enforcement type/bounds checks
-# (_reject_serde_unsafe_filters — the cache-poisoning class) and contextual checks (cohort
-# existence, circular dependencies, size limits) always reject, so off means exactly the
-# protection that predated enforcement, plus observation logging. Rollout: ship with the
-# default off, compare the bypass-log volume against the audit's per-rule predictions for
-# a few days of live traffic (the audit measured stored data, never request-shaped input),
-# then enable via env var. The follow-up flip PR removes this switch and the now-redundant
-# _reject_serde_unsafe_filters. Defaults on under TEST so the suite validates the enforced
-# behavior.
-FEATURE_FLAG_FILTERS_ENFORCEMENT: bool = get_from_env("FEATURE_FLAG_FILTERS_ENFORCEMENT", TEST, type_cast=str_to_bool)
+# Staged rollout for feature flag filters validation (#50084). Rule ids to reject on, comma
+# separated, matching the ids in the violation metrics and in the `code` of each error this
+# returns (e.g. "cross_field.variant_rollout_sum_not_100").
+# "*" enforces every rule. Unset is the production default and rejects nothing new: both
+# tiers still log and count violations, and the checks that predate #50084 still reject.
+# Rules go in one at a time, each once its violation counter has stayed at zero — see the
+# per-rule rollout in the issue. Defaults to "*" under TEST so the suite covers enforcement.
+FEATURE_FLAG_FILTERS_ENFORCED_RULES: set[str] = get_set(
+    get_from_env("FEATURE_FLAG_FILTERS_ENFORCED_RULES", "*" if TEST else "")
+)
 
 # Team ID for the local-evaluation canary. Unset disables the canary task.
 FEATURE_FLAGS_CANARY_TEAM_ID: int | None = get_from_env("FEATURE_FLAGS_CANARY_TEAM_ID", optional=True, type_cast=int)

products/feature_flags/backend/api/feature_flag.py

@@ -236,7 +236,7 @@
 ENCRYPTED_VERSION_HISTORY_UNAVAILABLE = "Version history is not available for flags with encrypted payloads."
 
 
 def parse_created_by_ids(value: Any) -> list[int]:
     """Parse a `created_by_id` filter value into a list of user IDs.
 
     Accepts a single int/str, a comma-separated string, or a JSON-encoded list
@@ -867,16 +867,35 @@
 _RUST_PROPERTY_TYPES: frozenset[str] = frozenset({*FEATURE_FLAG_PROPERTY_TYPES, "person_metadata"})
 
 
+def _filters_rule_is_enforced(rule_id: str | None) -> bool:
+    """Whether a violation of `rule_id` rejects the write, per the #50084 staged rollout.
+
+    DRF types an error's code as optional, so a violation can arrive without one. It matches
+    no rule id, which leaves it rejecting only once every rule is enforced.
+    """
+    enforced = settings.FEATURE_FLAG_FILTERS_ENFORCED_RULES
+    return "*" in enforced or rule_id in enforced
+
+
+def _all_filters_rules_are_enforced() -> bool:
+    """Whether the rollout has reached every rule.
+
+    Two behaviors stay all-or-nothing because they are not attached to any single rule:
+    the serde-fidelity guard below, and preserving unknown keys through normalization.
+    Both hold their pre-rollout behavior until the last rule goes in.
+    """
+    return "*" in settings.FEATURE_FLAG_FILTERS_ENFORCED_RULES
+
+
 def _reject_serde_unsafe_filters(filters: Any) -> None:
-    """Unconditional serde-fidelity guard on incoming filters, active regardless of the
-    FEATURE_FLAG_FILTERS_ENFORCEMENT switch.
+    """Serde-fidelity guard on incoming filters, independent of which rules are enforced.
 
     These are the pre-#50084 procedural type/bounds checks: values that fail serde in the
-    Rust flag service and poison the team's flag cache. While the enforcement switch is off,
-    the structural tier only logs, so this keeps the poisoning class rejected exactly as it
-    was before enforcement shipped. Only called while the switch is off: with it on, the
-    structural tier covers all of this with better per-rule error codes. DELETE together with
-    the switch in the follow-up PR that defaults enforcement on.
+    Rust flag service and poison the team's flag cache. A rule that is not yet enforced only
+    logs, so this keeps the poisoning class rejected exactly as it was before the rollout
+    started. Only called until every rule is enforced: after that the structural tier covers
+    all of this with better per-rule error codes. DELETE together with the rollout setting
+    once every rule is in.
     """
     if not isinstance(filters, dict):
         raise serializers.ValidationError(f"Filters must be a dictionary, got {type(filters).__name__}")
@@ -1056,8 +1075,8 @@
 def _iter_flag_filter_properties(groups: Any) -> Iterator[_FlagFilterProperty]:
     """Yield each well-formed property filter found under `groups`.
 
-    Malformed entries are skipped instead of crashing: they can only occur while the #50084
-    enforcement kill switch is off, and log-only mode must never 500.
+    Malformed entries are skipped instead of crashing: they can only occur while the rule
+    that would reject them is still rolling out (#50084), and logging must never 500.
     """
     if not isinstance(groups, list):
         return
@@ -1573,7 +1592,7 @@
             )
             raise
 
     def _validate_filters_inner(self, filters, operation: str):
         # An empty filters dict on an update carries no instruction, so the merged state is
         # the stored state and there is nothing to validate. Returning it untouched also
         # keeps normalization off flags the request never addressed: DRF hands DictField an
@@ -1586,13 +1605,13 @@
 
         # `filters` arrives as the raw request dict, so the structural tier runs once below,
         # on the merged state.
-        enforcement: bool = settings.FEATURE_FLAG_FILTERS_ENFORCEMENT
+        fully_enforced = _all_filters_rules_are_enforced()
 
-        # Log-only mode must never accept what the pre-enforcement validator rejected, so the
-        # cache-poisoning class stays rejected while the switch is off. With the switch on the
-        # structural tier covers every one of these rules and reports them with per-rule codes
-        # instead of this guard's flat messages, so it would only shadow the better errors.
-        if not enforcement:
+        # A rule that only logs must never accept what the pre-enforcement validator rejected,
+        # so the cache-poisoning class stays rejected throughout the rollout. Once every rule
+        # is enforced the structural tier covers all of these and reports them with per-rule
+        # codes instead of this guard's flat messages, so it would only shadow better errors.
+        if not fully_enforced:
             try:
                 _reject_serde_unsafe_filters(filters)
             except serializers.ValidationError:
@@ -1638,7 +1657,7 @@
             data=merged,
             context={
                 FLAG_ID_CONTEXT_KEY: getattr(self.instance, "id", None),
-                PRESERVE_UNKNOWN_KEYS_CONTEXT_KEY: not enforcement,
+                PRESERVE_UNKNOWN_KEYS_CONTEXT_KEY: not fully_enforced,
             },
         )
         structurally_valid = structural.is_valid()
@@ -1657,9 +1676,12 @@
             # about are mostly cross-field violators whose merged state fails structurally, so a
             # bare zero on the cross_field series would read as "none left to fix".
             _count_filters_violations("cross_field", operation, ["not_evaluated"])
-            if enforcement:
+            # Only the enforced rules reject, and only they are reported: a rule still rolling
+            # out must not turn into a 400 by sharing a request with one that is enforced.
+            enforced_details = [detail for detail in details if _filters_rule_is_enforced(detail.code)]
+            if enforced_details:
                 _mark_filters_enforced_rejection(self)
-                raise serializers.ValidationError(details)
+                raise serializers.ValidationError(enforced_details)
             _mark_filters_bypassed(self)
             filters_enforcement_logger.warning(
                 "feature_flag_filters_enforcement_bypassed",
@@ -1771,12 +1793,15 @@
                 _count_filters_violations(
                     "cross_field", operation, [violation.rule_id for violation in cross_field_violations]
                 )
-                if enforcement:
+                enforced_violations = [
+                    violation for violation in cross_field_violations if _filters_rule_is_enforced(violation.rule_id)
+                ]
+                if enforced_violations:
                     _mark_filters_enforced_rejection(self)
                     raise serializers.ValidationError(
                         [
                             ErrorDetail(f"{violation.path}: {violation.message}", code=violation.rule_id)
-                            for violation in cross_field_violations
+                            for violation in enforced_violations
                         ]
                     )
                 _mark_filters_bypassed(self)
@@ -2101,7 +2126,7 @@
         return instance
 
     @approval_gate(["feature_flag.enable", "feature_flag.disable", "feature_flag.update"])
     def update(self, instance: FeatureFlag, validated_data: dict, *args: Any, **kwargs: Any) -> FeatureFlag:
         request = self.context["request"]
         # Service-layer callers may carry no body, and an endpoint that declares no request schema
         # can hand us a non-dict one (e.g. a JSON array posted to /experiments/:id/launch/).
@@ -2418,7 +2443,7 @@
         flag_status, _ = checker.get_status()
         return flag_status.name
 
     def to_representation(self, instance):
         representation = super().to_representation(instance)
         filters = representation.get("filters", {})
 

products/feature_flags/backend/api/test/test_feature_flag.py

@@ -8855,7 +8855,91 @@ def test_stored_violating_flag_blocks_filters_edits_only(self):
         self.assertEqual(filters_edit.status_code, status.HTTP_400_BAD_REQUEST)
         self.assertEqual(filters_edit.json()["attr"], "filters")
 
-    @override_settings(FEATURE_FLAG_FILTERS_ENFORCEMENT=False)
+    # Two cross-field violations that reach the same decision point, so a case can enforce one
+    # and leave the other rolling out. Both are structurally valid; a structural failure would
+    # short-circuit before the cross-field tier runs.
+    _SUM_RULE = "cross_field.variant_rollout_sum_not_100"
+    _VARIANT_RULE = "cross_field.group_variant_not_a_variant"
+    _VIOLATES_SUM: dict = {
+        "groups": [{"properties": [], "rollout_percentage": 100}],
+        "multivariate": {"variants": [{"key": "a", "rollout_percentage": 30}, {"key": "b", "rollout_percentage": 30}]},
+    }
+    _VIOLATES_VARIANT: dict = {
+        "groups": [{"properties": [], "rollout_percentage": 100, "variant": "ghost"}],
+        "multivariate": {"variants": [{"key": "a", "rollout_percentage": 50}, {"key": "b", "rollout_percentage": 50}]},
+    }
+
+    @parameterized.expand(
+        [
+            ("enforced rule rejects", {_SUM_RULE}, _VIOLATES_SUM, status.HTTP_400_BAD_REQUEST),
+            ("rule still rolling out is accepted", {_SUM_RULE}, _VIOLATES_VARIANT, status.HTTP_201_CREATED),
+            ("unset enforces nothing", set(), _VIOLATES_SUM, status.HTTP_201_CREATED),
+            ("wildcard enforces every rule", {"*"}, _VIOLATES_VARIANT, status.HTTP_400_BAD_REQUEST),
+        ]
+    )
+    def test_only_the_rules_in_the_setting_reject(
+        self, name: str, enforced_rules: set[str], filters: dict, expected_status: int
+    ) -> None:
+        with override_settings(FEATURE_FLAG_FILTERS_ENFORCED_RULES=enforced_rules):
+            response = self.client.post(
+                f"/api/projects/{self.team.id}/feature_flags/",
+                {"key": f"rollout-{abs(hash(name))}", "filters": filters},
+                format="json",
+            )
+
+        self.assertEqual(response.status_code, expected_status, response.json())
+
+    @override_settings(FEATURE_FLAG_FILTERS_ENFORCED_RULES={_SUM_RULE})
+    def test_a_rule_still_rolling_out_does_not_reject_by_sharing_a_request(self) -> None:
+        # Both rules are violated but only one is enforced. The write is rejected for that one
+        # alone, so turning on a rule cannot make a second rule's message reach a customer.
+        response = self.client.post(
+            f"/api/projects/{self.team.id}/feature_flags/",
+            {
+                "key": "mixed-violations",
+                "filters": {
+                    "groups": [{"properties": [], "rollout_percentage": 100, "variant": "ghost"}],
+                    "multivariate": {
+                        "variants": [{"key": "a", "rollout_percentage": 30}, {"key": "b", "rollout_percentage": 30}]
+                    },
+                },
+            },
+            format="json",
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        body = response.json()
+        self.assertEqual(body["code"], self._SUM_RULE)
+        self.assertNotIn("ghost", body["detail"])
+
+    @override_settings(FEATURE_FLAG_FILTERS_ENFORCED_RULES={"structural.multivariate.variants[].key.max_length"})
+    def test_structural_tier_also_rejects_only_the_enforced_rule(self) -> None:
+        # The structural tier decides separately from the cross-field one, so it needs its own
+        # case. Both violations are on variant keys, which the serde guard only type-checks, so
+        # they reach the structural tier instead of being rejected before it.
+        response = self.client.post(
+            f"/api/projects/{self.team.id}/feature_flags/",
+            {
+                "key": "mixed-structural-violations",
+                "filters": {
+                    "groups": [{"properties": [], "rollout_percentage": 100}],
+                    "multivariate": {
+                        "variants": [
+                            {"key": "", "rollout_percentage": 50},
+                            {"key": "x" * 401, "rollout_percentage": 50},
+                        ]
+                    },
+                },
+            },
+            format="json",
+        )
+
+        self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST)
+        body = response.json()
+        self.assertEqual(body["code"], "structural.multivariate.variants[].key.max_length")
+        self.assertNotIn("blank", body["detail"])
+
+    @override_settings(FEATURE_FLAG_FILTERS_ENFORCED_RULES=set())
     def test_kill_switch_logs_new_rules_but_still_rejects_serde_unsafe_input(self):
         # An empty variant key is structurally invalid but still deserializes as a Rust String,
         # so log-only mode records it without widening the cache-poisoning surface.
@@ -14469,7 +14553,7 @@ def test_write_outcomes_and_violations_are_counted(self) -> None:
             violation_before + 1,
         )
 
-    @override_settings(FEATURE_FLAG_FILTERS_ENFORCEMENT=False)
+    @override_settings(FEATURE_FLAG_FILTERS_ENFORCED_RULES=set())
     def test_bypassed_write_is_not_also_counted_as_accepted(self) -> None:
         accepted_before = self._write_count("create", "accepted")
         bypassed_before = self._write_count("create", "bypassed")
@@ -14543,7 +14627,7 @@ def test_preexisting_rejections_are_counted_apart_from_enforcement(self) -> None
         self.assertEqual(self._write_count("create", "rejected_preexisting"), preexisting_before + 1)
         self.assertEqual(self._write_count("create", "rejected"), rejected_before)
 
-    @override_settings(FEATURE_FLAG_FILTERS_ENFORCEMENT=False)
+    @override_settings(FEATURE_FLAG_FILTERS_ENFORCED_RULES=set())
     def test_cross_field_bypass_is_logged_counted_and_persisted(self) -> None:
         # The branch production runs while the switch is off: structurally valid input with a
         # cross-field violation. It marks the write bypassed, counts the rule, and saves anyway.
@@ -14569,7 +14653,7 @@ def test_cross_field_bypass_is_logged_counted_and_persisted(self) -> None:
             rule_before + 1,
         )
 
-    @override_settings(FEATURE_FLAG_FILTERS_ENFORCEMENT=False)
+    @override_settings(FEATURE_FLAG_FILTERS_ENFORCED_RULES=set())
     def test_structural_failure_records_cross_field_as_not_evaluated(self) -> None:
         # Cross-field checks need structurally valid input, so they never run for these writes.
         # The dashboard has to see that, or a bare zero reads as "nothing left to fix".