Issues with URL Params escaping and proposals to fix

[nawa]

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 using encodeURI(). 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 value

See 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 value

---

Personally, 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

[nawa]

@pkieltyka Is it not important? Many users are faced with the issue

I made detailed investigation and proposed few solutions how to resolve it, could you please share any thoughts on it? If we agree about some decision I would be glad to make a PR for that

[tomqwpl]

Yep, also have this problem, and I'm surprised more don't (I suspect they do, they just don't know it :-))

[Hellysonrp]

I have this problem too. My development environment is Windows, and I don't have this problem, but in my staging and production environments (Linux machines), I can consistently find this issue in any path with a parenthesis.

[nawa]

@Hellysonrp Could you try the workaround I described?

value := URLParam(r, name)
if r.URL.RawPath != "" {
	value, _ = url.PathUnescape(value) // it is better to handle the error
}

return value

https://github.com/nawa/chi/commit/00f16715671c0749031abb98468af44d1192ff19

[Hellysonrp]

Could you try the workaround I described?

The workaround works flawlessly both on Windows and Linux. I don't know if it has any effect on Windows because I didn't have this issue with Windows, but at least now the same code works as expected in both systems. Thank you for the workaround :slightly_smiling_face:

[aksdb]

We also just ran into this problem and I am a little shocked that this is "known" for so long and not tackled yet, even though there is a proposal (or even multiple ones) for a fix.

Thanks a lot for the workaround!

[pkieltyka]

The fix on your end to pick your solution is an easy one. I haven’t had the head space to really feel put this problem and decide on the best intuition to land on an opinionated decision while keeping all previous goals and performance in tact.

[aksdb]

@pkieltyka Thanks for the feedback and sorry for my slightly harsh words. Keep up the good work!

[silverwind]

Another test case:

a b path param is decoded as a b (ok) a & b path param is decoded as a%20%26%20b (fail)

I'll use PathUnescape to manually decode path parameters and I suggest chi to do the same. I'm quite shocked actually that this is not fixed yet.

[gucki]

@pkieltyka Why not simply introduce an additional method URLParamDecoded which uses the proposed workaround?

[andrewerogers]

Another test case which we surfaced in integration tests:

/v1/internal/resource//resource2/subscribed is encoded to /v1/internal/resource/%2Fresource2%2Fsubscribed

However, the request in this case is actually mapped to a separate handler on this path v1/internal/resource/{resourceId} so the aforementioned workarounds do not work in this case. The request is mapped to the wrong handler in the first place.

Note: This is not performance degrading given an empty path such as this should result in a bad request, but can pose security vulnerability if proper sanitation is not in place.

[treeder]

I surprisingly just ran into this after many years of using Chi...

@nawa 's workaround works (url.PathUnescape(value)), but now I feel like I should be doing that everywhere I'm using chi.URLParam which doesn't seem like a nice solution.