Spring Cloud Netflix Netflix Zuul query string encoding

This is a copy of the stackoverflow issue: http://stackoverflow.com/questions/36475845/netflix-zuul-query-string-encoding

When sending a request via Zuul to a client, Zuul seems to change the query String. More specifically, if the client should receive an url-encoded query String, Zuul decodes the query String once. Here is a concrete example:

If "http://localhost:8080/demo/demo?a=http%3A%2F%2Fsomething/" is sent to the client, the client receives as a query String "a=http://something/".

Looking into Zuul`s code, the function "buildZuulRequestQueryParams" uses "HTTPRequestUtils.getInstance().getQueryParams();" which decodes the query String.

Is this a desired feature or a bug?

Comment From: dsyer

What version of Spring Cloud Netflix is this? Did you try from master?

Comment From: jebeaudet

I believe query strings should not be decoded when passed to the backend since Zuul is only a proxy. @anoll is right, in master branch, the ProxyRequestHelper still uses HTTPRequestUtils.getInstance().getQueryParams() which decode the query strings.

Comment From: yashanet

I have the same issue. But when we have slash in parameter name, for example : http://localhost:8080/demo/?name/withSlash=value". So please remove URLDecoder.decode() from HTTPRequestUtils.getQueryParams()

Comment From: dsyer

HTTPRequestUtils is a Netflix utility class. If you want to change it please go to https://github.com/Netflix/zuul. If you want to fix it here, then a pull request is more than welcome.

Comment From: jebeaudet

First, there is 2 code paths with this problem. 1. If you're using SimpleHostRoutingFilter, the query params will be decoded using the URLDecoder in HTTPRequestUtils which is wrapped inside the ProxyRequestHelper::buildZuulRequestQueryParams. These will get reencoded in ProxyRequestHelper::getQueryString using UriTemplate. 2. If you're using RibbonRoutingFilter, the query params will be decoded as in 1 in ProxyRequestHelper::buildZuulRequestQueryParams but will not get reencoded and will get send to the backend as is in HttpClientRibbonCommand or RestClientRibbonCommand.

I've read quite a lot about encoding in the past days trying to come up with a solution for this. This issue sum some of it up : https://github.com/Netflix/zuul/issues/215 and https://www.talisman.org/~erlkonig/misc/lunatech%5Ewhat-every-webdev-must-know-about-url-encoding/ gives good hints about dos and don't.

Since Zuul is used as a proxy, I believe it should not change the query params sent to him and let the backend handle it. Moreover, using URLDecoder to decode query params is IMO a bad idea since this class is for the application/x-www-form-urlencoded MIME format which is not always the case here. Decoding and reencoding can sometimes lead to different result in the end so we have to be careful about this. Example :

URLEncoder.encode(URLDecoder.decode("some%20%2520value", "UTF-8"), "UTF-8") //some+%2520value

I've come up with a solution but that involves reverting https://github.com/spring-cloud/spring-cloud-netflix/issues/682 and copy pasting HttpRequestUtils::getQueryParams method to remove the decoding calls. By doing this, the query params are sent intact to the backend. Is this something worth submitting? I'm absolutely not a fan a copy pasting but fixing this upstream would probably break other stuff in zuul core...

Comment From: dsyer

Sounds like a plan

Comment From: jebeaudet

Finally had a minute to get back to this, sorry about the delay. Long post ahead, temporary fix is at the bottom.

Analysis

First of all, some of my analysis in my previous post was partly wrong. The point about SimpleHostRoutingFilter is correct however, the point about RibbonRoutingFilter is not. The query params do get reencoded down the road, wheter you use a RestClientRibbonCommand or a HttpClientRibbonCommand. Here's where it gets ugly.

RestClientRibbonCommand

RestClientRibbonCommand relies on netflix RestClient who use jersey as the client. When setting the query params in the WebResource (https://github.com/Netflix/ribbon/blob/master/ribbon-httpclient/src/main/java/com/netflix/niws/client/http/RestClient.java#L598), it uses the com.sun.jersey.api.uri.UriComponent::contextualEncode method to encode the query param. Quote of the javadoc :

* Contextually encodes the characters of string that are either non-ASCII * characters or are ASCII characters that must be percent-encoded using the * UTF-8 encoding. Percent-encoded characters will be recognized and not * double encoded.

In other words, it encodes parameters only if necessary. Clearly this is not desirable in a proxy context, where query params might be double encoded for a final redirect_uri for example. It works well when query params are single encoded, they get decoded in the ProxyRequestHelper and reencoded in this class. However, if you pass something double encoded, here is what will get sent to the backend : In ProxyRequestHelper :

URLDecoder.decode("foo%2520bar", "UTF-8") // foo%20bar

In UriComponent :

UriComponent.contextualEncode("foo%20bar", UriComponent.Type.QUERY_PARAM, true) // foo%20bar

HttpClientRibbonCommand

As for HttpClientRibbonCommand, they get encoded in URIBuilder with URLEncodedUtils.format which doesn't encode "contextually" like Jersey does. The output is not necessarily the same as the input before the URLDecoder of ProxyRequestHelper (I've noticed that %20 ends up as +) but from all the extensive tests I've done, it looks fine.

Solution

Sadly, there is no easy fix for this that would work for both RibbonCommands: - Disabling the decoding in ProxyRequestHelper with RestClientRibbonCommand almost work but there is a problem with the +. It is a valid encoding of a space (like %20) but when it goes through UriComponent::contextualEncode, the method encodes the + to %2B which garbles the query string in the backend. Disabling the decoding also breaks HttpClientRibbonCommand as it is right now. - We could use a RestClient that has a custom Client that uses a different method for encoding. - We could make HttpClientRibbonCommand the default and deprecate RestClientRibbonCommand since RestClient is itself deprecated. By using it, you don't get problem with query param as of now. - (My preferred solution) Modify HttpClientRibbonCommand to reuse the code in SimpleHostRoutingFilter for building a HttpUriRequest and remove the decoding of query strings in ProxyRequestHelper::buildZuulRequestQueryParams. Make it the default and deprecate RestClientRibbonCommand (we could fix the + bug directly in the factory). I prefer this way because you don't mangle with the query strings at all, they get sent to the backend exactly how they came, no decoding/reencoding necessary.

Thoughts?

Temporary fix

If you only use RibbonRoutingFilter (service discovery client), just add this bean to use HttpClientRibbonCommandFactory instead of the default and you should be good to go :

@Configuration
public class HttpClientRibbonCommandFactoryConfiguration
{
    @Bean
    @Autowired
    public RibbonCommandFactory<?> ribbonCommandFactory(SpringClientFactory clientFactory)
    {
        return new HttpClientRibbonCommandFactory(clientFactory);
    }
}

If you use SimpleHostRoutingFilter, you need to modify the ProxyRequestHelper. I used a BeanPostProcessor to modify the bean to this :

public class ExtendedProxyRequestHelper extends ProxyRequestHelper
{
    @Override
    public String getQueryString(MultiValueMap<String, String> params)
    {
        HttpServletRequest request = RequestContext.getCurrentContext().getRequest();
        String query = request.getQueryString();
        return (query != null) ? "?" + query : "";
    }
}

I guess you could also disable SimpleHostRoutingFilter and use your own extension.

Comment From: spencergibb

Thanks for the detailed analysis! I agree that we need to deprecate RestClientRibbonCommand and make HttpClientRibbonCommand the default for other reasons as well as this one.

Comment From: tomaszglinski

@jebeaudet, thank you, your HttpClientRibbonCommandFactoryConfiguration saved my day:)

Comment From: spencergibb

HttpClientRibbonCommand is now the default.

Comment From: jebeaudet

@spencergibb, somehow RestClientRibbonCommandFactory is still the default in SR2, is this intended? (EDIT: scratch that, I just saw that this was a bug fix release sorry. I'm still interested by my other question though)

Also, if you'd revert https://github.com/spring-cloud/spring-cloud-netflix/commit/41c364002fe2e0d9e9c1f59ccf4a9c530dcde04a, it would also fix the query string encoding problems with SimpleHostRoutingFilter.

Comment From: spencergibb

@anoll or @jebeaudet can you try with Brixton.SR6 or Camden.SR1? There have been various encoding fixes.

Comment From: jebeaudet

I've proposed a PR @spencergibb

Comment From: vtahlani

When release branch with be available?

Comment From: ryanjbaxter

@vtahlani this is in the Dalston release train already

Comment From: vtahlani

I mean 1.3.0.RELEASE, branch

Comment From: dsyer

1.3.4 was already released. This issue was closed in 1.3.0.

Comment From: akatkar

I had a similar problem. When a query parameter has multiple value, all '=' encoded with %3D but commas do not encoded. I need to encode commas as well. So I used to BeanPostProcessor and replace ProxyRequestHelper inside it. Then in my ProxyRequestHelper I just replaced all commas like that

public class ExtendedProxyRequestHelper extends ProxyRequestHelper
{
    @Override
    public String getQueryString(MultiValueMap<String, String> params) {
        return super.getQueryString(params).replaceAll(",","%26");
    }
}

Thanks to @jebeaudet for valuable explanation.

Comment From: d3minem

https://github.com/spring-cloud/spring-cloud-netflix/issues/971#issuecomment-225073937 - @jebeaudet: Based on your analysis and suggested solution, I have another go.

Analysis:

I tried to disable the SimpleHostRoutingFilter and added your extension ExtendedProxyRequestHelper. Just want to add that ExtendedProxyRequestHelper will never execute by disabling the above routing filter because it trigger by this line of code:

SimpleHostRoutingFilter

String uriWithQueryString = uri + (this.forceOriginalQueryStringEncoding ? this.getEncodedQueryString(request) : this.helper.getQueryString(params));

Note: Initially, I'm using the url prefixed routing which is bind with SimpleHostRoutingFilter. Later on, I'll switch to serviceId which is bind to RibbonRoutingFilter.

ExtendedProxyRequestHelper

Now I have another problem that request request.getQueryString() in ExtendedProxyRequestHelper gives empty query string as you've suggested, while getQueryString(MultiValueMap<String, String> params) in ExtendedProxyRequestHelper gives all the request params which were set in the custom pre filter via ctx.setRequestQueryParams(params). So I have transformed the getQueryString method as below:

public class ExtendedProxyRequestHelper extends ProxyRequestHelper
{
    public ExtendedProxyRequestHelper(ZuulProperties zuulProperties) {
        super(zuulProperties);
    }

    @Override
    public String getQueryString(MultiValueMap<String, String> params) {
        int counter = 0;
        StringBuffer query = new StringBuffer();
        for (Map.Entry<String, List<String>> param : params.entrySet()) {
            String queryValue = String.join(",", param.getValue());
            query.append(param.getKey() + "=" + queryValue);
            if (counter != params.size() - 1) {
                query.append("&");
            }
            ++counter;
        }
        return (query != null) ? "?" + query.toString() : "";
    }
}

Now when I see the basic http request, I can see all the request params are appended in the final URI as query params and the authorisation server shows login page as expected.

Basic Http Request (Netflix Zuul)

/auth/protocol/openid-connect/auth?scope=read&response_type=code&redirect_uri=http://localhost:8080/auth/redirect/&client_id=newClient