Steinsplitter reported to me on IRC that they suppressed a page on beta labs and the AbuseFilter logged it like a normal deletion (visible to me, a non OS at the time).
http://commons.wikimedia.beta.wmflabs.org/wiki/Special:AbuseLog/25130 is the AF entry, the log action is: 17:26, 15 August 2014 Steinsplitter (talk | contribs | block) suppressed page GWToolset:BLAHAHA (testing abf) (view/restore)
My simple-ish idea of how to fix this is pass $suppress to the ArticleDelete hook, and in AF just don't try to filter it if $suppress === true.
https://en.wikipedia.org/wiki/Special:AbuseFilter/626 and https://commons.wikimedia.org/wiki/Special:AbuseFilter/134 are very likely to trigger this bug, but have been temporarily disabled until this can be fixed.
(In reply to Kunal Mehta (Legoktm) from comment #1)
My simple-ish idea of how to fix this is pass $suppress to the ArticleDelete
hook, and in AF just don't try to filter it if $suppress === true.
That seems reasonable. I'm surprised we don't already pass it.
(In reply to Chris Steipp from comment #3)
(In reply to Kunal Mehta (Legoktm) from comment #1)
My simple-ish idea of how to fix this is pass $suppress to the ArticleDelete
hook, and in AF just don't try to filter it if $suppress === true.That seems reasonable. I'm surprised we don't already pass it.
Except we probably do want to still filter it, just additionally suppress any logs.
We could also hook ArticleDeleteComplete and do the suppression of the log from there.
For some reason I was looking at the ArticleDelete hook recently and wondered why there was no $suppress parameter provided (ugh, now I'm wondering why it was because it'd probably be useful to link here). Shall we just upload a patch to Gerrit to do that?
Should probably check the other extensions using this hook to ensure nothing is being left exposed in suppression deletions
@sbassett @Reedy @Urbanecm Anybody who would be willing to take a final look at T71617#6722431 and sec-deploy it? Thanks!
+2, patch approved. I'll ping Daimona on Monday so we can deploy&test the patch together.
+2, patch approved. I'll ping Daimona on Monday so we can deploy&test the patch together.
Thanks - if you need any help, let me know.
Also - there's a handful of these kinds of lingering sec patches, unfortunately. These are the other ones for AF of which I am aware:
Deployed:
12:25 <Urbanecm> !log Deploy security patch for T71617 12:25 <+stashbot> Logged the message at https://wikitech.wikimedia.org/wiki/Server_Admin_Log
@sbassett Moving to Incoming. Could you please do the final honors (backport mainly), please?
I'll do my best to look at the other waiting patches.
Change 670302 had a related patch set uploaded (by SBassett; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] SECURITY: Don't filter suppressions
Change 670302 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] SECURITY: Don't filter suppressions
Change 678659 had a related patch set uploaded (by Reedy; author: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Don't filter suppressions
Change 678660 had a related patch set uploaded (by Reedy; author: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Don't filter suppressions
Change 678660 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_31] SECURITY: Don't filter suppressions
Change 678659 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@REL1_35] SECURITY: Don't filter suppressions