Fix accidental double-escaping when Router.UseEncodedPath() is enabled

[nvx]

Bug originally mentioned in https://github.com/gorilla/mux/issues/354#issuecomment-899377755

Summary of Changes

When Router.UseEncodedPath() is enabled (and SkipClean is not enabled), a request to http://localhost/foo/../bar%25 causes a redirect to http://localhost/bar%2525 as there is a bug where this code path results in double-encoding (the % from the %25 is itself encoded as %25, resulting in %2525).

The expected output for this should have been a redirect to http://localhost/bar%25. A unit test for this behaviour has been added which originally failed with the following output:

Expected redirect location to http://localhost/bar%25, found http://localhost/bar%2525

The remaining changes allows the new unit test to pass and does not break any existing unit tests.

[nvx]

In an unrelated note, Router.UseEncodedPath() does not follow the same conventions as eg Router.SkipClean(bool) in that it is only possible to enable this function, and once called it is not possible to disable.

Due to the potential security vulnerabilities mentioned in #354 when UseEncodedPath is not enabled, arguably this should be the default, but then this would require a function to disable the behaviour if unwanted. Of interest setting UseEncodedPath by default does not break any existing unit tests.

[nvx]

Something else that stood out when writing the unit tests for this is https://github.com/gorilla/mux/commit/d10d5466f2db2758bb616f627775692d2f69fe8a mentions some changes that were made to prevent the query string and # fragment from being stopped.

Oddly though looking at the commit to the Golang standard library it references https://github.com/golang/go/commit/b5842308892e0c4f9e772a42d5826f6f62f57be3 specifically mentions that it uses a relative URL to fix the issue, yet the behaviour of Gorilla Mux is to include an absolute URL in the Location header.

While the current fix does indeed include the query parameters, I'm not sure if there's other reasons for preferring a relative URL. Fragments are not sent to the server, so it's a bit harder to guess how browsers behave and if there's value in using a relative vs absolute redirect.

I'm happy to fix that up as well to make it match the standard library if that's what's desired, either as an additional commit in this PR, or as a separate PR after this is merged.

[nvx]

I noticed #639 mentions that UseEncodedPath also affects behaviour elsewhere in decoding path components. Maybe the real answer is that UseEncodedPath shouldn't affect the behaviour of path cleaning and instead the path cleaning behaviour should always act as if it is enabled leaving UseEncodedPath to control the path matching behaviour only?

[francescomari]

@nvx I think there is value in this PR, and I believe that they actually fix a genuine bug. But I'm new here, so take this with a generous grain of salt.

I noticed #639 mentions that UseEncodedPath also affects behaviour elsewhere in decoding path components. Maybe the real answer is that UseEncodedPath shouldn't affect the behaviour of path cleaning and instead the path cleaning behaviour should always act as if it is enabled leaving UseEncodedPath to control the path matching behaviour only?

I'm not sure whether enabling UseEncodedPath by default right now is a good idea because, as you mention, it affects the behaviour around the unescaping of the captured variables. I worked for a fix related to this in #662. I think we could pull off changing the default behaviour of the router if we enable both UseEncodedPath and UnescapeVars (from #662) by default. I think that this wouldn't break the implementation of existing applications, even if they don't activate those flags in their own configuration.

In an unrelated note, Router.UseEncodedPath() does not follow the same conventions as eg Router.SkipClean(bool) in that it is only possible to enable this function, and once called it is not possible to disable.

A similar conversation happened in #662, too. @elithrar offered some guidance about this and preferred to have UnescapeVars not accept any configurable flag in order to match the API of UseEncodedPath. I think this is a minor problem though, and the real focus should be defining what is the most secure default behaviour for the router, as long as we don't break existing code.

Some minor nitpicking: could you please rebase your PR onto the latest master? 91708ff fixed the build for the latest Go version, and that's the only thing that is making the checks fail for your PR.

[nvx]

@nvx I think there is value in this PR, and I believe that they actually fix a genuine bug. But I'm new here, so take this with a generous grain of salt.

I noticed #639 mentions that UseEncodedPath also affects behaviour elsewhere in decoding path components. Maybe the real answer is that UseEncodedPath shouldn't affect the behaviour of path cleaning and instead the path cleaning behaviour should always act as if it is enabled leaving UseEncodedPath to control the path matching behaviour only?

I'm not sure whether enabling UseEncodedPath by default right now is a good idea because, as you mention, it affects the behaviour around the unescaping of the captured variables. I worked for a fix related to this in #662. I think we could pull off changing the default behaviour of the router if we enable both UseEncodedPath and UnescapeVars (from #662) by default. I think that this wouldn't break the implementation of existing applications, even if they don't activate those flags in their own configuration.

In an unrelated note, Router.UseEncodedPath() does not follow the same conventions as eg Router.SkipClean(bool) in that it is only possible to enable this function, and once called it is not possible to disable.

A similar conversation happened in #662, too. @elithrar offered some guidance about this and preferred to have UnescapeVars not accept any configurable flag in order to match the API of UseEncodedPath. I think this is a minor problem though, and the real focus should be defining what is the most secure default behaviour for the router, as long as we don't break existing code.

Note that my changes do not break any existing tests. If we care about backwards compatibility we really need to first start by codifying what we consider valid uses of the library into some new unit tests, then work out how we can maintain compatibility and decide upon what edge cases we're comfortable with breaking in the name of "secure by default".

I daresay breaking a very small (perhaps nonexistent) number of users doing something sufficiently weird is going to be less bad overall than opening the majority of users up to some security vulnerabilities unless a few not very well understood options are enabled.

The way the params and escaping works currently is definitely a bit non-intuitive and I'm sure there's a better route we can take (perhaps matching on the unescaped path, pulling out the params, then unescaping the params after extraction for example) that might incur some minor breakages, but overall result in a better experience for all. The other option of course is the nuclear approach, cut a v2, deprecate v1 and tell everyone to move over with a warning that v1 has some undesirable security edge cases.

Needless to say these decisions are probably a bit larger than this PR, but I'd be happy to throw some commits at it if we can decide on which way forward we want to take.

Some minor nitpicking: could you please rebase your PR onto the latest master? 91708ff fixed the build for the latest Go version, and that's the only thing that is making the checks fail for your PR.

Done

[stale[bot]]

This issue has been automatically marked as stale because it hasn't seen a recent update. It'll be automatically closed in a few days.

[nvx]

this is still relevant

[amustaque97]

this is still relevant

Hey @nvx, sorry for the delay. Are you still following up with the PR? I will be reviewing the PR by this weekend.

[nvx]

this is still relevant

Hey @nvx, sorry for the delay. Are you still following up with the PR? I will be reviewing the PR by this weekend.

Yup. Will be good to see some movement on this.

[codecov[bot]]

Codecov Report

Merging #641 (ad3fe98) into main (395ad81) will increase coverage by 0.13%. Report is 1 commits behind head on main. The diff coverage is 77.77%.

@@            Coverage Diff             @@
##             main     #641      +/-   ##
==========================================
+ Coverage   78.01%   78.14%   +0.13%     
==========================================
  Files           5        5              
  Lines         887      897      +10     
==========================================
+ Hits          692      701       +9     
  Misses        140      140              
- Partials       55       56       +1     

Files Changed	Coverage Δ
route.go	68.46% <ø> (ø)
mux.go	83.76% <77.77%> (+0.23%)	:arrow_up: