Echo.Reverse doesn't encode parameters

[sazzer]

Issue Description

When using Echo.Reverse() to generate URLs to other handlers, the resulting URL is not correctly encoded. This can mean that the URLs generated are not valid.

For example, if I pass in a value of "abc?def" as a parameter to Echo.Reverse(), I'd expect the resulting URL to be "abc%3Fdef". Instead it's just "abc?def", which then means that the resulting URL is wrong - the "?def" becomes the start of the query string and not part of the path parameter.

Checklist

• Dependencies installed
• No typos
• Searched existing issues and docs

Expected behaviour

The parameters passed in to the URL should be URL encoded.

Actual behaviour

The parameters are not URL encoded.

Working code to debug

package main

import (
	"github.com/labstack/echo/v4"
)

func main() {
    server := echo.New()
    server.Add("GET", "/example/:value", func (c echo.Context) error {
        return c.String(200, c.Echo().Reverse("example_route", c.Param("value")));
    }).Name = "example_route";
    server.Start(":8000");
}

Then call with:

-> % http localhost:8000/example/abc%3fdef
HTTP/1.1 200 OK
Content-Length: 16
Content-Type: text/plain; charset=UTF-8
Date: Fri, 29 Apr 2022 18:01:35 GMT

/example/abc?def

-> % http localhost:8000/example/abc?def
HTTP/1.1 200 OK
Content-Length: 16
Content-Type: text/plain; charset=UTF-8
Date: Fri, 29 Apr 2022 18:01:35 GMT

/example/abc

Version/commit

• github.com/labstack/echo/v4 v4.7.2

[aldas]

@sazzer are you sure that http or your cli is not converting that %3f to ?.

When I try with curl I get this

x@x:~/code$ curl -v localhost:8000/example/abc%3fdef
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /example/abc%3fdef HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.74.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: text/plain; charset=UTF-8
< Date: Fri, 29 Apr 2022 18:11:58 GMT
< Content-Length: 18
< 
* Connection #0 to host localhost left intact
/example/abc%3fdef

Note: when you request localhost:8000/example/abc?def it is correct that current code returns /example/abc because first ? in request uri is start of query (RFC 3986 3.4 Query) and Go standard library (which does request parsing) does not include it in path and therefore c.Param("value") can not return anything but abc in this case.

Now issue if echo.Reverse() should encode parameter values or not is different. As Echo does not encode/escape parameter values (ones that you can access with c.Param("value")) it probably does not make sense for Reverse do it also.

For example take this:

func main() {
	server := echo.New()
	server.Add("GET", "/example/:value", func(c echo.Context) error {
		value := url.PathEscape(c.Param("value"))
		return c.String(200, c.Echo().Reverse("example_route", value))
	}).Name = "example_route"
	server.Start(":8000")
}

and lets test it with

x@x:~/code$ curl -v localhost:8000/example/abc%3fdef
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /example/abc%3fdef HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.74.0
> Accept: */*
> 
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: text/plain; charset=UTF-8
< Date: Fri, 29 Apr 2022 18:31:43 GMT
< Content-Length: 20
< 
* Connection #0 to host localhost left intact
/example/abc%253fdef

Result is /example/abc%253fdef - is it better?

Anyway better test/example would be

func TestEcho_Reverse(t *testing.T) {
	e := New()
	e.GET("/example/:value", func(Context) error { return nil }).Name = "myHandler"

	assert.Equal(t, "/example/abc%3fdef", e.Reverse("myHandler", "abc%3fdef"))
	assert.Equal(t, "/example/abc%3fdef", e.Reverse("myHandler", "abc?def")) // Actual: `/example/abc?def`
}

[sazzer]

I've just tweaked my test a bit to output the values that Go sees to the terminal, so the test now looks like:

package main

import (
    "fmt"

    "github.com/labstack/echo/v4"
)

func main() {
    server := echo.New()
    server.Add("GET", "/example/:value", func (c echo.Context) error {
        value := c.Param("value")
        url := c.Echo().Reverse("example_route", value)

        fmt.Printf("Raw value: %s\nBuilt URI: %s\n", value, url)

        return c.String(200, url)
    }).Name = "example_route";
    server.Start(":8000");
}

Curiously it does act differently between http and curl, but I'm unsure why...

With http I get:

-> % http -v localhost:8000/example/abc%3fdef
GET /example/abc%3Fdef HTTP/1.1
Accept: */*
Accept-Encoding: gzip, deflate
Connection: keep-alive
Host: localhost:8000
User-Agent: HTTPie/3.1.0



HTTP/1.1 200 OK
Content-Length: 16
Content-Type: text/plain; charset=UTF-8
Date: Sat, 30 Apr 2022 08:07:54 GMT

/example/abc?def

And output from the test app is:

Raw value: abc?def
Built URI: /example/abc?def

However, if I use curl then I get this:

-> % curl -v localhost:8000/example/abc%3fdef
*   Trying 127.0.0.1:8000...
* Connected to localhost (127.0.0.1) port 8000 (#0)
> GET /example/abc%3fdef HTTP/1.1
> Host: localhost:8000
> User-Agent: curl/7.79.1
> Accept: */*
>
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Content-Type: text/plain; charset=UTF-8
< Date: Sat, 30 Apr 2022 08:07:56 GMT
< Content-Length: 18
<
* Connection #0 to host localhost left intact
/example/abc%3fdef

And the output is:

Raw value: abc%3fdef
Built URI: /example/abc%3fdef

And then doing it in Firefox acts the same as curl!

I've also run this through Wireshark to make sure that the output from http isn't lying, and it's not. It reall

Notably, the URL that both http and curl are sending is almost identical, but the way the server handles it is not! And, in fact, that "almost" is important. It turns out that if you send %3f like curl has then the value is not decoded in the app, but if you send %3F then it is decoded. So I think there are actually two issues here, and the second one is much more serious than the first!

1. The reverse router isn't encoding values, meaning that you generate incorrect URLs
2. The inbound router sometimes decodes values but not always.

That second one is more serious because it's impossible to tell if a value needs decoding or not. Take for example the input abc%253fdef. Echo correctly decodes the %25 here into a % symbol, but if the application things it needs to decode it again then it will decode what is now %3f into a ? - and thus get the wrong string. However, it's impossible to tell from the value abc%3fdef if that's an input of abc%253fdef that has already been decoded, or an input of abc%3fdef that has not yet been decoded.