Issues with URL Params escaping and proposals to fix
- Dominant language
- Go
- Stars
- 22.8k
- Forks
- 1.2k
- Avg merge
- 5h 17m
- Merged PRs (30d)
- 10
Description
### 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
```go
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
Contributor guide
Research direction
Start with URLParam and the URL escaping behavior described in the issue, then compare the linked commits for the proposed router changes. Reproduce both request examples and review the related issues, especially #641 and #148. Done means the router has one documented, consistent contract for URL parameter escaping and the examples behave accordingly.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- go
- Domain
- api, backend
- Issue type
- Bug
- Difficulty
- 5/5
- Estimated time
- Over a week
- Activity status
- Stale
- Clarity
- Mostly clear
- Newbie friendliness
- 25/100