Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resteasy Reactive Multipart fails if fileName is not set for a file part #20938

Closed
andreas-eberle opened this issue Oct 21, 2021 · 24 comments · Fixed by #29716
Closed

Resteasy Reactive Multipart fails if fileName is not set for a file part #20938

andreas-eberle opened this issue Oct 21, 2021 · 24 comments · Fixed by #29716
Labels
area/rest kind/bug Something isn't working
Milestone

Comments

@andreas-eberle
Copy link
Contributor

Describe the bug

We use resteasy reactive's multipart to upload a file to our Quarkus (2.3.1.Final) service. This works fine with most clients (e.g. Postman) because they set the filename attribute in the content-disposition.

Unfortunately, one of our clients does not set that and then the upload fails with http error 413 Request Entity Too Large.
While debugging, I think I was able to track this down to https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/multipart/MultiPartParserDefinition.java#L237 . In this line, it is checked if the content-disposition header contains the filename part. Only if the filename is there, the content is assumed to be a file.

Therefore, in our case, resteasy reactive thinks the content cannot be a file and thus continous to read the content as an attribute and thus fails when the attribute becomes too large (https://github.com/quarkusio/quarkus/blob/main/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/multipart/MultiPartParserDefinition.java#L292).

Since the filename is only an optional part of a file upload, I think resteasy reactive should not depend on it. Probably it would be better to check the content type to see if it is something like application/octet-stream or an image or so.

Expected behavior

The upload works as expected.

Actual behavior

The upload fails with a 413 error because the file upload is misinterpreted.

How to Reproduce?

I wasn't able to reproduce the missing filename with curl or postman. The client that did not send the request was a flutter app. So I cannot easily provide a reproducer.

Output of uname -a or ver

No response

Output of java -version

No response

GraalVM version (if different from Java)

No response

Quarkus version or git rev

No response

Build tool (ie. output of mvnw --version or gradlew --version)

No response

Additional information

No response

@andreas-eberle andreas-eberle added the kind/bug Something isn't working label Oct 21, 2021
@quarkus-bot
Copy link

quarkus-bot bot commented Oct 21, 2021

/cc @FroMage, @geoand, @stuartwdouglas

@andreas-eberle
Copy link
Contributor Author

andreas-eberle commented Oct 21, 2021

The failing request looks like this:

--dart-http-boundary-t+ZEewIzHXFt99qK-r0j70dGk1N3VC-cLOctyNq6J6jPkCx6Fa9
content-disposition: form-data; name="fileName"

miscellaneous_image.jpg
--dart-http-boundary-t+ZEewIzHXFt99qK-r0j70dGk1N3VC-cLOctyNq6J6jPkCx6Fa9
content-disposition: form-data; name="description"

Additional image
--dart-http-boundary-t+ZEewIzHXFt99qK-r0j70dGk1N3VC-cLOctyNq6J6jPkCx6Fa9
content-type: application/octet-stream
content-disposition: form-data; name="file"

<THE FILE CONTENTS>
--dart-http-boundary-t+ZEewIzHXFt99qK-r0j70dGk1N3VC-cLOctyNq6J6jPkCx6Fa9--

A working request (from Postman) looks like this:

----------------------------821547038962622871140886
Content-Disposition: form-data; name="fileName"

miscellaneous_image.jpg
----------------------------821547038962622871140886
Content-Disposition: form-data; name="description"

Additional image
----------------------------821547038962622871140886
Content-Disposition: form-data; name="file"; filename="mango-small.jpg"
Content-Type: application/octet-stream

<THE FILE CONTENTS>
----------------------------821547038962622871140886--

So the only real difference is the content-disposition header containing the filename attribute.

@geoand
Copy link
Contributor

geoand commented Oct 21, 2021

To be honest, I am not in favor of fixing this as it's clearly a case of a problematic client request.

@stuartwdouglas WDYT?

@andreas-eberle andreas-eberle changed the title Resteasy Reactive Multipart fails if filenName is not set for a file part Resteasy Reactive Multipart fails if fileName is not set for a file part Oct 21, 2021
@andreas-eberle
Copy link
Contributor Author

Well, the RFC states that it is allowed to omit the filename:

   ... The file name isn't mandatory
   for cases where the file name isn't available or is meaningless or
   private; this might result, for example, when selection or drag-and-
   drop is used or when the form data content is streamed directly from
   a device. ...

https://datatracker.ietf.org/doc/html/rfc7578#section-4.2

In our case, the file name is also meaningless. As a workaround, we now just specify some dummy value that we ignore on the server side.

@geoand
Copy link
Contributor

geoand commented Nov 3, 2021

The thing is that when fileName is missing, we don't know that the input should be written to disk.

@andreas-eberle
Copy link
Contributor Author

Why do you think so? Couldn't you use the content type to determine this? You could write the stream to disk, when the content type of a part is application/octet-stream or something like this (e.g. jpeg etc.).

@geoand
Copy link
Contributor

geoand commented Nov 3, 2021

That sounds like a hack TBH

@andreas-eberle
Copy link
Contributor Author

Why do you think so? I mean the content type specifies what is in the field. To me the filename check looks more like a hack. That fields does not have to be there. But the content type has to be specified and specifies what the actual type of the field is.

@stuartwdouglas
Copy link
Member

So we could kinda make this work by looking at the content type, as if it is anything other than 'text/plain' (the default) we could likely assume it was a file, however uploading text files would still fail.

I guess we could attempt to somehow explicitly tell the parser which fields are files based on the name.

@andreas-eberle
Copy link
Contributor Author

What if you combine both ideas? I mean in the case there is a filename, go with it and handle it like you do at the moment. And if there is no filename but the content type is binary (octet-stream, image, ...), also handle it like a file. What do you think about that?

@stuartwdouglas
Copy link
Member

I think if we are going to fix this it needs to be done via the application somehow notify the parser that a given field is a file, anything else will be somewhat hacky and have the potential to cause other problems.

@teedjay
Copy link

teedjay commented Apr 15, 2022

I have the same issue in one of our apps, and it's really the only thing blocking from switching over to reactive (it works today using classic resteasy multipart).

Since our application already specify the multipart field as type FileUpload, there should be enough information 🤔

@NotNull
@RestForm
@PartType(MediaType.APPLICATION_OCTET_STREAM)
public FileUpload file;

@rvansa
Copy link
Contributor

rvansa commented Dec 5, 2022

I've just ran into this as well. In my case I can control the client; however can't Quarkus just store stuff in a file any time the consuming method uses a FileUpload parameter?

@pedroigor
Copy link
Contributor

In Keycloak, we are also facing the same issue. For us, it will be a breaking change (and as mentioned before, not 100% aligned with the specs) if we start forcing the filename to be set.

In our case, the file name is also meaningless.

@geoand
Copy link
Contributor

geoand commented Dec 6, 2022

I will have another look tomorrow

@geoand
Copy link
Contributor

geoand commented Dec 6, 2022

I think if we are going to fix this it needs to be done via the application somehow notify the parser that a given field is a file, anything else will be somewhat hacky and have the potential to cause other problems.

This seems like the most robust approach, so I'll look into it

@geoand
Copy link
Contributor

geoand commented Dec 6, 2022

I think if we are going to fix this it needs to be done via the application somehow notify the parser that a given field is a file, anything else will be somewhat hacky and have the potential to cause other problems.

This seems like the most robust approach, so I'll look into it

This does seem to work, I'll clean up what I have and open a PR tomorrow

geoand added a commit to geoand/quarkus that referenced this issue Dec 7, 2022
…ts it

With this change, if the JAX-RS endpoint expects a multipart request to
contain a file, then that file is always written to disk regardless
of whether the content-disposition contains the filename attribute
of not.

Fixes: quarkusio#20938
@geoand
Copy link
Contributor

geoand commented Dec 7, 2022

#29716 takes care of the issue

geoand added a commit to geoand/quarkus that referenced this issue Dec 7, 2022
…ts it

With this change, if the JAX-RS endpoint expects a multipart request to
contain a file, then that file is always written to disk regardless
of whether the content-disposition contains the filename attribute
of not.

Fixes: quarkusio#20938
@rvansa
Copy link
Contributor

rvansa commented Dec 7, 2022

Thanks!

@geoand
Copy link
Contributor

geoand commented Dec 7, 2022

🙏🏼

@andreas-eberle
Copy link
Contributor Author

@geoand : Thanks for the fix! Can you explain what we have to do in the future? Do we need a new annotation?

@geoand
Copy link
Contributor

geoand commented Dec 7, 2022

Users don't have to do anything.

If you use one of the supported file types, then RR will always save the contents of that part of the multipart request as a file

@andreas-eberle
Copy link
Contributor Author

That's great news! Thanks a lot for providing this fix!

@geoand
Copy link
Contributor

geoand commented Dec 7, 2022

🙏

geoand added a commit to geoand/quarkus that referenced this issue Dec 7, 2022
…ts it

With this change, if the JAX-RS endpoint expects a multipart request to
contain a file, then that file is always written to disk regardless
of whether the content-disposition contains the filename attribute
of not.

Fixes: quarkusio#20938
geoand added a commit that referenced this issue Dec 8, 2022
Ensure that file is written on disk for multipart when endpoint expects it
@quarkus-bot quarkus-bot bot added this to the 2.16 - main milestone Dec 8, 2022
essobedo pushed a commit to essobedo/quarkus that referenced this issue Dec 12, 2022
…ts it

With this change, if the JAX-RS endpoint expects a multipart request to
contain a file, then that file is always written to disk regardless
of whether the content-disposition contains the filename attribute
of not.

Fixes: quarkusio#20938
@gsmet gsmet modified the milestones: 2.16 - main, 2.13.6.Final Dec 14, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue Dec 14, 2022
…ts it

With this change, if the JAX-RS endpoint expects a multipart request to
contain a file, then that file is always written to disk regardless
of whether the content-disposition contains the filename attribute
of not.

Fixes: quarkusio#20938
(cherry picked from commit ec2a4a4)
@gsmet gsmet modified the milestones: 2.13.6.Final, 2.15.1.Final Dec 20, 2022
gsmet pushed a commit to gsmet/quarkus that referenced this issue Dec 20, 2022
…ts it

With this change, if the JAX-RS endpoint expects a multipart request to
contain a file, then that file is always written to disk regardless
of whether the content-disposition contains the filename attribute
of not.

Fixes: quarkusio#20938
(cherry picked from commit ec2a4a4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/rest kind/bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants