Skip to content
This repository has been archived by the owner on Jan 24, 2021. It is now read-only.

Normalize path decoding, append fragment to path #2117

Closed
wants to merge 8 commits into from

Conversation

khellang
Copy link
Member

This should fix (or kill) the infamous #1280 and #1154 for good 馃懐 I hope 馃槤

Current State

This is how the different hosts treat path and query strings today:

Host Path Query
Self Decoded* Encoded
ASP.NET Encoded Encoded
OWIN Decoded Encoded

* This decoding is actually done by Nancy.

In addition to this, Nancy also decodes the path two places in the core pipeline; for route matching and for static content.
This means that for OWIN and self-host, the path would be double decoded today.

There's also a weird issue with the ASP.NET and self-host where it pulls out everything after the first # and adds it to the Fragment property. This is not being picked up today.

This PR

This PR does a couple of things:

  1. Remove all usages of HttpUtility.UrlDecode (for path decoding) inside Nancy core, including some tests.
  2. Makes sure that all hosts now decode the path. I've tested them and all now give the same canonical Url to the Nancy engine.
  3. Appends a possible fragment for ASP.NET and self-host.

There's still the question of whether we want to decode the query string as well, but it seems all hosts are doing the same thing here, so it's not really pressing.

@khellang khellang added this to the 2.0 milestone Nov 18, 2015
@khellang
Copy link
Member Author

Talked with @grumpydev about this. There's a substantial drawback to this approach as well; by decoding in the host, you won't be able to separate between %2F and /, which means that routes like /artist/AC%2FDC/albums would fail. It would always be seen by routing as /artist/AC/DC/albums, which would (most likely) 404.

The only host where we can't get rid of the decoding is in the OWIN host, since it's specified in the spec:

The server MUST provide percent-decoded values for "owin.RequestPath" and "owin.RequestPathBase".

So we have to choose;

  1. Keep the inconsistent state by double decoding OWIN paths (once in underlying OWIN host, and once in routing) and keep the ability to route %2F.
  2. Have consistent, single decoding across the board and loose the ability to route %2F.

There could be a 3rd alternative as well, and that would be to introduce some config passed from the hosts that specifies whether Nancy needs to decode or if the path's already been decoded.

@thecodejunkie
Copy link
Member

The 3d option is going to be possible with the new INancyEnvironment and INancyBoostrapper.GetEnvironment()-method if that's the route we decide upon.

But, if we're going all-in on OWIN (making Nancy.Hosting.* packages meta-packages), then wouldn't it be "simpler" for us to fix this, because everthing would be speaking OWIN under the hood no matter which host?

@khellang
Copy link
Member Author

khellang commented Nov 20, 2015

The problem is that there's no way to fix it. Once the path has been decoded, you've lost some vital data. The OWIN spec should never have said that IMO. The fix would be to fix the spec and make all hosts (including Katana) follow it. I don't know if that'll happen. The damage is done.

@thecodejunkie
Copy link
Member

@khellang @damianh Wouldn't this be more appropriate to do once we've unified the hosting story around OWIN?

@khellang
Copy link
Member Author

Once we unify around OWIN, this PR is obsolete anyway.

@thecodejunkie
Copy link
Member

So maybe that should be the priority?

@khellang
Copy link
Member Author

Maybe. Then we/someone need to come up with a plan. How do we move things forward? What happens to the ASP.NET and self-hosts? Are we basically changing the interface of INancyEngine to "speak OWIN" and move the NancyMiddleware code into the engine itself?

@thecodejunkie
Copy link
Member

The discussion so far have been

  1. Make INancyEngine speak OWIN
  2. Update Nancy.Hosting.* packages to be meta-packages to help provide the F5-experience

@khellang khellang mentioned this pull request Dec 1, 2015
@damianh
Copy link
Member

damianh commented Dec 1, 2015

What are the aspnet5 hosts doing?

@khellang khellang removed this from the 2.0 milestone Jan 17, 2016
@khellang
Copy link
Member Author

This'll "fix" itself once everything moves to OWIN. Let's not hold back 2.0 with this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants