go-chi / go-chi/chi

Issues with URL Params escaping and proposals to fix

Open
#642 14 comments 9 reactions 0 assignees View on GitHub
url-params
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.