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.

Sign up for GitHub

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

Jump to bottom

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

Open
mshambharkar opened this issue Jan 20, 2021 · 4 comments · May be fixed by #506
Open

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

mshambharkar opened this issue Jan 20, 2021 · 4 comments · May be fixed by #506
Milestone
Backlog

Comments

@mshambharkar
Copy link

mshambharkar commented Jan 20, 2021 •
edited

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
Copy link
Member

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

@mshambharkar
Copy link
Author

mshambharkar commented Jan 20, 2021 •
edited

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 Doesn't support matrix urls because semicolons get encoded. #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
Copy link

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

@Tratcher Tratcher added this to the Backlog milestone Apr 29, 2021
@ificator
Copy link

ificator commented Jun 10, 2023 •
edited

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).

@ificator ificator linked a pull request Jun 10, 2023 that will close this issue
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
Backlog
Development

Successfully merging a pull request may close this issue.

Make sure 'FromUriComponent' and 'ToUriComponent' are symmetric ificator/AspNetKatana
4 participants
@Tratcher @mshambharkar @ificator @minune-msft