Affects: Spring Boot 2.7.5

Hello,

I recently realised that one of my GET endpoints has issues if the values passed in the query params contain commas (,). Basically, I expect to be able to embed a comma into a single value being passed into a query param, however, the receiving end must define the data type as List<String> & Spring splits the value into 2 rather than keeping it in one piece.

To give an example, assume you have this endpoint:

@GetMapping
public ResponseEntity<List<String>> getSomeResource(@RequestParam("res") List<String> resources) {
    return ResponseEntity.ok(resources);
}

If you hit it with this request:

curl http://localhost:8080/test?res=xx%2Cyy

then you get back ["xx","yy"], which is exactly the same result as requesting curl http://localhost:8080/test?res=xx,yy, but I expect to get back a list with a single element instead, ["xx,yy"].

Demo app can be found here.

Thank you very much!

Comment From: ionel-sirbu-crunch

Probably in the same functional area, spaces () embedded in query params don't get decoded properly either, they are left as pluses (+). I've added a test to reflect that too in the demo app.

Comment From: ionel-sirbu-crunch

The way I see it, at a very high level, the parameter resolution needs to be processed in 2 rather distinct initial stages:

  1. extraction of raw query parameters from the query string - this is where we account for commas (?q=what,where) & repeated params (?q=what&q=where);
  2. URL decoding of the extracted values.

That way commas, equal signs & ampersands encoded into the values (stage 2) are not mixed with the ones that are actually part of the URL syntax (stage 1).

Obviously, further processing will take place after these initial steps, e.g. data conversion. How conversion is handled when multiple values are provided, but the method contract dictates a single value (e.g. a plain string), is probably up for debate. Off the top of my head, I'm thinking some flexibility between choosing the first value & ignoring the rest, and rejecting the request altogether by throwing some exception, depending on the requirements of the application. The level of flexibility could then vary from a global setting, e.g. via a Spring property, down to endpoint level, via some annotation.

Comment From: sbrannen

Potential duplicate of:

  • 23820

Comment From: ionel-sirbu-crunch

Potential duplicate of:

Looks like it. Thanks for pointing that out! I see the other ticket got closed because the reporter eventually found a workaround. To be honest, I also ended up doing a workaround, I had no other choice. But in the end, the reason why we log these tickets is so that the framework gets better, not necessarily to find workarounds. Granted those come in handy more than often, but still. 😄

Comment From: rstoyanchev

Thanks for raising the issue, and indeed that is how the framework gets better.

I'm afraid in this case there isn't much we can do. For once the HttpServlettRequest#getParameter* methods, which is what @RequestParam is a shortcut to, returns decoded values so we see "xx,yy" as the value in both cases. We could parse the queryString but that is a much more involved change because we rely on HttpServlettRequest#getParameter not only for the query, but also for form data, and multipart data in the body. And it would be a breaking change for others.

At best it would need to be a different annotation, but we will not introduce one just for this. The recommendations under #23820, although you may see them as a workaround, do provide flexibility around how this should be handled, and that is probably the best option in this case. You could also create your own custom argument resolver.

Comment From: ionel-sirbu-crunch

Thanks for raising the issue, and indeed that is how the framework gets better.

I'm afraid in this case there isn't much we can do. For once the HttpServlettRequest#getParameter* methods, which is what @RequestParam is a shortcut to, returns decoded values so we see "xx,yy" as the value in both cases. We could parse the queryString but that is a much more involved change because we rely on HttpServlettRequest#getParameter not only for the query, but also for form data, and multipart data in the body. And it would be a breaking change for others.

At best it would need to be a different annotation, but we will not introduce one just for this. The recommendations under #23820, although you may see them as a workaround, do provide flexibility around how this should be handled, and that is probably the best option in this case. You could also create your own custom argument resolver.

Well, something as simple as:

Index: spring-web/src/main/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.java
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/spring-web/src/main/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.java b/spring-web/src/main/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.java
--- a/spring-web/src/main/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.java    (revision 5ea8ba1b96084792a802cf7eef1ef176f53efe8d)
+++ b/spring-web/src/main/java/org/springframework/web/method/annotation/RequestParamMethodArgumentResolver.java    (date 1675861396950)
@@ -180,7 +180,11 @@
        if (arg == null) {
            String[] paramValues = request.getParameterValues(name);
            if (paramValues != null) {
-               arg = (paramValues.length == 1 ? paramValues[0] : paramValues);
+               if (Collection.class.isAssignableFrom(parameter.getParameterType())) {
+                   arg = paramValues;
+               } else {
+                   arg = (paramValues.length == 1 ? paramValues[0] : paramValues);
+               }
            }
        }
        return arg;

could potentially sort out the encoded comma issue, depending on how the underlying HttpServletRequest implementation handles getParameterValues(). e.g. with Spring's MockHttpServletRequest it works fine save for the "/test?res=xx,yy%2Czz" case, when it generates a single string. But "/test?res=xx&res=yy%2Czz" correctly generates 2 strings with the above change.