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.tschlatsc...@proxmox.com>
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
