Unstable processing encoded query string parameters in the AddQueriesToRequest class

[scottambroseio]

Expected Behavior / New Feature

Hey all, I've come across what I believe is a bug with the ClaimsToQueryString handler.

Actual Behavior / Motivation for New Feature

If the query string has url encoded parameters (at the very least, url encoded ampersands), they end up getting decoded by this handler which will cause problems for downstream services.

Doing some brief investigation, I believe that the problem stems from this file and the fact that QueryHelpers.ParseQuery url decodes values.

https://github.com/ThreeMammals/Ocelot/blob/13.5.0/src/Ocelot/QueryStrings/AddQueriesToRequest.cs

Minimal code I threw together to display the issue

            var queryDictionary = Microsoft.AspNetCore.WebUtilities.QueryHelpers.ParseQuery("?term=m%26s");

            var builder = new StringBuilder();

            builder.Append("?");
            
            int outerCount = 0;

            foreach (var query in queryDictionary)
            {
                for (int innerCount = 0; innerCount < query.Value.Count; innerCount++)
                {
                    builder.Append($"{query.Key}={query.Value[innerCount]}");

                    if(innerCount < (query.Value.Count - 1))
                    {
                        builder.Append("&");
                    }
                }

                if(outerCount < (queryDictionary.Count - 1))
                {
                    builder.Append("&");
                }

                outerCount++;
            }

            var result = builder.ToString(); // result = ?term=m&s, not ?term=m%26s

Specifications

• Version: 13.5.0
• Platform: Linux
• Subsystem:

The solution I'm using currently is to double encode the the value in the query string which works, it just feels a little wrong.

Thanks!

[thiagoloureiro]

Hi, I think it's related to this no?

https://stackoverflow.com/questions/15572682/how-to-correct-parse-query-string-in-c-sharp-if-value-contains-ampersand

If I understood correctly you need a ampersand in the querystring right?

from StackOverflow:

Your problem is that & carries special meaning in a query string, namely to separate arguments. The & in the middle of your perceived artist parameter is actually being parsed as an argument separator. You need to URL encode the & when constructing the URL.

For example, one correct query string would be:

?artist=singer1+%26+singer2&song=name+song

[scottambroseio]

Hey,

Apologies, after rereading my initial post it definitely could of been clearer. The problem is that my query string has been correctly url encoded but the ClaimsToQueryString is removing said encoding.

Lets imagine I have a route setup which uses the ClaimsToQuery functionality to add the subject claim to the query string

Expected behaviour:

If I have a query string of "?searchTerm=m%26s" the result should be "?searchTerm=m%26s&sub=123"

Actual behaviour:

If I have a query string of "?searchTerm=m%26s" the result is "?searchTerm=m&s&sub=123". As you can see, the %26 is being decoded which breaks downstream services. This seems like a bug to me as I can't think of any reason to do this, and even if there was, ClaimsToQuery seems to be the wrong place to do it. Logically, I'd expect ClaimsToQuery to simply add claims to the query string and leave any existing values as is.

Workaround:

The current workaround I'm using is to double encode the query string values, as then if I have a query string of "?searchTerm=m%2526s" the result is "?searchTerm=m%26s&sub=123"

[MaChao9914]

Hi, I hava the same problem.
if I have a route string http/locahost/api/route/cnn%2Fcdd02, after being forwarded by ocelot，It may have become like this http/locahost/api/route/cnn/cdd02, and downstream services cannot resolve this route.

[raman-m]

@scottambroseio
Are you still with Ocelot?
Could you check this scenario for latest version 23.1 please?

@scottambroseio @MaChao9914
In any case, you should understand that query string scenarios are complex and their behavior is difficult to track.
As a team, we recommend to define Catch-All routes to have less pain with query strings 👉

• Catch All
• Catch All Query String

Finally, if Catch-All cannot help you then consider PreQueryStringBuilderMiddleware middleware injection to do whatever you want with your query strings 😉

For now I can't accept this and can't consider this issue as Ocelot's bug, because of invalid arguments for Microsoft.AspNetCore.WebUtilities.QueryHelpers.ParseQuery() helper. Why not to report bug to MS Dotnet team? 😄

Ocelot/src/Ocelot/QueryStrings/AddQueriesToRequest.cs

Lines 49 to 55 in d6eefc8

	private static Dictionary<string, StringValues> ConvertQueryStringToDictionary(string queryString)
	{
	var query = Microsoft.AspNetCore.WebUtilities.QueryHelpers
	.ParseQuery(queryString);
	return query;
	}

We still use the helper but I don't see any help from you guys as ideas or PRs to improve the behavior.

[raman-m]

@scottambroseio Sound like good advice?