posthog icon indicating copy to clipboard operation
posthog copied to clipboard

fix(filters): Don't match on an invalid filter condition; if such a condition exists, fail the match

Open dmarticus opened this issue 1 year ago • 1 comments

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.

dmarticus avatar Jun 27 '24 20:06 dmarticus

Size Change: 0 B

Total Size: 1.06 MB

ℹ️ View Unchanged
Filename Size
frontend/dist/toolbar.js 1.06 MB

compressed-size-action

github-actions[bot] avatar Jun 28 '24 20:06 github-actions[bot]