Page MenuHomePhabricator

Some abuse filters stopped matching after empty data introduction
Open, HighPublic

Description

Summary for people coming here from Tech News

Recently, there have been some changes to the AbuseFilter parser (T214674). This changed the way variables are handled. The most important changes are:

  1. Only variables for the current action are available. Which means, during an edit you cannot use e.g. moved_from_namespace or accountname, during an account creation you cannot check added_lines, and so on. I recommend to check the action variable when using action-specific variables; in the future, we'll introduce a new function to check whether a variable is set.
  2. Custom variables defined inside a branch may not be available outside of the branch. For instance, if you have: page_namespace == 0 & (my_var := 'foo'); added_lines contains my_var, the variable my_var is used outside of the branch (= pair of parentheses) where it's declared. Trying to do so could produce unexpected results in certain cases. [More precisely: the assignment will be skipped in short-circuit evaluation, which in this case happens when page_namespace is non-zero.] Note: This is only known to affect arwiki's filter 130 and wikidata filter 34.

This could affect existing abuse filters, making them not match some actions. If you see that a filter has not been matching anything for a long time, while it used to match constantly, please check that it's not affected by this bug. In the vast majority of the cases, checking point 1. above is enough.

In the future, we may be able to provide a list of affected filters. If you're interested in such a list, please Subscribe to this task.

Details

Related Gerrit Patches:
mediawiki/extensions/AbuseFilter : masterAdd a new 'is_set' function
mediawiki/extensions/AbuseFilter : masterReturn a DNULL in VariableHolder::getVar with GET_STRICT
mediawiki/extensions/AbuseFilter : masterBetter logging for unset variables
mediawiki/extensions/AbuseFilter : masterAdd a 'strict' option to VariableHolder::getVar

Event Timeline

Daimona created this task.Aug 10 2019, 9:30 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 10 2019, 9:30 AM
Daimona triaged this task as High priority.Aug 10 2019, 11:54 AM

And what's even more weird, I cannot reproduce the problem locally. Sure, I had to change the affected filters a little bit in order to make them match, but yet, they match my edits correctly. I'll investigate further.

OK, so, additionally, we have some weird caching-related problem or the like. After having unsuccessfully tried testing locally and on itwiki itself, I came up with the following test: make the broken filter match my edit with a different IP address, copy its content to a brand new filter, and edit a page. Surprisingly, despite having the very same content, the new filter matched, while the old one did not.
So I suspect we have either caching problems (e.g. with tokenization or the like), or something like that. Note that editing the broken filter is not enough to "refresh something" or make it start working again.

OK, so, additionally, we have some weird caching-related problem or the like. After having unsuccessfully tried testing locally and on itwiki itself, I came up with the following test: make the broken filter match my edit with a different IP address, copy its content to a brand new filter, and edit a page. Surprisingly, despite having the very same content, the new filter matched, while the old one did not.
So I suspect we have either caching problems (e.g. with tokenization or the like), or something like that. Note that editing the broken filter is not enough to "refresh something" or make it start working again.

Oh my goodness... I forgot to enable the "broken" filter after fixing it... So the actual explanation for that problem is me being stoopid, no more and no less.

Note that the problem in the task description is still a thing, and I still don't know what the affected filters are. Instead, the part saying that you should go to Special:AbuseFilter/test to verify the bug could be wrong. I learned that in T228526, and it was the same for the itwiki filter, but for the latter it's just because the filter was disabled, and /test only checks the pattern regardless of whether the filter is enabled.

Change 529598 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add a 'strict' option to VariableHolder::getVar

https://gerrit.wikimedia.org/r/529598

Change 529601 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Add a new 'is_set' function

https://gerrit.wikimedia.org/r/529601

Johan added a subscriber: Johan.Aug 15 2019, 9:57 AM

@Daimona So I tried to write a summary in Tech News, but there's enough information that I think people actually need to read the ticket. But something like this?

"Some abusefilters stopped working because of a change. Only variables for the current action will work. Variables defined inside a branch may not work outside of that branch. You can [[phab:T230256|read more]].

@Daimona So I tried to write a summary in Tech News, but there's enough information that I think people actually need to read the ticket.

Thanks! Unfortunately yes, there's too much information to be able to summarize it in the announcement only.

"Some abusefilters stopped working because of a change. Only variables for the current action will work. Variables defined inside a branch may not work outside of that branch. You can [[phab:T230256|read more]].

Looks good. Maybe specify "stopped working because of a backend change", so that maybe it's clearer that it's not an on-wiki change made by some user? And then I could clean up the task description to provide a short summary for people who click the link.

Daimona updated the task description. (Show Details)Aug 15 2019, 11:08 AM
Jar added a subscriber: Jar.Aug 19 2019, 5:26 PM
Restricted Application added a subscriber: alanajjar. · View Herald TranscriptAug 19 2019, 5:26 PM
Udo_T added a subscriber: Udo_T.Aug 19 2019, 9:23 PM

A list would truly be useful.

A list would truly be useful.

Yes, I know, but at the moment there's no way to do that. I'll add some logging to the parser in order to do that.

Ah, right, I already sent T230256#5408092 to get a list and T230256#5408110 as helper. Note that the only filter known to be affected because of custom variables used out of scope is arwiki's 130, for which I'm adding the parent task.

Daimona updated the task description. (Show Details)Aug 22 2019, 9:18 AM
MusikAnimal added a comment.EditedAug 27 2019, 4:00 AM

@Daimona I see now that user_rights for instance is no longer available for the createaccount action. This makes sense, based on your description, but the issue is we now have to handle createaccount and autocreateaccount separately, leading to much more complicated filters with more conditions.

Before:

action contains "createaccount" &
!('override-antispoof' in user_rights) &
accountname contains "Foobar"

After:

(
    action == "createaccount" |
    (
        action == "autocreateaccount" &
        !('override-antispoof' in user_rights)
    )
) &
accountname contains "Foobar"

Having to use a new function to check for the existence of a variable seems less ideal, too. Couldn't we make it so that the parser just returns a blank or null value when an unavailable variable is encountered? So in the above case, user_rights is blank, so !('override-antispoof' in user_rights) returns true and the filter doesn't break.

On enwiki, nearly all of the account creation-related filters appear to have broken.

@Daimona I see now that user_rights for instance is no longer available for the createaccount action.

Yes, this is correct.

This makes sense, based on your description, but the issue is we now have to handle createaccount and autocreateaccount separately, leading to much more complicated filters with more conditions.

Unfortunately, this is also true in some cases, i.e. when the filter is also using variables that aren't guaranteed to be set (e.g. user_groups).

Having to use a new function to check for the existence of a variable seems less ideal, too.

That's not really supposed to make things easier, it's just for cases where checking the action is not practical: for instance, instead of

( action === 'createaccount' | action === 'autocreateaccount' ) & accountname === 'xxx'

(yes, this could be written as action contains 'createaccount', but that wasn't intended, so let's pretend it doesn't work)

you could write

is_set( 'accountname' ) & accountname === 'xxx'

Couldn't we make it so that the parser just returns a blank or null value when an unavailable variable is encountered?

It used to return NULL. The problem with returning null is that, while filters could be simplified, they could behave unexpectedly. For instance (something similar has happened to me several times), suppose that you want to catch a serial page blanker. You may write something as

!"confirmed" in user_groups &
edit_delta < -5000

with throttle by page at a given rate, and enable the block. That could seem perfect. But it would start blocking *all* account creations, because you'd have

!"confirmed" in "" &
null < -5000 /* Yes, this is true, and it's the same in PHP */

the page being always Special:CreateAccount. That could be catastrophic if no-one notices it on time.
And I'm sure there are other cases of ambiguity. So basically, it's a trade-off between verbosity and unexpected behaviours.

On enwiki, nearly all of the account creation-related filters appear to have broken.

Sad to hear that. We currently don't have a list of involved filters. https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/529598/ would provide one, and then we could also understand how many filters are affected, and what to do. Which could be either keep returning null for back-compat until all filters are fixed, or even take a step back and start returning null again for unset variables. Would you mind reviewing the patch, please? It's not very complicated. Although I'm unsure whether we're on time to get it included in this week's train.
As a side note, I'd be happy to help in fixing broken filters, but this is yet another case where the new global group would be super-helpful.

Change 529598 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Add a 'strict' option to VariableHolder::getVar

https://gerrit.wikimedia.org/r/529598

Change 536166 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Better logging for unset variables

https://gerrit.wikimedia.org/r/536166

Change 536166 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Better logging for unset variables

https://gerrit.wikimedia.org/r/536166

OK, so, there are lots of affected filters. Searching channel:AbuseFilter AND "unset variable" for the last 15 minutes yield 150000 results. Hence, I think we should take a step back, and keep returning NULL in strict mode.

Most of the log entries could be resolved by checking the action (e.g. action==='move'), but I see that it's subpar to do that for all filters.

I'm not happy with NULL, due to the ambiguities it could cause. But again, I think it's our only option given the amount of "broken" filters.

Change 538598 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Return a DNULL in VariableHolder::getVar with GET_STRICT

https://gerrit.wikimedia.org/r/538598

Summing up my comments on gerrit:

I don't like this change because it can cause unexpected behaviour at runtime

(e.g. edit_delta < -5000 will match any non-edit action)

However, this is necessary due to the high amount of affected filters.

For other examples of stuff that can match at runtime when the expected variables are unset, see this data provider; specifically, test sets 2, 3, 4, 7, 14, 15, 17, and 18 (0-based).

I understand that having action === 'xxx' checks in all filters may seem unnecessary. Although some communities do that for all filters, most people do not (myself included, I should say). However, I also think that returning NULL is too false-positive-prone. Hence, ideally we should find a way to update all the existing filters to check for unset variables.

Daimona claimed this task.Sep 23 2019, 8:11 PM