Repository: labstack/echo
Pull request #1630 (2020-09-03 07:49:13)

bugfix proxy and rewrite, updated test with actual call settings
Issue: Proxy and Rewrite Middleware don't set URL appropriately in tests and this causes different behavior when running tests vs when proxy and rewrite middleware are used in real server mode.

We should use [url Parse](https://golang.org/pkg/net/url/#URL.Parse) method instead.

Check how path, raw path is set when using URL.Parse in this [playground](https://play.golang.org/p/1PvaqYTm7vD) example. This is how url path and raw path is set when running echo server and invoking API endpoints.

After updating tests, was able to find a bug in proxy and rewrite and was able to fix it (and made sure it passes in real scenario calls as well).

Also, refactored some common code that's shared between proxy with rewrite rules and rewrite middleware rules. This also fixed one other bug e.g. the [regex rules](https://github.com/labstack/echo/blob/master/middleware/rewrite.go#L60-L68) for rewrite is different from proxy [regex rules](https://github.com/labstack/echo/blob/master/middleware/proxy.go#L212-L215) when they should be same (rewrite regex rules works as expected).

After refactoring and using common function the below test works in proxy and rewrite (in current master code, proxy test for below scenario would fail):

	u, _:= url.Parse("/api/new users")
	req.URL = u
	rec = httptest.NewRecorder()
	e.ServeHTTP(rec, req)
	assert.Equal(t, "/new%20users", req.URL.EscapedPath()) 

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

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

```diff
@@            Coverage Diff             @@
##           master    #1630      +/-   ##
==========================================
- Coverage   85.28%   85.01%   -0.28%     
==========================================
  Files          28       28              
  Lines        2216     2216              
==========================================
- Hits         1890     1884       -6     
- Misses        212      215       +3     
- Partials      114      117       +3     
```


| [Impacted Files](https://codecov.io/gh/labstack/echo/pull/1630?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [middleware/proxy.go](https://codecov.io/gh/labstack/echo/pull/1630/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9wcm94eS5nbw==) | `64.35% <100.00%> (-1.99%)` | :arrow_down: |
| [middleware/rewrite.go](https://codecov.io/gh/labstack/echo/pull/1630/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9yZXdyaXRlLmdv) | `65.51% <100.00%> (-6.90%)` | :arrow_down: |
| [middleware/middleware.go](https://codecov.io/gh/labstack/echo/pull/1630/diff?src=pr&el=tree#diff-bWlkZGxld2FyZS9taWRkbGV3YXJlLmdv) | `90.00% <0.00%> (-10.00%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/labstack/echo/pull/1630?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/1630?src=pr&el=footer). Last update [28ec3f7...0200aab](https://codecov.io/gh/labstack/echo/pull/1630?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

lammel: I could not check on it yet. Will look into next week.
arun0009: @lammel - I updated the issue description and fixed code accordingly. Let me know if you have any questions.

Patch:
diff --git a/middleware/middleware.go b/middleware/middleware.go
index 12260dd..60834b5 100644
--- a/middleware/middleware.go
+++ b/middleware/middleware.go
@@ -2,7 +2,6 @@ package middleware
 
 import (
 	"net/http"
-	"net/url"
 	"regexp"
 	"strconv"
 	"strings"
@@ -34,15 +33,29 @@ 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
+func rewriteRulesRegex(rewrite map[string]string) map[*regexp.Regexp]string {
+	// Initialize
+	rulesRegex := map[*regexp.Regexp]string{}
+	for k, v := range rewrite {
+		k = regexp.QuoteMeta(k)
+		k = strings.Replace(k, `\*`, "(.*)", -1)
+		if strings.HasPrefix(k, `\^`) {
+			k = strings.Replace(k, `\^`, "^", -1)
+		}
+		k = k + "$"
+		rulesRegex[regexp.MustCompile(k)] = v
+	}
+	return rulesRegex
+}
+
+func rewritePath(rewriteRegex map[*regexp.Regexp]string, req *http.Request) {
+	for k, v := range rewriteRegex {
+		replacerRawPath := captureTokens(k, req.URL.EscapedPath())
+		if replacerRawPath != nil {
+			replacerPath := captureTokens(k, req.URL.Path)
+			req.URL.RawPath, req.URL.Path = replacerRawPath.Replace(v), replacerPath.Replace(v)
+		}
 	}
-	req.URL.Path, req.URL.RawPath = replacerPath, replacerRawPath
-	return nil
 }
 
 // DefaultSkipper returns false which processes the middleware.
diff --git a/middleware/proxy.go b/middleware/proxy.go
index cd50b76..1b972eb 100644
--- a/middleware/proxy.go
+++ b/middleware/proxy.go
@@ -8,7 +8,6 @@ import (
 	"net/http"
 	"net/url"
 	"regexp"
-	"strings"
 	"sync"
 	"sync/atomic"
 	"time"
@@ -206,13 +205,8 @@ func ProxyWithConfig(config ProxyConfig) echo.MiddlewareFunc {
 	if config.Balancer == nil {
 		panic("echo: proxy middleware requires balancer")
 	}
-	config.rewriteRegex = map[*regexp.Regexp]string{}
 
-	// Initialize
-	for k, v := range config.Rewrite {
-		k = strings.Replace(k, "*", "(\\S*)", -1)
-		config.rewriteRegex[regexp.MustCompile(k)] = v
-	}
+	config.rewriteRegex = rewriteRulesRegex(config.Rewrite)
 
 	return func(next echo.HandlerFunc) echo.HandlerFunc {
 		return func(c echo.Context) (err error) {
@@ -225,16 +219,8 @@ func ProxyWithConfig(config ProxyConfig) echo.MiddlewareFunc {
 			tgt := config.Balancer.Next(c)
 			c.Set(config.ContextKey, tgt)
 
-			// Rewrite
-			for k, v := range config.rewriteRegex {
-				//use req.URL.Path here or else we will have double escaping
-				replacer := captureTokens(k, req.URL.Path)
-				if replacer != nil {
-					if err := rewritePath(replacer, v, req); err != nil {
-						return echo.NewHTTPError(http.StatusBadRequest, "invalid url")
-					}
-				}
-			}
+			// Set rewrite path and raw path
+			rewritePath(config.rewriteRegex, req)
 
 			// Fix header
 			// Basically it's not good practice to unconditionally pass incoming x-real-ip header to upstream.
@@ -265,3 +251,5 @@ func ProxyWithConfig(config ProxyConfig) echo.MiddlewareFunc {
 		}
 	}
 }
+
+
diff --git a/middleware/rewrite.go b/middleware/rewrite.go
index 855c863..0965e31 100644
--- a/middleware/rewrite.go
+++ b/middleware/rewrite.go
@@ -1,11 +1,8 @@
 package middleware
 
 import (
-	"net/http"
-	"regexp"
-	"strings"
-
 	"github.com/labstack/echo/v4"
+	"regexp"
 )
 
 type (
@@ -54,18 +51,8 @@ func RewriteWithConfig(config RewriteConfig) echo.MiddlewareFunc {
 	if config.Skipper == nil {
 		config.Skipper = DefaultBodyDumpConfig.Skipper
 	}
-	config.rulesRegex = map[*regexp.Regexp]string{}
 
-	// Initialize
-	for k, v := range config.Rules {
-		k = regexp.QuoteMeta(k)
-		k = strings.Replace(k, `\*`, "(.*)", -1)
-		if strings.HasPrefix(k, `\^`) {
-			k = strings.Replace(k, `\^`, "^", -1)
-		}
-		k = k + "$"
-		config.rulesRegex[regexp.MustCompile(k)] = v
-	}
+	config.rulesRegex = rewriteRulesRegex(config.Rules)
 
 	return func(next echo.HandlerFunc) echo.HandlerFunc {
 		return func(c echo.Context) (err error) {
@@ -74,17 +61,8 @@ 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 {
-					if err := rewritePath(replacer, v, req); err != nil {
-						return echo.NewHTTPError(http.StatusBadRequest, "invalid url")
-					}
-					break
-				}
-			}
+			// Set rewrite path and raw path
+			rewritePath(config.rulesRegex, req)
 			return next(c)
 		}
 	}


Test patch:
diff --git a/middleware/proxy_test.go b/middleware/proxy_test.go
index 4bb7464..534e45f 100644
--- a/middleware/proxy_test.go
+++ b/middleware/proxy_test.go
@@ -94,36 +94,35 @@ func TestProxy(t *testing.T) {
 			"/users/*/orders/*": "/user/$1/order/$2",
 		},
 	}))
-	req.URL.Path = "/api/users"
+	req.URL, _ = url.Parse("/api/users")
 	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
 	assert.Equal(t, "/users", req.URL.EscapedPath())
 	assert.Equal(t, http.StatusOK, rec.Code)
-	req.URL.Path = "/js/main.js"
+	req.URL, _ = url.Parse( "/js/main.js")
 	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
 	assert.Equal(t, "/public/javascripts/main.js", req.URL.EscapedPath())
 	assert.Equal(t, http.StatusOK, rec.Code)
-	req.URL.Path = "/old"
+	req.URL, _ = url.Parse("/old")
 	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
 	assert.Equal(t, "/new", req.URL.EscapedPath())
 	assert.Equal(t, http.StatusOK, rec.Code)
-	req.URL.Path = "/users/jack/orders/1"
+	req.URL, _ = url.Parse( "/users/jack/orders/1")
 	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
 	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"
+	req.URL, _ = url.Parse("/user/jill/order/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())
 	assert.Equal(t, http.StatusOK, rec.Code)
-	req.URL.Path =  "/users/jill/orders/%%%%"
+	req.URL, _ = url.Parse("/api/new users")
 	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, http.StatusBadRequest, rec.Code)
-
+	assert.Equal(t, "/new%20users", req.URL.EscapedPath())
 	// ModifyResponse
 	e = echo.New()
 	e.Use(ProxyWithConfig(ProxyConfig{
diff --git a/middleware/rewrite_test.go b/middleware/rewrite_test.go
index a9b3437..abf11b2 100644
--- a/middleware/rewrite_test.go
+++ b/middleware/rewrite_test.go
@@ -4,6 +4,7 @@ import (
 	"io/ioutil"
 	"net/http"
 	"net/http/httptest"
+	"net/url"
 	"testing"
 
 	"github.com/labstack/echo/v4"
@@ -23,33 +24,28 @@ func TestRewrite(t *testing.T) {
 	}))
 	req := httptest.NewRequest(http.MethodGet, "/", nil)
 	rec := httptest.NewRecorder()
-	req.URL.Path = "/api/users"
+	req.URL, _ = url.Parse("/api/users")
 	e.ServeHTTP(rec, req)
 	assert.Equal(t, "/users", req.URL.EscapedPath())
-	req.URL.Path = "/js/main.js"
+	req.URL, _ = url.Parse("/js/main.js")
 	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
 	assert.Equal(t, "/public/javascripts/main.js", req.URL.EscapedPath())
-	req.URL.Path = "/old"
+	req.URL, _ = url.Parse("/old")
 	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
 	assert.Equal(t, "/new", req.URL.EscapedPath())
-	req.URL.Path = "/users/jack/orders/1"
+	req.URL, _ = url.Parse("/users/jack/orders/1")
 	rec = httptest.NewRecorder()
 	e.ServeHTTP(rec, req)
 	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%20users", req.URL.EscapedPath())
-	req.URL.Path = "/users/jill/orders/T%2FcO4lW%2Ft%2FVp%2F"
+	req.URL, _ = url.Parse("/user/jill/order/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()
+	req.URL, _ = url.Parse("/api/new users")
 	e.ServeHTTP(rec, req)
-	assert.Equal(t, http.StatusBadRequest, rec.Code)
+	assert.Equal(t, "/new%20users", req.URL.EscapedPath())
 }
 
 // Issue #1086
@@ -58,11 +54,10 @@ func TestEchoRewritePreMiddleware(t *testing.T) {
 	r := e.Router()
 
 	// Rewrite old url to new one
-	e.Pre(RewriteWithConfig(RewriteConfig{
-		Rules: map[string]string{
+	e.Pre(Rewrite(map[string]string{
 			"/old": "/new",
 		},
-	}))
+	))
 
 	// Route
 	r.Add(http.MethodGet, "/new", func(c echo.Context) error {