From fb03a7b1f63b51b7434b1cadb664cb84524b6893 Mon Sep 17 00:00:00 2001 From: Mikko Valkonen Date: Wed, 11 Sep 2024 12:15:27 +0300 Subject: [PATCH 1/4] Add normalizePath filter Adds a filter called `normalizePath` which implements URL Path normalization by removing empty path segments and trailing slashes from the path. This follows the guidance from the Zalando Restful API Guidelines rule [136][1]. [1]: https://opensource.zalando.com/restful-api-guidelines/#136 Signed-off-by: Mikko Valkonen --- filters/builtin/builtin.go | 2 + filters/filters.go | 1 + filters/normalizepath/normalizepath.go | 28 +++++++++++++ filters/normalizepath/normalizepath_test.go | 45 +++++++++++++++++++++ 4 files changed, 76 insertions(+) create mode 100644 filters/normalizepath/normalizepath.go create mode 100644 filters/normalizepath/normalizepath_test.go diff --git a/filters/builtin/builtin.go b/filters/builtin/builtin.go index 1003756c60..b0dc73611f 100644 --- a/filters/builtin/builtin.go +++ b/filters/builtin/builtin.go @@ -16,6 +16,7 @@ import ( "github.com/zalando/skipper/filters/fadein" "github.com/zalando/skipper/filters/flowid" logfilter "github.com/zalando/skipper/filters/log" + "github.com/zalando/skipper/filters/normalizepath" "github.com/zalando/skipper/filters/rfc" "github.com/zalando/skipper/filters/scheduler" "github.com/zalando/skipper/filters/sed" @@ -233,6 +234,7 @@ func Filters() []filters.Spec { consistenthash.NewConsistentHashKey(), consistenthash.NewConsistentHashBalanceFactor(), tls.New(), + normalizepath.NewNormalizePath(), } } diff --git a/filters/filters.go b/filters/filters.go index 358e666b17..110a970076 100644 --- a/filters/filters.go +++ b/filters/filters.go @@ -370,4 +370,5 @@ const ( SetFastCgiFilenameName = "setFastCgiFilename" DisableRatelimitName = "disableRatelimit" UnknownRatelimitName = "unknownRatelimit" + NormalizePath = "normalizePath" ) diff --git a/filters/normalizepath/normalizepath.go b/filters/normalizepath/normalizepath.go new file mode 100644 index 0000000000..bc6b5ccd17 --- /dev/null +++ b/filters/normalizepath/normalizepath.go @@ -0,0 +1,28 @@ +package normalizepath + +import ( + "path" + + "github.com/zalando/skipper/filters" +) + +const ( + Name = filters.NormalizePath +) + +type normalizePath struct{} + +func NewNormalizePath() filters.Spec { return normalizePath{} } + +func (spec normalizePath) Name() string { return "normalizePath" } + +func (spec normalizePath) CreateFilter(config []interface{}) (filters.Filter, error) { + return normalizePath{}, nil +} + +func (f normalizePath) Request(ctx filters.FilterContext) { + req := ctx.Request() + req.URL.Path = path.Clean(req.URL.Path) +} + +func (f normalizePath) Response(ctx filters.FilterContext) {} diff --git a/filters/normalizepath/normalizepath_test.go b/filters/normalizepath/normalizepath_test.go new file mode 100644 index 0000000000..a490749384 --- /dev/null +++ b/filters/normalizepath/normalizepath_test.go @@ -0,0 +1,45 @@ +package normalizepath + +import ( + "net/http" + "net/url" + "testing" + + "github.com/zalando/skipper/filters/filtertest" +) + +// TestNormalizePath tests the NormalizePath function in accordance to the +// https://opensource.zalando.com/restful-api-guidelines/#136 +// specifically the notion of normalization of request paths: +// +// All services should normalize request paths before processing by removing +// duplicate and trailing slashes. Hence, the following requests should refer +// to the same resource: +// GET /orders/{order-id} +// GET /orders/{order-id}/ +// GET /orders//{order-id} +func TestNormalizePath(t *testing.T) { + urls := []string{ + "/orders/{order-id}", + "/orders/{order-id}/", + "/orders//{order-id}", + "/orders/{order-id}//", + "/orders/{order-id}///", + "/orders///{order-id}//", + } + + for _, u := range urls { + req := &http.Request{URL: &url.URL{Path: u}} + ctx := &filtertest.Context{ + FRequest: req, + } + f, err := NewNormalizePath().CreateFilter(nil) + if err != nil { + t.Fatal(err) + } + f.Request(ctx) + if req.URL.Path != "/orders/{order-id}" { + t.Errorf("failed to normalize the path: %s", req.URL.Path) + } + } +} From deb82fbec0285b30e4974f7a455a1b5f88ed0475 Mon Sep 17 00:00:00 2001 From: Mikko Valkonen Date: Thu, 12 Sep 2024 10:42:33 +0300 Subject: [PATCH 2/4] Handle the path normalization manually path.Clean() may break certain edge-cases where the path-variables would use `.` or `..` characters. This commit removes the usage of path.Clean() and implements the normalization manually. Signed-off-by: Mikko Valkonen --- filters/normalizepath/normalizepath.go | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/filters/normalizepath/normalizepath.go b/filters/normalizepath/normalizepath.go index bc6b5ccd17..3073ee2a00 100644 --- a/filters/normalizepath/normalizepath.go +++ b/filters/normalizepath/normalizepath.go @@ -1,7 +1,7 @@ package normalizepath import ( - "path" + "strings" "github.com/zalando/skipper/filters" ) @@ -22,7 +22,22 @@ func (spec normalizePath) CreateFilter(config []interface{}) (filters.Filter, er func (f normalizePath) Request(ctx filters.FilterContext) { req := ctx.Request() - req.URL.Path = path.Clean(req.URL.Path) + + segments := strings.Split(req.URL.Path, "/") + var filteredSegments []string + for _, seg := range segments { + if seg != "" { + filteredSegments = append(filteredSegments, seg) + } + } + normalizedPath := "/" + strings.Join(filteredSegments, "/") + + // Ensure there's no trailing slash, unless the path is just "/" + if len(normalizedPath) > 1 && strings.HasSuffix(normalizedPath, "/") { + normalizedPath = normalizedPath[:len(normalizedPath)-1] + } + + req.URL.Path = normalizedPath } func (f normalizePath) Response(ctx filters.FilterContext) {} From 6c4fa2602060515e3439f9e69d45f03ec450bf6d Mon Sep 17 00:00:00 2001 From: Mikko Valkonen Date: Mon, 16 Sep 2024 17:04:36 +0300 Subject: [PATCH 3/4] Simplify the implementation Removes some dead code and adds test to cover for root path segment. Signed-off-by: Mikko Valkonen --- filters/normalizepath/normalizepath.go | 5 ----- filters/normalizepath/normalizepath_test.go | 14 ++++++++++++++ 2 files changed, 14 insertions(+), 5 deletions(-) diff --git a/filters/normalizepath/normalizepath.go b/filters/normalizepath/normalizepath.go index 3073ee2a00..b6c5dd6416 100644 --- a/filters/normalizepath/normalizepath.go +++ b/filters/normalizepath/normalizepath.go @@ -32,11 +32,6 @@ func (f normalizePath) Request(ctx filters.FilterContext) { } normalizedPath := "/" + strings.Join(filteredSegments, "/") - // Ensure there's no trailing slash, unless the path is just "/" - if len(normalizedPath) > 1 && strings.HasSuffix(normalizedPath, "/") { - normalizedPath = normalizedPath[:len(normalizedPath)-1] - } - req.URL.Path = normalizedPath } diff --git a/filters/normalizepath/normalizepath_test.go b/filters/normalizepath/normalizepath_test.go index a490749384..58dbee3221 100644 --- a/filters/normalizepath/normalizepath_test.go +++ b/filters/normalizepath/normalizepath_test.go @@ -42,4 +42,18 @@ func TestNormalizePath(t *testing.T) { t.Errorf("failed to normalize the path: %s", req.URL.Path) } } + + // Ensure that root paths work as expected + req := &http.Request{URL: &url.URL{Path: "/"}} + ctx := &filtertest.Context{ + FRequest: req, + } + f, err := NewNormalizePath().CreateFilter(nil) + if err != nil { + t.Fatal(err) + } + f.Request(ctx) + if req.URL.Path != "/" { + t.Errorf("unexpected URL path change: %s, expected /", req.URL.Path) + } } From a3b67653b824d6d34f88635ca3f245b5e8aa4c8f Mon Sep 17 00:00:00 2001 From: Mikko Valkonen Date: Thu, 19 Sep 2024 11:48:41 +0300 Subject: [PATCH 4/4] Add documentation for the normalizePath()-filter Adds rudimentary documentation for the normalizePath()-filter with examples. Signed-off-by: Mikko Valkonen --- docs/reference/filters.md | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/docs/reference/filters.md b/docs/reference/filters.md index 584333ba68..bf87dcbb97 100644 --- a/docs/reference/filters.md +++ b/docs/reference/filters.md @@ -365,6 +365,32 @@ Parameters: The replacement may contain [template placeholders](#template-placeholders). If a template placeholder can't be resolved then empty value is used for it. +### normalizePath + +Normalize the URL path by removing empty path segments and trailing slashes. + +Parameters: + +- The URL path + +Example: + +``` +all: * -> normalizePath() -> "https://backend.example.org/api/v1; +``` + +Requests to + +``` +https://backend.example.org//api/v1/ +https://backend.example.org//api/v1 +https://backend.example.org/api//v1 +https://backend.example.org/api//v1/ +https://backend.example.org/api/v1/ +``` + +will all be normalized to `https://backend.example.org/api/v1`.
 + ## HTTP Redirect ### redirectTo