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.

Sign up for GitHub

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

Jump to bottom

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
291d8a3
Fix to reject any unintended parameters in multipart request.
Bue-von-hon Apr 5, 2024
ed23db1
Fix to use parameter lists instead of putting parameters in the Servi…
Bue-von-hon May 17, 2024
30c20f0
Update core/src/main/java/com/linecorp/armeria/internal/server/annota…
Bue-von-hon May 27, 2024
ad2c7e7
Change to elegant exception handling
Bue-von-hon Jun 19, 2024
9a79b95
Merge branch 'main' into armeria-5549
Bue-von-hon Jun 19, 2024
f3e68f1
Merge branch 'main' into armeria-5549
Bue-von-hon Jul 5, 2024
3f4eb6a
Resolves code review
Bue-von-hon Jul 8, 2024
2543a9c
Merge remote-tracking branch 'downstream/armeria-5549' into armeria-5549
Bue-von-hon Jul 8, 2024
0497cd4
Fix lint
Bue-von-hon Jul 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion core/src/main/java/com/linecorp/armeria/internal/server/FileAggregatedMultipart.java
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import java.nio.file.Files;
import java.nio.file.Path;
import java.nio.file.StandardCopyOption;
import java.util.Collections;
import java.util.List;
import java.util.Map.Entry;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutorService;
Expand Down Expand Up @@ -58,13 +60,20 @@ public ListMultimap<String, MultipartFile> files() {

public static CompletableFuture<FileAggregatedMultipart> aggregateMultipart(ServiceRequestContext ctx,
HttpRequest req) {
return aggregateMultipart(ctx, req, Collections.emptyList());
}

public static CompletableFuture<FileAggregatedMultipart> aggregateMultipart(
ServiceRequestContext ctx, HttpRequest req, List<String> parameters) {
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)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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();
Expand Down
8 changes: 7 additions & 1 deletion ...rc/main/java/com/linecorp/armeria/internal/server/annotation/DefaultAnnotatedService.java
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -108,6 +109,7 @@ final class DefaultAnnotatedService implements AnnotatedService {

private final ResponseType responseType;
private final boolean useBlockingTaskExecutor;
private final List<String> parameters;
@Nullable
private final String name;

Expand All @@ -130,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());
Bue-von-hon marked this conversation as resolved.
Show resolved Hide resolved

requireNonNull(exceptionHandlers, "exceptionHandlers");
if (exceptionHandlers.isEmpty()) {
Expand Down Expand Up @@ -318,7 +323,8 @@ private CompletionStage<HttpResponse> serve1(ServiceRequestContext ctx, HttpRequ
final CompletableFuture<AggregatedResult> f;
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);
Expand Down
20 changes: 20 additions & 0 deletions ...t/java/com/linecorp/armeria/internal/server/annotation/AnnotatedServiceMultipartTest.java
100644 → 100755
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
Loading