fix URL Rewrite issues (#2301)

Fixes #2268 
Fixes #1598 
Fixes #2618 

`URLRewrite` middleware changes `r.URL` to the `target` path. This breaks the middleware chain, as each middleware using `r.URL`, to check if it is enabled. So instead of changing `r.URL` directly in the URLRewrite middleware, I moved it to DummyProxyHandler.

Similarly, I also changed `Method Tranform` middleware.

The PR also changes `cache` middleware, moving it at the end of middleware chain and also incorporating above changes.

ctx/ctx.go

@@ -10,8 +10,10 @@ import (
 	"github.com/TykTechnologies/tyk/user"
 )
 
+type Key uint
+
 const (
-	SessionData = iota
+	SessionData Key = iota
 	UpdateSession
 	AuthToken
 	HashedAuthToken
@@ -31,6 +33,8 @@ const (
 	ThrottleLevelLimit
 	Trace
 	CheckLoopLimits
+	UrlRewriteTarget
+	TransformedRequestMethod
 	Definition
 	RequestStatus
 )

gateway/api.go

@@ -2061,6 +2061,20 @@ func ctxGetOrigRequestURL(r *http.Request) *url.URL {
 	return nil
 }
 
+func ctxSetURLRewriteTarget(r *http.Request, url *url.URL) {
+	setCtxValue(r, ctx.UrlRewriteTarget, url)
+}
+
+func ctxGetURLRewriteTarget(r *http.Request) *url.URL {
+	if v := r.Context().Value(ctx.UrlRewriteTarget); v != nil {
+		if urlVal, ok := v.(*url.URL); ok {
+			return urlVal
+		}
+	}
+
+	return nil
+}
+
 func ctxSetUrlRewritePath(r *http.Request, path string) {
 	setCtxValue(r, ctx.UrlRewritePath, path)
 }
@@ -2105,6 +2119,19 @@ func ctxGetRequestMethod(r *http.Request) string {
 	return r.Method
 }
 
+func ctxSetTransformRequestMethod(r *http.Request, path string) {
+	setCtxValue(r, ctx.TransformedRequestMethod, path)
+}
+
+func ctxGetTransformRequestMethod(r *http.Request) string {
+	if v := r.Context().Value(ctx.TransformedRequestMethod); v != nil {
+		if strVal, ok := v.(string); ok {
+			return strVal
+		}
+	}
+	return r.Method
+}
+
 func ctxGetDefaultVersion(r *http.Request) bool {
 	return r.Context().Value(ctx.VersionDefault) != nil
 }

gateway/api_loader.go

@@ -401,7 +401,6 @@ func processSpec(spec *APISpec, apisByListen map[string]int,
 	mwAppendEnabled(&chainArray, &TransformHeaders{BaseMiddleware: baseMid})
 	mwAppendEnabled(&chainArray, &URLRewriteMiddleware{BaseMiddleware: baseMid})
 	mwAppendEnabled(&chainArray, &TransformMethod{BaseMiddleware: baseMid})
-	mwAppendEnabled(&chainArray, &RedisCacheMiddleware{BaseMiddleware: baseMid, CacheStore: &cacheStore})
 	mwAppendEnabled(&chainArray, &VirtualEndpoint{BaseMiddleware: baseMid})
 	mwAppendEnabled(&chainArray, &RequestSigning{BaseMiddleware: baseMid})
 
@@ -422,7 +421,9 @@ func processSpec(spec *APISpec, apisByListen map[string]int,
 			chainArray = append(chainArray, createDynamicMiddleware(obj.Name, false, obj.RequireSession, baseMid))
 		}
 	}
-
+	//Do not add middlewares after cache middleware.
+	//It will not get executed
+	mwAppendEnabled(&chainArray, &RedisCacheMiddleware{BaseMiddleware: baseMid, CacheStore: &cacheStore})
 	chain = alice.New(chainArray...).Then(&DummyProxyHandler{SH: SuccessHandler{baseMid}})
 
 	if !spec.UseKeylessAccess {
@@ -489,6 +490,14 @@ type DummyProxyHandler struct {
 }
 
 func (d *DummyProxyHandler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
+	if newURL := ctxGetURLRewriteTarget(r); newURL != nil {
+		r.URL = newURL
+		ctxSetURLRewriteTarget(r, nil)
+	}
+	if newMethod := ctxGetTransformRequestMethod(r); newMethod != "" {
+		r.Method = newMethod
+		ctxSetTransformRequestMethod(r, "")
+	}
 	if found, err := isLoop(r); found {
 		if err != nil {
 			handler := ErrorHandler{*d.SH.Base()}

gateway/gateway_test.go

@@ -1206,6 +1206,61 @@ func TestCacheAllSafeRequests(t *testing.T) {
 	}...)
 }
 
+func TestCacheWithAdvanceUrlRewrite(t *testing.T) {
+	ts := StartTest()
+	defer ts.Close()
+	cache := storage.RedisCluster{KeyPrefix: "cache-"}
+	defer cache.DeleteScanMatch("*")
+
+	BuildAndLoadAPI(func(spec *APISpec) {
+		version := spec.VersionData.Versions["v1"]
+		version.UseExtendedPaths = true
+		version.ExtendedPaths = apidef.ExtendedPathsSet{
+			URLRewrite: []apidef.URLRewriteMeta{
+				{
+					Path:         "/test",
+					Method:       http.MethodGet,
+					MatchPattern: "/test(.*)",
+					Triggers: []apidef.RoutingTrigger{
+						{
+							On: "all",
+							Options: apidef.RoutingTriggerOptions{
+								HeaderMatches: map[string]apidef.StringRegexMap{
+									"rewritePath": apidef.StringRegexMap{
+										MatchPattern: "newpath",
+									},
+								},
+							},
+							RewriteTo: "/newpath",
+						},
+					},
+				},
+			},
+			Cached: []string{"/test"},
+		}
+		spec.CacheOptions = apidef.CacheOptions{
+			CacheTimeout: 120,
+			EnableCache:  true,
+		}
+		spec.Proxy.ListenPath = "/"
+		spec.VersionData.Versions["v1"] = version
+	})
+
+	headerCache := map[string]string{"x-tyk-cached-response": "1"}
+	matchHeaders := map[string]string{"rewritePath": "newpath"}
+	randomheaders := map[string]string{"something": "abcd"}
+
+	ts.Run(t, []test.TestCase{
+		{Method: http.MethodGet, Path: "/test", Headers: matchHeaders, HeadersNotMatch: headerCache, Delay: 10 * time.Millisecond},
+		{Method: http.MethodGet, Path: "/test", Headers: matchHeaders, HeadersMatch: headerCache},
+		//Even if trigger condition failed, as response is cached
+		// will still get redirected response
+		{Method: http.MethodGet, Path: "/test", Headers: randomheaders, HeadersMatch: headerCache, BodyMatch: `"Url":"/newpath"`},
+		{Method: http.MethodPost, Path: "/test", HeadersNotMatch: headerCache},
+		{Method: http.MethodGet, Path: "/test", HeadersMatch: headerCache},
+	}...)
+}
+
 func TestCachePostRequest(t *testing.T) {
 	ts := StartTest()
 	defer ts.Close()
@@ -1220,12 +1275,13 @@ func TestCachePostRequest(t *testing.T) {
 		}
 
 		UpdateAPIVersion(spec, "v1", func(v *apidef.VersionInfo) {
-			json.Unmarshal([]byte(`[{
-						"method":"POST",
-						"path":"/",
-						"cache_key_regex":"\"id\":[^,]*"
-					}
-                                ]`), &v.ExtendedPaths.AdvanceCacheConfig)
+			v.ExtendedPaths.AdvanceCacheConfig = []apidef.CacheMeta{
+				{
+					Method:        http.MethodPost,
+					Path:          "/",
+					CacheKeyRegex: "\"id\":[^,]*",
+				},
+			}
 		})
 
 		spec.Proxy.ListenPath = "/"
@@ -1234,12 +1290,12 @@ func TestCachePostRequest(t *testing.T) {
 	headerCache := map[string]string{"x-tyk-cached-response": "1"}
 
 	ts.Run(t, []test.TestCase{
-		{Method: "POST", Path: "/", Data: "{\"id\":\"1\",\"name\":\"test\"}", HeadersNotMatch: headerCache, Delay: 10 * time.Millisecond},
-		{Method: "POST", Path: "/", Data: "{\"id\":\"1\",\"name\":\"test\"}", HeadersMatch: headerCache, Delay: 10 * time.Millisecond},
-		{Method: "POST", Path: "/", Data: "{\"id\":\"2\",\"name\":\"test\"}", HeadersNotMatch: headerCache, Delay: 10 * time.Millisecond},
+		{Method: http.MethodPost, Path: "/", Data: "{\"id\":\"1\",\"name\":\"test\"}", HeadersNotMatch: headerCache, Delay: 10 * time.Millisecond},
+		{Method: http.MethodPost, Path: "/", Data: "{\"id\":\"1\",\"name\":\"test\"}", HeadersMatch: headerCache, Delay: 10 * time.Millisecond},
+		{Method: http.MethodPost, Path: "/", Data: "{\"id\":\"2\",\"name\":\"test\"}", HeadersNotMatch: headerCache, Delay: 10 * time.Millisecond},
 		// if regex match returns nil, then request body is ignored while generating cache key
-		{Method: "POST", Path: "/", Data: "{\"name\":\"test\"}", HeadersNotMatch: headerCache, Delay: 10 * time.Millisecond},
-		{Method: "POST", Path: "/", Data: "{\"name\":\"test2\"}", HeadersMatch: headerCache, Delay: 10 * time.Millisecond},
+		{Method: http.MethodPost, Path: "/", Data: "{\"name\":\"test\"}", HeadersNotMatch: headerCache, Delay: 10 * time.Millisecond},
+		{Method: http.MethodPost, Path: "/", Data: "{\"name\":\"test2\"}", HeadersMatch: headerCache, Delay: 10 * time.Millisecond},
 	}...)
 }
 

gateway/mw_method_transform.go

@@ -40,7 +40,7 @@ func (t *TransformMethod) ProcessRequest(w http.ResponseWriter, r *http.Request,
 
 	switch strings.ToUpper(mmeta.ToMethod) {
 	case "GET", "POST", "PUT", "DELETE", "OPTIONS", "PATCH":
-		r.Method = toMethod
+		ctxSetTransformRequestMethod(r, toMethod)
 	default:
 		return errors.New("Method not allowed"), http.StatusMethodNotAllowed
 	}

gateway/mw_method_transform_test.go

@@ -0,0 +1,71 @@
+package gateway
+
+import (
+	"testing"
+
+	"github.com/TykTechnologies/tyk/apidef"
+	"github.com/TykTechnologies/tyk/test"
+)
+
+func TestMethodTransform(t *testing.T) {
+	ts := StartTest()
+	defer ts.Close()
+
+	t.Run("Using URL rewrite", func(t *testing.T) {
+
+		methodTransform := apidef.MethodTransformMeta{}
+		methodTransform.Path = "/get"
+		methodTransform.Method = "GET"
+		methodTransform.ToMethod = "POST"
+
+		urlRewrite := apidef.URLRewriteMeta{}
+		urlRewrite.Path = "/get"
+		urlRewrite.Method = "GET"
+		urlRewrite.MatchPattern = "/get(.*)"
+		urlRewrite.RewriteTo = "/post$1"
+
+		BuildAndLoadAPI(func(spec *APISpec) {
+			spec.Proxy.ListenPath = "/"
+			UpdateAPIVersion(spec, "v1", func(v *apidef.VersionInfo) {
+				v.UseExtendedPaths = true
+				v.ExtendedPaths.MethodTransforms = append(v.ExtendedPaths.MethodTransforms, methodTransform)
+				v.ExtendedPaths.URLRewrite = append(v.ExtendedPaths.URLRewrite, urlRewrite)
+			})
+		})
+
+		ts.Run(t, []test.TestCase{
+			{Method: "GET", Path: "/get", BodyMatch: `"Url":"/post"`},
+
+			{Method: "GET", Path: "/get?a=b", BodyMatch: `"Method":"POST"`},
+		}...)
+	})
+
+	t.Run("Using cache", func(t *testing.T) {
+		methodTransform := apidef.MethodTransformMeta{}
+		methodTransform.Path = "/testing"
+		methodTransform.Method = "GET"
+		methodTransform.ToMethod = "POST"
+
+		BuildAndLoadAPI(func(spec *APISpec) {
+			spec.CacheOptions = apidef.CacheOptions{
+				CacheTimeout: 120,
+				EnableCache:  true,
+			}
+			spec.Proxy.ListenPath = "/"
+			UpdateAPIVersion(spec, "v1", func(v *apidef.VersionInfo) {
+				v.UseExtendedPaths = true
+				v.ExtendedPaths.Cached = []string{"/testing"}
+				v.ExtendedPaths.MethodTransforms = append(v.ExtendedPaths.MethodTransforms, methodTransform)
+			})
+		})
+
+		headerCache := map[string]string{"x-tyk-cached-response": "1"}
+
+		ts.Run(t, []test.TestCase{
+			{Method: "GET", Path: "/testing", HeadersNotMatch: headerCache, BodyMatch: `"Method":"POST"`},
+			{Method: "GET", Path: "/testing", HeadersMatch: headerCache, BodyMatch: `"Method":"POST"`},
+			{Method: "POST", Path: "/testing", HeadersNotMatch: headerCache},
+			{Method: "GET", Path: "/testing", HeadersMatch: headerCache},
+		}...)
+	})
+}

gateway/mw_redis_cache.go

@@ -203,6 +203,14 @@ func (m *RedisCacheMiddleware) ProcessRequest(w http.ResponseWriter, r *http.Req
 		} else {
 			// This passes through and will write the value to the writer, but spit out a copy for the cache
 			log.Debug("Not virtual, passing")
+			if newURL := ctxGetURLRewriteTarget(r); newURL != nil {
+				r.URL = newURL
+				ctxSetURLRewriteTarget(r, nil)
+			}
+			if newMethod := ctxGetTransformRequestMethod(r); newMethod != "" {
+				r.Method = newMethod
+				ctxSetTransformRequestMethod(r, "")
+			}
 			resVal = m.sh.ServeHTTPWithCache(w, r)
 		}