Page MenuHomePhabricator

UploadBase::checkWarnings could throw exception on null object access
Closed, DuplicatePublic

Description

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

Details

Reference
bz38222

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 12:59 AM
bzimport set Reference to bz38222.
bzimport added a subscriber: Unknown Object (MLST).
cneubauer created this task.Jul 6 2012, 6:52 PM

Bryan.TongMinh wrote:

*** Bug 43832 has been marked as a duplicate of this bug. ***

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.

cc'd Andre and Quim. Seems like this would be an easy task for the code-in...

Qgil added a comment.Dec 27 2013, 6:53 AM

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

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

Change 105111 had a related patch set uploaded by Mayankmadan:
getApiWarnings() throws an exception if upload is invalid

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

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/

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

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!

Krinkle set Security to None.
Krinkle removed a subscriber: Unknown Object (MLST).
Jdforrester-WMF moved this task from Untriaged to Backlog on the Multimedia board.Sep 4 2015, 5:58 PM
Restricted Application added subscribers: Steinsplitter, Matanya. · View Herald TranscriptSep 4 2015, 5:58 PM

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.

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptMar 10 2017, 9:29 AM
Qgil removed a subscriber: Qgil.Mar 13 2017, 5:38 PM

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 :)

Harjotsingh removed Harjotsingh as the assignee of this task.Apr 9 2017, 4:13 PM