Since this is caused by old entries, the script will fix it. Subsequent patches will reinforce the method.
@daniel yes... And then blocked on normalizeThrottleParameters.php being executed (without --dry-run). That alone should resolve most of the subtasks.
@daniel Not really; actually, there are still two different PHP notices and a fatal caused by malformed parameters (all seen in production), plus affected filters don't do what they should. Not UBN, but neither normal.
OK, so I think I understood this. First of, this is the link with the broken log entry. Using quarry for abuse_filter_log.afl_id === 101, you can get the whole row. Since it was inserted in 2009, the var dump is inside afl_var_dump instead of the text table/external storage. Reading it, you can see that the serialized string is truncated, and thus unserialization obviously fails. My guess is that this happened because the string was too long for an SQL blob , thus being truncated upon insertion. In fact, strlen returns 65361 of length, whereas BLOB is capped at 65535. I guess the delta has some explanation unrelated to this bug (maybe I didn't count bytes properly). The only thing I'm not sure of is the presence of the special character U+FFFD at the end of the serialized string. But again, I guess this is unrelated to the bug, and probably added e.g. by quarry or upon DB insertion.
@MarcoAurelio This will be announced in Tech/News... On Jan 28th... The announcement also suggests to add a message in MediaWiki:Delete and move confirm. I see that Johan has also added the gadget above as a suggestion. However, I must note that the gadget isn't really localizable, and chinese writings are probably fine only for zhwiki. We could ask intadmins to manually localize it (but this would create code duplication), or make it retrieve messages from a customizable variable. Or, probably easier & quicker, just change the messages to be in english.
@Jdforrester-WMF I'd create the task myself, although I'm not into MCR so I don't really know what's left to do here. Should the EditFilterMergedContent always pass a slot, or what else? Ping @Addshore.
@Tgr I'm sorry but I'm not sure I understood. Is it correct that, after $user = User::newSystemUser( $wgAbuseFilterUserName, [ 'steal' => true ] ); (line 720) we should do
MediaWiki\Auth\AuthManager::singleton()->autoCreateUser( $user, MediaWiki\Auth\AuthManager::AUTOCREATE_SOURCE_MAINT, false );
@MarcoAurelio Sure! I can do better: write down a list of urgent patches, ordered by priority (regardless of how complex are them), and a list of easy patches under review (usually doc-only or other minor changes). The latter can be as important as the former, since they often make writing other patches easier, and of course are much quicker to review. Matěj did a great job in reviewing a dozen of them lately, but there are others. Where should I post the list once done?
(Excuse me, typo upon committing)
Sun, Jan 20
Sat, Jan 19
The whole MediaWiki sofware is affected... Unfortunately T/N/04 is already translated and we'll probably have to announce it in /05 (which will be published on Jan 28th). CC'ing @Johan to be sure. The message could be:
I guess <= is fine and => is the problem. Not sure how to deal with that though.
Fri, Jan 18
Since a solution doesn't seem to be near, I guess we should at least inform communities of the problem. Many people already know, but not everybody.
I see that the problem here is using WHERE rc_user = 'XXXXXXXX'. The field is not indexed, and thus the filesort. However, I also see that the field is deprecated in favour of rc_actor (which has an index together with timestamp), and thus this should be fixed with actor conversion. Leaving open as memo.
I could trigger this again via the same script. This leads me to think that the bug is very likely to happen when several protection requests are sent subsequently.
Thu, Jan 17
@Leaderboard Thanks, I'm working on it (i.e. on understanding why filter 59 didn't trigger), while for testing flow edits we still need someone who knows Flow's codebase.
Adding to radar for the moment. However, I feel that (unless someone offers to review the patch) we should just avoid adding yet another hack and go ahead with the stable solution.
I checked on my wiki and found the cause.
First, filters are executed and we eventually save an AbuseLog entry. There we use $user->getId() and $user->getName() to get such data. Now, if the action is an account creation $user is the creator, passed by the AuthenticationProvider here, and is an anonymous user. Thus, calling getName and getId on it returns respectively the IP and 0. This way, we would just end up saving the IP as log performer, however it's not over. As I was saying in my previous comment, AbuseFilter performs the horrible hack of changing afl_user_text to the account name, in order to avoid showing the IP. Thus, what we get in the end is a DB row where afl_user is 0 and afl_user_text is the account name.
Back to Special:AbuseLog, if a user is specified we try to build a User object from the given name. This creates two cases:
- If the account doesn't exist (assume creation was disallowed by AbuseFilter), then we'll query the abuse_filter_log to find rows with afl_user === 0 and afl_user_text = the_one_specified_in_the_search_bar. This will succeed, proof.
- If the account exists (we can assume that this means that the creation wasn't disallowed by AbuseFilter or anything else), the User object will have a non-zero ID and a name, so we'll try to match an abuse_filter_log with such ID and name, but this obviously fails because we stored a 0 as user ID, proof.
@Leaderboard May I please have some spam examples which should have triggered filter 59?
Will investigate later. Below is a possible explanation.
On "createaccount" the filter is triggered by anonymous users (i.e. you would find it by searching the IP which triggered it). However, in order not to publicly show the IP, AF replaces it with the new account name before saving the log entry, which is pretty hacky, has lead to other problems, and doesn't actually completely prevents the IP from being leaked in several specific cases. A proper solution would probably be to change this behaviour and make it somehow saner. As for this specific problem, I think (but I have to check) that we inconsistently use anonymous users (i.e. IPs) and fake users with the new username in different parts of the code, and thus searching by username doesn't work because entries reference the IP.
Wed, Jan 16
@1997kB The patch above needs some love, I'll try to add some reviewers on gerrit. Making the global renamer group a real global group, so that it could be read via global_user_groups would also be useful for this specific case.
@Leaderboard IMHO this isn't UBN! per https://www.mediawiki.org/wiki/Phabricator/Project_management#Priority_levels. Again, the patch above (which I'm going to rebase) is WIP because something in Flow codebase prevents it from working, and I don't want to mess with it. I also had several troubles in getting Flow to work properly on my wiki, and thus testing the patch. Developers/Maintainers says stewards are Growth-Team, and this task is already on their workboard. We should get someone from there to give a look to the patch.
This is also not UBN because filters still work -even though they cannot be tested-, and being myself a sysop on MediaWiki I can try to help tuning spam-related filters, I just need some time and examples.
Tue, Jan 15
I tried to test this on my wiki, and it succeeded 3-4 times in a row. I wonder if this could be due to some race condition... Also, the stream of errors is pretty steady, and is of ~100 errors a day. I guess the only reason for this not to be UBN! is that a simple workaround (delete first, then move) exists.
Sun, Jan 13
The MWException is T210739.
Sat, Jan 12
For the record, this happened 127 times in the last 24 hours. Not a lot, but not a little.
Fri, Jan 11
That's what I thought. If so, we need community consensus for that.
@Neriman2003 Aklapper's response is correct. If this is a support request, this is not the right place for it. If you're asking whether it's possible for interface admins to edit abuse filters, the answer is: it depends. Editing abuse filters require the abusefilter-modify right, which I don't think interface admins have. Usually people who can edit abuse filters are sysops or have a specific user group.
Huh, I lost my train... @Addshore is the change stable, and are we fine with it for now?
@Tgr Well, storing to the text table makes it possible to use its flags just like revision does, e.g. 'external'... Of course we could also store the var dump directly inside afl_var_dump, but I can't say if it's worth it.
The system used for applying tags is not ideal (relies on global state etc.), but I'm unsure what an alternative could be, since tags cannot be applied when filters are executed (per design in core). To keep the process simple, we use an identifier which can be constructed with data available both when running filters (and we have plenty of data at that point), AND when saving the recent change. However, for the latter we only have a RecentChange object passed in by the hook, and not much info we can extract from there aside from user, action and title.
I rebased the patch and bumped geoip version to 2.9.0. I'm also tracking this task, although I'm afraid there's not much I can do.
The problem here is the usual one. Option A is less sane, but should be totally back-compat (unless a function call relied on this feature to pass a null parameter, but that would be crazy). Option B is the one I like most, since it helps having a saner syntax. However, it would break any filter using trailing commas. Unfortunately, filter editor won't have any way to know that, as filter failures are only sent to logstash. The patch for it is https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/AbuseFilter/+/429381/ (and related ones), which adds a new page for filter failures at runtime.
I like Tgr's idea. However, I can also gladly say that this is a good moment to fix the problem: T213006 outlines a script to make various fixes to the afl_var_dump column, and consequently to the text table. If we want to change something in old entries, speak now...
Thu, Jan 10
So currently there's no way to retrieve the slot being edited via parameters passed to the EditFilterMergedContent hook, is that right?
@Jdforrester-WMF So getContent should be replaced. Is there something which allows getting the content for a certain slot from a revision, and a way to extract the current slot from the hook params?
I see that the wrong one is old_wikitext, while new_wikitext is correct (although it contains 'M60452106' and I'm unsure if it should).
Diffs are computed basing on old_wikitext and new_wikitext, which are the only base variables used to compute edit_delta, added_lines, edit_diff, new_size etc. This means that filters are very likely to explode.
So now we have a great opportunity to fix this, which is T213006. The task outlines the tasks of a new script used to clean afl_var_dump and related, and since this is strongly related, we should handle it as well.
My only question is how to fix it. Should we add the 'utf-8' flag to every entry, or what else? Just let me know and I can amend the patch.
I have alerts for all AF tasks, so I have actually read this, and I'll be glad to help :)
Wed, Jan 9
That's what I thought, too... Mostly because the front-end way of bypassing a filter shouldn't exist. That is, if you want someone not to hit a filter, you should code the filter to do that, mostly by excluding user groups. And assigning this right to global renamers would make them unaffected (or, respecting the points in the task description, less affected) by other filters for which they don't really need this flag. This is IMHO not ideal. Edge-cases like rename job should instead be handled back-end.
Indeed. It's impossible to change anything related to this group (assigned rights, assign or remove the group...).
@Tgr Well, nice point for 3. As for 1 in task description, I had to go to REL1_30 to find it; here is the bad line, but it doesn't happen anymore (for AF, of course) since rEABF799a2fb1ed73c210adcdb8e3713805430f590960 removed parsing at that point.
I added 'systemuser' to wgImplicitGroups. This should be saner, although it really prevents this group to be shown e.g. on ListUsers. I think this should be fine for now. If we'll find out that we actually want this group to be displayed, there's still time to do it.
Happened 11 times in the last 30 days, and now we have the filter ID. Given that it is public, I'm gonna say that this happen due to filter 192. Shares the cause - and possibly the solution - with T212082.
Hm, unseen in the last 30 days. Maybe this was fixed in rEABF688eccea477a3a537c86066112bda7da202a8463?