[TT-9327] Decoding the URL request first, before handling any additio…

…nal logic (#5345)

<!-- Provide a general summary of your changes in the Title above -->
this path works: /payment-intents
but this path doesn't: /payment%2Dintents

Encoded URLs aren't being rewritten when URL rewrite is applied.

One edge case scenario that could break backwards compatibility (as
described by @buger ), is that users can rely on escaped characters, and
try to match them from the the url rewrite rules.

In order to accomodate that, we are running url rewrite middleware
twice:
- once on the raw path
- if transformations are failing and the url contains encoded
characters, then we run it second time, with decoded URL

<!-- Describe your changes in detail -->

<!-- This project only accepts pull requests related to open issues. -->
<!-- If suggesting a new feature or change, please discuss it in an
issue first. -->
<!-- If fixing a bug, there should be an issue describing it with steps
to reproduce. -->
<!-- OSS: Please link to the issue here. Tyk: please create/link the
JIRA ticket. -->

<!-- Why is this change required? What problem does it solve? -->

Unit test and manually

<!-- What types of changes does your code introduce? Put an `x` in all
the boxes that apply: -->

- [√ ] Bug fix (non-breaking change which fixes an issue)
- [ ] New feature (non-breaking change which adds functionality)
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] Refactoring or add test (improvements in base code or adds test
coverage to functionality)

<!-- Go over all the following points, and put an `x` in all the boxes
that apply -->
<!-- If there are no documentation updates required, mark the item as
checked. -->
<!-- Raise up any additional concerns not covered by the checklist. -->

- [ ] I ensured that the documentation is up to date
- [ ] I explained why this PR updates go.mod in detail with reasoning
why it's required
- [ ] I would like a code coverage CI quality gate exception and have
explained why

(cherry picked from commit 4346303)

gateway/mw_url_rewrite.go

@@ -38,8 +38,18 @@ var envValueMatch = regexp.MustCompile(`\$secret_env.([A-Za-z0-9_\-\.]+)`)
 var metaMatch = regexp.MustCompile(`\$tyk_meta.([A-Za-z0-9_\-\.]+)`)
 var secretsConfMatch = regexp.MustCompile(`\$secret_conf.([A-Za-z0-9[.\-\_]+)`)
 
-func (gw *Gateway) urlRewrite(meta *apidef.URLRewriteMeta, r *http.Request) (string, error) {
-	path := r.URL.String()
+func (gw *Gateway) urlRewrite(meta *apidef.URLRewriteMeta, r *http.Request, decodeURL bool) (string, error) {
+	rawPath := r.URL.String()
+	path := rawPath
+
+	if decodeURL {
+		var err error
+		path, err = url.PathUnescape(rawPath)
+		if err != nil {
+			return rawPath, fmt.Errorf("failed to decode URL path: %s", rawPath)
+		}
+	}
+
 	log.Debug("Inbound path: ", path)
 	newpath := path
 
@@ -196,6 +206,10 @@ func (gw *Gateway) urlRewrite(meta *apidef.URLRewriteMeta, r *http.Request) (str
 
 	newpath = gw.replaceTykVariables(r, newpath, true)
 
+	if rawPath == newpath && containsEscapedChars(rawPath) {
+		newpath, _ = gw.urlRewrite(meta, r, true)
+	}
+
 	return newpath, nil
 }
 
@@ -478,7 +492,8 @@ func (m *URLRewriteMiddleware) ProcessRequest(w http.ResponseWriter, r *http.Req
 	umeta := meta.(*apidef.URLRewriteMeta)
 	log.Debug(r.URL)
 	oldPath := r.URL.String()
-	p, err := m.Gw.urlRewrite(umeta, r)
+
+	p, err := m.Gw.urlRewrite(umeta, r, false)
 	if err != nil {
 		log.Error(err)
 		return err, http.StatusInternalServerError

gateway/mw_url_rewrite_test.go

@@ -19,6 +19,16 @@ var testRewriterData = []struct {
 	pattern, to string
 	in, want    string
 }{
+	{
+		"Encoded",
+		"/test/payment-intents", "/change/to/me",
+		"/test/payment%2Dintents", "/change/to/me",
+	},
+	{
+		"MatchEncodedChars",
+		"^(.+)%2[Dd](.+)$", "/change/to/me",
+		"/test/payment%2Dintents", "/change/to/me",
+	},
 	{
 		"Straight",
 		"/test/straight/rewrite", "/change/to/me",
@@ -95,7 +105,7 @@ func TestRewriter(t *testing.T) {
 	for _, tc := range cases {
 		t.Run(tc.name, func(t *testing.T) {
 			r := tc.reqMaker()
-			got, err := ts.Gw.urlRewrite(tc.meta, r)
+			got, err := ts.Gw.urlRewrite(tc.meta, r, false)
 			if err != nil {
 				t.Error("compile failed:", err)
 			}
@@ -113,7 +123,7 @@ func BenchmarkRewriter(b *testing.B) {
 	//warm-up regexp caches
 	for _, tc := range cases {
 		r := tc.reqMaker()
-		ts.Gw.urlRewrite(tc.meta, r)
+		ts.Gw.urlRewrite(tc.meta, r, false)
 	}
 
 	b.ReportAllocs()
@@ -123,7 +133,7 @@ func BenchmarkRewriter(b *testing.B) {
 			b.StopTimer()
 			r := tc.reqMaker()
 			b.StartTimer()
-			ts.Gw.urlRewrite(tc.meta, r)
+			ts.Gw.urlRewrite(tc.meta, r, false)
 		}
 	}
 }
@@ -1081,7 +1091,7 @@ func TestRewriterTriggers(t *testing.T) {
 				Triggers:     tc.triggerConf,
 			}
 
-			got, err := ts.Gw.urlRewrite(&testConf, tc.req)
+			got, err := ts.Gw.urlRewrite(&testConf, tc.req, false)
 			if err != nil {
 				t.Error("compile failed:", err)
 			}

gateway/util.go

@@ -158,3 +158,29 @@ func shouldReloadSpec(existingSpec, newSpec *APISpec) bool {
 
 	return false
 }
+<<<<<<< HEAD
+=======
+
+// check if 2 maps are the same
+func areMapsEqual(a, b map[string]string) bool {
+	if len(a) != len(b) {
+		return false
+	}
+	for k, v := range a {
+		if b[k] != v {
+			return false
+		}
+	}
+	return true
+}
+
+// checks if a string contains escaped characters
+func containsEscapedChars(str string) bool {
+	unescaped, err := url.PathUnescape(str)
+	if err != nil {
+		return true
+	}
+
+	return str != unescaped
+}
+>>>>>>> 4346303f... [TT-9327] Decoding the URL request first, before handling any additional logic (#5345)

gateway/util_test.go

@@ -215,3 +215,68 @@ func Test_shouldReloadSpec(t *testing.T) {
 		assertionHelper(t, tcs)
 	})
 }
+<<<<<<< HEAD
+=======
+
+func TestAreMapsEqual(t *testing.T) {
+	tests := []struct {
+		name     string
+		map1     map[string]string
+		map2     map[string]string
+		expected bool
+	}{
+		{
+			name:     "Equal maps",
+			map1:     map[string]string{"key1": "value1", "key2": "value2"},
+			map2:     map[string]string{"key1": "value1", "key2": "value2"},
+			expected: true,
+		},
+		{
+			name:     "Different maps",
+			map1:     map[string]string{"key1": "value1", "key2": "value2"},
+			map2:     map[string]string{"key1": "value1", "key2": "value3"},
+			expected: false,
+		},
+		{
+			name:     "Different sizes",
+			map1:     map[string]string{"key1": "value1", "key2": "value2", "key3": "value3"},
+			map2:     map[string]string{"key1": "value1", "key2": "value2"},
+			expected: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			result := areMapsEqual(test.map1, test.map2)
+			if result != test.expected {
+				t.Errorf("areMapsEqual() = %v, want %v", result, test.expected)
+			}
+		})
+	}
+}
+
+func TestContainsEscapedCharacters(t *testing.T) {
+	tests := []struct {
+		value    string
+		expected bool
+	}{
+		{
+			value:    "payment%2Dintents",
+			expected: true,
+		},
+		{
+			value:    "payment-intents",
+			expected: false,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.value, func(t *testing.T) {
+			result := containsEscapedChars(test.value)
+			if result != test.expected {
+				t.Errorf("containsEscapedChars() = %v, want %v", result, test.expected)
+			}
+		})
+	}
+}
+>>>>>>> 4346303f... [TT-9327] Decoding the URL request first, before handling any additional logic (#5345)