aspnet / AspNetKatana

Microsoft's OWIN implementation, the Katana project
Apache License 2.0
968 stars 334 forks source link

With 4.x release behavior of decoding url parameters (path and query parameters) has changed. #393

Open mshambharkar opened 3 years ago

mshambharkar commented 3 years ago

I recently updated the version of Microsoft.Owin.dll from 3.0 to 4.1 and found a couple of issues that impact backward compatibility. I have this url http://localhost:${Port}/api/values/${Parameter}/${Parameter}?query1=${Parameter}&query2=${Parameter}&query3=${Parameter} , where parameter contains url encoded characters (trying to pass URL reserved characters in the route as path and query parameter. Simple Controller and it's action is defined as

[RoutePrefix("api/values")]
public class ValuesController : ApiController
{
    [HttpGet]
    [Route("{Path1}/{Path2}")]
    public object Get([FromUri]string Path1, [FromUri] string Path2, string query1, string query2, string query3)
    {
        return new ResponseModel(Path1, Path2, query1, query2, query3);
    }
}

In 3.x release, Owin (self-hosted Web API server running) would decode only once before passing those parameters to ActionFilter or Action. In 4.x parameters are decoded twice before passing it to ActionFilter or Action.

For example: Parameter = :?#[]@"!$&'()*,;= Owin version Encoding level Encoded parameter Path Query
3.x 1 %3A%3F%23%5B%5D%40%22%21%24%26%27%28%29%2A%2C%3B%3D :?#[]@"!$&'()*,;= :?#[]@"!$&'()*,;=
4.x 1 :?#[]@"!$&'()*,;= :?#[]@"!$&'()*,;=
3.x 2 %253A%253F%2523%255B%255D%2540%2522%2521%2524%2526%2527%2528%2529%252A%252C%253B%253D %3A%3F%23%5B%5D%40%22%21%24%26%27%28%29%2A%2C%3B%3D %3A%3F%23%5B%5D%40%22%21%24%26%27%28%29%2A%2C%3B%3D
4.x 2 :?#[]@"!$&'()*,;= %3A%3F%23%5B%5D%40%22%21%24%26%27%28%29%2A%2C%3B%3D

The above table is just a sample and there are other possible scenarios like adding '+' & '/' to the parameter string and encoding three times at the client side before making a request.

I see a couple of issues

  1. Breaking change from 3.x to 4.x, the client now needs to encode 3 times the same string which was working with 2 times encoding. 3 times encoding is required if you want to pass '/' as a parameter.
  2. Inconsistent decoding at Path parameters and Query parameters, path parameters are decoded twice whereas query params keep older behavior of 3.x.

After looking at a code and after several trials, I believe this behavior in 4.x was introduced with a resolution of bug 104

I have placed sample server and test client code at GitHub repo for reference (it also includes other scenarios to verify) AspNetKatana-Issue

Tratcher commented 3 years ago

104 (#135) affected encoding, not decoding. Can you explain more why you think it's connected here?

mshambharkar commented 3 years ago

I agree code is related to encoding and not decoding and it is just my understanding, would need to debug (owin and webapi assemblies both) to ascertain that #104 is the reason for this issue. There are multiple reasons for my thinking

  1. I am using Web API self-hosted configuration, I am only modifying 3 libraries - Microsoft.owin.dll,Microsoft.Owin.Hosting.dll and Microsoft.Owin.Host.HttpListener.dll keeping other libraries at a fixed version (verified behavior with Owin 3.0 and 4.0 )
  2. Comparing code of Microsoft.Owin/PathString.cs between two versions, 3.0 code would always encode the input (even encoded string as well) whereas 4.0 code would encode only when unescaped characters are present. This behavior is consistent with Path parameters behavior and their decoding.
  3. In 4.0 release only Bug #104 was related to Uri

I believe WebApi pipeline is also taking care of decoding at 1 level and that is the reason the difference in this encoding of the path is creating a difference at Action and ActionFilter.

minune-msft commented 3 years ago

We are observing a similar behavior updating from version 3.0.1 to 4.1.1.

Previously are sending requests that can look like this, (note the percentage symbol): /object/Object-:%25_*?a689c3ce-90a3-4317-a260-dfc61ad3e1f6

The object value is extracted using the [FromUri] attribute

On version 3.0.1, the [FromUri] attribute resolves to: Device-:%25_*?a689c3ce-90a3-4317-a260-dfc61ad3e1f6

On version 4.1.1 it resolves to Device-:%_*?a689c3ce-90a3-4317-a260-dfc61ad3e1f6

ificator commented 1 year ago

I'm seeing a similar issue upgrading from 3.0.1 to 4.2.2 and it looks like #135 is the root cause. The change to ToUriComponent() not only reduced the set of characters that are percent encoded but also skipped encoding the % if it looks like looks like it's part of an encoded character. This is fundamentally wrong.

In OwinRequest the Path property is generated from owin.RequestPath which is the decoded request path. Since ToUriComponent() is meant to return a correctly encoded string that can be used in a URI, it MUST encode any % characters it encounters otherwise the resulting encoded string will not be semantically equivalent to what the client provided.

Here's a walkthrough of the current behavior: stage value
literal file name test%20name.txt
encoded file name in request path test%2520name.txt
value of owin.RequestPath test%20name.txt
value in PathString test%20name.txt
value from ToUriComponent test%20name.txt
value in HttpRequestMessage.RequestUri test%20name.txt
final file name test name.txt
Here's a walkthrough of the expected behavior: stage value
literal file name test%20name.txt
encoded file name in request path test%2520name.txt
value of owin.RequestPath test%20name.txt
value in PathString test%20name.txt
value from ToUriComponent test%2520name.txt
value in HttpRequestMessage.RequestUri test%2520name.txt
final file name test%20name.txt

As it stands, the only work around I have is to violate the OWIN specification be rewriting owin.RequestPath value in the environment to be the encoded path. Not only is this a horrible hack, but it risks introducing other encoding issues into my product (e.g. if some other code somewhere uses IOwinRequest.Path directly instead of using the HttpRequestMessage generated from it).