Vertx-web: File upload handling should be optional in BodyHandler or a separate handler

[aschrijver]

Version

• vert.x web: 3.2.1
  

  Context

RoutingContext in its JavaDoc advises BodyHandler for various uses, e.g. getting at the body of the HttpServerRequest. But the body handler assumes I am interested in file uploads as well and pre-emptively creates the 'file-upload' directory, making it pop up all over the place depending how I run my tests/deployments.

The BodyHandler should either have optional file upload handling, or not create a directory if none specified, or maybe file uploads should be a separate FileUploadHandler entirely.

Code: https://github.com/vert-x3/vertx-web/blob/master/vertx-web/src/main/java/io/vertx/ext/web/handler/BodyHandler.java

[aschrijver]

I am not sure this is an enhancement. Its more like a bug. Currently the handler is dealing with multiple concerns, i.e. handling body payloads, _and_ handling file uploads. If only interested in the body (like most people using REST will) you will get useless file_upload directories created (also on production systems).

As a quick fix is it an idea to have the default constructor (without params) disable the upload handling? I.e. only when explicitly passing an upload location will the directory be created.

[pmlopes]

well i don't agree that it is a bug, since the design and documented behavior do not state that BodyHandler has separated concerns. I do think it is a good idea to have then configurable and probable the arguments less constructor would be fine. Now it is just a matter of time to implement or some community pull request ;-)

[aschrijver]

Thanks! While the no-argument constructor is most straightforward it would break backwards compatibility though. Is that a problem?

[pmlopes]

Yes that is a concern, we should be always backward compatible according to the semantic version.

[pmlopes]

Actually the PR was not about the full issue, so reopening...

[aschrijver]

Maybe renaming the issue is best. With this small change the file_uploads or custom upload dir is never created when not uploading, which fixes my issue.

[hakanilter]

If I understand correctly, Body Handler automatically accepts file upload and saves to the folder by default. If it is true, it can cause huge security problems! It doesn't check anything like file size, file type, etc. Oh my!

[pmlopes]

No there is no real security issue here. You can control the behavior, if you check the documentation the handler can control upload size limits as well if files should persist after the request or not. This pr is just a enhancement to split multipart upload from http body uploads.

[castarco]

Hi, as far as I know, the issue is not solved. I have configured the body handler to accept payloads with 0 bytes, but when I receive a POST request (having a handler or not for POST requests)... the directory is created anyway.

In certain circumstances this could imply stability problems.

[pmlopes]

@castarco the issue hasn't been worked on, so we still need to add a enable disable feature for upload, multipart data and raw body. Only then we can configure to create or not the directory.

Since this isn't really a blocker we've been working on other issues first. Of course we welcome pull requests too if you would like to help.

[harjot-oberai]

Hi, @pmlopes

I know this is an old issue, but it still hasn't been solved and it really is a blocker for my use case.

I am deploying a fatjar on a host. The fatjar configures a Vertx HTTP server.

Now we don't have write access the the directory from where the fatjar is loaded, so while creating the file-uploads directory, an exception is thrown and the server terminates.

GET requests work fine, but with POST requests there is an issue.

I don't upload any files, so there should not be any need to create the file-uploads directory, but as @castarco mentioned, the directory is created anyways.

Please resolve this issue ASAP. I will try to work on a PR for the same, but in the meantime I would highly appreciate if your team can work on this.

Also if there is some sort of workaround that I can use in the meantime, please mention that as well

[pmlopes]

You could configure the uploads dir to a location you can write, for example, /tmp .

Alternatively you can propose a pull request for this. It's not hard, and has been quite low on the project priorities.

[tnolli]

I just started using vert.x and vert.x web and found them amazing; I had some spare time to work on something possibly usefull for the community and I decidet to try with this ticket.

@pmlopes can you or someone else from the team have a look at my commit and see if looks good, before I create a pull request?

[aschrijver]

Hi @tnolli

I created a bunch of review remarks on your commits. I did not go through all of the tests, nor did I run and test the code, so I can't vouch for that, but hopefully I've helped you further :)

[tnolli]

Hi @aschrijver

thanks for the code review; I applied most of your remarks but the one that allows changing the file handling capability after object construction. I agree that one should know what she/he is doing but I am not sure that allowing BodyHandler.create(false).setUploadDirectory(someDir) or BodyHandler.create().setHandleFileUploads(false).setUploadDirectory(someDir) is a good thing. Happy to discuss further on this before me creating the PR.

[tnolli]

I had a look at settings management in other areas of verx-web and noticed that possibly incompatible settings are already allowed (i.e. new HttpClientOptions().setKeepAliveTimeout(300).setKeepAlive(false);) so I implemented also #setHandleFileUploads(boolean) as @aschrijver was suggesting

[tnolli]

Just created PR #1014

[pmlopes]

Fixed by #1014