Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ysmood
Copy link

@ysmood ysmood commented Jul 12, 2023

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 ysmood changed the title fix(Context.Bind): should escape special char in path fix(Context.Bind): should unescape special char in path Jul 12, 2023
Other famous frameworks, such as expressjs or rails, will unescape the path params. We should follow the industry convention too.
@ysmood
Copy link
Author

ysmood commented Jul 17, 2023

@aldas Could you please check this PR?

@aldas
Copy link
Contributor

aldas commented Jul 17, 2023

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
Copy link
Author

ysmood commented Jul 17, 2023

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

I think it will only affect path, not others:

image

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

This comment was marked as off-topic.

@CodiumAI-Agent

This comment was marked as resolved.

@aldas
Copy link
Contributor

aldas commented Apr 4, 2024

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

This comment was marked as off-topic.

@ysmood

This comment was marked as off-topic.

@aldas
Copy link
Contributor

aldas commented Apr 4, 2024

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
Copy link
Contributor

aldas commented Apr 4, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants