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

[ysmood]

trafficstars

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 https://github.com/labstack/echo/blob/74022662be4adf4e6744fb8abacb65516735176f/router.go#L1068-L1075

[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.

[ysmood]

@CodiumAI-Agent /review

[CodiumAI-Agent]

PR Review

⏱️ Estimated effort to review [1-5]	2, because the changes are straightforward and focused on a specific functionality (path parameter unescaping). The PR is small, with changes in one main file and tests added to cover the new functionality. The logic is not complex, but careful attention is needed to ensure that the error handling and the unescaping logic are correctly implemented.
🧪 Relevant tests	Yes
🔍 Possible issues	Possible Bug: If the unescaping of path parameters fails, the current implementation returns an error and halts the processing of the request. This behavior might not be desirable in all cases, as it could lead to service disruption for requests with malformed path parameters.
🔒 Security concerns	No

Code feedback:

---

relevant file	bind.go
suggestion	Consider logging the error when url.QueryUnescape fails before returning the error. This could help in diagnosing issues with malformed path parameters in production environments. [medium]
relevant line	return err

---

relevant file	bind.go
suggestion	It might be beneficial to allow the application to decide whether to halt processing when path parameter unescaping fails. Consider providing a configuration option to control this behavior. [important]
relevant line	return err

---

relevant file	bind_test.go
suggestion	Add a test case to verify the behavior when url.QueryUnescape fails. This will ensure the error handling logic works as expected. [important]
relevant line	c.SetParamValues("John%2FSnow")

---

relevant file	bind_test.go
suggestion	Consider adding more test cases with different types of escaped characters in path parameters to ensure comprehensive coverage of the unescaping logic. [medium]
relevant line	c.SetParamValues("John%2FSnow")

---

---

✨ Review tool usage guide:

---

Overview: The review tool scans the PR code changes, and generates a PR review which includes several types of feedbacks, such as possible PR issues, security threats and relevant test in the PR. More feedbacks can be added by configuring the tool.

The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.

• When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:

/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...

• With a configuration file, use the following template:

[pr_reviewer]
some_config1=...
some_config2=...

See the review usage page for a comprehensive guide on using this tool.

[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.

[ysmood]

Sorry for the noise, I will remove them in 30min, I just picked a random PR to test it.

[ysmood]

Sorry, I don't have the permission, I can only remove my comments, not sure if you want to help.

[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.