Issues with URL Params escaping and proposals to fix
The initial issue
I was faced with issues in URL params escaping. I can show them using two examples, but there could be more. So, clients can send to the server two requests with the next path
/api/first%25%20second- it is a usual path in which all symbols that need to be escaped are escaped/api/first%20(second)- it is the case of how Chrome/Firefox/Safari escape URL in address bar or usingencodeURI(). Parenthesis aren't escaped, but Go escaping algo expects that they are
Then I'm trying to read variable param using pattern /api/{param} with URLParam(r, name) and expecting that
| request URL | Expected param |
Actual param |
|---|---|---|
| /api/first%25%20second | first% second | first% second |
| /api/first%20(second) | first (second) | first%20(second) |
The first case passed correctly but in the second case I read first%20(second) instead of first (second). The main misunderstanding - what I need to do in handlers, do I need to unescape params myself or not, why sometimes params are still escaped?
Example: https://github.com/nawa/chi/commit/b0c424038f640e0d72035edea75ed06589dc5ce0
Having escaped params, I'm assuming that I have to unescape them myself and trying to do that https://github.com/nawa/chi/commit/11ccf398f5bea11486dfbc10f3777f508ba385f7
After that, I see opposite results - the second case passed because first%20(second) is unescaped to first (second) correctly. But the first case failed at all because the string first% second is incorrect to unescape and url.PathUnescape(URLParam(r, name)) returns error
So, as you can see client can't easily determine how to read the correct value
Workaround for the initial issue
Seems that I found a workaround that I'm using now in my handlers to read all URL params
value := URLParam(r, name)
if r.URL.RawPath != "" {
value, _ = url.PathUnescape(value) // it is better to handle error
}
return valueSee full workaround and more test cases - https://github.com/nawa/chi/commit/00f16715671c0749031abb98468af44d1192ff19
Solutions resolving the issues
Described inconsistency should be resolved in chi and it should give a clear understanding to the client - does he always need to unescape params or not
1. All URL params are UNESCAPED in the router and client doesn't need to unescape them. All methods impact benchmarks and contain breaking changes
First method
https://github.com/nawa/chi/commit/92c5ba66d2b99e8799c5a93b5088322b2b6ed1ee
Benchmarks comparison
enchmark old ns/op new ns/op delta
BenchmarkMux/route:/-8 345 353 +2.44%
BenchmarkMux/route:/hi-8 368 380 +3.29%
BenchmarkMux/route:/sup/123/and/this-8 479 584 +21.94%
BenchmarkMux/route:/sup/123/foo/this-8 601 716 +19.10%
BenchmarkMux/route:/sharing/z/aBc-8 600 670 +11.68%
BenchmarkMux/route:/sharing/z/aBc/twitter-8 716 833 +16.23%
BenchmarkMux/route:/sharing/z/aBc/direct-8 842 956 +13.57%
BenchmarkMux/route:/sharing/z/aBc/direct/download-8 940 1131 +20.26%
BenchmarkTreeGet-8 131 153 +16.44%
benchmark old allocs new allocs delta
BenchmarkMux/route:/-8 3 3 +0.00%
BenchmarkMux/route:/hi-8 3 3 +0.00%
BenchmarkMux/route:/sup/123/and/this-8 3 3 +0.00%
BenchmarkMux/route:/sup/123/foo/this-8 3 3 +0.00%
BenchmarkMux/route:/sharing/z/aBc-8 3 3 +0.00%
BenchmarkMux/route:/sharing/z/aBc/twitter-8 4 4 +0.00%
BenchmarkMux/route:/sharing/z/aBc/direct-8 4 4 +0.00%
BenchmarkMux/route:/sharing/z/aBc/direct/download-8 5 5 +0.00%
BenchmarkTreeGet-8 0 0 +0.00%
benchmark old bytes new bytes delta
BenchmarkMux/route:/-8 448 448 +0.00%
BenchmarkMux/route:/hi-8 448 448 +0.00%
BenchmarkMux/route:/sup/123/and/this-8 448 448 +0.00%
BenchmarkMux/route:/sup/123/foo/this-8 448 448 +0.00%
BenchmarkMux/route:/sharing/z/aBc-8 448 448 +0.00%
BenchmarkMux/route:/sharing/z/aBc/twitter-8 456 456 +0.00%
BenchmarkMux/route:/sharing/z/aBc/direct-8 456 456 +0.00%
BenchmarkMux/route:/sharing/z/aBc/direct/download-8 480 480 +0.00%
BenchmarkTreeGet-8 0 0 +0.00%Second method. Based on my workaround
https://github.com/nawa/chi/commit/0be3415486bf835f516da428785563696f95d535
Benchmarks comparison
benchmark old ns/op new ns/op delta
BenchmarkMux/route:/-8 345 340 -1.33%
BenchmarkMux/route:/hi-8 368 356 -3.32%
BenchmarkMux/route:/sup/123/and/this-8 479 475 -0.79%
BenchmarkMux/route:/sup/123/foo/this-8 601 598 -0.58%
BenchmarkMux/route:/sharing/z/aBc-8 600 591 -1.55%
BenchmarkMux/route:/sharing/z/aBc/twitter-8 716 708 -1.10%
BenchmarkMux/route:/sharing/z/aBc/direct-8 842 846 +0.45%
BenchmarkMux/route:/sharing/z/aBc/direct/download-8 940 947 +0.70%
BenchmarkTreeGet-8 131 140 +6.47%
benchmark old allocs new allocs delta
BenchmarkMux/route:/-8 3 3 +0.00%
BenchmarkMux/route:/hi-8 3 3 +0.00%
BenchmarkMux/route:/sup/123/and/this-8 3 3 +0.00%
BenchmarkMux/route:/sup/123/foo/this-8 3 3 +0.00%
BenchmarkMux/route:/sharing/z/aBc-8 3 3 +0.00%
BenchmarkMux/route:/sharing/z/aBc/twitter-8 4 4 +0.00%
BenchmarkMux/route:/sharing/z/aBc/direct-8 4 4 +0.00%
BenchmarkMux/route:/sharing/z/aBc/direct/download-8 5 5 +0.00%
BenchmarkTreeGet-8 0 0 +0.00%
benchmark old bytes new bytes delta
BenchmarkMux/route:/-8 448 448 +0.00%
BenchmarkMux/route:/hi-8 448 448 +0.00%
BenchmarkMux/route:/sup/123/and/this-8 448 448 +0.00%
BenchmarkMux/route:/sup/123/foo/this-8 448 448 +0.00%
BenchmarkMux/route:/sharing/z/aBc-8 448 448 +0.00%
BenchmarkMux/route:/sharing/z/aBc/twitter-8 456 456 +0.00%
BenchmarkMux/route:/sharing/z/aBc/direct-8 456 456 +0.00%
BenchmarkMux/route:/sharing/z/aBc/direct/download-8 480 480 +0.00%
BenchmarkTreeGet-8 0 0 +0.00%It is possible that the benchmarks don't execute URLParam(), so the results are close to the current code
2. All URL params are ESCAPED in the router and client must always unescape params.
This solution isn't implemented because I'm not sure that it is good
3. Just leave the code as is but warn users in README that if they expect to have escaped values then they always need to perform the workaround that I do
value := URLParam(r, name)
if r.URL.RawPath != "" {
value, _ = url.PathUnescape(value) // it is better to handle error
}
return valuePersonally, I like the Solution 1/Second method https://github.com/nawa/chi/commit/0be3415486bf835f516da428785563696f95d535
Related links: https://github.com/go-chi/chi/issues/148, https://github.com/go-chi/chi/commit/822e7b85e22b3f7a573782c3674edf8e732fb427
Any proposed solution solves this issue https://github.com/go-chi/chi/issues/641
Source: go-chi/chi