Path resolution

CHANGELOG.md

@@ -4,6 +4,12 @@
 
 Unreleased changes are available as `avenga/couper:edge` container.
 
+* **Fixed**
+  * Disallow empty path parameters ([#526](https://github.com/avenga/couper/pull/526))
+
+* **Removed**
+  * Endpoint path normalization to better match OpenAPI behavior ([#526](https://github.com/avenga/couper/pull/526))
+
 ---
 
 ## [1.9.1](https://github.com/avenga/couper/releases/tag/v1.9.1)

config/configload/load_test.go

@@ -155,3 +155,97 @@ func TestHealthCheck(t *testing.T) {
 		})
 	}
 }
+
+func TestEndpointPaths(t *testing.T) {
+	tests := []struct {
+		name       string
+		serverBase string
+		apiBase    string
+		endpoint   string
+		expected   string
+	}{
+		{"only /", "", "", "/", "/"},
+		{"missing /", "", "", "path", "/path"},
+		{"simple path", "", "", "/pa/th", "/pa/th"},
+		{"trailing /", "", "", "/pa/th/", "/pa/th/"},
+		{"double /", "", "", "//", "//"},
+		{"double /", "", "", "//path", "//path"},
+		{"double /", "", "", "/pa//th", "/pa//th"},
+		{"/./", "", "", "/./", "/./"},
+		{"/../", "", "", "/../", "/../"},
+
+		{"param", "", "", "/{param}", "/{param}"},
+
+		{"server base_path /", "/", "", "/", "/"},
+		{"server base_path /", "/", "", "/path", "/path"},
+		{"server base_path /", "/", "", "pa/th", "/pa/th"},
+		{"server base_path /", "/", "", "pa/th/", "/pa/th/"},
+		{"server base_path", "/server", "", "/path", "/server/path"},
+		{"server base_path with / endpoint", "/server", "", "/", "/server"},
+		{"server base_path missing /", "server", "", "/path", "/server/path"},
+		{"server base_path trailing /", "/server/", "", "/path", "/server/path"},
+		{"server base_path double /", "/server", "", "//path", "/server//path"},
+		{"server base_path trailing + double /", "/server/", "", "//path", "/server//path"},
+
+		{"api base_path /", "", "/", "/", "/"},
+		{"api base_path /", "", "/", "/path", "/path"},
+		{"api base_path /", "", "/", "pa/th", "/pa/th"},
+		{"api base_path /", "", "/", "pa/th/", "/pa/th/"},
+		{"api base_path", "", "/api", "/path", "/api/path"},
+		{"api base_path with / endpoint", "", "/api", "/", "/api"},
+		{"api base_path missing /", "", "api", "/path", "/api/path"},
+		{"api base_path trailing /", "", "/api/", "/path", "/api/path"},
+		{"api base_path double /", "", "/api", "//path", "/api//path"},
+		{"api base_path trailing + double /", "/api/", "", "//path", "/api//path"},
+
+		{"server + api base_path /", "/", "/", "/", "/"},
+		{"server + api base_path", "/server", "/api", "/", "/server/api"},
+		{"server + api base_path", "/server", "/api", "/path", "/server/api/path"},
+		{"server + api base_path missing /", "server", "api", "/", "/server/api"},
+	}
+
+	logger, _ := test.NewLogger()
+	log := logger.WithContext(context.TODO())
+
+	template := `
+		server {
+		  base_path = "%s"
+		  api {
+		    base_path = "%s"
+		    endpoint "%s" {
+		      response {}
+		    }
+		  }
+		}`
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(subT *testing.T) {
+			configBytes := []byte(fmt.Sprintf(template, tt.serverBase, tt.apiBase, tt.endpoint))
+			config, err := configload.LoadBytes(configBytes, "couper.hcl")
+
+			closeCh := make(chan struct{})
+			defer close(closeCh)
+			memStore := cache.New(log, closeCh)
+
+			var serverConfig runtime.ServerConfiguration
+			if err == nil {
+				serverConfig, err = runtime.NewServerConfiguration(config, log, memStore)
+			}
+
+			if err != nil {
+				subT.Errorf("%q: Unexpected configuration error:\n\tWant: <nil>\n\tGot:  %q", tt.name, err)
+				return
+			}
+
+			var pattern string
+			for key := range serverConfig[8080]["*"].EndpointRoutes {
+				pattern = key
+				break
+			}
+
+			if pattern != tt.expected {
+				subT.Errorf("%q: Unexpected endpoint path:\n\tWant: %q\n\tGot:  %q", tt.name, tt.expected, pattern)
+			}
+		})
+	}
+}

config/runtime/server.go

@@ -260,8 +260,7 @@ func NewServerConfiguration(conf *config.Couper, log *logrus.Entry, memStore *ca
 			if parentAPI != nil {
 				basePath = serverOptions.APIBasePaths[parentAPI]
 			}
-
-			pattern := utils.JoinPath(basePath, endpointConf.Pattern)
+			pattern := utils.JoinOpenAPIPath(basePath, endpointConf.Pattern)
 			unique, cleanPattern := isUnique(endpointPatterns, pattern)
 			if !unique {
 				return nil, fmt.Errorf("%s: duplicate endpoint: '%s'", endpointConf.HCLBody().MissingItemRange().String(), pattern)
@@ -650,8 +649,6 @@ func setRoutesFromHosts(
 	srvConf ServerConfiguration, portsHosts Ports,
 	path string, handler http.Handler, kind HandlerKind,
 ) error {
-	path = utils.JoinPath("/", path)
-
 	for port, hosts := range portsHosts {
 		for host := range hosts {
 			var routes map[string]http.Handler

config/runtime/server/options.go

@@ -84,11 +84,11 @@ func NewServerOptions(conf *config.Server, logger *logrus.Entry) (*Options, erro
 			options.FilesErrTpls[i] = tpl
 		}
 
-		options.FilesBasePaths[i] = utils.JoinPath(options.SrvBasePath, f.BasePath)
+		options.FilesBasePaths[i] = utils.JoinOpenAPIPath(options.SrvBasePath, f.BasePath)
 	}
 
 	for _, s := range conf.SPAs {
-		options.SPABasePaths = append(options.SPABasePaths, utils.JoinPath(options.SrvBasePath, s.BasePath))
+		options.SPABasePaths = append(options.SPABasePaths, utils.JoinOpenAPIPath(options.SrvBasePath, s.BasePath))
 	}
 
 	return options, nil

handler/transport/backend_test.go

@@ -180,7 +180,7 @@ func TestBackend_RoundTrip_Validation(t *testing.T) {
 	}))
 	defer origin.Close()
 
-	openAPIYAML := helper.NewOpenAPIConf("/get")
+	openAPIYAML := helper.NewOpenAPIConf("/pa/./th")
 
 	tests := []struct {
 		name               string
@@ -194,39 +194,79 @@ func TestBackend_RoundTrip_Validation(t *testing.T) {
 			"valid request / valid response",
 			&config.OpenAPI{File: "testdata/upstream.yaml"},
 			http.MethodGet,
-			"/get",
+			"/pa/./th",
 			"",
 			"",
 		},
+		{
+			"valid path: trailing /",
+			&config.OpenAPI{File: "testdata/upstream.yaml"},
+			http.MethodPost,
+			"/pa/./th/",
+			"backend error",
+			"",
+		},
+		{
+			"invalid path: no path resolution",
+			&config.OpenAPI{File: "testdata/upstream.yaml"},
+			http.MethodGet,
+			"/pa/th",
+			"backend error",
+			"'GET /pa/th': no matching operation was found",
+		},
+		{
+			"invalid path: double /",
+			&config.OpenAPI{File: "testdata/upstream.yaml"},
+			http.MethodGet,
+			"/pa/.//th",
+			"backend error",
+			"'GET /pa/.//th': no matching operation was found",
+		},
+		{
+			"URL encoded request",
+			&config.OpenAPI{File: "testdata/upstream.yaml"},
+			http.MethodGet,
+			"/pa%2f%2e%2fth",
+			"",
+			"",
+		},
+		{
+			"URL encoded request, wrong method",
+			&config.OpenAPI{File: "testdata/upstream.yaml"},
+			http.MethodPost,
+			"/pa%2f%2e%2fth",
+			"backend error",
+			"'POST /pa/./th': method not allowed",
+		},
 		{
 			"invalid request",
 			&config.OpenAPI{File: "testdata/upstream.yaml"},
 			http.MethodPost,
-			"/get",
+			"/pa/./th",
 			"backend error",
-			"'POST /get': method not allowed",
+			"'POST /pa/./th': method not allowed",
 		},
 		{
 			"invalid request, IgnoreRequestViolations",
 			&config.OpenAPI{File: "testdata/upstream.yaml", IgnoreRequestViolations: true, IgnoreResponseViolations: true},
 			http.MethodPost,
-			"/get",
+			"/pa/./th",
 			"",
-			"'POST /get': method not allowed",
+			"'POST /pa/./th': method not allowed",
 		},
 		{
 			"invalid response",
 			&config.OpenAPI{File: "testdata/upstream.yaml"},
 			http.MethodGet,
-			"/get?404",
+			"/pa/./th?404",
 			"backend error",
 			"status is not supported",
 		},
 		{
 			"invalid response, IgnoreResponseViolations",
 			&config.OpenAPI{File: "testdata/upstream.yaml", IgnoreResponseViolations: true},
 			http.MethodGet,
-			"/get?404",
+			"/pa/./th?404",
 			"",
 			"status is not supported",
 		},

server/mux.go

@@ -32,7 +32,8 @@ type Mux struct {
 
 const (
 	serverOptionsKey    = "serverContextOptions"
-	wildcardReplacement = "/{_couper_wildcardMatch*}"
+	wildcardVariable    = "_couper_wildcardMatch"
+	wildcardReplacement = "/{" + wildcardVariable + "*}"
 	wildcardSearch      = "/**"
 )
 
@@ -56,7 +57,7 @@ func NewMux(options *runtime.MuxOptions) *Mux {
 	}
 
 	for path, h := range opts.FileRoutes {
-		mux.mustAddRoute(mux.fileRoot, utils.JoinPath(path, "/**"), h, false)
+		mux.mustAddRoute(mux.fileRoot, utils.JoinOpenAPIPath(path, "/**"), h, false)
 	}
 
 	for path, h := range opts.SPARoutes {
@@ -89,7 +90,7 @@ func (m *Mux) mustAddRoute(root *pathpattern.Node, path string, handler http.Han
 func (m *Mux) FindHandler(req *http.Request) http.Handler {
 	var route *routers.Route
 
-	node, paramValues := m.match(m.endpointRoot, req)
+	node, paramValues := m.matchWithMethod(m.endpointRoot, req)
 	if node == nil {
 		node, paramValues = m.matchWithoutMethod(m.endpointRoot, req)
 	}
@@ -130,10 +131,9 @@ func (m *Mux) FindHandler(req *http.Request) http.Handler {
 
 	ctx := req.Context()
 
-	const wcm = "_couper_wildcardMatch"
-	if wildcardMatch, ok := pathParams[wcm]; ok {
+	if wildcardMatch, ok := pathParams[wildcardVariable]; ok {
 		ctx = context.WithValue(ctx, request.Wildcard, wildcardMatch)
-		delete(pathParams, wcm)
+		delete(pathParams, wildcardVariable)
 	}
 
 	ctx = context.WithValue(ctx, request.PathParams, pathParams)
@@ -142,16 +142,27 @@ func (m *Mux) FindHandler(req *http.Request) http.Handler {
 	return m.handler[route]
 }
 
-func (m *Mux) match(root *pathpattern.Node, req *http.Request) (*pathpattern.Node, []string) {
+func (m *Mux) matchWithMethod(root *pathpattern.Node, req *http.Request) (*pathpattern.Node, []string) {
 	*req = *req.WithContext(context.WithValue(req.Context(), request.ServerName, m.opts.ServerOptions.ServerName))
 
-	return root.Match(req.Method + " " + req.URL.Path)
+	return m.match(root, req.Method+" "+req.URL.Path)
 }
 
 func (m *Mux) matchWithoutMethod(root *pathpattern.Node, req *http.Request) (*pathpattern.Node, []string) {
 	*req = *req.WithContext(context.WithValue(req.Context(), request.ServerName, m.opts.ServerOptions.ServerName))
 
-	return root.Match(req.URL.Path)
+	return m.match(root, req.URL.Path)
+}
+
+func (m *Mux) match(root *pathpattern.Node, requestLine string) (*pathpattern.Node, []string) {
+	node, values := root.Match(requestLine)
+	for i, value := range values {
+		if value == "" && node.VariableNames[i] != wildcardVariable+"*" {
+			// Path params must not be empty.
+			return nil, nil
+		}
+	}
+	return node, values
 }
 
 func (m *Mux) hasFileResponse(req *http.Request) (http.Handler, bool) {

server/mux_test.go

@@ -31,6 +31,7 @@ func TestMux_FindHandler_PathParamContext(t *testing.T) {
 			"/{b}/a":                noContent,
 			"/{section}/{project}":  noContent,
 			"/project/{project}/**": noContent,
+			"/w/c/{param}/**":       noContent,
 		},
 		FileRoutes: map[string]http.Handler{
 			"/htdocs/test.html": noContent,
@@ -59,13 +60,18 @@ func TestMux_FindHandler_PathParamContext(t *testing.T) {
 		{" /w 1st path param #2", newReq("/c/a"), noContent, request.PathParameter{
 			"b": "c",
 		}, ""},
+		{" w/o 1nd path param", newReq("//a"), http.NotFoundHandler(), nil, ""},
 		{" /w 2nd path param", newReq("/a/c"), noContent, request.PathParameter{
 			"b": "c",
 		}, ""},
+		{" w/o 2nd path param", newReq("/a"), http.NotFoundHandler(), nil, ""},
+		{" w/o 2nd path param", newReq("/a/"), http.NotFoundHandler(), nil, ""},
+		{" w/o 2nd path param", newReq("/a//"), http.NotFoundHandler(), nil, ""},
 		{" /w two path param", newReq("/foo/bar"), noContent, request.PathParameter{
 			"section": "foo",
 			"project": "bar",
 		}, ""},
+		{" w/o two path params", newReq("//"), http.NotFoundHandler(), nil, ""},
 		{" /w path param and expWildcard", newReq("/project/foo/bar/ha"), noContent, request.PathParameter{
 			"project": "foo",
 		}, "bar/ha"},
@@ -74,6 +80,20 @@ func TestMux_FindHandler_PathParamContext(t *testing.T) {
 			"section": "htdocs",
 			"project": "test.html",
 		}, ""},
+		{" w/ path param and wildcard", newReq("/w/c/my-param/wild/ca/rd"), noContent, request.PathParameter{
+			"param": "my-param",
+		}, "wild/ca/rd"},
+		{" w/ path param, w/o wildcard", newReq("/w/c/my-param"), noContent, request.PathParameter{
+			"param": "my-param",
+		}, ""},
+		{" w/ path param, trailing /, w/o wildcard", newReq("/w/c/my-param/"), noContent, request.PathParameter{
+			"param": "my-param",
+		}, ""},
+		{" w/o path param, w/ wildcard", newReq("/w/c//wild/card"), http.NotFoundHandler(), nil, ""},
+		{" w/o path param, w/o wildcard", newReq("/w/c"), http.NotFoundHandler(), nil, ""},
+		{" w/o path param, w/o wildcard", newReq("/w/c/"), http.NotFoundHandler(), nil, ""},
+		{" w/o path param, w/o wildcard", newReq("/w/c//"), http.NotFoundHandler(), nil, ""},
+		{" w/o path param, w/o wildcard", newReq("/w/c///"), http.NotFoundHandler(), nil, ""},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(subT *testing.T) {

utils/path.go

@@ -8,6 +8,9 @@ import (
 // JoinPath ensures the file-muxer behaviour for redirecting
 // '/path' to '/path/' if not explicitly specified.
 func JoinPath(elements ...string) string {
+	if len(elements) == 0 {
+		return ""
+	}
 	suffix := "/"
 	if !strings.HasSuffix(elements[len(elements)-1], "/") {
 		suffix = ""
@@ -20,3 +23,27 @@ func JoinPath(elements ...string) string {
 
 	return path + suffix
 }
+
+func JoinOpenAPIPath(elements ...string) string {
+	if len(elements) == 0 {
+		return ""
+	}
+
+	if elements[len(elements)-1] == "/" {
+		elements = elements[:len(elements)-1]
+	}
+
+	result := ""
+	for _, element := range elements {
+		if element == "" {
+			element = "/"
+		}
+		resultHasSlashSuffix := result != "" && strings.HasSuffix(result, "/")
+		if strings.HasPrefix(element, "/") && !resultHasSlashSuffix {
+			result += "/"
+		}
+
+		result += strings.TrimPrefix(element, "/")
+	}
+	return result
+}