Follow up #2150: Special chars in values of query strings

[raman-m]

Follow up #2150

• #2116 Escaping unsafe pattern values of Regex constructor ​​derived from URL query parameter values containing special Regex chars #2150

Special chars in values of query strings

A 3rd additional test case:

-    [InlineData("api/debug()")] // no query
-    [InlineData("api/debug%28%29")] // encoded debug()
+    [InlineData("api/debug()?special=(\\,*,+,?,|,{,},[,],(,),^,$,.,#, ,)&2=(2)&3=[3]&4={4}")] // with query

The third test has revealed issues within the middleware logic, indicating that this test case is likely to fail. The MergeQueryStringsWithoutDuplicateValues method struggles to handle URLs containing query strings with special characters, such as Regex patterns and reserved URL specification characters. This issue became apparent in release 20.0.0 during the refactoring of the MergeQueryStringsWithoutDuplicateValues method, which aimed to integrate existing and new logic (for example OData filters in query parameters, with bug fixes, with new feature for query string placeholders). Regrettably, the method fails to process special characters in parameter values.

Originally posted by @raman-m in #2150 (comment)

Subject

• Unit tests
  

  Ocelot/test/Ocelot.UnitTests/DownstreamUrlCreator/DownstreamUrlCreatorMiddlewareTests.cs

  Lines 614 to 618 in d310508

  	[Theory]
  	[Trait("Bug", "2116")]
  	[InlineData("api/debug()")] // no query
  	[InlineData("api/debug%28%29")] // debug()
  	public async Task ShouldNotFailToHandleUrlWithSpecialRegexChars(string urlPath)

• Acceptance tests
  

  Ocelot/test/Ocelot.AcceptanceTests/Routing/RoutingTests.cs

  Lines 1171 to 1175 in d310508

  	[Theory]
  	[Trait("Bug", "2116")]
  	[InlineData("debug()")] // no query
  	[InlineData("debug%28%29")] // debug()
  	public void Should_change_downstream_path_by_upstream_path_when_path_contains_malicious_characters(string path)

[raman-m]

Possible duplicate of #2143

• Routing not working with special character in UpstreamPathTemplate #2143

[raman-m]

@int0x81, welcome to your assignment!

[raman-m]

@int0x81 Finn, where are you? Are you available?

[int0x81]

@int0x81 Finn, where are you? Are you available?

Hello Raman. I started working on an issue #2132 this week. Unfurtunatly I am sick right now and will continue to work on it next Monday. Do you think its a good idea to prior #2150 ?

[raman-m]

@int0x81

I started working on an issue #2132 this week.

If you have started the issue, please inform me and Guillaume with a message in the #2132 thread.
Additionally, if you have begun any task from the board, kindly move a ticket to the 'In Progress' state.

Unfurtunatly I am sick right now and will continue to work on it next Monday.

🆗 Wishing you a speedy and an easy recovery!

Do you think its a good idea to prior #2150 ?

I haven't assigned priority labels to the tickets on your mentoring board, so you're free to tackle them in any order. Please consider this follow-up task after you've completed #2132.

[int0x81]

@raman-m I tried to move items on the projects board last week, but it seems I do not have permission.

[raman-m]

@int0x81 OK I've moved the ticket. Guillaume (@ggnaegi) should have this permission, because he is the member of the team.
The process is the following:

1. Required. Write a message to issue thread. After that you will be assigned.
2. Desired. Ask Guillaume (or me) to manage your ticket which you're working on.

[int0x81]

hey @raman-m I would like to provide you with a quick status update: I am working on the issue right now and digging deeper into the URL replacement logic. Unfortunately, it takes longer than expected for me to understand the whole logic. Especially the test cases are quite hard to read :/. I also have another task within my company ATM but I will come up with a first draft PR on Friday that (hopefully fixes issue #2150)

[raman-m]

@int0x81 Sure thing, you have to prioritize your employer's tasks 😄 not to be hangry 🥲

Unfortunately, it takes longer than expected for me to understand the whole logic. Especially the test cases are quite hard to read :/.

I believe it would be beneficial to begin with debugging the unit test.

Especially the test cases are quite hard to read :/.

Apologies, but are we experiencing a readability issue with the unit test, or it difficult for you to read English texts? The unit test consists solely of a series of private methods. I am quite perplexed, as I am unsure how to assist you... So, ask your boss, Guillaume: he knows Ocelot C# code well.

I will come up with a first draft PR on Friday...

LoL! Remember, developer: Friday is a pay day in USA 😄
But I would rephrase: Friday is an open-source day! 😋

[int0x81]

Hello @raman-m, hello @ggnaegi

I think I investigated the Software enough to understand the issue but

Currently, the DownstreamUrlCreatorMiddleware is unable to build a proper downstream request when the original Uri contains special characters in the query, especially ?, &, and =. This is because the TemplatePlaceholderNameAndValues property of the httpContext only provides the query as a plain string. This happens in the DownstreamRouteFinderMiddleware. If the plain string contains these characters it is impossible for the MergeQueryStringsWithoutDuplicateValues method to differentiate between dividers of query parameters and actual content. Even if I could find a hacky way to fix this, it would probably be very fragile and also a bad idea in terms of security reasons.

I believe the {path} template value should never contain the unescaped query. The only solution that I see right now is, to change that logic in the DownstreamPathPlaceholderReplacer, so the {path} value will always contain the escaped query instead of the unescaped one.

What do you think?

[ggnaegi]

@int0x81 I haven't tried myself, but the method ParseQueryString can decode url encoded characters.

The ParseQueryString method uses UTF8 format to parse the query string In the returned NameValueCollection, URL encoded characters are decoded and multiple occurrences of the same query string parameter are listed as a single entry with a comma separating each value.

so you might aswell try with UrlEncode

Your approach could work though...

[int0x81]

After even more investigations I am not quite sure if this test case even makes sense ( @raman-m , If I am missing something, please explain)

As far as I reverse-engineered it, the 'DownstreamUrlCreatorMiddleware' can only receive such a query if

1. The client inputs such a query into Ocelot via a DownstreamRequest. In that case I expect the query gets handled by other middleware before and I would expect such a query never even arrives at the 'MergeQueryStringsWithoutDuplicateValues' method.
2. Invalid values were provided in the TemplateNameValue collection. In that case the configuration should be validated on application start.

The current 'MergeQueryStringsWithoutDuplicateValues' already uses the ParseQueryString method, but still fails to function with the query specific in this issue. I don't think I can write a better implementation than Microsoft 😄

[raman-m]

@int0x81 wrote:

Currently, the DownstreamUrlCreatorMiddleware is unable to build a proper downstream request when the original Uri contains special characters in the query, especially ?, &, and =.

Yes, right!

This is because the TemplatePlaceholderNameAndValues property of the httpContext only provides the query as a plain string. This happens in the DownstreamRouteFinderMiddleware. If the plain string contains these characters it is impossible for the MergeQueryStringsWithoutDuplicateValues method to differentiate between dividers of query parameters and actual content.

Bingo!🔥 The MergeQueryStringsWithoutDuplicateValues is a crucial component that implements the legacy query string feat. When placeholders in the parameters are replaced, they transition to the URL path segment leading to the complete removal of the parameter. Please refer to the documentation → Merging of Query Parameters.
Additionally, we have an advanced feature that manages the existence of query parameters based on user input → Control over parameter existence
Finally, I would say that MergeQueryStringsWithoutDuplicateValues method is the house where the bug lives 😄🐞

Even if I could find a hacky way to fix this, it would probably be very fragile and also a bad idea in terms of security reasons.

Security concerns? That may be valid. Therefore, I believe there are no security issues in this context. The merging of query parameters is an important feature of Ocelot.

[raman-m]

@int0x81 wrote:

After even more investigations I am not quite sure if this test case even makes sense ( @raman-m , If I am missing something, please explain)

The unit test just will help you to refactor the MergeQueryStringsWithoutDuplicateValues method to support special chars in query parameter values, not names, but values only. It is imperative thing that some clients don't encode param values in URLs. So, it will be cool Ocelot feature if we will process such param values without errors.

As far as I reverse-engineered it, the 'DownstreamUrlCreatorMiddleware' can only receive such a query if

1. The client inputs such a query into Ocelot via a DownstreamRequest. In that case I expect the query gets handled by other middleware before and I would expect such a query never even arrives at the 'MergeQueryStringsWithoutDuplicateValues' method.

Probably true, But this fact can be easily checked by acceptance test. Just add new test data to existing theory and you will see what's hapenning.

2. Invalid values were provided in the TemplateNameValue collection. In that case the configuration should be validated on application start.

No, we will not implement such validations, but it makes sense as a 2nd possible solution if we will fail with refactoring of the 'MergeQueryStringsWithoutDuplicateValues' method.

The current 'MergeQueryStringsWithoutDuplicateValues' already uses the ParseQueryString method, but still fails to function with the query specific in this issue. I don't think I can write a better implementation than Microsoft 😄

Please do not be nervous. As a team, we do require you to reimplement the default Microsoft functionality. The issue is more profound, and the requirement is clear: we need to verify special characters in the query parameters.

Please read #2181 discussion:

• + sign in URL (query) encoded as %2B finaly is replaced by SPACE #2181

You will understand the problem. You need to reuse test data from this thread.
The possible ready solution is #2181 (reply in thread) ❕ 😉
I guess this code should help 99%. But we need to cover by new unit & acceptance tests...
Good luck! 😉

I will allow you to complete this and rectify my mistakes from the previous refactoring 🙈

[raman-m]

Bug lives here

Ocelot/src/Ocelot/DownstreamUrlCreator/Middleware/DownstreamUrlCreatorMiddleware.cs

Line 121 in e9a8bfc

private static string MapQueryParameter(KeyValuePair<string, string> pair) => $"{pair.Key}={pair.Value}";

it should be:

private static string MapQueryParameter(KeyValuePair<string, string> pair)
    => $"{pair.Key}={HttpUtility.UrlEncode(pair.Value, Encoding.UTF8)}";

Don't say me Thank You ))
But if you are a boy with good home education, then please say 🥲

[int0x81]

@raman-m
I am not quite sure if it's that easy. As far as I observed, if such a query (like you requested in this issue) is provided, the query parameters do not even arrive correctly at the 'MapQueryParameter' function. This is because the 'ParseQueryString' method fails to parse such a query already and cannot extract the correct parameters from it. Just putting an escape in the 'MapQueryParameter' would not fix the problem I think

[raman-m]

@int0x81 Ok, stop any research and development because you waste the time.
Please switch to another bug from the board. Thank you for your time!

@ggnaegi FYI I will close this by my own, after .NET9 release.

[int0x81]

Ok, stop any research and development because you waste the time.
Please switch to another bug from the board. Thank you for your time!

Alright, I will try to tackle #2132 this week 👍