Page MenuHomePhabricator

AbuseFilterFilterAction hook sometimes passes null instead of Title object
Closed, ResolvedPublic

Description

Follow-up to T143073: Fatal error: Argument 1 passed to MessageHandle::__construct() must be an instance of Title, null given. It's documented that filterAction would always pass a Title object. I don't know whether this is intentional or not but someone should probably look into this and other hook handlers of this to check whether it's okay.

Got non-Title in AbuseFilterHooks::onUploadStashFile/AbuseFilterHooks::filterUpload/AbuseFilter::filterAction/Hooks::run/TranslateHooks::onAbuseFilterFilterAction

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 30 2016, 12:28 PM
Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptOct 21 2017, 3:46 PM
Daimona claimed this task.Oct 10 2018, 5:26 PM
Daimona added a subscriber: Daimona.

Could someone with log access please check if this still happens? The log to be checked is the one used here, i.e. T143073 (also, it'd be really useful to know if the logged callers are always the same). Anyway, I had independently written a patch some weeks ago to avoid running filters with null titles, andI'll add this task to it in a minute. While writing it, I got to the conclusion that a null title can only be passed during uploads, and even in this case I wasn't really sure about when it could happen. Understanding it could make it easier to decide whether disabling filters for null titles is a good solution or not.

Change 464119 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Don't run filters with null title

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

Daimona moved this task from Backlog to Under review on the User-Daimona board.

Last 30 days, type:mediawiki AND +message:"Got non-Title" yields 793 results.

  • By channel: all have channel:T143073 as expected.
  • By wiki: all from commonwiki.
  • By version: 1.32.0-wmf.20, 1.32.0-wmf.22, 1.32.0-wmf.23, 1.32.0-wmf.24.

Sample:

message
Got non-Title in AbuseFilterHooks::filterUpload/AbuseFilter::filterAction/Hooks::run/Hooks::callHook/TranslateHooks::onAbuseFilterFilterAction

It seems most are from POST requests to /w/api.php. There are no details beyond that.

Change 466842 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Emit debug logs when filtering without title

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

@Krinkle many thanks, that's way too much. The patch above adds some info to the debug logs, and also covers null title for edits. After having fully understood the problem, we should fix it upstream. For instance, IIUC uploads have a null title when the specified destination is invalid. What we should do is validate the title and immediately reject invalid ones before calling AbuseFilter.

Alright, for uploads it's pretty simple. If the specified destination name isn't valid, then the title is null. This may happen both via Special:Upload or API, and you can try it by specifying a name longer than 240 bytes. The cool thing is that doing so throws an exception from AbuseFilter (we call getNamespace() on null), so it already doesn't work. However, AFAICS an invalid title makes the upload fail anyway, so as I was saying above we should simply check the title before running the hook.

Change 479531 had a related patch set uploaded (by Krinkle; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@wmf/1.33.0-wmf.8] Emit debug logs when filtering without title

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

Change 466842 merged by jenkins-bot:
[mediawiki/extensions/AbuseFilter@master] Emit debug logs when filtering without title

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

@Krinkle are you planning to deploy the patch above yourself? Unfortunately I won't be around for SWAT next week. Otherwise we could wait wmf.9 in production, then another week or two to gather results.

Change 479531 abandoned by Krinkle:
Emit debug logs when filtering without title

Reason:
wmf.9 everywhere

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

Now the change has been live in production for a couple of weeks; what are the results?

+type:mediawiki +channel:T143073 +message:"Got non-Title", last 30 days, 945 results.

Countwiki
943commonswiki
2mediawikiwiki
Countmesssage
945Got non-Title in AbuseFilterHooks::filterUpload/AbuseFilter::filterAction/Hooks::run/Hooks::callHook/TranslateHooks::onAbuseFilterFilterAction

@Krinkle thanks! That's a lot of results! The log message includes the title error, what error codes do we have here?

Ah, actually that one is the log from Translate extension. The new one is in channel "AbuseFilter" and the messages contain "received an invalid title" and "received a null title". The former includes the title error (and the title itself, although of course that's always null...).

I was trying to retrieve the data myself, having just received access to Logstash. However, I cannot find any record. I can find the ones from Translate in channel T143073, but nothing shows up in the AbuseFilter channel (aside from slow filters). Is it me being noob, or these logs aren't really there? And if so, why?

Data has started to flow into Logstash. I only see 9 hits in the past 24 hours, all for action = 'stashupload'. Actually, this is easy to explain: users may enter an invalid destination title for the upload, and we run filters with action = 'stashupload' before warning them that the title isn't valid. So here are possible cases and proposed solution

  • Null title for filterEdit. This shouldn't really happen, and I think we can just avoid filtering if it does.
  • Null title for 'upload' action. Again, this shouldn't happen. The hook is called from UploadBase::performUpload, which assumes $this->getLocalFile() always return a File object (since it calls methods on it), and not NULL as it would for invalid title. Avoid filtering should be OK.
  • Null title for 'stashupload' action. This is what already happens, and should cause all of the null title filtering. Avoid filtering isn't an option because 'stashupload' is meant to be used with partial data (see docblock for the onUploadStashFile handler). Filtering with a null title is equally bad because that lacks uniformity with other cases. If a desiredDestName is specified, we could build a Title object from that, however desiredDestName could be null as well, or it could be an illegal title (i.e. not only illegal as a file name, but also illegal as a title itself). Thus, we'd still end up with null titles. Another idea could be to use a bogus title, but that could create false positives.
Alroilim closed this task as Invalid.Feb 2 2019, 7:21 PM
Alroilim removed Daimona as the assignee of this task.
Alroilim set Due Date to Feb 1 2019, 9:00 PM.
Alroilim updated the task description. (Show Details)
Alroilim removed subscribers: Krinkle, gerritbot, Daimona and 3 others.
Restricted Application changed the subtype of this task from "Task" to "Deadline". · View Herald TranscriptFeb 2 2019, 7:21 PM
Gopavasanth reopened this task as Open.Feb 2 2019, 7:36 PM
Gopavasanth assigned this task to Daimona.
Restricted Application changed the subtype of this task from "Deadline" to "Task". · View Herald TranscriptFeb 2 2019, 7:59 PM

Now we have 76 hits in the last two weeks. My final idea is to avoid filtering. Given that it can only happen for 'stashupload', if an invalid title is specified, then the UploadBase code will warn the user about that without the need to invoke AbuseFilter. If the user then specifies a valid title, we can filter the action as usual. Which is to say, the action will still be filtered. The only important thing is to clearly state this in the docs.

Change 464119 had a related patch set uploaded (by Daimona Eaytoy; owner: Daimona Eaytoy):
[mediawiki/extensions/AbuseFilter@master] Don't run filters with null title

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

Change 464119 merged by Krinkle:
[mediawiki/extensions/AbuseFilter@master] Don't run filters with null title

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

Daimona closed this task as Resolved.May 26 2019, 3:24 PM

Now filters aren't executed if we have no title, thus marking as resolved.

Daimona moved this task from Under review to Done on the User-Daimona board.May 26 2019, 3:25 PM