Repository: labstack/echo
Pull request #1628 (2020-08-28 22:09:31)

set raw path and path in proxy, so url.EscapePath uses raw path
Some context: 

Golang recommends using EscapedPath instead of calling u.RawPath directly.

URL's String method uses the EscapedPath method to obtain the path. See the EscapedPath method for more details.

Go [reverseproxy](https://github.com/golang/go/blob/master/src/net/http/httputil/reverseproxy.go#L142) uses escapedPath to create ReverseProxy URL.

What's **[escapedPath](https://golang.org/pkg/net/url/#URL.EscapedPath)**?

_EscapedPath returns the escaped form of u.Path. In general, there are multiple possible escaped forms of any path. EscapedPath returns u.RawPath when it is a valid escaping of u.Path. Otherwise EscapedPath ignores u.RawPath and computes an escaped form on its own. The String and RequestURI methods use EscapedPath to construct their results. **In general, code should call EscapedPath instead of reading u.RawPath directly**._

Issue(s): So if we don't set RawPath as a valid escape of Path, EscapedPath ignores RawPath and computes on its own. This can cause double escaping when using `reverseproxy` when you have escaped path parameters e.g.

```
proxying : /user/jill/order/T%2FcO4lW%2Ft%2FVp%2F
would construct proxy URL as : /users/jill/orders/T%252FcO4lW%252Ft%252FVp%252F (double escaping)

```
This PR sets Path and RawPath in proxy.go and rewrite.go using rewritePath from middleware.go and updated proxy_test.go rewrite_test.go to assert with Url String method.

References: https://github.com/golang/go/issues/41082#event-3702348018
URL double escaping if the raw path isn't set: https://play.golang.org/p/rOrVzW8ZJCQ

Also, rewrite was updated with the same logic:

e.g. 

	e.Pre(middleware.Rewrite(map[string]string{
		"/api/*":            "/$1",
	}))

	e.POST("/test/*", func(context echo.Context) error {
		return context.String(http.StatusAccepted, "test endpoint called")
	})


before PR curling: /api/test/EbnfwIoiiXbtr6Ec44sfedeEsjrf0RcXkJneYukTXa%2BIFVla4ZdfRiMzfh%2FEGs7f returns {"message":"Not Found"} 

with this PR curling /api/test/EbnfwIoiiXbtr6Ec44sfedeEsjrf0RcXkJneYukTXa%2BIFVla4ZdfRiMzfh%2FEGs7f returns "test endpoint called"

codecov[bot]: # [Codecov](https://codecov.io/gh/labstack/echo/pull/1628?src=pr&el=h1) Report
> Merging [#1628](https://codecov.io/gh/labstack/echo/pull/1628?src=pr&el=desc) into [master](https://codecov.io/gh/labstack/echo/commit/cb84205219f6b0962ab5bb39ab6c0911f5f1652d?el=desc) will **decrease** coverage by `0.11%`.
> The diff coverage is `69.23%`.

[![Impacted file tree graph](https://codecov.io/gh/labstack/echo/pull/1628/graphs/tree.svg?width=650&height=150&src=pr&token=vqnT76l79i)](https://codecov.io/gh/labstack/echo/pull/1628?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master    #1628      +/-   ##
==========================================
- Coverage   85.12%   85.01%   -0.12%     
==========================================
  Files          28       28              
  Lines        2192     2202      +10     
==========================================
+ Hits         1866     1872       +6     
- Misses        212      214       +2     
- Partials      114      116       +2     
```


| [Impacted Files](https://codecov.io/gh/labstack/echo/pull/1628?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [middleware/proxy.go](https://codecov.io/gh/labstack/echo/pull/1628/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9wcm94eS5nbw==) | `65.71% <66.66%> (-0.29%)` | :arrow_down: |
| [middleware/rewrite.go](https://codecov.io/gh/labstack/echo/pull/1628/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9yZXdyaXRlLmdv) | `67.74% <71.42%> (-1.49%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/labstack/echo/pull/1628?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/labstack/echo/pull/1628?src=pr&el=footer). Last update [cb84205...6b42286](https://codecov.io/gh/labstack/echo/pull/1628?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

lammel: Looking good so far! 
Haven't come around to check on performance regressions yet.  

@arun0009: Does it make sense to use `URL.String()` in the tests, although the actual purpose for testing is against the encoded path where `URL.EscapedPath()` would suffice. So either using `EscapedPath()` or adding additional tests that check on other parts of the URL (like fragment, query params, ...) would make sense.

Patch:
diff --git a/echo.go b/echo.go
index 18c1101..128f84f 100644
--- a/echo.go
+++ b/echo.go
@@ -612,16 +612,15 @@ func (e *Echo) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	// Acquire context
 	c := e.pool.Get().(*context)
 	c.Reset(r, w)
-
 	h := NotFoundHandler
 
 	if e.premiddleware == nil {
-		e.findRouter(r.Host).Find(r.Method, GetPath(r), c)
+		e.findRouter(r.Host).Find(r.Method, r.URL.EscapedPath(), c)
 		h = c.Handler()
 		h = applyMiddleware(h, e.middleware...)
 	} else {
 		h = func(c Context) error {
-			e.findRouter(r.Host).Find(r.Method, GetPath(r), c)
+			e.findRouter(r.Host).Find(r.Method, r.URL.EscapedPath(), c)
 			h := c.Handler()
 			h = applyMiddleware(h, e.middleware...)
 			return h(c)
@@ -832,15 +831,6 @@ func WrapMiddleware(m func(http.Handler) http.Handler) MiddlewareFunc {
 	}
 }
 
-// GetPath returns RawPath, if it's empty returns Path from URL
-func GetPath(r *http.Request) string {
-	path := r.URL.RawPath
-	if path == "" {
-		path = r.URL.Path
-	}
-	return path
-}
-
 func (e *Echo) findRouter(host string) *Router {
 	if len(e.routers) > 0 {
 		if r, ok := e.routers[host]; ok {
diff --git a/middleware/middleware.go b/middleware/middleware.go
index d0b7153..12260dd 100644
--- a/middleware/middleware.go
+++ b/middleware/middleware.go
@@ -1,6 +1,8 @@
 package middleware
 
 import (
+	"net/http"
+	"net/url"
 	"regexp"
 	"strconv"
 	"strings"
@@ -32,6 +34,17 @@ func captureTokens(pattern *regexp.Regexp, input string) *strings.Replacer {
 	return strings.NewReplacer(replace...)
 }
 
+//rewritePath sets request url path and raw path
+func rewritePath(replacer *strings.Replacer, target string, req *http.Request) error {
+	replacerRawPath := replacer.Replace(target)
+	replacerPath, err := url.PathUnescape(replacerRawPath)
+	if err != nil {
+		return err
+	}
+	req.URL.Path, req.URL.RawPath = replacerPath, replacerRawPath
+	return nil
+}
+
 // DefaultSkipper returns false which processes the middleware.
 func DefaultSkipper(echo.Context) bool {
 	return false
diff --git a/middleware/proxy.go b/middleware/proxy.go
index a9b91f6..cd50b76 100644
--- a/middleware/proxy.go
+++ b/middleware/proxy.go
@@ -227,9 +227,12 @@ func ProxyWithConfig(config ProxyConfig) echo.MiddlewareFunc {
 
 			// Rewrite
 			for k, v := range config.rewriteRegex {
-				replacer := captureTokens(k, echo.GetPath(req))
+				//use req.URL.Path here or else we will have double escaping
+				replacer := captureTokens(k, req.URL.Path)
 				if replacer != nil {
-					req.URL.Path = replacer.Replace(v)
+					if err := rewritePath(replacer, v, req); err != nil {
+						return echo.NewHTTPError(http.StatusBadRequest, "invalid url")
+					}
 				}
 			}
 
diff --git a/middleware/rewrite.go b/middleware/rewrite.go
index d1387af..a4d851c 100644
--- a/middleware/rewrite.go
+++ b/middleware/rewrite.go
@@ -1,6 +1,7 @@
 package middleware
 
 import (
+	"net/http"
 	"regexp"
 	"strings"
 
@@ -70,12 +71,14 @@ func RewriteWithConfig(config RewriteConfig) echo.MiddlewareFunc {
 			}
 
 			req := c.Request()
-
 			// Rewrite
 			for k, v := range config.rulesRegex {
+				//use req.URL.Path here or else we will have double escaping
 				replacer := captureTokens(k, req.URL.Path)
 				if replacer != nil {
-					req.URL.Path = replacer.Replace(v)
+					if err := rewritePath(replacer, v, req); err != nil {
+						return echo.NewHTTPError(http.StatusBadRequest, "invalid url")
+					}
 					break
 				}
 			}


Test patch:
diff --git a/middleware/proxy_test.go b/middleware/proxy_test.go
index b19bf4f..4bb7464 100644
--- a/middleware/proxy_test.go
+++ b/middleware/proxy_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+//Assert expected with url.EscapedPath method to obtain the path.
 func TestProxy(t *testing.T) {
 	// Setup
 	t1 := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
@@ -94,22 +95,34 @@ func TestProxy(t *testing.T) {
 		},
 	}))
 	req.URL.Path = "/api/users"
+	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/users", req.URL.Path)
+	assert.Equal(t, "/users", req.URL.EscapedPath())
+	assert.Equal(t, http.StatusOK, rec.Code)
 	req.URL.Path = "/js/main.js"
+	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/public/javascripts/main.js", req.URL.Path)
+	assert.Equal(t, "/public/javascripts/main.js", req.URL.EscapedPath())
+	assert.Equal(t, http.StatusOK, rec.Code)
 	req.URL.Path = "/old"
+	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/new", req.URL.Path)
+	assert.Equal(t, "/new", req.URL.EscapedPath())
+	assert.Equal(t, http.StatusOK, rec.Code)
 	req.URL.Path = "/users/jack/orders/1"
+	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/user/jack/order/1", req.URL.Path)
+	assert.Equal(t, "/user/jack/order/1", req.URL.EscapedPath())
 	assert.Equal(t, http.StatusOK, rec.Code)
 	req.URL.Path = "/users/jill/orders/T%2FcO4lW%2Ft%2FVp%2F"
+	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/user/jill/order/T%2FcO4lW%2Ft%2FVp%2F", req.URL.Path)
+	assert.Equal(t, "/user/jill/order/T%2FcO4lW%2Ft%2FVp%2F", req.URL.EscapedPath())
 	assert.Equal(t, http.StatusOK, rec.Code)
+	req.URL.Path =  "/users/jill/orders/%%%%"
+	rec = httptest.NewRecorder()
+	e.ServeHTTP(rec, req)
+	assert.Equal(t, http.StatusBadRequest, rec.Code)
 
 	// ModifyResponse
 	e = echo.New()
diff --git a/middleware/rewrite_test.go b/middleware/rewrite_test.go
index eb5a46d..dbbcb20 100644
--- a/middleware/rewrite_test.go
+++ b/middleware/rewrite_test.go
@@ -10,6 +10,7 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
+//Assert expected with url.EscapedPath method to obtain the path.
 func TestRewrite(t *testing.T) {
 	e := echo.New()
 	e.Use(RewriteWithConfig(RewriteConfig{
@@ -24,19 +25,31 @@ func TestRewrite(t *testing.T) {
 	rec := httptest.NewRecorder()
 	req.URL.Path = "/api/users"
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/users", req.URL.Path)
+	assert.Equal(t, "/users", req.URL.EscapedPath())
 	req.URL.Path = "/js/main.js"
+	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/public/javascripts/main.js", req.URL.Path)
+	assert.Equal(t, "/public/javascripts/main.js", req.URL.EscapedPath())
 	req.URL.Path = "/old"
+	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/new", req.URL.Path)
+	assert.Equal(t, "/new", req.URL.EscapedPath())
 	req.URL.Path = "/users/jack/orders/1"
+	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/user/jack/order/1", req.URL.Path)
+	assert.Equal(t, "/user/jack/order/1", req.URL.EscapedPath())
 	req.URL.Path = "/api/new users"
+	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/new users", req.URL.Path)
+	assert.Equal(t, "/new%20users", req.URL.EscapedPath())
+	req.URL.Path =  "/users/jill/orders/T%2FcO4lW%2Ft%2FVp%2F"
+	rec = httptest.NewRecorder()
+	e.ServeHTTP(rec, req)
+	assert.Equal(t, "/user/jill/order/T%2FcO4lW%2Ft%2FVp%2F", req.URL.EscapedPath())
+	req.URL.Path =  "/users/jill/orders/%%%%"
+	rec = httptest.NewRecorder()
+	e.ServeHTTP(rec, req)
+	assert.Equal(t, http.StatusBadRequest, rec.Code)
 }
 
 // Issue #1086
@@ -59,7 +72,7 @@ func TestEchoRewritePreMiddleware(t *testing.T) {
 	req := httptest.NewRequest(http.MethodGet, "/old", nil)
 	rec := httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, "/new", req.URL.Path)
+	assert.Equal(t, "/new", req.URL.EscapedPath())
 	assert.Equal(t, 200, rec.Code)
 }
 
@@ -86,7 +99,7 @@ func TestRewriteWithConfigPreMiddleware_Issue1143(t *testing.T) {
 		req := httptest.NewRequest(http.MethodGet, "/api/v1/mgmt/proj/test/agt", nil)
 		rec := httptest.NewRecorder()
 		e.ServeHTTP(rec, req)
-		assert.Equal(t, "/api/v1/hosts/test", req.URL.Path)
+		assert.Equal(t, "/api/v1/hosts/test", req.URL.EscapedPath())
 		assert.Equal(t, 200, rec.Code)
 
 		defer rec.Result().Body.Close()