Re: [pve-devel] [PATCH v2 http-server 2/2] AnyEvent: Fix #3990 - make small files uploadable

[Daniel Tschlatscher <d.tschlatscher@proxmox.com>]

The GUI works without problems. Files of arbitrary size can be
uploaded without issues.
Apart from that, I mostly focused on usage via the API using curl and
Postman.


What worked as expected:
* Upload in the GUI
  Tested by uploading files of sizes:
  0B, 1kB, 8kB, 15kB (did not work before), 16kB, 32kB, 1MB, 1GB, 10GB
* Uploading a 8kB and a 32kB file with and without the corresponding
  checksums.
* Unknown values for the "metadata" in the body (e.g. Content-
  Disposition, filename, ...) fail expectedly
* Unmatching file extensions (extension of the file passed in phase 0
  and file extension assigned in the filename) fail expectedly

All errors above return HTTP status 501 ("Not implemented") which I
think is rather confusing, even if they include a descriptive error
message.
I'd wager that it would be better to differentiate between malformed
multi-form data and internal server errors, and return 400 for incorrect
user inputs. This makes it much easier for anyone (that is not a
browser) to interface with this part of the API.


Below problems seem to originate in file_upload_multipart() or at least
reach this part of the code (and could therefore probably improve error
status communication for the user here):
* Using an unknown value for the "Content-Type" Http-Header or
  assigning boundary differently to what is used in the body
=> Connection dies with error "empty reply from server"

* Adding an unknown "metadata" field or changing an existing one (e.g.
  filename to fielname)
=> results in the same error as above

* Malforming the boundary for phase 1 (the second one in the http body)
  (Seems this is parsed incorrectly?)
=> 501 wrong field `name` for file upload, expected `filename` - abort
   upload

* Malforming the very last boundary
=> Connection dies, "empty reply from server".
   Syslog error: "problem with client [...] Connection timed out"


I am not quite sure how much precedence fixing the errors above should
have, as even most API users will probably never encounter them.
However, I feel like that at the very least the basic error message
should be revised to no longer return 501 and to include a more
descriptive error message than just "upload failed".

With no major problems and depending on how important the (edge-)cases
above are, consider this patch

Tested-by: Daniel Tschlatscher <d.tschlatscher@proxmox.com>