Add support for parsing query string in QueryParameters #10521

Artur- · 2021-03-31T07:29:34Z

This is a prerequisite for #10431 split out to a separate PR for easier review

flow-server/src/main/java/com/vaadin/flow/router/QueryParameters.java

pleku · 2021-04-01T10:11:41Z

flow-server/src/main/java/com/vaadin/flow/router/QueryParameters.java

+    private static Map<String, List<String>> parseQueryString(
+            String queryString) {
+        List<NameValuePair> params = URLEncodedUtils.parse(queryString,
+                StandardCharsets.UTF_8);


The only place where apache.http.client is used is in devmode for downloading node - that has been copied from frontend-maven-plugin. And that code should be removed from flow-server to its own place so that would not be leaking to user applications. But this is not your fault.

But if you introduce a further dependency to it, then it will be more difficult for us to make this refactoring.
This code is not the same exactly, but close to what Location does too to query parameters. Would it be possible to reuse-modify the code there instead ?

I will move the code from Location.parseParams to replace this. Would have helped a lot if it had been in the correct class earlier.

Sadly the implementation is partly broken so it also needs to be fixed at the same time

Overall I don't understand why we have this generic util methods implemented in the framework in a slightly broken way when there are perfectly fine and tested util libraries out there that you can use.

The current method needed two fixes to be compatible with what HttpServletRequest.getParameters returns:

It needs to decode values

A parameter like "?hello" should map to an empty string and not no value at all like the old method did

Hopefully this does not break something else then

Overall I don't understand why we have this generic util methods implemented in the framework in a slightly broken way when there are perfectly fine and tested util libraries out there that you can use

The problem is the broken code in the code base. We should not try reinvent the wheel, but also not bring in libraries only to depend on a subset of the code there. It is a balancing act.

If you want to avoid external dependencies, there is always the possibility of rebasing the given class and including it directly in the project. Then it is possible to update it etc for new releases, there are no conflicts and no dependencies either

Yes I think that is the least bad solution, to copy-paste the code and update it manually, in case the code cannot be / is not released as a standalone module.

So should we do it here? In that case I would revert to the original version more or less to use the lib and then make a separate PR about including it directly in the project instead of as a dependency

Because of the internal dependencies, it might not make sense to copy paste... It would lead to extracting the whole httpcore jar into the project

pleku

(just changing status to show that this is not waiting for review)

pleku

One question and suggestion (for the code in another comment).

--

The changes in this PR are fine, but when I look at the code flow (which this PR doesn't change, just the implementation), it does smell a bit. It seems like the code in the framework could be (later) improved to

only parse the query parameters when needed
cache the query strings for the ongoing request, now parse again and again
see if the query string parsing code from the http request implementation could be reused, like from AtmosphereRequest since it handles both 1+2 from above
I can create an issue for this after this is merged.

pleku · 2021-04-06T08:15:42Z

flow-server/src/main/java/com/vaadin/flow/router/QueryParameters.java

@@ -97,6 +99,66 @@ public static QueryParameters simple(Map<String, String> parameters) {
                        entry -> Collections.singletonList(entry.getValue())));
    }

+    /**
+     * Creates parameters from a query string.
+     *


I think the code is fine. But as the string can come from the user, this parsing might be a subject for DoS exploit, but I think the computations done inside the stream-map-collect are so simple that the query string limit of 1024 characters should not make it exploitable. Do you agree ?

I would consider adding some clarification to the method javadocs that this is code here is not checking for the length of the given query string - it is the responsibility of the caller.

Where would the DoS exploit possibility come from?

Like discussed separately, from using this code to parse a string that originates from client and it is very very long and because it is for some reason limited to 1024. This comment is enough for now.

vaadin-bot · 2021-04-06T11:02:13Z

flow-server/src/main/java/com/vaadin/flow/router/QueryParameters.java

+     * @return query parameters information
+     */
+    public static QueryParameters fromString(String queryString) {
+        return new QueryParameters(parseQueryString(queryString));


Immediately return this expression instead of assigning it to the temporary variable "parsedParams".

vaadin-bot · 2021-04-06T11:02:14Z

flow-server/src/main/java/com/vaadin/flow/router/QueryParameters.java

+            return Collections.singletonList(paramAndValue);
+        }
+        String param = paramAndValue.substring(0, index);
+        String value = paramAndValue.substring(index + 1);


Define and throw a dedicated exception instead of using a generic one.

vaadin-bot · 2021-04-06T11:02:18Z

SonarQube analysis reported 2 issues

1 major
1 minor

Watch the comments in this conversation to review them.

#10521) A minor API behavior change: `Location:getQueryParameters` is unified with how `HttpServletRequest` works: For a query string `?foo&bar` a list containing `""` is returned for both `foo` and `bar` whereas the existing implementation returned empty lists.

vaadin-bot · 2021-08-31T10:40:45Z

This ticket/PR has been released with platform 14.7.0.rc1 and is also targeting the upcoming stable 14.7.0 version.

Add support for parsing query string in QueryParameters

83dc1c3

vaadin-bot added the +0.1.0 label Mar 31, 2021

caalador added this to Iteration Reviews in OLD Vaadin Flow ongoing work (Vaadin 10+) Mar 31, 2021

pleku reviewed Apr 1, 2021

View reviewed changes

pleku suggested changes Apr 1, 2021

View reviewed changes

Artur- added 5 commits April 1, 2021 19:23

Move query parameter parsing to QueryParameter

e8f95b3

Fix custom query string parsing implementation

eee4839

Make it compatible with HttpServletRequest.getParameterMap

8f8917d

Fix query string generation

dea02c9

Fix test

400f6d5

pleku suggested changes Apr 6, 2021

View reviewed changes

vaadin-bot added +1.0.0 and removed +0.1.0 labels Apr 6, 2021

Artur- added 3 commits April 6, 2021 12:22

Fix Sonar issues

401ac53

Fix Sonar issues

1eca38e

Add comment

2b965e9

pleku approved these changes Apr 6, 2021

View reviewed changes

pleku enabled auto-merge (squash) April 6, 2021 09:47

vaadin-bot reviewed Apr 6, 2021

View reviewed changes

pleku merged commit a868f2f into master Apr 6, 2021

OLD Vaadin Flow ongoing work (Vaadin 10+) automation moved this from Iteration Reviews to Done - pending release Apr 6, 2021

pleku deleted the queryparams-parse branch April 6, 2021 11:07

vaadin-bot added prerelease version for platform 20.0.0 Released with Platform 20.0.0.alpha6 labels Apr 8, 2021

This was referenced Aug 18, 2021

Cherry picking two changes to 2.7 #11590

Merged

History.pushState drops fragment from location if query parameters are included #11628

Closed

vaadin-bot added prerelease version for platform 14.7.0 Released with Platform 14.7.0.rc1 labels Aug 31, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for parsing query string in QueryParameters #10521

Add support for parsing query string in QueryParameters #10521

Artur- commented Mar 31, 2021 •

edited

pleku Apr 1, 2021 •

edited

Artur- Apr 1, 2021

Artur- Apr 1, 2021

pleku Apr 6, 2021

Artur- Apr 6, 2021

pleku Apr 6, 2021

Artur- Apr 6, 2021

Artur- Apr 6, 2021

pleku left a comment

pleku left a comment •

edited

pleku Apr 6, 2021

Artur- Apr 6, 2021

pleku Apr 6, 2021

vaadin-bot Apr 6, 2021

vaadin-bot Apr 6, 2021

vaadin-bot commented Apr 6, 2021

vaadin-bot commented Aug 31, 2021

Add support for parsing query string in QueryParameters #10521

Add support for parsing query string in QueryParameters #10521

Conversation

Artur- commented Mar 31, 2021 • edited

pleku Apr 1, 2021 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pleku left a comment

Choose a reason for hiding this comment

pleku left a comment • edited

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

Choose a reason for hiding this comment

vaadin-bot commented Apr 6, 2021

vaadin-bot commented Aug 31, 2021

Artur- commented Mar 31, 2021 •

edited

pleku Apr 1, 2021 •

edited

pleku left a comment •

edited