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

Make annotated services reject the multipart requests that contain an uninjectable file upload #5549

Open
trustin opened this issue Mar 30, 2024 · 2 comments · May be fixed by #5569
Open

Make annotated services reject the multipart requests that contain an uninjectable file upload #5549

trustin opened this issue Mar 30, 2024 · 2 comments · May be fixed by #5569
Milestone

Comments

@trustin
Copy link
Member

trustin commented Mar 30, 2024

Given the following service:

@Consumes("multipart/form-data")
public class FileUploadService {
    @Post("/upload")
    public HttpResponse upload(@Param String text, @Param File file) throws IOException {
        ...
    }
}

A client can send a multipart request that contains more than one file, even if /upload expects the request to contains one single file in the file field. Regardless of whether the received multipart request contains the file field or not, FileAggregatedMultipart.aggregateMultipart() will decode and store all file fields into the upload location (filesystem). It means, a client can incur unnecessary disk writes by sending the multipart requests like the following:

  • Two file uploads in the field file and file2. (not an error but unnecessary disk write for file2)
  • One file upload in the field foo. (an error with completely unnecessary disk write)

We could:

  • collect the list of required parameters during annotation scanning,
  • pass the list of required parameters to the resolvers,
  • so that the resolvers reject the requests with missing or unnecessary fields.

We might reject the requests with unnecessary fields only for a certain type of requests such as multipart file uploads, though, because sending an unnecessary fields are often harmless.

Alternatively, we might want to silently discard the body part of unnecessary fields, given that we limit the total content length anyway.

@trustin trustin added this to the 1.28.0 milestone Mar 30, 2024
@Bue-von-hon
Copy link
Contributor

If it's not urgent, I'll give it a try.

@trustin
Copy link
Member Author

trustin commented Apr 3, 2024

Sure. Why not? 😄

Bue-von-hon added a commit to Bue-von-hon/armeria that referenced this issue Apr 5, 2024
Motivation:
this resolves line#5549

Modifications:
- Fix(AnnotatedServiceMultipartTest): Add test for upload multipart file with unexpected parameters in AnnotatedServiceMultipartTest.
- Fix(AnnotatedService): Fix to include a list of intended parameters in the ServiceRequestContext.
- Fix(FileAggregatedMultipart): Fix to check if any variables are passed in the list of intended parameters and throw an acceptance if not.

Result:
Multipart requests with unintended parameters will no longer create files.
Bue-von-hon added a commit to Bue-von-hon/armeria that referenced this issue Apr 5, 2024
Motivation:
this resolves line#5549

Modifications:
- Fix(AnnotatedServiceMultipartTest): Add test for upload multipart file with unexpected parameters in AnnotatedServiceMultipartTest.
- Fix(AnnotatedService): Fix to include a list of intended parameters in the ServiceRequestContext.
- Fix(FileAggregatedMultipart): Fix to check if any variables are passed in the list of intended parameters and throw an acceptance if not.

Result:
Multipart requests with unintended parameters will no longer create files.
@jrhee17 jrhee17 modified the milestones: 1.28.0, 1.29.0 Apr 8, 2024
Bue-von-hon added a commit to Bue-von-hon/armeria that referenced this issue May 17, 2024
Motivation:
this resolves line#5549

Modifications:
- Fix(AnnotatedServiceMultipartTest): Add test for upload multipart file with unexpected parameters in AnnotatedServiceMultipartTest.
- Fix(AnnotatedService): Fix to include a list of intended parameters in the ServiceRequestContext.
- Fix(FileAggregatedMultipart): Fix to check if any variables are passed in the list of intended parameters and throw an acceptance if not.

Result:
Multipart requests with unintended parameters will no longer create files.
Bue-von-hon added a commit to Bue-von-hon/armeria that referenced this issue May 17, 2024
Motivation:
this resolves line#5549

Modifications:
- Fix(AnnotatedServiceMultipartTest): Add test for upload multipart file with unexpected parameters in AnnotatedServiceMultipartTest.
- Fix(AnnotatedService): Fix to include a list of intended parameters in the ServiceRequestContext.
- Fix(FileAggregatedMultipart): Fix to check if any variables are passed in the list of intended parameters and throw an acceptance if not.

Result:
Multipart requests with unintended parameters will no longer create files.
Bue-von-hon added a commit to Bue-von-hon/armeria that referenced this issue May 17, 2024
Motivation:
this resolves line#5549

Modifications:
- Fix(AnnotatedServiceMultipartTest): Add test for upload multipart file with unexpected parameters in AnnotatedServiceMultipartTest.
- Fix(AnnotatedService): Fix to include a list of intended parameters in the ServiceRequestContext.
- Fix(FileAggregatedMultipart): Fix to check if any variables are passed in the list of intended parameters and throw an acceptance if not.

Result:
Multipart requests with unintended parameters will no longer create files.
Bue-von-hon added a commit to Bue-von-hon/armeria that referenced this issue May 17, 2024
Motivation:
this resolves line#5549

Modifications:
- Fix(AnnotatedServiceMultipartTest): Add test for upload multipart file with unexpected parameters in AnnotatedServiceMultipartTest.
- Fix(AnnotatedService): Fix to include a list of intended parameters in the ServiceRequestContext.
- Fix(FileAggregatedMultipart): Fix to check if any variables are passed in the list of intended parameters and throw an acceptance if not.

Result:
Multipart requests with unintended parameters will no longer create files.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants