Fix performance regression caused by path escaping (#1777, #1798, #1799)

* Fix performance regression #1777 and avoid double escaping in rewrite/proxy middleware.
* Add rewrite test for correct escaping of replacement (#1798)

Co-authored-by: Roland Lammel <[email protected]>

Author: aldas

echo.go

@@ -629,12 +629,12 @@ func (e *Echo) ServeHTTP(w http.ResponseWriter, r *http.Request) {
 	h := NotFoundHandler
 
 	if e.premiddleware == nil {
-		e.findRouter(r.Host).Find(r.Method, r.URL.EscapedPath(), c)
+		e.findRouter(r.Host).Find(r.Method, GetPath(r), c)
 		h = c.Handler()
 		h = applyMiddleware(h, e.middleware...)
 	} else {
 		h = func(c Context) error {
-			e.findRouter(r.Host).Find(r.Method, r.URL.EscapedPath(), c)
+			e.findRouter(r.Host).Find(r.Method, GetPath(r), c)
 			h := c.Handler()
 			h = applyMiddleware(h, e.middleware...)
 			return h(c)
@@ -909,6 +909,18 @@ func WrapMiddleware(m func(http.Handler) http.Handler) MiddlewareFunc {
 	}
 }
 
+// GetPath returns RawPath, if it's empty returns Path from URL
+// Difference between RawPath and Path is:
+//  * Path is where request path is stored. Value is stored in decoded form: /%47%6f%2f becomes /Go/.
+//  * RawPath is an optional field which only gets set if the default encoding is different from Path.
+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 {

echo_test.go

@@ -468,15 +468,46 @@ func TestEchoRoutes(t *testing.T) {
 	}
 }
 
-func TestEchoEncodedPath(t *testing.T) {
+func TestEchoServeHTTPPathEncoding(t *testing.T) {
 	e := New()
+	e.GET("/with/slash", func(c Context) error {
+		return c.String(http.StatusOK, "/with/slash")
+	})
 	e.GET("/:id", func(c Context) error {
-		return c.NoContent(http.StatusOK)
+		return c.String(http.StatusOK, c.Param("id"))
 	})
-	req := httptest.NewRequest(http.MethodGet, "/with%2Fslash", nil)
-	rec := httptest.NewRecorder()
-	e.ServeHTTP(rec, req)
-	assert.Equal(t, http.StatusOK, rec.Code)
+
+	var testCases = []struct {
+		name         string
+		whenURL      string
+		expectURL    string
+		expectStatus int
+	}{
+		{
+			name:         "url with encoding is not decoded for routing",
+			whenURL:      "/with%2Fslash",
+			expectURL:    "with%2Fslash", // `%2F` is not decoded to `/` for routing
+			expectStatus: http.StatusOK,
+		},
+		{
+			name:         "url without encoding is used as is",
+			whenURL:      "/with/slash",
+			expectURL:    "/with/slash",
+			expectStatus: http.StatusOK,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil)
+			rec := httptest.NewRecorder()
+
+			e.ServeHTTP(rec, req)
+
+			assert.Equal(t, tc.expectStatus, rec.Code)
+			assert.Equal(t, tc.expectURL, rec.Body.String())
+		})
+	}
 }
 
 func TestEchoGroup(t *testing.T) {
@@ -1211,3 +1242,49 @@ func TestEcho_StartServer(t *testing.T) {
 		})
 	}
 }
+
+func benchmarkEchoRoutes(b *testing.B, routes []*Route) {
+	e := New()
+	req := httptest.NewRequest("GET", "/", nil)
+	u := req.URL
+	w := httptest.NewRecorder()
+
+	b.ReportAllocs()
+
+	// Add routes
+	for _, route := range routes {
+		e.Add(route.Method, route.Path, func(c Context) error {
+			return nil
+		})
+	}
+
+	// Find routes
+	b.ResetTimer()
+	for i := 0; i < b.N; i++ {
+		for _, route := range routes {
+			req.Method = route.Method
+			u.Path = route.Path
+			e.ServeHTTP(w, req)
+		}
+	}
+}
+
+func BenchmarkEchoStaticRoutes(b *testing.B) {
+	benchmarkEchoRoutes(b, staticRoutes)
+}
+
+func BenchmarkEchoStaticRoutesMisses(b *testing.B) {
+	benchmarkEchoRoutes(b, staticRoutes)
+}
+
+func BenchmarkEchoGitHubAPI(b *testing.B) {
+	benchmarkEchoRoutes(b, gitHubAPI)
+}
+
+func BenchmarkEchoGitHubAPIMisses(b *testing.B) {
+	benchmarkEchoRoutes(b, gitHubAPI)
+}
+
+func BenchmarkEchoParseAPI(b *testing.B) {
+	benchmarkEchoRoutes(b, parseAPI)
+}

middleware/middleware.go

@@ -2,6 +2,7 @@ package middleware
 
 import (
 	"net/http"
+	"net/url"
 	"regexp"
 	"strconv"
 	"strings"
@@ -50,10 +51,26 @@ func rewriteRulesRegex(rewrite map[string]string) map[*regexp.Regexp]string {
 
 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)
+		rawPath := req.URL.RawPath
+		if rawPath != "" {
+			// RawPath is only set when there has been escaping done. In that case Path must be deduced from rewritten RawPath
+			// because encoded Path could match rules that RawPath did not
+			if replacer := captureTokens(k, rawPath); replacer != nil {
+				rawPath = replacer.Replace(v)
+
+				req.URL.RawPath = rawPath
+				req.URL.Path, _ = url.PathUnescape(rawPath)
+
+				return // rewrite only once
+			}
+
+			continue
+		}
+
+		if replacer := captureTokens(k, req.URL.Path); replacer != nil {
+			req.URL.Path = replacer.Replace(v)
+
+			return // rewrite only once
 		}
 	}
 }

middleware/middleware_test.go

@@ -0,0 +1,69 @@
+package middleware
+
+import (
+	"github.com/stretchr/testify/assert"
+	"net/http"
+	"net/http/httptest"
+	"regexp"
+	"testing"
+)
+
+func TestRewritePath(t *testing.T) {
+	var testCases = []struct {
+		whenURL       string
+		expectPath    string
+		expectRawPath string
+	}{
+		{
+			whenURL:       "http://localhost:8080/old",
+			expectPath:    "/new",
+			expectRawPath: "",
+		},
+		{ // encoded `ol%64` (decoded `old`) should not be rewritten to `/new`
+			whenURL:       "/ol%64", // `%64` is decoded `d`
+			expectPath:    "/old",
+			expectRawPath: "/ol%64",
+		},
+		{
+			whenURL:       "http://localhost:8080/users/+_+/orders/___++++?test=1",
+			expectPath:    "/user/+_+/order/___++++",
+			expectRawPath: "",
+		},
+		{
+			whenURL:       "http://localhost:8080/users/%20a/orders/%20aa",
+			expectPath:    "/user/ a/order/ aa",
+			expectRawPath: "",
+		},
+		{
+			whenURL:       "http://localhost:8080/%47%6f%2f",
+			expectPath:    "/Go/",
+			expectRawPath: "/%47%6f%2f",
+		},
+		{
+			whenURL:       "/users/jill/orders/T%2FcO4lW%2Ft%2FVp%2F",
+			expectPath:    "/user/jill/order/T/cO4lW/t/Vp/",
+			expectRawPath: "/user/jill/order/T%2FcO4lW%2Ft%2FVp%2F",
+		},
+		{ // do nothing, replace nothing
+			whenURL:       "http://localhost:8080/user/jill/order/T%2FcO4lW%2Ft%2FVp%2F",
+			expectPath:    "/user/jill/order/T/cO4lW/t/Vp/",
+			expectRawPath: "/user/jill/order/T%2FcO4lW%2Ft%2FVp%2F",
+		},
+	}
+
+	rules := map[*regexp.Regexp]string{
+		regexp.MustCompile("^/old$"):                      "/new",
+		regexp.MustCompile("^/users/(.*?)/orders/(.*?)$"): "/user/$1/order/$2",
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.whenURL, func(t *testing.T) {
+			req := httptest.NewRequest(http.MethodGet, tc.whenURL, nil)
+
+			rewritePath(rules, req)
+
+			assert.Equal(t, tc.expectPath, req.URL.Path)       // Path field is stored in decoded form: /%47%6f%2f becomes /Go/.
+			assert.Equal(t, tc.expectRawPath, req.URL.RawPath) // RawPath, an optional field which only gets set if the default encoding is different from Path.
+		})
+	}
+}