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.

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

Add option to encode MultiValueMap of query params #24043

Closed
OrangeDog opened this issue Nov 20, 2019 · 7 comments
Closed

Add option to encode MultiValueMap of query params #24043

OrangeDog opened this issue Nov 20, 2019 · 7 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@OrangeDog
Copy link

Parameters passed using %-encoding will be double escaped by fromCurrentRequest():

@GetMapping
@ResponseBody
public Map<String, Object> test(@RequestParam MultiValueMap<String, String> params) {
    return Map.of(
            "params", params,
            "uri", ServletUriComponentsBuilder.fromCurrentRequest().toUriString()
    );
}
$ curl http://localhost:8080/?p=*
{"uri":"http://localhost:8080/?p=*","params":{"p":["*"]}}

$ curl http://localhost:8080/?p=%2a
{"uri":"http://localhost:8080/?p=%252a","params":{"p":["*"]}}

Spring Web 5.2.1.RELEASE

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Nov 20, 2019
@OrangeDog
Copy link
Author

Current workaround:

RequestAttributes attributes = RequestContextHolder.currentRequestAttributes();
HttpServletRequest servletRequest = ((ServletRequestAttributes) attributes).getRequest();
WebRequest webRequest = new ServletWebRequest(servletRequest);
MultiValueMap<String, String> params = new LinkedMultiValueMap<>();
for (Map.Entry<String, String[]> e : webRequest.getParameterMap().entrySet()) {
    params.addAll(e.getKey(), Arrays.asList(e.getValue()));
}

ServletUriComponentsBuilder.fromRequest(servletRequest).replaceQueryParams(params)

There might be a better way to get the MultiValueMap from the current request.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 25, 2019

The Javadoc for for toUriString() says it is a shortcut for builder.build().encode().toUriString(). However, ServletUriComponentsBuilder was initialized from already encoded components (requestUri, queryString in HttpServletRequest), so it should be builder.build(true).toUriString() instead.

So this is expected behavior. We could improve the Javadoc on ServletUriComponentsBuilder to mention specifically requestUri and queryString and that they're encoded, so be sure to use build(true)..

@rstoyanchev rstoyanchev added the status: waiting-for-feedback We need additional information before we can continue label Nov 25, 2019
@OrangeDog
Copy link
Author

The greater problem comes when you then use that builder to e.g. add another parameter that will need encoding. It would be better IMO to always create builders with their source parts decoded.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Nov 25, 2019
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Nov 25, 2019

I see your point but there is ambiguity once a URI component (e.g. path, query) is decoded. For example requestUri="/abc%3Bdef" would decode to "/abc;def" but when encoding such a path you might want the ";" to remain as is in some cases or have it encoded in others so the reserved meaning is either preserved or suppressed. UriComponents does the former but also provides extra options for stricter encoding of URI variables as described in the docs.

Regardless, we can't change how ServletUriComponents#fromRequest works already, and an overloaded variant that does decode won't help much either for the above mentioned reason. I think it's best to rely on UriUtils to encode the extra query params.

Currently there is a method to encode individual query params or a query string. We can add an additional one to encode from a MultValueMap to match the options available in UriComponentsBuilder. So in the end it would look like this:

MultiValueMap<String, String> params = new LinkedMultiValueMap<>();
// add params

ServletUriComponentsBuilder.fromCurrentRequest()
        .queryParams(UriUtils.encodeQueryParams(params))
        .build(true)
        .toUriString();

@rstoyanchev rstoyanchev added status: waiting-for-feedback We need additional information before we can continue and removed status: feedback-provided Feedback has been provided labels Nov 25, 2019
@spring-projects-issues
Copy link
Collaborator

If you would like us to look at this issue, please provide the requested information. If the information is not provided within the next 7 days this issue will be closed.

@spring-projects-issues spring-projects-issues added the status: feedback-reminder We've sent a reminder that we need additional information before we can continue label Dec 2, 2019
@OrangeDog
Copy link
Author

@rstoyanchev what feedback are you waiting for?

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue status: feedback-reminder We've sent a reminder that we need additional information before we can continue labels Dec 2, 2019
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 3, 2019
@rstoyanchev rstoyanchev self-assigned this Dec 3, 2019
@rstoyanchev rstoyanchev added this to the 5.2.3 milestone Dec 3, 2019
@rstoyanchev
Copy link
Contributor

I guess the issue bot doesn't react to emojis :), thanks.

@rstoyanchev rstoyanchev changed the title ServletUriComponentsBuilder double-encoding Add option to encode MultiValueMap of query params Dec 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants