[TT-12550] policy key path permissions problem (#6437)

### **User description**
Fixes two issues:

- invalid regex in allowed urls would allow all requests to pass
- mux path parameters unsupported `{id}` was considered literal (only
regex supported)

Input accessURL patterns are now handled by the mux library
(GetPathRegexp).

https://tyktech.atlassian.net/browse/TT-12550

___

### **PR Type**
Bug fix, Enhancement, Tests


___

### **Description**
- Enhanced URL matching in `GranularAccessMiddleware` by converting mux
path parameters to regex.
- Improved logging to include path and pattern matching details.
- Handled regex compilation errors gracefully by skipping invalid
patterns.
- Added new `RouteRegexString` function to convert mux routes to regex.
- Introduced tests for `RouteRegexString` function.
- Added test case for invalid regex in allowed URLs.


___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Enhancement</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>mw_granular_access.go</strong><dd><code>Enhance URL
matching and logging in GranularAccessMiddleware</code></dd></summary>
<hr>

gateway/mw_granular_access.go

<li>Added conversion of mux path parameters to regex.<br> <li> Improved
logging for path and pattern matching.<br> <li> Handled regex
compilation errors gracefully.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6437/files#diff-618f7d55751d572562a29506a13beba2da969436e974f8b51df7d9708c925436">+12/-6</a>&nbsp;
&nbsp; </td>

</tr>                    

<tr>
  <td>
    <details>
<summary><strong>route.go</strong><dd><code>Add RouteRegexString
function for mux route conversion</code>&nbsp; &nbsp; &nbsp;
</dd></summary>
<hr>

internal/httputil/route.go

<li>Introduced <code>RouteRegexString</code> function to convert mux
routes to regex.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6437/files#diff-be202cd339a918297746198e9c73364977d25886235f105762bce816ff46e11e">+44/-0</a>&nbsp;
&nbsp; </td>

</tr>                    
</table></td></tr><tr><td><strong>Tests</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>mw_granular_access_test.go</strong><dd><code>Add test
case for invalid regex in allowed URLs</code>&nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
<hr>

gateway/mw_granular_access_test.go

- Added test case for invalid regex in allowed URLs.



</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6437/files#diff-8e0d7cfef26688edd7d08334d955039dab5deb3caf860d29eff6d09894eaba20">+4/-0</a>&nbsp;
&nbsp; &nbsp; </td>

</tr>                    

<tr>
  <td>
    <details>
<summary><strong>route_test.go</strong><dd><code>Add tests for
RouteRegexString function</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; </dd></summary>
<hr>

internal/httputil/route_test.go

- Added tests for `RouteRegexString` function.



</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6437/files#diff-a291c7018ab8c1fabe97ad7c94b8820dc47c889f79fb7376eb78c0abc3ccaed6">+24/-0</a>&nbsp;
&nbsp; </td>

</tr>                    
</table></td></tr></tr></tbody></table>

___

> 💡 **PR-Agent usage**:
>Comment `/help` on the PR to get a list of all available PR-Agent tools
and their descriptions

---------

Co-authored-by: Tit Petric <tit@tyk.io>

gateway/api_definition.go

@@ -24,6 +24,7 @@ import (
 	"github.com/getkin/kin-openapi/routers"
 
 	"github.com/TykTechnologies/tyk/internal/graphengine"
+	"github.com/TykTechnologies/tyk/internal/httputil"
 
 	"github.com/getkin/kin-openapi/routers/gorillamux"
 
@@ -35,7 +36,6 @@ import (
 
 	sprig "github.com/Masterminds/sprig/v3"
 
-	"github.com/gorilla/mux"
 	"github.com/sirupsen/logrus"
 
 	circuit "github.com/TykTechnologies/circuitbreaker"
@@ -1634,7 +1634,7 @@ func (a *APISpec) getVersionFromRequest(r *http.Request) string {
 
 		return vName
 	case apidef.URLLocation:
-		uPath := a.StripListenPath(r, r.URL.Path)
+		uPath := a.StripListenPath(r.URL.Path)
 		uPath = strings.TrimPrefix(uPath, "/"+a.Slug)
 
 		// First non-empty part of the path is the version ID
@@ -1776,8 +1776,8 @@ func (a *APISpec) Version(r *http.Request) (*apidef.VersionInfo, RequestStatus)
 	return &version, StatusOk
 }
 
-func (a *APISpec) StripListenPath(r *http.Request, path string) string {
-	return stripListenPath(a.Proxy.ListenPath, path)
+func (a *APISpec) StripListenPath(reqPath string) string {
+	return httputil.StripListenPath(a.Proxy.ListenPath, reqPath)
 }
 
 func (a *APISpec) SanitizeProxyPaths(r *http.Request) {
@@ -1787,9 +1787,9 @@ func (a *APISpec) SanitizeProxyPaths(r *http.Request) {
 
 	log.Debug("Stripping proxy listen path: ", a.Proxy.ListenPath)
 
-	r.URL.Path = a.StripListenPath(r, r.URL.Path)
+	r.URL.Path = a.StripListenPath(r.URL.Path)
 	if r.URL.RawPath != "" {
-		r.URL.RawPath = a.StripListenPath(r, r.URL.RawPath)
+		r.URL.RawPath = a.StripListenPath(r.URL.RawPath)
 	}
 
 	log.Debug("Upstream path is: ", r.URL.Path)
@@ -1839,27 +1839,6 @@ func (r *RoundRobin) WithLen(len int) int {
 	return int(cur) % len
 }
 
-func stripListenPath(listenPath, path string) (res string) {
-	defer func() {
-		if !strings.HasPrefix(res, "/") {
-			res = "/" + res
-		}
-	}()
-
-	if !strings.Contains(listenPath, "{") {
-		res = strings.TrimPrefix(path, listenPath)
-		return
-	}
-
-	tmp := new(mux.Route).PathPrefix(listenPath)
-	s, err := tmp.GetPathRegexp()
-	if err != nil {
-		return path
-	}
-	reg := regexp.MustCompile(s)
-	return reg.ReplaceAllString(path, "")
-}
-
 func (s *APISpec) hasVirtualEndpoint() bool {
 	for _, version := range s.VersionData.Versions {
 		for _, virtual := range version.ExtendedPaths.Virtual {

gateway/api_definition_test.go

@@ -1339,23 +1339,6 @@ func TestAPIExpiration(t *testing.T) {
 	}
 }
 
-func TestStripListenPath(t *testing.T) {
-	assert.Equal(t, "/get", stripListenPath("/listen", "/listen/get"))
-	assert.Equal(t, "/get", stripListenPath("/listen/", "/listen/get"))
-	assert.Equal(t, "/get", stripListenPath("listen", "listen/get"))
-	assert.Equal(t, "/get", stripListenPath("listen/", "listen/get"))
-	assert.Equal(t, "/", stripListenPath("/listen/", "/listen/"))
-	assert.Equal(t, "/", stripListenPath("/listen", "/listen"))
-	assert.Equal(t, "/", stripListenPath("listen/", ""))
-
-	assert.Equal(t, "/get", stripListenPath("/{_:.*}/post/", "/listen/post/get"))
-	assert.Equal(t, "/get", stripListenPath("/{_:.*}/", "/listen/get"))
-	assert.Equal(t, "/get", stripListenPath("/pre/{_:.*}/", "/pre/listen/get"))
-	assert.Equal(t, "/", stripListenPath("/{_:.*}", "/listen"))
-	assert.Equal(t, "/get", stripListenPath("/{myPattern:foo|bar}", "/foo/get"))
-	assert.Equal(t, "/anything/get", stripListenPath("/{myPattern:foo|bar}", "/anything/get"))
-}
-
 func TestAPISpec_SanitizeProxyPaths(t *testing.T) {
 	a := APISpec{APIDefinition: &apidef.APIDefinition{}}
 	a.Proxy.ListenPath = "/listen/"

gateway/handler_error.go

@@ -193,7 +193,7 @@ func (e *ErrorHandler) HandleError(w http.ResponseWriter, r *http.Request, errMs
 		}
 
 		if e.Spec.Proxy.StripListenPath {
-			r.URL.Path = e.Spec.StripListenPath(r, r.URL.Path)
+			r.URL.Path = e.Spec.StripListenPath(r.URL.Path)
 		}
 
 		oauthClientID := ""

gateway/mw_granular_access.go

@@ -4,6 +4,7 @@ import (
 	"errors"
 	"net/http"
 
+	"github.com/TykTechnologies/tyk/internal/httputil"
 	"github.com/TykTechnologies/tyk/regexp"
 )
 
@@ -22,7 +23,7 @@ func (m *GranularAccessMiddleware) ProcessRequest(w http.ResponseWriter, r *http
 		return nil, http.StatusOK
 	}
 
-	logger := m.Logger()
+	logger := m.Logger().WithField("path", r.URL.Path)
 	session := ctxGetSession(r)
 
 	sessionVersionData, foundAPI := session.AccessRights[m.Spec.APIID]
@@ -34,17 +35,29 @@ func (m *GranularAccessMiddleware) ProcessRequest(w http.ResponseWriter, r *http
 		return nil, http.StatusOK
 	}
 
+	urlPath := m.Spec.StripListenPath(r.URL.Path)
+
 	for _, accessSpec := range sessionVersionData.AllowedURLs {
-		logger.Debug("Checking: ", r.URL.Path, " Against:", accessSpec.URL)
-		asRegex, err := regexp.Compile(accessSpec.URL)
+		url := accessSpec.URL
+		clean, err := httputil.GetPathRegexp(url)
+		if err != nil {
+			logger.WithError(err).Errorf("error getting path regex: %q, skipping", url)
+			continue
+		}
+
+		asRegex, err := regexp.Compile(clean)
 		if err != nil {
-			logger.WithError(err).Error("Regex error")
-			return nil, http.StatusOK
+			logger.WithError(err).Errorf("error compiling path regex: %q, skipping", url)
+			continue
 		}
 
-		match := asRegex.MatchString(r.URL.Path)
+		match := asRegex.MatchString(urlPath)
+
+		logger.WithField("pattern", url).WithField("match", match).Debug("checking allowed url")
+
 		if match {
-			logger.Debug("Match!")
+			// if a path is matched, but isn't matched on method,
+			// then we continue onto the next path for evaluation.
 			for _, method := range accessSpec.Methods {
 				if method == r.Method {
 					return nil, http.StatusOK

gateway/mw_granular_access_test.go

@@ -24,6 +24,10 @@ func TestGranularAccessMiddleware_ProcessRequest(t *testing.T) {
 				APIID:   api.APIID,
 				APIName: api.Name,
 				AllowedURLs: []user.AccessSpec{
+					{
+						URL:     "^/*.$",
+						Methods: []string{"GET"},
+					},
 					{
 						URL:     "^/valid_path.*",
 						Methods: []string{"GET"},

gateway/mw_request_signing.go

@@ -77,17 +77,17 @@ func generateHeaderList(r *http.Request, headerList []string) []string {
 }
 
 func (s *RequestSigning) getRequestPath(r *http.Request) string {
-	path := r.URL.RequestURI()
+	urlPath := r.URL.RequestURI()
 
 	if newURL := ctxGetURLRewriteTarget(r); newURL != nil {
-		path = newURL.RequestURI()
+		urlPath = newURL.RequestURI()
 	} else {
 		if s.Spec.Proxy.StripListenPath {
-			path = s.Spec.StripListenPath(r, path)
+			urlPath = s.Spec.StripListenPath(urlPath)
 		}
 	}
 
-	return path
+	return urlPath
 }
 
 func (s *RequestSigning) ProcessRequest(w http.ResponseWriter, r *http.Request, _ interface{}) (error, int) {

internal/httputil/Taskfile.yml

@@ -1,10 +1,49 @@
 ---
 version: "3"
 
+vars:
+  testArgs: -v
+
 tasks:
-  default:
-    desc: "Run tests"
+  test:
+    desc: "Run tests (requires redis)"
+    cmds:
+      - task: fmt
+      - go test {{.testArgs}} -count=1 -cover -coverprofile=rate.cov -coverpkg=./... ./...
+
+  bench:
+    desc: "Run benchmarks"
+    cmds:
+      - task: fmt
+      - go test {{.testArgs}} -count=1 -tags integration -bench=. -benchtime=10s -benchmem ./...
+
+  fmt:
+    internal: true
+    desc: "Invoke fmt"
     cmds:
-      - go fmt ./...
       - goimports -w .
-      - go test -race -count=100 -cover .
+      - go fmt ./...
+
+  cover:
+    desc: "Show source coverage"
+    aliases: [coverage, cov]
+    cmds:
+      - go tool cover -func=rate.cov
+
+  uncover:
+    desc: "Show uncovered source"
+    cmds:
+      - uncover rate.cov
+
+  lint:
+    desc: "Lint docs"
+    cmds:
+      - schema-gen extract -o - | schema-gen lint -i -
+
+  install:uncover:
+    desc: "Install uncover"
+    internal: true
+    env:
+      GOBIN: /usr/local/bin
+    cmds:
+      - go install github.com/gregoryv/uncover/...@latest

internal/httputil/connection_watcher.go

@@ -29,7 +29,6 @@ func (cw *ConnectionWatcher) OnStateChange(_ net.Conn, state http.ConnState) {
 }
 
 // Count returns the number of connections at the time the call.
-
 func (cw *ConnectionWatcher) Count() int {
 	return int(atomic.LoadInt64(&cw.n))
 }

internal/httputil/mux.go

@@ -0,0 +1,79 @@
+package httputil
+
+import (
+	"regexp"
+	"strings"
+
+	"github.com/gorilla/mux"
+
+	"github.com/TykTechnologies/tyk/internal/maps"
+)
+
+// routeCache holds the raw routes as they are mapped to mux regular expressions.
+// e.g. `/foo` becomes `^/foo$` or similar, and parameters get matched and replaced.
+var pathRegexpCache = maps.NewStringMap()
+
+// GetPathRegexp will convert a mux route url to a regular expression string.
+// The results for subsequent invocations with the same parameters are cached.
+func GetPathRegexp(pattern string) (string, error) {
+	val, ok := pathRegexpCache.Get(pattern)
+	if ok {
+		return val, nil
+	}
+
+	if IsMuxTemplate(pattern) {
+		dummyRouter := mux.NewRouter()
+		route := dummyRouter.PathPrefix(pattern)
+		result, err := route.GetPathRegexp()
+		if err != nil {
+			return "", err
+		}
+
+		pathRegexpCache.Set(pattern, result)
+		return result, nil
+	}
+
+	if strings.HasPrefix(pattern, "/") {
+		return "^" + pattern, nil
+	}
+	return "^.*" + pattern, nil
+}
+
+// IsMuxTemplate determines if a pattern is a mux template by counting the number of opening and closing braces.
+func IsMuxTemplate(pattern string) bool {
+	openBraces := strings.Count(pattern, "{")
+	closeBraces := strings.Count(pattern, "}")
+	return openBraces > 0 && openBraces == closeBraces
+}
+
+// StripListenPath will strip the listenPath from the passed urlPath.
+// If the listenPath contains mux variables, it will trim away the
+// matching pattern with a regular expression that mux provides.
+func StripListenPath(listenPath, urlPath string) (res string) {
+	defer func() {
+		if !strings.HasPrefix(res, "/") {
+			res = "/" + res
+		}
+	}()
+
+	res = urlPath
+
+	// early return on the simple case
+	if strings.HasPrefix(urlPath, listenPath) {
+		res = strings.TrimPrefix(res, listenPath)
+		return res
+	}
+
+	if !IsMuxTemplate(listenPath) {
+		return res
+	}
+
+	tmp := new(mux.Route).PathPrefix(listenPath)
+	s, err := tmp.GetPathRegexp()
+	if err != nil {
+		return res
+	}
+
+	reg := regexp.MustCompile(s)
+	return reg.ReplaceAllString(res, "")
+}