From 22956c0888b5aadb63720060e9f9b4ab4b1cfea6 Mon Sep 17 00:00:00 2001 From: Patricio Date: Fri, 4 Sep 2026 10:36:27 -0300 Subject: [PATCH 1/2] feat(feature-flags): enforce filters validation one rule at a time Replaces the all-or-nothing enforcement switch with a set of rule ids to reject on. A rule that is not listed keeps logging, including when another rule in the same request rejects, so a rule can go live once its own violations reach zero. Unset matches the old off and "*" matches the old on. --- posthog/settings/feature_flags.py | 25 ++++--- .../feature_flags/backend/api/feature_flag.py | 65 +++++++++++++------ .../backend/api/test/test_feature_flag.py | 63 ++++++++++++++++-- 3 files changed, 115 insertions(+), 38 deletions(-) diff --git a/posthog/settings/feature_flags.py b/posthog/settings/feature_flags.py index f62ea2165a9d..70ebc562be7f 100644 --- a/posthog/settings/feature_flags.py +++ b/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) diff --git a/products/feature_flags/backend/api/feature_flag.py b/products/feature_flags/backend/api/feature_flag.py index 7ccce54ad9c5..1ae6f7c51d41 100644 --- a/products/feature_flags/backend/api/feature_flag.py +++ b/products/feature_flags/backend/api/feature_flag.py @@ -867,16 +867,35 @@ def to_representation(self, obj): _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 path(self) -> str: 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 @@ -1586,13 +1605,13 @@ def _validate_filters_inner(self, filters, operation: str): # `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 @@ def _validate_filters_inner(self, filters, operation: str): 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 @@ def _validate_filters_inner(self, filters, operation: str): # 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 @@ def _validate_filters_inner(self, filters, operation: str): _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) diff --git a/products/feature_flags/backend/api/test/test_feature_flag.py b/products/feature_flags/backend/api/test/test_feature_flag.py index 65e73b8a329e..dabecec4c25d 100644 --- a/products/feature_flags/backend/api/test/test_feature_flag.py +++ b/products/feature_flags/backend/api/test/test_feature_flag.py @@ -8855,7 +8855,62 @@ 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, enforced_rules, filters, expected_status): + 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): + # 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=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 +14524,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 +14598,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 +14624,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". From d87cf4bb4b3f025213f57d4b33c2f1f3899eacf0 Mon Sep 17 00:00:00 2001 From: Patricio Date: Fri, 4 Sep 2026 12:59:04 -0300 Subject: [PATCH 2/2] chore(feature-flags): cover the structural tier's own rule filter The structural tier decides separately from the cross-field one, and only the latter was covered. Uses two variant-key violations, which the serde guard type checks but does not reject, so they reach the structural tier. --- .../backend/api/test/test_feature_flag.py | 33 +++++++++++++++++-- 1 file changed, 31 insertions(+), 2 deletions(-) diff --git a/products/feature_flags/backend/api/test/test_feature_flag.py b/products/feature_flags/backend/api/test/test_feature_flag.py index dabecec4c25d..fbe1fd62a91f 100644 --- a/products/feature_flags/backend/api/test/test_feature_flag.py +++ b/products/feature_flags/backend/api/test/test_feature_flag.py @@ -8877,7 +8877,9 @@ def test_stored_violating_flag_blocks_filters_edits_only(self): ("wildcard enforces every rule", {"*"}, _VIOLATES_VARIANT, status.HTTP_400_BAD_REQUEST), ] ) - def test_only_the_rules_in_the_setting_reject(self, name, enforced_rules, filters, expected_status): + 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/", @@ -8888,7 +8890,7 @@ def test_only_the_rules_in_the_setting_reject(self, name, enforced_rules, filter 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): + 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( @@ -8910,6 +8912,33 @@ def test_a_rule_still_rolling_out_does_not_reject_by_sharing_a_request(self): 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,