From 20c744321e85ca4b142431ab178531fa1cd96d6b Mon Sep 17 00:00:00 2001 From: Ernest Micklei Date: Tue, 28 Feb 2023 08:39:06 +0100 Subject: [PATCH 1/3] introduce MergePathStrategy for #521 #519 --- route_builder.go | 19 ++++++++++++++++++- web_service_test.go | 12 ++++++------ 2 files changed, 24 insertions(+), 7 deletions(-) diff --git a/route_builder.go b/route_builder.go index 830ebf14..85bc41c5 100644 --- a/route_builder.go +++ b/route_builder.go @@ -353,8 +353,25 @@ func (b *RouteBuilder) Build() Route { return route } +type MergePathStrategyFunc func(path1, path2 string) string + +var ( + // behavior 3.10.* + PathJoinStrategy = path.Join + + // behavior <= 3.9 + TrimSlashStrategy = func(path1, path2 string) string { + return strings.TrimRight(path1, "/") + "/" + strings.TrimLeft(path2, "/") + } + + // MergePathStrategy is the active strategy for merging a Route path when building the routing of all WebServices. + // The value is set to TrimSlashStrategy + // PathJoinStrategy is an alternative strategy that is more strict [Security - PRISMA-2022-0227] + MergePathStrategy = TrimSlashStrategy +) + func concatPath(path1, path2 string) string { - return path.Join(path1, path2) + return MergePathStrategy(path1, path2) } var anonymousFuncCount int32 diff --git a/web_service_test.go b/web_service_test.go index d2218169..47a496a9 100644 --- a/web_service_test.go +++ b/web_service_test.go @@ -337,12 +337,12 @@ func TestClientWithAndWithoutTrailingSlash(t *testing.T) { url string wantCode int }{ - // behavior before #520 - // {url: "http://here.com/test", wantCode: 404}, - // {url: "http://here.com/test/", wantCode: 200}, - // current behavior - {url: "http://here.com/test", wantCode: 200}, - {url: "http://here.com/test/", wantCode: 404}, + // TrimSlashStrategy + {url: "http://here.com/test", wantCode: 404}, + {url: "http://here.com/test/", wantCode: 200}, + // PathJoinStrategy + //{url: "http://here.com/test", wantCode: 200}, + //{url: "http://here.com/test/", wantCode: 404}, } { t.Run(tt.url, func(t *testing.T) { httpRequest, _ := http.NewRequest("PUT", tt.url, nil) From 333ff0f1259354baf7d615c19c560a8376c8a558 Mon Sep 17 00:00:00 2001 From: Ernest Micklei Date: Sun, 5 Mar 2023 22:50:02 +0100 Subject: [PATCH 2/3] update readme, set default to new strategy, add extra test --- README.md | 4 ++++ filter_test.go | 1 + route_builder.go | 23 +++++++++++++---------- web_service_test.go | 34 +++++++++++++++++++++++++++++++++- 4 files changed, 51 insertions(+), 11 deletions(-) diff --git a/README.md b/README.md index 0625359d..59ebc824 100644 --- a/README.md +++ b/README.md @@ -96,6 +96,10 @@ There are several hooks to customize the behavior of the go-restful package. - Compression - Encoders for other serializers - Use [jsoniter](https://github.com/json-iterator/go) by building this package using a build tag, e.g. `go build -tags=jsoniter .` +- Use the variable `MergePathStrategy` to change the behaviour of composing the Route path given a root path and a local route path + - versions >= 3.10.1 has set the value to `PathJoinStrategy` that fixes a reported security issue but may cause your services not to work correctly anymore. + - versions <= 3.9 had the behaviour that can be restored in newer versions by setting the value to `TrimSlashStrategy`. + - you can set value to a custom implementation (must implement MergePathStrategyFunc) ## Resources diff --git a/filter_test.go b/filter_test.go index fadfb570..df6fa4bc 100644 --- a/filter_test.go +++ b/filter_test.go @@ -18,6 +18,7 @@ func tearDown() { DefaultContainer.webServices = []*WebService{} DefaultContainer.isRegisteredOnRoot = true // this allows for setupServices multiple times DefaultContainer.containerFilters = []FilterFunction{} + MergePathStrategy = PathJoinStrategy } func newTestService(addServiceFilter bool, addRouteFilter bool) *WebService { diff --git a/route_builder.go b/route_builder.go index 85bc41c5..827f471d 100644 --- a/route_builder.go +++ b/route_builder.go @@ -353,25 +353,28 @@ func (b *RouteBuilder) Build() Route { return route } -type MergePathStrategyFunc func(path1, path2 string) string +type MergePathStrategyFunc func(rootPath, routePath string) string var ( - // behavior 3.10.* - PathJoinStrategy = path.Join + // behavior >= 3.10 + PathJoinStrategy = func(rootPath, routePath string) string { + return path.Join(rootPath, routePath) + } // behavior <= 3.9 - TrimSlashStrategy = func(path1, path2 string) string { - return strings.TrimRight(path1, "/") + "/" + strings.TrimLeft(path2, "/") + TrimSlashStrategy = func(rootPath, routePath string) string { + return strings.TrimRight(rootPath, "/") + "/" + strings.TrimLeft(routePath, "/") } // MergePathStrategy is the active strategy for merging a Route path when building the routing of all WebServices. - // The value is set to TrimSlashStrategy - // PathJoinStrategy is an alternative strategy that is more strict [Security - PRISMA-2022-0227] - MergePathStrategy = TrimSlashStrategy + // The value is set to PathJoinStrategy + // PathJoinStrategy is a strategy that is more strict [Security - PRISMA-2022-0227] + MergePathStrategy = PathJoinStrategy ) -func concatPath(path1, path2 string) string { - return MergePathStrategy(path1, path2) +// merge two paths using the current (package global) merge path strategy. +func concatPath(rootPath, routePath string) string { + return MergePathStrategy(rootPath, routePath) } var anonymousFuncCount int32 diff --git a/web_service_test.go b/web_service_test.go index 47a496a9..c5a1ac10 100644 --- a/web_service_test.go +++ b/web_service_test.go @@ -327,8 +327,9 @@ func TestOptionsShortcut(t *testing.T) { } } -func TestClientWithAndWithoutTrailingSlash(t *testing.T) { +func TestClientWithAndWithoutTrailingSlashOld(t *testing.T) { tearDown() + MergePathStrategy = TrimSlashStrategy ws := new(WebService).Path("/test") ws.Route(ws.PUT("/").To(return200)) Add(ws) @@ -358,6 +359,37 @@ func TestClientWithAndWithoutTrailingSlash(t *testing.T) { } } +func TestClientWithAndWithoutTrailingSlash(t *testing.T) { + tearDown() + ws := new(WebService).Path("/test") + ws.Route(ws.PUT("/").To(return200)) + Add(ws) + + for _, tt := range []struct { + url string + wantCode int + }{ + // TrimSlashStrategy + //{url: "http://here.com/test", wantCode: 404}, + //{url: "http://here.com/test/", wantCode: 200}, + // PathJoinStrategy + {url: "http://here.com/test", wantCode: 200}, + {url: "http://here.com/test/", wantCode: 404}, + } { + t.Run(tt.url, func(t *testing.T) { + httpRequest, _ := http.NewRequest("PUT", tt.url, nil) + httpRequest.Header.Set("Accept", "*/*") + httpWriter := httptest.NewRecorder() + // override the default here + DefaultContainer.DoNotRecover(false) + DefaultContainer.dispatch(httpWriter, httpRequest) + if tt.wantCode != httpWriter.Code { + t.Errorf("Expected %d, got %d", tt.wantCode, httpWriter.Code) + } + }) + } +} + func newPanicingService() *WebService { ws := new(WebService).Path("") ws.Route(ws.GET("/fire").To(doPanic)) From 96502fec7605b4a15bfe4ba0267cf45bd99ec1ba Mon Sep 17 00:00:00 2001 From: Ernest Micklei Date: Mon, 6 Mar 2023 16:26:06 +0100 Subject: [PATCH 3/3] link to security issue --- README.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/README.md b/README.md index 59ebc824..85da9012 100644 --- a/README.md +++ b/README.md @@ -97,7 +97,7 @@ There are several hooks to customize the behavior of the go-restful package. - Encoders for other serializers - Use [jsoniter](https://github.com/json-iterator/go) by building this package using a build tag, e.g. `go build -tags=jsoniter .` - Use the variable `MergePathStrategy` to change the behaviour of composing the Route path given a root path and a local route path - - versions >= 3.10.1 has set the value to `PathJoinStrategy` that fixes a reported security issue but may cause your services not to work correctly anymore. + - versions >= 3.10.1 has set the value to `PathJoinStrategy` that fixes a reported [security issue](https://github.com/advisories/GHSA-r48q-9g5r-8q2h) but may cause your services not to work correctly anymore. - versions <= 3.9 had the behaviour that can be restored in newer versions by setting the value to `TrimSlashStrategy`. - you can set value to a custom implementation (must implement MergePathStrategyFunc)