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

parameters are returned undecoded #266

Closed
pvg opened this issue Apr 11, 2015 · 3 comments
Closed

parameters are returned undecoded #266

pvg opened this issue Apr 11, 2015 · 3 comments

Comments

@pvg
Copy link
Contributor

pvg commented Apr 11, 2015

Request.params returns undecoded strings which makes any parameters with, say, a space or non-latin character inconvenient to use. Even a low-level API like HttpServletRequest.getParameter does this for you.

One (possibly naive) solution is to tweak line 90 in MatcherFilter.java from

    String uri = httpRequest.getRequestUri(); // NOSONAR to

    String uri = httpRequest.getPathInfo(); // NOSONAR

This works and passes the embedded Jetty tests. Inexplicably, it fails all the ServletTest tests because in that setup, getPathInfo returns null. Maybe this is something that can be fixed by tweaking the config of the web app example? It seems really odd that the 'embedding in an arbitrary servlet container' example/test fails on the failure of a standard servlet API call.

Of course, one could also make MatcherInfo decode the getRequestURI although getPathInfo looks like the more appropriate call for route matching.

Edit: I'd completely missed the SparkFilter wrapper which fakes out the getRequestUri for the servlet container integration case. I went ahead and changed it to override getPathInfo and made a small change to FilterTools to do the urldecoding and submitted a PR for discussion

#267

@tipsy
Copy link
Contributor

tipsy commented May 27, 2015

Can this be closed now?

@pvg
Copy link
Contributor Author

pvg commented May 29, 2015

No, because the implementation is wrong. It's just less wrong than it used to be. The problem is encoding/decoding standards are slightly different for different parts of a URI and the current implementation uses URIDecoder which is intended for query strings, not paths. Unfortunately, there is no built-in API for handling this, particularly in the servlet case where there are no guaranteed dependencies like jetty or commons http-client. The solutions is, perhaps, to lift the code just for this from an open source servlet implementation or possibly from guava, without introducing another fat dependency.

@tipsy
Copy link
Contributor

tipsy commented Mar 1, 2017

Not sure if fixed. Closing this because we need to clean up. @pvg Please create a new issue if this is still a problem.

@tipsy tipsy closed this as completed Mar 1, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants