Fix to reject any unintended parameters in multipart request. #5569

Bue-von-hon · 2024-04-05T07:55:48Z

Motivation:
this resolves #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 · 2024-04-05T08:12:33Z

I'm concerned that the behavior of pulling values from ServiceRequestContext is not clean.
Moreover, since the graphql service doesn't take parameters, I realized that I should have allowed all cases where the value is not set in the ServiceRequestContext.

github-actions · 2024-04-05T22:01:54Z

🔍 Build Scan® (commit: `5211efd`)

Job name	Status	Build Scan®
build-ubicloud-standard-8-jdk-8	✅	https://ge.armeria.dev/s/tjnbimrbdlo64
build-ubicloud-standard-8-jdk-21-snapshot-blockhound	✅	https://ge.armeria.dev/s/wjmbiavljk56k
build-ubicloud-standard-8-jdk-17-min-java-17-coverage	✅	https://ge.armeria.dev/s/isjuwmw7dnlhm
build-ubicloud-standard-8-jdk-17-min-java-11	✅	https://ge.armeria.dev/s/l3ugdtsejdgmy
build-ubicloud-standard-8-jdk-17-leak	✅	https://ge.armeria.dev/s/mzklvjqnuo5bs
build-ubicloud-standard-8-jdk-11	✅	https://ge.armeria.dev/s/xkxdhlfe5eiik
build-macos-12-jdk-21	❌ (failure)	https://ge.armeria.dev/s/gaplaivqhakg6

trustin · 2024-04-09T07:46:17Z

core/src/main/java/com/linecorp/armeria/internal/server/annotation/AnnotatedService.java

+        final AttributeKey<List<String>> PARAM_LIST_KEY = AttributeKey.valueOf(List.class, "names");
+        final List<String> names = resolvers.stream()
+                                            .map(AnnotatedValueResolver::httpElementName)
+                                            .collect(Collectors.toList());
+        ctx.setAttr(PARAM_LIST_KEY, names);


We could just pass the param list as an additional parameter of aggregateMultipart().

We could collect this information when creating an AnnotatedService instance, not every time service each request.

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.

…ceRequestContext

Bue-von-hon · 2024-05-17T08:57:43Z

Resolved all comments PTAL @trustin

Bue-von-hon · 2024-05-17T09:20:00Z

Fixed missing GPG signature.
But now I have a duplicate commit.
I'll fix it soon. 🥲

Bue-von-hon · 2024-05-20T01:04:17Z

Fixed missing GPG signature. But now I have a duplicate commit. I'll fix it soon. 🥲

Fixed it all. 🙂

minwoox

Thanks! I left my opinion. 😉

core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java

minwoox · 2024-05-22T04:58:40Z

core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java

        final Path destination = ctx.config().multipartUploadsLocation();
        return Multipart.from(req).collect(bodyPart -> {
            final String name = bodyPart.name();
            assert name != null;
+            if (!parameters.isEmpty() && !parameters.contains(name)) {


I prefer to silently discard it because it might already process all the necessary files.
Let's say that there are 10 files and the last one is the one that the service doesnt' need.
This will reject the request after processing all the necessary files.

Rather than rejecting it by throwing an exception (the loud way), I'll modify it to ignore unintended files (the quiet way)

📝 leaves memo for myself to remember later.

I think it's fine to discard them as long as we don't write them into filesystem. Do we?

…tion/DefaultAnnotatedService.java Co-authored-by: minux <[email protected]>

Motivation: If an unintended file was entered, it threw an exception and terminated. This behavior was frustrating to the user, so changed to a calmer method that simply ignores the file. Modification: - Fix(AnnotatedServiceMultipartTest): Change from BAD REQUEST to OK, and change the file name to be more distinguishable. - Fix(FileAggregatedMultipart): Add filtering - Fix(Multipart): Add filtering. I wanted to name the filtering method filter, but because the StreamMessage interface already has a filter function, and because the DefaultMultipart class implements both the multipart interface and the StreamMessage interface, I used a different name. Result: It no longer raises an acceptance. Instead, unintended files are filtered out before they are written to disk.

Bue-von-hon · 2024-06-19T11:21:06Z

Resolves all! PTAL

trustin · 2024-06-19T08:21:42Z

core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java

+     * @param predicate certain conditions
+     * @return multipart matching the condition
+     */
+    default Multipart filterBodyParts(Predicate<? super BodyPart> predicate) {


How about renaming to filter()?

trustin · 2024-06-19T08:22:58Z

core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java

+    private static boolean shouldSkip(BodyPart bodyPart, List<String> parameters) {
+        final String name = bodyPart.name();
+        assert name != null;
+        return !parameters.isEmpty() && !parameters.contains(name);


Maybe I misunderstood, but should we return true if parameters is empty?

trustin · 2024-06-20T13:03:40Z

core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java

        final Path destination = ctx.config().multipartUploadsLocation();
-        return Multipart.from(req).collect(bodyPart -> {
+        return Multipart.from(req)
+                        .filterBodyParts(bodyPart -> !shouldSkip(bodyPart, parameters)).collect(bodyPart -> {


If we always negate the result of shouldSkip(), we could maybe replace !shouldSkip() with shouldHandle()?

trustin · 2024-06-20T13:08:21Z

...test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java

+        final AggregatedHttpResponse response =
+                server.blockingWebClient().execute(multipart.toHttpRequest(path));
+        assertEquals(HttpStatus.OK, response.status());


This doesn't make sure the multipartFile3 has been either discarded or processed at all. What about writing a new endpoint in MyAnnotatedService that makes sure the file is not created for multipartFile3 in the directory that contains multipartFile1 and multipartFile2.

Bue-von-hon · 2024-07-15T06:01:39Z

``I've resolved code review. @trustin @minwoox @jrhee17 @ikhoon
I hold off on renaming the filterBodyParts method of the Multipart interface.
The method named filter exists in the StreamMessage interface because the DefaultMultipart class uses the StreamMessage interface and the Multipart interface.
If we change the name of the method, it will cause a conflict, so it's better to have different names to distinguish them.
Another option is to use composition.
Implementations are always open to change, so feel free to comment if there is a better way.

by the way, multipartSingleFile and multipartFileList test broken in my local. (java/com/linecorp/armeria/graphql/GraphqlTest.java)
I think the changes I added might have had an impact, but I'm not sure.
I checked the configure method of that class, expecting to find something related to operations, but there isn't.

GraphqlTest > multipartFileList() FAILED
    org.springframework.web.client.HttpClientErrorException$BadRequest: 400 Bad Request: "'operations' form field is missing"
        at app//org.springframework.web.client.HttpClientErrorException.create(HttpClientErrorException.java:103)
        at app//org.springframework.web.client.DefaultResponseErrorHandler.handleError(DefaultResponseErrorHandler.java:183)
        at app//org.springframework.web.client.DefaultResponseErrorHandler.handleError(DefaultResponseErrorHandler.java:137)
        at app//org.springframework.web.client.ResponseErrorHandler.handleError(ResponseErrorHandler.java:63)
        at app//org.springframework.web.client.RestTemplate.handleResponse(RestTemplate.java:942)
        at app//org.springframework.web.client.RestTemplate.doExecute(RestTemplate.java:891)
        at app//org.springframework.web.client.RestTemplate.execute(RestTemplate.java:830)
        at app//org.springframework.web.client.RestTemplate.postForEntity(RestTemplate.java:556)
        at app//com.linecorp.armeria.graphql.GraphqlTest.multipartFileList(GraphqlTest.java:135)

GraphqlTest > multipartSingleFile() FAILED
    org.springframework.web.client.HttpClientErrorException$BadRequest: 400 Bad Request: "'operations' form field is missing"
        at app//org.springframework.web.client.HttpClientErrorException.create(HttpClientErrorException.java:103)
        at app//org.springframework.web.client.DefaultResponseErrorHandler.handleError(DefaultResponseErrorHandler.java:183)
        at app//org.springframework.web.client.DefaultResponseErrorHandler.handleError(DefaultResponseErrorHandler.java:137)
        at app//org.springframework.web.client.ResponseErrorHandler.handleError(ResponseErrorHandler.java:63)
        at app//org.springframework.web.client.RestTemplate.handleResponse(RestTemplate.java:942)
        at app//org.springframework.web.client.RestTemplate.doExecute(RestTemplate.java:891)
        at app//org.springframework.web.client.RestTemplate.execute(RestTemplate.java:830)
        at app//org.springframework.web.client.RestTemplate.postForEntity(RestTemplate.java:556)
        at app//com.linecorp.armeria.graphql.GraphqlTest.multipartSingleFile(GraphqlTest.java:116)

Bue-von-hon · 2024-08-02T04:39:55Z

Since the graphql api had no parameters, the tests were failing.
To fix this, I modified it to not consider the parameter if it is empty.
PTAL @trustin @minwoox @jrhee17 @ikhoon

Since the request is processed and the file is deleted, change to testing with the response value

…into armeria-5549

Bue-von-hon · 2024-09-17T12:55:56Z

The file is deleted as soon as the request is processed, so the test was always succeeding.
This is now fixed by using the response value to validate.
PTAL @jrhee17 @minwoox @trustin @ikhoon

jrhee17

Looks good overall, left some minor suggestions 🙇

jrhee17 · 2024-09-19T05:51:15Z

core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java

Can you revert the file permission changes? ditto for the other files as well

jrhee17 · 2024-09-19T06:18:04Z

core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java

+     * @param predicate certain conditions
+     * @return multipart matching the condition
+     */
+    default Multipart filterBodyParts(Predicate<BodyPart> predicate) {


I think it's more intuitive to stick with names often used in fp

Suggested change

default Multipart filterBodyParts(Predicate<BodyPart> predicate) {

default Multipart filter(Predicate<BodyPart> predicate) {

jrhee17 · 2024-09-19T06:29:24Z

core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java

+    private static boolean shouldHandle(BodyPart bodyPart, List<String> parameters) {
+        final String name = bodyPart.name();
+        assert name != null;
+        if (parameters.isEmpty()) {


If an annotated service doesn't have any parameters, I think we'll run into the same issue.

What do you think of just accepting a Multipart to avoid ambiguity?

e.g.

public static CompletableFuture<FileAggregatedMultipart> aggregateMultipart( ServiceRequestContext ctx, HttpRequest req) { final Multipart multipart = Multipart.from(req); return aggregateMultipart(ctx, multipart); } public static CompletableFuture<FileAggregatedMultipart> aggregateMultipart( ServiceRequestContext ctx, Multipart multipart) { ...

and then, we can pass in the filtered Multipart from AnnotatedService

... private CompletionStage<HttpResponse> serve1(ServiceRequestContext ctx, HttpRequest req, AggregationType aggregationType) { final CompletableFuture<AggregatedResult> f; switch (aggregationType) { case MULTIPART: f = FileAggregatedMultipart.aggregateMultipart(ctx, Multipart.from(req).filter(bodyPart -> shouldHandle(bodyPart, parameters))) .thenApply(AggregatedResult::new); ...

...test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java

jrhee17 · 2024-09-19T06:47:43Z

...test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java

+        final AggregatedHttpResponse response =
+                server.blockingWebClient().execute(multipart.toHttpRequest(path));
+        assertEquals(HttpStatus.OK, response.status());
+        assertThatJson(response.contentUtf8())


In addition to checking the response, can we also check that the file has not been physically written?

e.g.

Setting the strategy to never remove automatically:

@TempDir static java.nio.file.Path sharedTempDir; @RegisterExtension static final ServerExtension server = new ServerExtension() { @Override protected void configure(ServerBuilder sb) throws Exception { sb.annotatedService("/", new MyAnnotatedService()); sb.decorator(LoggingService.newDecorator()); sb.multipartRemovalStrategy(MultipartRemovalStrategy.NEVER); sb.multipartUploadsLocation(sharedTempDir);

and then, check if any of the temporary files contains "qux3"

assertThat(sharedTempDir.resolve("complete").toFile().listFiles()) .noneSatisfy(file -> assertThat(file).content(StandardCharsets.UTF_8).isEqualTo("qux3"));

…tion/AnnotatedServiceMultipartTest.java Co-authored-by: jrhee17 <[email protected]>

github-actions bot requested review from ikhoon, jrhee17, minwoox and trustin April 5, 2024 07:56

Bue-von-hon marked this pull request as draft April 5, 2024 08:24

trustin reviewed Apr 9, 2024

View reviewed changes

github-actions bot added the Stale label May 10, 2024

Bue-von-hon added 2 commits May 17, 2024 17:47

Fix to use parameter lists instead of putting parameters in the Servi…

ed23db1

…ceRequestContext

Bue-von-hon force-pushed the armeria-5549 branch from c6863c2 to 961cdb4 Compare May 17, 2024 08:54

Bue-von-hon marked this pull request as ready for review May 17, 2024 08:56

github-actions bot removed the Stale label May 18, 2024

Bue-von-hon force-pushed the armeria-5549 branch from 961cdb4 to ed23db1 Compare May 20, 2024 01:03

minwoox reviewed May 22, 2024

View reviewed changes

Bue-von-hon marked this pull request as draft May 22, 2024 07:33

Bue-von-hon and others added 3 commits May 27, 2024 16:14

Update core/src/main/java/com/linecorp/armeria/internal/server/annota…

30c20f0

…tion/DefaultAnnotatedService.java Co-authored-by: minux <[email protected]>

Merge branch 'main' into armeria-5549

9a79b95

Bue-von-hon marked this pull request as ready for review June 19, 2024 11:19

Bue-von-hon requested review from minwoox and trustin June 19, 2024 11:20

trustin reviewed Jun 20, 2024

View reviewed changes

Bue-von-hon marked this pull request as draft July 5, 2024 05:21

Merge branch 'main' into armeria-5549

f3e68f1

Bue-von-hon added 2 commits July 8, 2024 12:44

Resolves code review

3f4eb6a

Merge remote-tracking branch 'downstream/armeria-5549' into armeria-5549

2543a9c

Bue-von-hon force-pushed the armeria-5549 branch from 10773e9 to 2543a9c Compare July 15, 2024 05:23

Fix lint

0497cd4

Bue-von-hon marked this pull request as ready for review July 15, 2024 06:02

Merge branch 'main' into armeria-5549

cdb1549

Bue-von-hon marked this pull request as draft August 2, 2024 01:34

Fix to return true if parameter is empty as graphql has no parameters

f2f4b6a

Bue-von-hon marked this pull request as ready for review August 2, 2024 04:39

Merge branch 'main' into armeria-5549

c6201c4

github-actions bot added the Stale label Sep 5, 2024

Merge branch 'line:main' into armeria-5549

c18bea1

github-actions bot removed the Stale label Sep 16, 2024

Bue-von-hon added 2 commits September 17, 2024 21:50

Fixing broken test

356f7ef

Since the request is processed and the file is deleted, change to testing with the response value

Merge branch 'armeria-5549' of https://github.com/Bue-von-hon/armeria …

b2683d3

…into armeria-5549

jrhee17 reviewed Sep 19, 2024

View reviewed changes

Merge branch 'main' into armeria-5549

0e9c7f6

Bue-von-hon marked this pull request as draft October 2, 2024 11:31

Bue-von-hon and others added 2 commits October 2, 2024 20:32

Update core/src/test/java/com/linecorp/armeria/internal/server/annota…

70bcdad

…tion/AnnotatedServiceMultipartTest.java Co-authored-by: jrhee17 <[email protected]>

Merge branch 'main' into armeria-5549

5211efd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix to reject any unintended parameters in multipart request. #5569

Fix to reject any unintended parameters in multipart request. #5569

Bue-von-hon commented Apr 5, 2024

Bue-von-hon commented Apr 5, 2024

github-actions bot commented Apr 5, 2024 •

edited

Loading

trustin Apr 9, 2024

Bue-von-hon commented May 17, 2024

Bue-von-hon commented May 17, 2024

Bue-von-hon commented May 20, 2024

minwoox left a comment

minwoox May 22, 2024

Bue-von-hon May 22, 2024

trustin Jun 7, 2024

Bue-von-hon commented Jun 19, 2024

trustin Jun 19, 2024

trustin Jun 19, 2024

trustin Jun 20, 2024

trustin Jun 20, 2024

Bue-von-hon commented Jul 15, 2024

Bue-von-hon commented Aug 2, 2024

Bue-von-hon commented Sep 17, 2024

jrhee17 left a comment

jrhee17 Sep 19, 2024

jrhee17 Sep 19, 2024

jrhee17 Sep 19, 2024

jrhee17 Sep 19, 2024

	default Multipart filterBodyParts(Predicate<BodyPart> predicate) {
	default Multipart filter(Predicate<BodyPart> predicate) {

Fix to reject any unintended parameters in multipart request. #5569

Are you sure you want to change the base?

Fix to reject any unintended parameters in multipart request. #5569

Conversation

Bue-von-hon commented Apr 5, 2024

Bue-von-hon commented Apr 5, 2024

github-actions bot commented Apr 5, 2024 • edited Loading

🔍 Build Scan® (commit: 5211efd)

Choose a reason for hiding this comment

Bue-von-hon commented May 17, 2024

Bue-von-hon commented May 17, 2024

Bue-von-hon commented May 20, 2024

minwoox left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bue-von-hon commented Jun 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bue-von-hon commented Jul 15, 2024

Bue-von-hon commented Aug 2, 2024

Bue-von-hon commented Sep 17, 2024

jrhee17 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 5, 2024 •

edited

Loading

🔍 Build Scan® (commit: `5211efd`)