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

Adding getSanitizedQueryString() #161

Merged
merged 5 commits into from
Sep 21, 2015
Merged

Adding getSanitizedQueryString() #161

merged 5 commits into from
Sep 21, 2015

Conversation

jusroberts
Copy link

Hi all,

This PR cleans up the request query to allow an HttpClient to process query strings that come in with invalid characters ({, }, |, ^, etc) without failing on a org.apache.http.client.ClientProtocolException.

Before this code change, if you try to hit localhost:8080/?a={}, you will receive a blank page caused by the above exception. This is poor behavior, as most web servers are able to handle these invalid characters without rendering an error. To fix this, we re-encode the query string so that it will have the proper characters according to https://tools.ietf.org/html/rfc3986#page-12 (Sections 2.1-2.3).

String getSanitizedQueryString() {
    String encoding = "UTF-8"
    String decodedQueryString = URLDecoder.decode(getQueryString(), encoding)
    String query = new URI(null, null, null, decodedQueryString, null).toString()
    if (query.equals("?"))
        query = ""
    return query
}

You will notice that I use URLDecoder to decode the query string, but then use URI (and not URLEncoder) to re-encode it. This is done to follow best practices outlined on StackOverflow [1] [2].

Questions, comments, concerns?

@NiteshKant
Copy link
Contributor

@jusroberts thanks for the PR, your code is outdated, you need to rebase to the master.

@jusroberts
Copy link
Author

Fixed. I seemed to have forked an old commit.

@jusroberts
Copy link
Author

I combined getSanitizedQueryString() with getQueryString() because getQueryString() isn't used by anything other than the forward() method.

@NiteshKant
Copy link
Contributor

Thanks @jusroberts

NiteshKant added a commit that referenced this pull request Sep 21, 2015
Adding getSanitizedQueryString()
@NiteshKant NiteshKant merged commit 16628f6 into Netflix:1.x Sep 21, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants