fix(Context.Bind): should unescape special char in path

[ysmood]

Other famous frameworks, such as expressjs or rails, will unescape the path params. We should follow the industry convention too.

For example:

package main
​
import (
	"fmt"
​
	"github.com/labstack/echo/v4"
)
​
type Req struct {
	A string `param:"a"`
	B string `query:"b"`
}
​
func main() {
	e := echo.New()
	e.GET("/:a", func(c echo.Context) error {
		req := Req{}
		c.Bind(&req)
		fmt.Println(req.A, req.B)
		return nil
	})
	e.Logger.Fatal(e.Start(":3000"))
}

If we send request curl http://localhost:3000/%26\?b\=%26, it will print %26 & not & &.

If we try expressjs, it won't have the problem:

const express = require("express");
const app = express();

app.get("/:name", (req, res) => {
  res.send(req.params);
});

app.listen("3000");

[ysmood]

@aldas Could you please check this PR?

[aldas]

I have to take some time for it. This is not that simple. Escaped path params are bigger thing that just binding. We have c.Param("myparam") etc.

also there is a question - if we change if path variables are escaped or not - this could cause problems for existing applications that have already coded their own escaping.

We have done something similar in v5 branch where router will escape if flag is set

echo/router.go

Lines 1068 to 1075 in 7402266

	// See issue #1531, #1258 - there are cases when path parameter need to be unescaped
	for i, p := range *pathParams {
	tmpVal, err := url.PathUnescape(p.Value)
	if err == nil { // handle problems by ignoring them.
	(*pathParams)[i].Value = tmpVal
	}
	}
	}

[ysmood]

We have c.Param("myparam") etc.

I think it will only affect path, not others:

Sure it should be part of next major version, since it will change the API.

For v5, I think the flag should be enabled by default.

[aldas]

Please do not start with this AI nonsense.

Escaping/Unescaping of path params starts from the Router, as this is the place where path params are "created". Adding enscaping only to Binder adds inconsistency how methods/functions work as there are other places as I mentioned that you can retrieve params and after this PR the retrieved values are different.

[aldas]

Consider this workaround - if you really need now to unescape path params that router creates you can escape them in middleware.

	e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			params := c.ParamValues()
			if len(params) > 0 {
				for i := range params {
					escaped, err := url.QueryUnescape(params[i])
					if err != nil {
						return err
					}
					params[i] = escaped
				}
				c.SetParamValues(params...)
			}
			return next(c)
		}
	})

Full example:

func main() {
	e := echo.New()

	e.Use(func(next echo.HandlerFunc) echo.HandlerFunc {
		return func(c echo.Context) error {
			params := c.ParamValues()
			if len(params) > 0 {
				for i := range params {
					escaped, err := url.QueryUnescape(params[i])
					if err != nil {
						return err
					}
					params[i] = escaped
				}
				c.SetParamValues(params...)
			}
			return next(c)
		}
	})

	e.GET("/:a", func(c echo.Context) error {
		type Req struct {
			A string `param:"a"`
			B string `query:"b"`
		}

		req := Req{}
		if err := c.Bind(&req); err != nil {
			return err
		}
		fmt.Printf("a='%s' b='%s'\n", req.A, req.B)
		return c.JSON(http.StatusOK, req)
	})

	if err := e.Start(":8080"); err != nil && !errors.Is(err, http.ErrServerClosed) {
		log.Fatal(err)
	}
}

[aldas]

I am reluctant to accept this PR or any related to same issue with Router because the people that want to unescape params and have it as a problem have probably already added their own escaping. Now when we start to automatically escaping they will do the double escape now. This is quite disruptive change in that sense.

and yes - I think we have done similar changes in past but path params is very prominent/popular feature.