Merging to release-5.7: [TT-11711] Fix listenpath validation (#6772)

[TT-11711] Fix listenpath validation (#6772)

### **User description**
<details open>
<summary><a href="https://tyktech.atlassian.net/browse/TT-11711"
title="TT-11711" target="_blank">TT-11711</a></summary>
  <br />
  <table>
    <tr>
      <th>Summary</th>
      <td>`listen path` formatting can panic worker gateway</td>
    </tr>
    <tr>
      <th>Type</th>
      <td>
<img alt="Bug"
src="https://tyktech.atlassian.net/rest/api/2/universal_avatar/view/type/issuetype/avatar/10303?size=medium"
/>
        Bug
      </td>
    </tr>
    <tr>
      <th>Status</th>
      <td>In Dev</td>
    </tr>
    <tr>
      <th>Points</th>
      <td>N/A</td>
    </tr>
    <tr>
      <th>Labels</th>
<td><a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20'24Bugsmash%20ORDER%20BY%20created%20DESC"
title="'24Bugsmash">'24Bugsmash</a>, <a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20Gold%20ORDER%20BY%20created%20DESC"
title="Gold">Gold</a>, <a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20customer_bug%20ORDER%20BY%20created%20DESC"
title="customer_bug">customer_bug</a>, <a
href="https://tyktech.atlassian.net/issues?jql=project%20%3D%20TT%20AND%20labels%20%3D%20jira_escalated%20ORDER%20BY%20created%20DESC"
title="jira_escalated">jira_escalated</a></td>
    </tr>
  </table>
</details>
<!--
  do not remove this marker as it will break jira-lint's functionality.
  added_by_jira_lint
-->

---

PR adds listenpath validation using the mux library.


___

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


___

### **Description**
- Added `listenPath` validation using `httputil.ValidatePath` to prevent
invalid paths from causing panics.
- Enhanced logging in `MakeSpec` and `loadHTTPService` to include more
context for debugging.
- Updated `loadHTTPService` to validate `listenPath` and return errors
when validation fails.
- Introduced `ValidatePath` function in `httputil` to centralize path
validation logic.
- Added comprehensive unit tests for `ValidatePath` to ensure
correctness and robustness.
- Added integration tests in `api_loader_test.go` to verify `listenPath`
validation during API loading.



___



### **Changes walkthrough** 📝
<table><thead><tr><th></th><th align="left">Relevant
files</th></tr></thead><tbody><tr><td><strong>Bug
fix</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>api_definition.go</strong><dd><code>Add
<code>listenPath</code> validation and improve logging in API definition
<br>loader.</code></dd></summary>
<hr>

gateway/api_definition.go

<li>Added validation for <code>listenPath</code> using
<code>httputil.ValidatePath</code> to prevent <br>invalid paths.<br>
<li> Enhanced logging with additional context fields for better
debugging.<br> <li> Ensured <code>MakeSpec</code> and
<code>loadHTTPService</code> validate <code>listenPath</code> to avoid
<br>panics.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6772/files#diff-0cf80174bbafb36f6d4f4308ebbd971b2833b76a936bad568220aa1a4ba0ee8b">+18/-10</a>&nbsp;
</td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>api_loader.go</strong><dd><code>Validate `listenPath`
in `loadHTTPService` and handle errors.</code></dd></summary>
<hr>

gateway/api_loader.go

<li>Added <code>httputil.ValidatePath</code> validation in
<code>loadHTTPService</code> to ensure <br>valid
<code>listenPath</code>.<br> <li> Modified <code>loadHTTPService</code>
to return an error when validation fails.<br> <li> Updated API loading
logic to handle errors from <code>loadHTTPService</code>.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6772/files#diff-cdf0b7f176c9d18e1a314b78ddefc2cb3a94b3de66f1f360174692c915734c68">+21/-5</a>&nbsp;
&nbsp; </td>

</tr>
</table></td></tr><tr><td><strong>Tests</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>api_loader_test.go</strong><dd><code>Add test for
`listenPath` validation in API loader.</code>&nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; </dd></summary>
<hr>

gateway/api_loader_test.go

<li>Added a new test case <code>TestAPILoaderValidation</code> to ensure
<code>listenPath</code> <br>validation works correctly.<br> <li>
Verified that invalid <code>listenPath</code> values do not cause
panics.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6772/files#diff-f696545a659f4d96421b253edef4bcc8da0e7f52120b8f8866d32cbbb7cc1afc">+39/-2</a>&nbsp;
&nbsp; </td>

</tr>

<tr>
  <td>
    <details>
<summary><strong>mux_test.go</strong><dd><code>Add unit tests for
`ValidatePath` function.</code>&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp;
&nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; &nbsp; </dd></summary>
<hr>

internal/httputil/mux_test.go

<li>Added unit tests for <code>ValidatePath</code> to verify its
behavior with valid <br>and invalid paths.<br> <li> Covered edge cases
such as invalid regex and missing leading slashes.<br>


</details>


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

</tr>
</table></td></tr><tr><td><strong>Enhancement</strong></td><td><table>
<tr>
  <td>
    <details>
<summary><strong>mux.go</strong><dd><code>Add `ValidatePath` function
for `listenPath` validation.</code>&nbsp; </dd></summary>
<hr>

internal/httputil/mux.go

<li>Introduced <code>ValidatePath</code> function to validate
<code>listenPath</code> using mux <br>router.<br> <li> Ensures invalid
paths are caught early to prevent runtime issues.<br>


</details>


  </td>
<td><a
href="https://github.com/TykTechnologies/tyk/pull/6772/files#diff-3d9ee5f5e946d72e6f2ae662ff03ee5253bbdc15203d2e4f6e9f46c13011ebf8">+7/-0</a>&nbsp;
&nbsp; &nbsp; </td>

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

___

> 💡 **PR-Agent usage**: Comment `/help "your question"` on any pull
request to receive relevant information

---------

Co-authored-by: Tit Petric <[email protected]>

apidef/migration.go

@@ -69,7 +69,16 @@ func (a *APIDefinition) MigrateVersioning() (versions []APIDefinition, err error
 			newAPI.Id = ""
 			newAPI.Name += "-" + url.QueryEscape(vName)
 			newAPI.Internal = true
-			newAPI.Proxy.ListenPath = strings.TrimSuffix(newAPI.Proxy.ListenPath, "/") + "-" + url.QueryEscape(vName) + "/"
+
+			listenPathClean := strings.TrimSuffix(newAPI.Proxy.ListenPath, "/")
+			if listenPathClean == "" {
+				listenPathClean = "/" + url.QueryEscape(vName) + "/"
+			} else {
+				listenPathClean += "-" + url.QueryEscape(vName) + "/"
+			}
+
+			newAPI.Proxy.ListenPath = listenPathClean
+
 			newAPI.VersionDefinition = VersionDefinition{BaseID: a.APIID}
 			newAPI.VersionName = vName
 

gateway/api_definition.go

@@ -333,10 +333,18 @@ type APIDefinitionLoader struct {
 // MakeSpec will generate a flattened URLSpec from and APIDefinitions' VersionInfo data. paths are
 // keyed to the Api version name, which is determined during routing to speed up lookups
 func (a APIDefinitionLoader) MakeSpec(def *model.MergedAPI, logger *logrus.Entry) (*APISpec, error) {
+	if logger == nil {
+		logger = logrus.NewEntry(log).WithFields(logrus.Fields{
+			"api_id": def.APIID,
+			"org_id": def.OrgID,
+			"name":   def.Name,
+		})
+	}
+
 	spec := &APISpec{}
 	apiString, err := json.Marshal(def)
 	if err != nil {
-		logger.WithError(err).WithField("name", def.Name).Error("Failed to JSON marshal API definition")
+		logger.WithError(err).Error("Failed to JSON marshal API definition")
 		return nil, err
 	}
 
@@ -350,15 +358,10 @@ func (a APIDefinitionLoader) MakeSpec(def *model.MergedAPI, logger *logrus.Entry
 		return currSpec, nil
 	}
 
-	if logger == nil {
-		logger = logrus.NewEntry(log)
-	}
-
 	// new expiration feature
 	if def.Expiration != "" {
 		if t, err := time.Parse(apidef.ExpirationTimeFormat, def.Expiration); err != nil {
-			logger.WithError(err).WithField("name", def.Name).WithField("Expiration", def.Expiration).
-				Error("Could not parse expiration date for API")
+			logger.WithError(err).WithField("Expiration", def.Expiration).Error("Could not parse expiration date for API")
 		} else {
 			def.ExpirationTs = t
 		}
@@ -372,7 +375,7 @@ func (a APIDefinitionLoader) MakeSpec(def *model.MergedAPI, logger *logrus.Entry
 		}
 		// calculate the time
 		if t, err := time.Parse(apidef.ExpirationTimeFormat, ver.Expires); err != nil {
-			logger.WithError(err).WithField("Expires", ver.Expires).Error("Could not parse expiry date for API")
+			logger.WithError(err).WithField("expires", ver.Expires).Error("Could not parse expiry date for API")
 		} else {
 			ver.ExpiresTs = t
 			def.VersionData.Versions[key] = ver
@@ -444,20 +447,25 @@ func (a APIDefinitionLoader) MakeSpec(def *model.MergedAPI, logger *logrus.Entry
 	if spec.IsOAS && def.OAS != nil {
 		loader := openapi3.NewLoader()
 		if err := loader.ResolveRefsIn(&def.OAS.T, nil); err != nil {
-			log.WithError(err).Errorf("Dashboard loaded API's OAS reference resolve failed: %s", def.APIID)
+			logger.WithError(err).Errorf("Dashboard loaded API's OAS reference resolve failed: %s", def.APIID)
 		}
 
 		spec.OAS = *def.OAS
 	}
 
+	if err := httputil.ValidatePath(spec.Proxy.ListenPath); err != nil {
+		logger.WithError(err).Error("Invalid listen path when creating router")
+		return nil, err
+	}
+
 	oasSpec := spec.OAS.T
 	oasSpec.Servers = openapi3.Servers{
 		{URL: spec.Proxy.ListenPath},
 	}
 
 	spec.OASRouter, err = gorillamux.NewRouter(&oasSpec)
 	if err != nil {
-		log.WithError(err).Error("Could not create OAS router")
+		logger.WithError(err).Error("Could not create OAS router")
 	}
 
 	spec.setHasMock()

gateway/api_loader.go

@@ -26,6 +26,7 @@ import (
 	"github.com/TykTechnologies/tyk/storage"
 	"github.com/TykTechnologies/tyk/trace"
 
+	"github.com/TykTechnologies/tyk/internal/httputil"
 	"github.com/TykTechnologies/tyk/internal/otel"
 )
 
@@ -751,7 +752,13 @@ func explicitRouteSubpaths(prefix string, handler http.Handler, enabled bool) ht
 //
 // - register gorilla/mux routing handless with proxyMux directly (wrapped),
 // - return a raw http.Handler for tyk://ID urls.
-func (gw *Gateway) loadHTTPService(spec *APISpec, apisByListen map[string]int, gs *generalStores, muxer *proxyMux) *ChainObject {
+func (gw *Gateway) loadHTTPService(spec *APISpec, apisByListen map[string]int, gs *generalStores, muxer *proxyMux) (*ChainObject, error) {
+	// MakeSpec validates listenpath, but we can't be sure that it's in all the invocation paths.
+	// Since the check is relatively inexpensive, do it here to prevent issues in uncovered paths.
+	if err := httputil.ValidatePath(spec.Proxy.ListenPath); err != nil {
+		return nil, fmt.Errorf("invalid listen path while loading api: %w", err)
+	}
+
 	gwConfig := gw.GetConfig()
 	port := gwConfig.ListenPort
 	if spec.ListenPort != 0 {
@@ -783,7 +790,7 @@ func (gw *Gateway) loadHTTPService(spec *APISpec, apisByListen map[string]int, g
 	}
 
 	if chainObj.Skip {
-		return chainObj
+		return chainObj, nil
 	}
 
 	// Prefixes are multiple paths that the API endpoints are listening on.
@@ -810,7 +817,7 @@ func (gw *Gateway) loadHTTPService(spec *APISpec, apisByListen map[string]int, g
 		subrouter.NewRoute().Handler(httpHandler)
 	}
 
-	return chainObj
+	return chainObj, nil
 }
 
 func (gw *Gateway) loadTCPService(spec *APISpec, gs *generalStores, muxer *proxyMux) {
@@ -993,7 +1000,12 @@ func (gw *Gateway) loadApps(specs []*APISpec) {
 						mainLog.Infof("Intialized tracer  api_name=%q", spec.Name)
 					}
 				}
-				tmpSpecHandles.Store(spec.APIID, gw.loadHTTPService(spec, apisByListen, &gs, muxer))
+				tmpSpecHandle, err := gw.loadHTTPService(spec, apisByListen, &gs, muxer)
+				if err != nil {
+					log.WithError(err).Errorf("error loading API")
+					return
+				}
+				tmpSpecHandles.Store(spec.APIID, tmpSpecHandle)
 			case "tcp", "tls":
 				gw.loadTCPService(spec, &gs, muxer)
 			}

gateway/api_loader_test.go

@@ -1115,7 +1115,7 @@ func TestLongerListenPathHasLongerDomainThanSubstringListenPath(t *testing.T) {
 		}...)
 	})
 
-	t.Run("strict and suffix  false", func(t *testing.T) {
+	t.Run("strict and suffix match false", func(t *testing.T) {
 		globalConf := ts.Gw.GetConfig()
 		globalConf.EnableCustomDomains = true
 
@@ -1151,7 +1151,7 @@ func TestLongerListenPathHasLongerDomainThanSubstringListenPath(t *testing.T) {
 		}...)
 	})
 
-	t.Run("strict and prefix  false", func(t *testing.T) {
+	t.Run("strict and prefix match false", func(t *testing.T) {
 		globalConf := ts.Gw.GetConfig()
 		globalConf.EnableCustomDomains = true
 
@@ -9491,3 +9491,29 @@ func TestSortAPISpecs(t *testing.T) {
 
 	}
 }
+
+func TestAPILoaderValidation(t *testing.T) {
+	ts := StartTest(nil)
+	t.Cleanup(ts.Close)
+
+	t.Run("invalid regexps", func(t *testing.T) {
+		cleanup := func() {
+			if e := recover(); e != nil {
+				t.Logf("Caught panic, shouldn't have: %+v", e)
+				t.Fail()
+			}
+		}
+		defer cleanup()
+
+		ts.Gw.BuildAndLoadAPI(
+			func(spec *APISpec) {
+				spec.APIID = "api-a"
+				spec.Proxy.ListenPath = "{/test-classic}"
+				spec.Proxy.TargetURL = "http://httpbin"
+				spec.Domain = "{subdomain:tyktest.io}"
+				spec.Proxy.DisableStripSlash = true
+				spec.Proxy.StripListenPath = true
+			},
+		)
+	})
+}

internal/httputil/mux.go

@@ -62,6 +62,13 @@ func PreparePathRegexp(pattern string, prefix bool, suffix bool) string {
 	return pattern
 }
 
+// ValidatePath validates if the path is valid. Returns an error.
+func ValidatePath(in string) error {
+	router := mux.NewRouter()
+	route := router.PathPrefix(in)
+	return route.GetError()
+}
+
 // 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, "{")

internal/httputil/mux_test.go

@@ -9,6 +9,30 @@ import (
 	"github.com/TykTechnologies/tyk/internal/httputil"
 )
 
+// TestValidatePath tests mux routes to avoid panics.
+// Routes must start with `/` which is an easy check.
+func TestValidatePath(t *testing.T) {
+	// invalid, paths must start with /
+	assert.Error(t, httputil.ValidatePath("{/foo}"))
+	assert.Error(t, httputil.ValidatePath("{foo}"))
+	assert.Error(t, httputil.ValidatePath("foo"))
+	assert.Error(t, httputil.ValidatePath("foo{/foo}"))
+
+	// strange but valid params (basically wildcard?)
+	assert.NoError(t, httputil.ValidatePath("/foo/{a!}"))
+	assert.NoError(t, httputil.ValidatePath("/foo/{.*}"))
+	assert.NoError(t, httputil.ValidatePath("/foo/{*}"))
+	assert.NoError(t, httputil.ValidatePath("/foo/{*.}"))
+
+	// invalid regexp in param
+	assert.Error(t, httputil.ValidatePath("/foo/{id:*.}"))
+
+	// green path: valid path, param, and param with regexp
+	assert.NoError(t, httputil.ValidatePath("/foo"))
+	assert.NoError(t, httputil.ValidatePath("/{foo}"))
+	assert.NoError(t, httputil.ValidatePath("/{foo:[a-zA-Z0-9]+}"))
+}
+
 func pathRegexp(tb testing.TB, in string, want string) string {
 	tb.Helper()
 

internal/model/merged_apis.go

@@ -1,10 +1,28 @@
 package model
 
 import (
+	"github.com/sirupsen/logrus"
+
 	"github.com/TykTechnologies/tyk/apidef"
 	"github.com/TykTechnologies/tyk/apidef/oas"
 )
 
+// MergedAPI combines the embeds the classic and adds the OAS API definition as a field.
+type MergedAPI struct {
+	*apidef.APIDefinition `json:"api_definition,inline"`
+	OAS                   *oas.OAS `json:"oas"`
+}
+
+// Logger returns API detail fields for logging.
+func (m *MergedAPI) LogFields() logrus.Fields {
+	return logrus.Fields{
+		"api_id": m.APIID,
+		"org_id": m.OrgID,
+		"name":   m.Name,
+		"path":   m.Proxy.ListenPath,
+	}
+}
+
 // MergedAPIList is the response body for FromDashboardService.
 type MergedAPIList struct {
 	Message []MergedAPI
@@ -17,12 +35,6 @@ func NewMergedAPIList(apis ...MergedAPI) *MergedAPIList {
 	}
 }
 
-// MergedAPI combines the embeds the classic and adds the OAS API definition as a field.
-type MergedAPI struct {
-	*apidef.APIDefinition `json:"api_definition,inline"`
-	OAS                   *oas.OAS `json:"oas"`
-}
-
 // Set sets the available classic API definitions to the MergedAPIList.
 func (f *MergedAPIList) SetClassic(defs []*apidef.APIDefinition) {
 	for _, def := range defs {