From 291d8a3632fc5a507b063170921c5f269bef6744 Mon Sep 17 00:00:00 2001 From: Bue-Von-Hun Date: Fri, 5 Apr 2024 16:52:21 +0900 Subject: [PATCH 1/7] Fix to reject any unintended parameters in multipart request. 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. --- .../server/FileAggregatedMultipart.java | 26 +++++++++++++++++++ .../annotation/DefaultAnnotatedService.java | 8 ++++++ .../AnnotatedServiceMultipartTest.java | 20 ++++++++++++++ 3 files changed, 54 insertions(+) diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java index bae2dd4e643..ca632d898d5 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java @@ -21,6 +21,8 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; +import java.util.Iterator; +import java.util.List; import java.util.Map.Entry; import java.util.concurrent.CompletableFuture; import java.util.concurrent.ExecutorService; @@ -37,6 +39,7 @@ import com.linecorp.armeria.server.ServiceRequestContext; import io.netty.channel.EventLoop; +import io.netty.util.AttributeKey; public final class FileAggregatedMultipart { private final ListMultimap params; @@ -65,6 +68,29 @@ public static CompletableFuture aggregateMultipart(Serv final String filename = bodyPart.filename(); final EventLoop eventLoop = ctx.eventLoop(); + final Iterator, Object>> attrs = ctx.attrs(); + boolean expectedValue = !attrs.hasNext(); + while (attrs.hasNext()) { + final Entry, Object> next = attrs.next(); + final Object nextValue = next.getValue(); + if (nextValue instanceof List) { + final List list = (List) nextValue; + for (Object obj : list) { + if (obj instanceof String) { + final String value = (String) obj; + if (name.equals(value)) { + expectedValue = true; + break; + } + } + } + } + } + + if (!expectedValue) { + throw new IllegalArgumentException("Unexpected parameter received: " + name); + } + if (filename != null) { final Path incompleteDir = destination.resolve("incomplete"); final ScheduledExecutorService executor = ctx.blockingTaskExecutor().withoutContext(); diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java index 14150bd2ba0..fe87ed56f4d 100644 --- a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java @@ -32,6 +32,7 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.ScheduledExecutorService; import java.util.function.Function; +import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -72,6 +73,8 @@ import com.linecorp.armeria.server.annotation.ResponseConverterFunction; import com.linecorp.armeria.server.annotation.ServiceName; +import io.netty.util.AttributeKey; + /** * An {@link HttpService} which is defined by a {@link Path} or HTTP method annotations. * This class is not supposed to be instantiated by a user. Please check out the documentation @@ -316,6 +319,11 @@ private HttpResponse serve0(ServiceRequestContext ctx, HttpRequest req) { private CompletionStage serve1(ServiceRequestContext ctx, HttpRequest req, AggregationType aggregationType) { final CompletableFuture f; + final AttributeKey> PARAM_LIST_KEY = AttributeKey.valueOf(List.class, "names"); + final List names = resolvers.stream() + .map(AnnotatedValueResolver::httpElementName) + .collect(Collectors.toList()); + ctx.setAttr(PARAM_LIST_KEY, names); switch (aggregationType) { case MULTIPART: f = FileAggregatedMultipart.aggregateMultipart(ctx, req).thenApply(AggregatedResult::new); diff --git a/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java b/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java index 495185e91cb..0d046180337 100644 --- a/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java @@ -19,6 +19,7 @@ import static com.google.common.collect.ImmutableMap.toImmutableMap; import static net.javacrumbs.jsonunit.fluent.JsonFluentAssert.assertThatJson; import static org.assertj.core.api.Assertions.assertThat; +import static org.junit.jupiter.api.Assertions.assertEquals; import java.io.File; import java.io.IOException; @@ -89,6 +90,25 @@ void testUploadFile(String path) throws Exception { "\"param1\":\"armeria\"}"); } + @ParameterizedTest + @ValueSource(strings = "/uploadWithFileParam") + void testUploadFileWithUnexpectedParameters(String path) throws Exception { + final Multipart multipart = Multipart.of( + BodyPart.of(ContentDisposition.of("form-data", "file1", "foo.txt"), "foo"), + BodyPart.of(ContentDisposition.of("form-data", "path1", "bar.txt"), "bar"), + BodyPart.of(ContentDisposition.of("form-data", "multipartFile1", "qux.txt"), "qux"), + BodyPart.of(ContentDisposition.of("form-data", "multipartFile2", "quz.txt"), + MediaType.PLAIN_TEXT, "quz"), + // Unexpected parameter: multipartFile3 + BodyPart.of(ContentDisposition.of("form-data", "multipartFile3", "qux.txt"), "qux"), + BodyPart.of(ContentDisposition.of("form-data", "param1"), "armeria") + + ); + final AggregatedHttpResponse response = + server.blockingWebClient().execute(multipart.toHttpRequest(path)); + assertEquals(HttpStatus.BAD_REQUEST, response.status()); + } + @Test void emptyBodyPart() { final Multipart multipart = Multipart.of(); From ed23db18ca7d083a66a9a6f6d6ae9bd7f677dd18 Mon Sep 17 00:00:00 2001 From: Bue-Von-Hun Date: Fri, 17 May 2024 17:03:09 +0900 Subject: [PATCH 2/7] Fix to use parameter lists instead of putting parameters in the ServiceRequestContext --- .../server/FileAggregatedMultipart.java | 35 +++++-------------- .../annotation/DefaultAnnotatedService.java | 14 ++++---- .../AnnotatedServiceMultipartTest.java | 0 3 files changed, 15 insertions(+), 34 deletions(-) mode change 100644 => 100755 core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java mode change 100644 => 100755 core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java mode change 100644 => 100755 core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java old mode 100644 new mode 100755 index ca632d898d5..92b55c720dd --- a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java @@ -21,7 +21,7 @@ import java.nio.file.Files; import java.nio.file.Path; import java.nio.file.StandardCopyOption; -import java.util.Iterator; +import java.util.Collections; import java.util.List; import java.util.Map.Entry; import java.util.concurrent.CompletableFuture; @@ -39,7 +39,6 @@ import com.linecorp.armeria.server.ServiceRequestContext; import io.netty.channel.EventLoop; -import io.netty.util.AttributeKey; public final class FileAggregatedMultipart { private final ListMultimap params; @@ -61,36 +60,20 @@ public ListMultimap files() { public static CompletableFuture aggregateMultipart(ServiceRequestContext ctx, HttpRequest req) { + return aggregateMultipart(ctx, req, Collections.emptyList()); + } + + public static CompletableFuture aggregateMultipart( + ServiceRequestContext ctx, HttpRequest req, List parameters) { final Path destination = ctx.config().multipartUploadsLocation(); return Multipart.from(req).collect(bodyPart -> { final String name = bodyPart.name(); assert name != null; - final String filename = bodyPart.filename(); - final EventLoop eventLoop = ctx.eventLoop(); - - final Iterator, Object>> attrs = ctx.attrs(); - boolean expectedValue = !attrs.hasNext(); - while (attrs.hasNext()) { - final Entry, Object> next = attrs.next(); - final Object nextValue = next.getValue(); - if (nextValue instanceof List) { - final List list = (List) nextValue; - for (Object obj : list) { - if (obj instanceof String) { - final String value = (String) obj; - if (name.equals(value)) { - expectedValue = true; - break; - } - } - } - } - } - - if (!expectedValue) { + if (!parameters.isEmpty() && !parameters.contains(name)) { throw new IllegalArgumentException("Unexpected parameter received: " + name); } - + final String filename = bodyPart.filename(); + final EventLoop eventLoop = ctx.eventLoop(); if (filename != null) { final Path incompleteDir = destination.resolve("incomplete"); final ScheduledExecutorService executor = ctx.blockingTaskExecutor().withoutContext(); diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java old mode 100644 new mode 100755 index fe87ed56f4d..1f232334916 --- a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java @@ -73,8 +73,6 @@ import com.linecorp.armeria.server.annotation.ResponseConverterFunction; import com.linecorp.armeria.server.annotation.ServiceName; -import io.netty.util.AttributeKey; - /** * An {@link HttpService} which is defined by a {@link Path} or HTTP method annotations. * This class is not supposed to be instantiated by a user. Please check out the documentation @@ -111,6 +109,7 @@ final class DefaultAnnotatedService implements AnnotatedService { private final ResponseType responseType; private final boolean useBlockingTaskExecutor; + private final List parameters; @Nullable private final String name; @@ -133,6 +132,9 @@ final class DefaultAnnotatedService implements AnnotatedService { method.getDeclaringClass().getSimpleName(), method.getName()); isKotlinSuspendingMethod = KotlinUtil.isSuspendingFunction(method); this.resolvers = requireNonNull(resolvers, "resolvers"); + parameters = resolvers.stream() + .map(AnnotatedValueResolver::httpElementName) + .collect(Collectors.toList()); requireNonNull(exceptionHandlers, "exceptionHandlers"); if (exceptionHandlers.isEmpty()) { @@ -319,14 +321,10 @@ private HttpResponse serve0(ServiceRequestContext ctx, HttpRequest req) { private CompletionStage serve1(ServiceRequestContext ctx, HttpRequest req, AggregationType aggregationType) { final CompletableFuture f; - final AttributeKey> PARAM_LIST_KEY = AttributeKey.valueOf(List.class, "names"); - final List names = resolvers.stream() - .map(AnnotatedValueResolver::httpElementName) - .collect(Collectors.toList()); - ctx.setAttr(PARAM_LIST_KEY, names); switch (aggregationType) { case MULTIPART: - f = FileAggregatedMultipart.aggregateMultipart(ctx, req).thenApply(AggregatedResult::new); + f = FileAggregatedMultipart.aggregateMultipart(ctx, req, parameters) + .thenApply(AggregatedResult::new); break; case ALL: f = req.aggregate().thenApply(AggregatedResult::new); diff --git a/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java b/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java old mode 100644 new mode 100755 From 30c20f04bfa4f68e0dc6b22e29c40c6f4c415986 Mon Sep 17 00:00:00 2001 From: kimtaehun <46879264+Bue-von-hon@users.noreply.github.com> Date: Mon, 27 May 2024 16:14:04 +0900 Subject: [PATCH 3/7] Update core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java Co-authored-by: minux --- .../internal/server/annotation/DefaultAnnotatedService.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java index 1f232334916..83593dd856a 100755 --- a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java @@ -134,7 +134,7 @@ final class DefaultAnnotatedService implements AnnotatedService { this.resolvers = requireNonNull(resolvers, "resolvers"); parameters = resolvers.stream() .map(AnnotatedValueResolver::httpElementName) - .collect(Collectors.toList()); + .collect(toImmutableList()); requireNonNull(exceptionHandlers, "exceptionHandlers"); if (exceptionHandlers.isEmpty()) { From ad2c7e71529efeb0ff5ebfb8efad40bf5ab99698 Mon Sep 17 00:00:00 2001 From: Bue-Von-Hun Date: Wed, 19 Jun 2024 14:43:01 +0900 Subject: [PATCH 4/7] Change to elegant exception handling 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. --- .../armeria/common/multipart/Multipart.java | 13 +++++++++++++ .../internal/server/FileAggregatedMultipart.java | 13 +++++++++---- .../server/annotation/DefaultAnnotatedService.java | 2 +- .../annotation/AnnotatedServiceMultipartTest.java | 4 ++-- 4 files changed, 25 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java b/core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java index 3b264fbae5a..027749870e8 100644 --- a/core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java +++ b/core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java @@ -23,6 +23,7 @@ import java.util.List; import java.util.concurrent.CompletableFuture; import java.util.function.Function; +import java.util.function.Predicate; import org.reactivestreams.Publisher; import org.reactivestreams.Subscriber; @@ -432,4 +433,16 @@ default CompletableFuture> collect( SubscriptionOption... options) { return bodyParts().mapAsync(function).collect(options); } + + /** + * Make only those that meet certain conditions in a body part a multipart. + * + * @param predicate certain conditions + * @return multipart matching the condition + */ + default Multipart filterBodyParts(Predicate predicate) { + requireNonNull(predicate, "predicate"); + final StreamMessage filteredParts = bodyParts().filter(predicate); + return new DefaultMultipart(boundary(), filteredParts); + } } diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java index 92b55c720dd..a484598d2d8 100755 --- a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java @@ -34,6 +34,7 @@ import com.linecorp.armeria.common.HttpData; import com.linecorp.armeria.common.HttpRequest; +import com.linecorp.armeria.common.multipart.BodyPart; import com.linecorp.armeria.common.multipart.Multipart; import com.linecorp.armeria.common.multipart.MultipartFile; import com.linecorp.armeria.server.ServiceRequestContext; @@ -58,6 +59,12 @@ public ListMultimap files() { return files; } + private static boolean shouldSkip(BodyPart bodyPart, List parameters) { + final String name = bodyPart.name(); + assert name != null; + return !parameters.isEmpty() && !parameters.contains(name); + } + public static CompletableFuture aggregateMultipart(ServiceRequestContext ctx, HttpRequest req) { return aggregateMultipart(ctx, req, Collections.emptyList()); @@ -66,12 +73,10 @@ public static CompletableFuture aggregateMultipart(Serv public static CompletableFuture aggregateMultipart( ServiceRequestContext ctx, HttpRequest req, List parameters) { final Path destination = ctx.config().multipartUploadsLocation(); - return Multipart.from(req).collect(bodyPart -> { + return Multipart.from(req) + .filterBodyParts(bodyPart -> !shouldSkip(bodyPart, parameters)).collect(bodyPart -> { final String name = bodyPart.name(); assert name != null; - if (!parameters.isEmpty() && !parameters.contains(name)) { - throw new IllegalArgumentException("Unexpected parameter received: " + name); - } final String filename = bodyPart.filename(); final EventLoop eventLoop = ctx.eventLoop(); if (filename != null) { diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java index 83593dd856a..54c3a924ab5 100755 --- a/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java @@ -17,6 +17,7 @@ package com.linecorp.armeria.internal.server.annotation; import static com.google.common.base.Preconditions.checkArgument; +import static com.google.common.collect.ImmutableList.toImmutableList; import static com.linecorp.armeria.internal.server.annotation.ResponseConverterFunctionUtil.newResponseConverter; import static java.util.Objects.requireNonNull; @@ -32,7 +33,6 @@ import java.util.concurrent.ExecutorService; import java.util.concurrent.ScheduledExecutorService; import java.util.function.Function; -import java.util.stream.Collectors; import org.slf4j.Logger; import org.slf4j.LoggerFactory; diff --git a/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java b/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java index 0d046180337..51262a87b3d 100755 --- a/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java @@ -100,13 +100,13 @@ void testUploadFileWithUnexpectedParameters(String path) throws Exception { BodyPart.of(ContentDisposition.of("form-data", "multipartFile2", "quz.txt"), MediaType.PLAIN_TEXT, "quz"), // Unexpected parameter: multipartFile3 - BodyPart.of(ContentDisposition.of("form-data", "multipartFile3", "qux.txt"), "qux"), + BodyPart.of(ContentDisposition.of("form-data", "multipartFile3", "qux3.txt"), "qux3"), BodyPart.of(ContentDisposition.of("form-data", "param1"), "armeria") ); final AggregatedHttpResponse response = server.blockingWebClient().execute(multipart.toHttpRequest(path)); - assertEquals(HttpStatus.BAD_REQUEST, response.status()); + assertEquals(HttpStatus.OK, response.status()); } @Test From 3f4eb6a543aa42e672a9e40ecfb7d992d9a36f4c Mon Sep 17 00:00:00 2001 From: Bue-Von-Hun Date: Mon, 8 Jul 2024 12:44:52 +0900 Subject: [PATCH 5/7] Resolves code review --- .../armeria/common/multipart/Multipart.java | 2 +- .../server/FileAggregatedMultipart.java | 6 +- .../AnnotatedServiceMultipartTest.java | 55 ++++++++++++++++++- 3 files changed, 58 insertions(+), 5 deletions(-) diff --git a/core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java b/core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java index 027749870e8..8c36e48aed0 100644 --- a/core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java +++ b/core/src/main/java/com/linecorp/armeria/common/multipart/Multipart.java @@ -440,7 +440,7 @@ default CompletableFuture> collect( * @param predicate certain conditions * @return multipart matching the condition */ - default Multipart filterBodyParts(Predicate predicate) { + default Multipart filterBodyParts(Predicate predicate) { requireNonNull(predicate, "predicate"); final StreamMessage filteredParts = bodyParts().filter(predicate); return new DefaultMultipart(boundary(), filteredParts); diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java index a484598d2d8..00f71dc02c9 100755 --- a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java @@ -59,10 +59,10 @@ public ListMultimap files() { return files; } - private static boolean shouldSkip(BodyPart bodyPart, List parameters) { + private static boolean shouldHandle(BodyPart bodyPart, List parameters) { final String name = bodyPart.name(); assert name != null; - return !parameters.isEmpty() && !parameters.contains(name); + return !parameters.isEmpty() && parameters.contains(name); } public static CompletableFuture aggregateMultipart(ServiceRequestContext ctx, @@ -74,7 +74,7 @@ public static CompletableFuture aggregateMultipart( ServiceRequestContext ctx, HttpRequest req, List parameters) { final Path destination = ctx.config().multipartUploadsLocation(); return Multipart.from(req) - .filterBodyParts(bodyPart -> !shouldSkip(bodyPart, parameters)).collect(bodyPart -> { + .filterBodyParts(bodyPart -> shouldHandle(bodyPart, parameters)).collect(bodyPart -> { final String name = bodyPart.name(); assert name != null; final String filename = bodyPart.filename(); diff --git a/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java b/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java index 51262a87b3d..cfeb0bb68fb 100755 --- a/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java @@ -20,11 +20,15 @@ import static net.javacrumbs.jsonunit.fluent.JsonFluentAssert.assertThatJson; import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import java.io.File; import java.io.IOException; import java.nio.charset.StandardCharsets; +import java.util.List; +import java.util.Optional; import java.util.function.Function; +import java.util.stream.Stream; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.RegisterExtension; @@ -49,6 +53,8 @@ import com.linecorp.armeria.common.multipart.Multipart; import com.linecorp.armeria.common.multipart.MultipartFile; import com.linecorp.armeria.server.ServerBuilder; +import com.linecorp.armeria.server.ServerConfig; +import com.linecorp.armeria.server.ServiceConfig; import com.linecorp.armeria.server.annotation.Blocking; import com.linecorp.armeria.server.annotation.Consumes; import com.linecorp.armeria.server.annotation.Param; @@ -91,7 +97,7 @@ void testUploadFile(String path) throws Exception { } @ParameterizedTest - @ValueSource(strings = "/uploadWithFileParam") + @ValueSource(strings = "/uploadWithUnintendedFileParam") void testUploadFileWithUnexpectedParameters(String path) throws Exception { final Multipart multipart = Multipart.of( BodyPart.of(ContentDisposition.of("form-data", "file1", "foo.txt"), "foo"), @@ -107,6 +113,22 @@ void testUploadFileWithUnexpectedParameters(String path) throws Exception { final AggregatedHttpResponse response = server.blockingWebClient().execute(multipart.toHttpRequest(path)); assertEquals(HttpStatus.OK, response.status()); + final List serviceConfigs = server.server().config().serviceConfigs(); + final Optional optionalDestination = serviceConfigs.stream() + .map(ServiceConfig::multipartUploadsLocation) + .reduce((first, second) -> second); + optionalDestination.ifPresent(destination -> { + final java.nio.file.Path completeDir = destination.resolve("complete"); + try (Stream paths = java.nio.file.Files.walk(completeDir)) { + paths.filter(java.nio.file.Files::isRegularFile).forEach(file -> { + try (Stream lines = java.nio.file.Files.lines(file)) { + lines.forEach(name -> assertNotEquals("multipartFile3", name)); + } catch (Exception ignored) { + } + }); + } catch (Exception ignored) { + } + }); } @Test @@ -187,6 +209,37 @@ public HttpResponse uploadWithFileParam(@Param File file1, @Param java.nio.file. return HttpResponse.ofJson(content); } + @Blocking + @Post + @Path("/uploadWithUnintendedFileParam") + public HttpResponse uploadWithUnintendedFileParam(@Param File file1, @Param java.nio.file.Path path1, + MultipartFile multipartFile1, + @Param MultipartFile multipartFile2, + @Param String param1) throws IOException { + final String file1Content = Files.asCharSource(file1, StandardCharsets.UTF_8).read(); + final String path1Content = Files.asCharSource(path1.toFile(), StandardCharsets.UTF_8) + .read(); + final MediaType multipartFile1ContentType = multipartFile1.headers().contentType(); + final String multipartFile1Content = + Files.asCharSource(multipartFile1.file(), StandardCharsets.UTF_8) + .read(); + final MediaType multipartFile2ContentType = multipartFile2.headers().contentType(); + final String multipartFile2Content = + Files.asCharSource(multipartFile2.file(), StandardCharsets.UTF_8) + .read(); + final ImmutableMap content = + ImmutableMap.of("file1", file1Content, + "path1", path1Content, + "multipartFile1", + multipartFile1.filename() + '_' + multipartFile1Content + + " (" + multipartFile1ContentType + ')', + "multipartFile2", + multipartFile2.filename() + '_' + multipartFile2Content + + " (" + multipartFile2ContentType + ')', + "param1", param1); + return HttpResponse.ofJson(content); + } + @Post @Path("/uploadWithMultipartObject") public HttpResponse uploadWithMultipartObject(Multipart multipart) { From 0497cd420ac271a72da99efb9516d7d42f63cd66 Mon Sep 17 00:00:00 2001 From: Bue-Von-Hun Date: Mon, 15 Jul 2024 14:55:26 +0900 Subject: [PATCH 6/7] Fix lint --- .../server/annotation/AnnotatedServiceMultipartTest.java | 1 - 1 file changed, 1 deletion(-) diff --git a/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java b/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java index cfeb0bb68fb..0ff7c85b2fa 100755 --- a/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java +++ b/core/src/test/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java @@ -53,7 +53,6 @@ import com.linecorp.armeria.common.multipart.Multipart; import com.linecorp.armeria.common.multipart.MultipartFile; import com.linecorp.armeria.server.ServerBuilder; -import com.linecorp.armeria.server.ServerConfig; import com.linecorp.armeria.server.ServiceConfig; import com.linecorp.armeria.server.annotation.Blocking; import com.linecorp.armeria.server.annotation.Consumes; From f2f4b6aff402dc70743fe47f2a13ce0591e40cb0 Mon Sep 17 00:00:00 2001 From: Bue-Von-Hun Date: Fri, 2 Aug 2024 13:03:48 +0900 Subject: [PATCH 7/7] Fix to return true if parameter is empty as graphql has no parameters --- .../armeria/internal/server/FileAggregatedMultipart.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java index 1604d062428..14984a640e1 100755 --- a/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java +++ b/core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java @@ -67,7 +67,10 @@ public ListMultimap files() { private static boolean shouldHandle(BodyPart bodyPart, List parameters) { final String name = bodyPart.name(); assert name != null; - return !parameters.isEmpty() && parameters.contains(name); + if (parameters.isEmpty()) { + return true; + } + return parameters.contains(name); } public static CompletableFuture aggregateMultipart(ServiceRequestContext ctx,