Fix to reject any unintended parameters in multipart request. #5569
Conversation
|
I'm concerned that the behavior of pulling values from ServiceRequestContext is not clean. |
| 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); |
There was a problem hiding this comment.
- We could just pass the param list as an additional parameter of
aggregateMultipart(). - We could collect this information when creating an
AnnotatedServiceinstance, 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.
|
Resolved all comments PTAL @trustin |
|
Fixed missing GPG signature. |
Fixed it 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()); | |||
There was a problem hiding this comment.
nit:
| .collect(Collectors.toList()); | |
| .collect(toImmutableList()); |
| 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)) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Motivation:
this resolves #5549
Modifications:
Result:
Multipart requests with unintended parameters will no longer create files.