search

richardgirges / express-fileupload

Simple express file upload middleware that wraps around busboy
MIT License
1.52k stars 261 forks source link

empty file issue #226

Closed koprda closed 4 years ago

koprda commented 4 years ago

uploading "empty" zero-length file end with error in both modes memory and temp file

simple reproduction - truncate test file basketball.png and run test

RomanBurunkov commented 4 years ago

That is strange. Can you turn module's debug output on and share it.

RomanBurunkov commented 4 years ago

I was able to reproduce it.

Looking into this.

RomanBurunkov commented 4 years ago

In case of in-memory upload this PR #15 affects empty file uploads. @r3wt , could you please elaborate #14 issue?

Since we already have the following check in processMultipart:

      // Do not add file instance to the req.files if original name and size are empty.
      // Empty name and zero size indicates empty file field in the posted form.
      if (!name && size === 0) return;

Do we still need that check in fileFactory:

  // see: https://github.com/richardgirges/express-fileupload/issues/14
  // firefox uploads empty file in case of cache miss when f5ing page.
  // resulting in unexpected behavior. if there is no file data, the file is invalid.
  if (!fileUploadOptions.useTempFiles && !options.buffer.length) return;

I've commented 

if (!fileUploadOptions.useTempFiles && !options.buffer.length) return;

And it looks checks in processMultipart do the job, so if I reproduced those issue correct, probably we can consider removing check in fileFactory.

r3wt commented 4 years ago

@RomanBurunkov Maybe. I'm not actually sure, we might have to summon Richard to see if he knows. This was so long ago I barely remember it.

RomanBurunkov commented 4 years ago

Hi @richardgirges , do you remember anything about issue #14 and PR #15. I can't figure out steps how to could be reproduced.

RomanBurunkov commented 4 years ago

Fixed with PR #233

nusu commented 4 years ago

I tried it, this doesn't seem fixed I'm going to fix it and push a following PR, can you open the issue @RomanBurunkov

edit: sorry I misread the issue, nothing wrong with the fix though it creates an empty file while it shouldn't I created a PR for that #241