fix(filters): Don't match on an invalid filter condition; if such a condition exists, fail the match
Problem
A customer had an issue where we created an invalid filter condition on the UI (we created a cohort that should not have matched anyone), but, instead of failing to evaluate that filter, we evaluated it to True, and unintentionally showed a feature flag to dozens of users (context).
The issue, when it came down to it, was property_to_q ended up generating a condition like this
(AND: (
(AND: (
('properties__registration_ts__lte', ['1716447600']),
('properties__has_key', 'registration_ts'),
(NOT (AND: ('properties__registration_ts', None)))
))
))
Which turned into a SQL statement like this (paraphrasing a bit)
SELECT * FROM table WHERE '1716447600' <= '["1716447600"]';
Which, to my surprise (I was expecting ProgrammingError), returns true, because postgres has a concept of ordering JSONB that goes like null < boolean < number < string < array < object, and since my first value is a number, it's always "less" than an array.
Annoying.
So the problem here is that the UI can occasionally pass us filter conditions that are invalid, and instead of dealing with these invalid comparison correctly, we were using them to evaluate Feature Flag conditions.
Further context: https://posthog.slack.com/archives/C034XD440RK/p1719258942611049
Fixes: https://github.com/PostHog/posthog/issues/23213
Changes
I elected to make it so that if we ever encounter an invalid state for these feature flag comparisons (i.e., if we ever try and apply any of the following operators (<, >, <=, >=) to an array), we just nullify that condition altogether. This means that if a cohort is created with any invalid conditions and the cohort needs to match ALL of them, we evaluate the flag as false. But, if it just needs to match a subset, and there's only one broken condition, we'll evaluate it as true.
As with most scary changes, there's about 11 lines of source code change, and several hundred lines of tests.
Does this work well for both Cloud and self-hosted?
Yes
How did you test this code?
Added several hundred lines of new tests.
Size Change: 0 B
Total Size: 1.06 MB
ℹ️ View Unchanged
| Filename | Size |
|---|---|
frontend/dist/toolbar.js |
1.06 MB |