Fix accidental double-escaping when Router.UseEncodedPath() is enabled #641
Conversation
|
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. |
|
Something else that stood out when writing the unit tests for this is d10d546 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 golang/go@b584230 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. |
|
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? |
|
@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'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.
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
force-pushed
the
fix/use_encoded_path_escaping
branch
from
ea5fda2
to
0228de0
Compare
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.
Done |
|
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. |
|
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 Report
@@ 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
|
Bug originally mentioned in #354 (comment)
Summary of Changes
When Router.UseEncodedPath() is enabled (and SkipClean is not enabled), a request to
http://localhost/foo/../bar%25causes a redirect tohttp://localhost/bar%2525as 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:The remaining changes allows the new unit test to pass and does not break any existing unit tests.