Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Path resolution #526

Merged
merged 5 commits into from
Jun 24, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
94 changes: 94 additions & 0 deletions config/configload/load_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
}
})
}
}
5 changes: 1 addition & 4 deletions config/runtime/server.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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
Expand Down
4 changes: 2 additions & 2 deletions config/runtime/server/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
56 changes: 48 additions & 8 deletions handler/transport/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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",
},
Expand Down
29 changes: 20 additions & 9 deletions server/mux.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,8 @@ type Mux struct {

const (
serverOptionsKey = "serverContextOptions"
wildcardReplacement = "/{_couper_wildcardMatch*}"
wildcardVariable = "_couper_wildcardMatch"
wildcardReplacement = "/{" + wildcardVariable + "*}"
wildcardSearch = "/**"
)

Expand All @@ -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 {
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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)
Expand All @@ -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) {
Expand Down
20 changes: 20 additions & 0 deletions server/mux_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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"},
Expand All @@ -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) {
Expand Down
27 changes: 27 additions & 0 deletions utils/path.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 = ""
Expand All @@ -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
}
Loading