At a glance this happens when files are uploaded with an invalid target name.

(Still seen on 1.33-wmf.21.)

Change 498984 had a related patch set uploaded (by Umherirrender; owner: Umherirrender):
[mediawiki/core@master] Handle null from UploadBase::getLocalFile

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

		// Check if the uploaded file is sane
		if ( $this->mParams['chunk'] ) {
			$maxSize = UploadBase::getMaxUploadSize();
			if ( $this->mParams['filesize'] > $maxSize ) {
				$this->dieWithError( 'file-too-large' );
			}
			if ( !$this->mUpload->getTitle() ) {
				$this->dieWithError( 'illegal-filename' );
			}
		} elseif ( $this->mParams['async'] && $this->mParams['filekey'] ) {
			// defer verification to background process
		} else {
			wfDebug( __METHOD__ . " about to verify\n" );
			$this->verifyUpload();
		}

if 'chunk' is set, UploadBase::getTitle is checked for null and api dies
elseif 'async' and 'filekey' is set, nothing is validated -> here a very long file name can cause this issue
else calling ApiUpload::verifyUpload -> UploadBase::verifyUpload -> UploadBase::validateName -> UploadBase::getTitle to check for null

So when doing a chunked upload (with a short valid filename) and than for the publish step (without the chunk param) the filekey and async is given with a long filename (>240) this error can occur.

First request:
action = upload
comment = test
token = csrf
filename = shortname.jpg
stash = 
chunk = your file
offset = 0
filesize = max

Result:
result=Success
filekey=xxxxxxxxxxxx.tmp

Second request:
action = upload
comment = test
token = csrf
filekey=xxxxxxxxxxxx.tmp
filename = verylongname012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789012345678901234567890123456789.jpg
async =

Without the async it gives the expected error: code=filename-toolong, info=Filenames may not be longer than 240 bytes.

I have not checked if my combine of the params makes sense, but there is a validation gap when defered to the background process
I see no security problem here (bypassing any validation for blacklist or such), because getTitle is called when looking for warnings and than the fatal has happen. The warning check path of the code is always running and not depending on params

Hm.. yeah, I don't know either. The path from the client interaction to this particular line of code where $localFile = $this->getLocalFile(); $localFile->load(…); happens is quite long.

Without the necessary context, I'm inclined to think that if something shouldn't be possible, it'd be better to fail explicitly (and we'll find out if it's really possible). Instead of skipping the validation and assuming it's fine.

Anyway, I don't know how to verify this or what the use cases are.

@MarkTraceur Could you/your team take a look here to see what we can do to fix and/or avoid this error?

I'm not entirely certain I follow the example code above (I looked for it in the patched file but it must be elsewhere), but I'm inclined to agree that it's better to fail on "impossible" behavior, then dig more to figure out why it's not impossible.

The async API parameter seems to only fire (in UploadWizard, at least) when uploading very large files (>10MB, higher than most images uploaded to Commons), and obviously this error will only occur when someone then further tries to input a too-long filename. That may be a problem for GLAM cases (where uploads are large, and filenames tend to have a lot of information in them), but as the error count shows, the impact here is relatively low.

Without a deep dive through the code path, I'm not sure how best to fix this. The bug is in our backlog and will be tracked and triaged in time, but unless we see a much larger spike, I doubt we'll get to it very soon. Especially given our day-to-day is currently focused on attempting to finally release an extraordinarily delayed feature.

If this becomes more urgent, I'll bring it up to the team.

Thanks. I'm landing Umherirrender patch and will circle back to see if this rare issue is still reported in Logstash after that. Also, be on the look out for any new upload-related errors or bugs.

Change 498984 merged by jenkins-bot:
[mediawiki/core@master] Always validate uploads over api

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

Given the timing, this is not going to be the sole cause of T190988, but possibly related? That's API-based chunked uploads being mis-reconstructed, and has a related category of failure in production: https://commons.wikimedia.org/wiki/Category:Incomplete_files_(5_MB_interruption)

Change 510905 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/extensions/UploadWizard@master] Fix title max length to match backend, and actually validate it

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

Change 510893 had a related patch set uploaded (by Matthias Mullie; owner: Matthias Mullie):
[mediawiki/core@master] Validate name for async uploads

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

@Umherirrender Can you take a look at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/510893 to see whether you think this would also adequately solve this task?

Change 510905 merged by jenkins-bot:
[mediawiki/extensions/UploadWizard@master] Fix title max length to match backend, and actually validate it

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

Not seen in WMF Logstash for 30 days.

Seen again.

Request: commons.wikimedia.org/w/api.php

Fatal error:
Call to a member function load() on null

#0 /includes/api/ApiUpload.php(664): UploadBase->checkWarnings()
#1 /includes/api/ApiUpload.php(127): ApiUpload->getApiWarnings()
#2 /includes/api/ApiUpload.php(104): ApiUpload->getContextResult()
#3 /includes/api/ApiMain.php(1602): ApiUpload->execute()
#4 /includes/api/ApiMain.php(538): ApiMain->executeAction()
#5 /includes/api/ApiMain.php(509): ApiMain->executeActionWithErrorHandling()
#6 /api.php(83): ApiMain->execute()
#7 /srv/mediawiki/w/api.php(3): require(string)
#8 {main}

In T40222#452661, @gerritbot wrote:

Change 104012 had a related patch set uploaded by Mayankmadan:
verifyUpload should be called before checkWarnings

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

In T40222#452679, @gerritbot wrote:

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

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

In T40222#3098016, @matthiasmullie wrote:

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

Ok we think this is now fixed (or will be when the train rolls), I guess we're back to monitoring logstash to see if it resurfaces

Change 510893 merged by jenkins-bot:
[mediawiki/core@master] Validate name for async uploads

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