The function checkWarnings() in UploadBase.php calls:
$localFile = $this->getLocalFile();
$filename = $localFile->getName();
But getLocalFile() will return null if the title of the upload is invalid for some reason (like it contains a blacklisted extension). This can cause an exception to be thrown. There should be a null check in there.
Version: 1.20.x
Severity: normal
Whiteboard: gci2014
Bryan.TongMinh wrote:
This should never happen because you should always call verifyUpload() before checkWarnings() (should be documented).
ApiUpload allows you to call checkWarnings() if you use async=true, which is I think the reason for the exceptions we see in production.
Thank you very much! Google Code-in task created. We hope to get code reviewers when a patch arrives.
Change 104012 had a related patch set uploaded by Mayankmadan:
verifyUpload should be called before checkWarnings
Change 105111 had a related patch set uploaded by Mayankmadan:
getApiWarnings() throws an exception if upload is invalid
Change 104012 abandoned by Mayankmadan:
verifyUpload should be called before checkWarnings
Reason:
Added a new patch for this bug
https://gerrit.wikimedia.org/r/#/c/105111/
Patch by Mayank in https://gerrit.wikimedia.org/r/#/c/105111/ needs rework.
Newcomers: See https://www.mediawiki.org/wiki/Gerrit/Tutorial#Amending_a_change
Would anybody be willing to be a mentor for fixing the existing patch as part of Google Code-in 2014? If yes, could you add it to
https://www.mediawiki.org/wiki/Google_Code-in_2014#Proposed_tasks until
Sunday (we can still improve the description until December 1st)?
If time is spare I can also do that - I just need a statement who
would mentor it.
See https://lists.wikimedia.org/pipermail/wikitech-l/2014-October/079264.html for more information. Don't hesitate to ask if you have questions!
I would like to work on this.
Doesn't checkWarnings() handle it if getLocalFile() return null ?
It has warnings for empty file, unwanted filetype and badfilename.
If getLocalFile returns null (instead of a LocalFile object), checkWarnings() won't even be able to proceed to the other checks.
Before those other checks are conducted, checkWarnings() will try to fetch $localFile->getName(), which will cause a fatal error (that method is not available in $localFile, if $localFile is null)
So no, it does not already handle this, but it probably should :)