Page MenuHomePhabricator

AbuseFilterFilterAction hook sometimes passes null instead of Title object
Closed, ResolvedPublicPRODUCTION ERROR


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

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

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.


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

@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

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

@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

wmf.9 everywhere

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.

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

Screen Shot 2019-01-06 at 13.26.46.png (354×2 px, 49 KB)

@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 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 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

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

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

mmodell changed the subtype of this task from "Task" to "Production Error".Aug 28 2019, 11:11 PM