From c03b29245e1ccdc672dc48034712332d8cc067e4 Mon Sep 17 00:00:00 2001 From: Sandeep Bhat Date: Tue, 4 Oct 2022 18:45:22 +0530 Subject: [PATCH 01/10] Support sanitising the URL by removing extra slashes in the URL --- go.mod | 1 + go.sum | 2 ++ routers/common/middleware.go | 34 +++++++++++++++++++-- routers/common/middleware_test.go | 49 +++++++++++++++++++++++++++++++ 4 files changed, 84 insertions(+), 2 deletions(-) create mode 100644 routers/common/middleware_test.go diff --git a/go.mod b/go.mod index 4501944a22064..14fb0d3d06f04 100644 --- a/go.mod +++ b/go.mod @@ -170,6 +170,7 @@ require ( github.com/fxamacker/cbor/v2 v2.4.0 // indirect github.com/go-ap/errors v0.0.0-20220917143055-4283ea5dae18 // indirect github.com/go-asn1-ber/asn1-ber v1.5.4 // indirect + github.com/go-chi/chi v1.5.4 // indirect github.com/go-enry/go-oniguruma v1.2.1 // indirect github.com/go-git/gcfg v1.5.0 // indirect github.com/go-openapi/analysis v0.21.4 // indirect diff --git a/go.sum b/go.sum index b6d76dfe6f462..6847e69efef8e 100644 --- a/go.sum +++ b/go.sum @@ -479,6 +479,8 @@ github.com/go-ap/jsonld v0.0.0-20220917142617-76bf51585778 h1:0tV3i8tE1NghMC4rXZ github.com/go-ap/jsonld v0.0.0-20220917142617-76bf51585778/go.mod h1:jyveZeGw5LaADntW+UEsMjl3IlIwk+DxlYNsbofQkGA= github.com/go-asn1-ber/asn1-ber v1.5.4 h1:vXT6d/FNDiELJnLb6hGNa309LMsrCoYFvpwHDF0+Y1A= github.com/go-asn1-ber/asn1-ber v1.5.4/go.mod h1:hEBeB/ic+5LoWskz+yKT7vGhhPYkProFKoKdwZRWMe0= +github.com/go-chi/chi v1.5.4 h1:QHdzF2szwjqVV4wmByUnTcsbIg7UGaQ0tPF2t5GcAIs= +github.com/go-chi/chi v1.5.4/go.mod h1:uaf8YgoFazUOkPBG7fxPftUylNumIev9awIWOENIuEg= github.com/go-chi/chi/v5 v5.0.1/go.mod h1:DslCQbL2OYiznFReuXYUmQ2hGd1aDpCnlMNITLSKoi8= github.com/go-chi/chi/v5 v5.0.4/go.mod h1:DslCQbL2OYiznFReuXYUmQ2hGd1aDpCnlMNITLSKoi8= github.com/go-chi/chi/v5 v5.0.7 h1:rDTPXLDHGATaeHvVlLcR4Qe0zftYethFucbjVQ1PxU8= diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 6ea1e1dfbe5ea..6cb6873ba67ac 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -16,7 +16,7 @@ import ( "code.gitea.io/gitea/modules/web/routing" "github.com/chi-middleware/proxy" - "github.com/go-chi/chi/v5/middleware" + "github.com/go-chi/chi" ) // Middlewares returns common middlewares @@ -48,7 +48,8 @@ func Middlewares() []func(http.Handler) http.Handler { handlers = append(handlers, proxy.ForwardedHeaders(opt)) } - handlers = append(handlers, middleware.StripSlashes) + // Strip slashes. + handlers = append(handlers, stripSlashesMiddleware) if !setting.DisableRouterLog { handlers = append(handlers, routing.NewLoggerHandler()) @@ -81,3 +82,32 @@ func Middlewares() []func(http.Handler) http.Handler { }) return handlers } + +func stripSlashesMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { + var path string + rctx := chi.RouteContext(req.Context()) + if rctx != nil && rctx.RoutePath != "" { + path = rctx.RoutePath + } else { + path = req.URL.Path + } + + if len(path) > 1 { + var sanitisedPath string + splitPath := strings.Split(path, "/") + for _, entry := range splitPath { + if len(entry) > 0 { + sanitisedPath = sanitisedPath + "/" + entry + } + } + + if rctx == nil { + req.URL.Path = sanitisedPath + } else { + rctx.RoutePath = sanitisedPath + } + } + next.ServeHTTP(resp, req) + }) +} diff --git a/routers/common/middleware_test.go b/routers/common/middleware_test.go new file mode 100644 index 0000000000000..ddf36511bb83f --- /dev/null +++ b/routers/common/middleware_test.go @@ -0,0 +1,49 @@ +package common + +import ( + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/assert" +) + +func TestStripSlashesMiddleware(t *testing.T) { + + type test struct { + name string + expectedPath string + inputPath string + } + + tests := []test{ + { + name: "path with multiple slashes", + inputPath: "https://github.com///go-gitea//gitea.git", + expectedPath: "https://github.com/go-gitea/gitea.git", + }, + { + name: "path with no slashes", + inputPath: "https://github.com/go-gitea/gitea.git", + expectedPath: "https://github.com/go-gitea/gitea.git", + }, + { + name: "path with slashes in the middle", + inputPath: "https://git.data.coop//halfd/new-website.git", + expectedPath: "https://git.data.coop/halfd/new-website.git", + }, + } + + for _, tt := range tests { + testMiddleware := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + assert.Equal(t, tt.expectedPath, r.URL.String()) + }) + + // pass the test middleware to validate the changes + handlerToTest := stripSlashesMiddleware(testMiddleware) + // create a mock request to use + req := httptest.NewRequest("GET", tt.inputPath, nil) + // call the handler using a mock response recorder + handlerToTest.ServeHTTP(httptest.NewRecorder(), req) + } +} From c31a914016b78c909642b82cecc9b6d320854d39 Mon Sep 17 00:00:00 2001 From: Sandeep Bhat Date: Tue, 4 Oct 2022 19:13:40 +0530 Subject: [PATCH 02/10] Fix linting issues --- go.mod | 2 +- routers/common/middleware_test.go | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/go.mod b/go.mod index 14fb0d3d06f04..c75d703dcb487 100644 --- a/go.mod +++ b/go.mod @@ -33,6 +33,7 @@ require ( github.com/gliderlabs/ssh v0.3.5 github.com/go-ap/activitypub v0.0.0-20220917143152-e4e7018838c0 github.com/go-ap/jsonld v0.0.0-20220917142617-76bf51585778 + github.com/go-chi/chi v1.5.4 github.com/go-chi/chi/v5 v5.0.7 github.com/go-chi/cors v1.2.1 github.com/go-enry/go-enry/v2 v2.8.2 @@ -170,7 +171,6 @@ require ( github.com/fxamacker/cbor/v2 v2.4.0 // indirect github.com/go-ap/errors v0.0.0-20220917143055-4283ea5dae18 // indirect github.com/go-asn1-ber/asn1-ber v1.5.4 // indirect - github.com/go-chi/chi v1.5.4 // indirect github.com/go-enry/go-oniguruma v1.2.1 // indirect github.com/go-git/gcfg v1.5.0 // indirect github.com/go-openapi/analysis v0.21.4 // indirect diff --git a/routers/common/middleware_test.go b/routers/common/middleware_test.go index ddf36511bb83f..b652125748204 100644 --- a/routers/common/middleware_test.go +++ b/routers/common/middleware_test.go @@ -1,3 +1,6 @@ +// Copyright 2022 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. package common import ( @@ -9,7 +12,6 @@ import ( ) func TestStripSlashesMiddleware(t *testing.T) { - type test struct { name string expectedPath string From 61a875c9d3350c43f0d6e8f6c3f92e134eaed5b7 Mon Sep 17 00:00:00 2001 From: Sandeep Bhat Date: Tue, 4 Oct 2022 20:15:59 +0530 Subject: [PATCH 03/10] Replace strings split with inbuilt path package --- routers/common/middleware.go | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 6cb6873ba67ac..8ea9afca90852 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -7,6 +7,7 @@ package common import ( "fmt" "net/http" + "path" "strings" "code.gitea.io/gitea/modules/context" @@ -85,27 +86,21 @@ func Middlewares() []func(http.Handler) http.Handler { func stripSlashesMiddleware(next http.Handler) http.Handler { return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) { - var path string + var urlPath string rctx := chi.RouteContext(req.Context()) if rctx != nil && rctx.RoutePath != "" { - path = rctx.RoutePath + urlPath = rctx.RoutePath } else { - path = req.URL.Path + urlPath = req.URL.Path } - if len(path) > 1 { - var sanitisedPath string - splitPath := strings.Split(path, "/") - for _, entry := range splitPath { - if len(entry) > 0 { - sanitisedPath = sanitisedPath + "/" + entry - } - } + if len(urlPath) > 1 { + urlPath = path.Clean(urlPath) if rctx == nil { - req.URL.Path = sanitisedPath + req.URL.Path = urlPath } else { - rctx.RoutePath = sanitisedPath + rctx.RoutePath = urlPath } } next.ServeHTTP(resp, req) From f01d054325278d9aca27cefa2058be8a354a997a Mon Sep 17 00:00:00 2001 From: Sandeep Bhat Date: Tue, 4 Oct 2022 21:54:52 +0530 Subject: [PATCH 04/10] Retain backslash suffix --- routers/common/middleware.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 8ea9afca90852..b2463ffd754bc 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -94,13 +94,18 @@ func stripSlashesMiddleware(next http.Handler) http.Handler { urlPath = req.URL.Path } + var sanitizedPath string if len(urlPath) > 1 { - urlPath = path.Clean(urlPath) + sanitizedPath = path.Clean(urlPath) + + if string(urlPath[len(urlPath)-1]) == "/" { + sanitizedPath = sanitizedPath + "/" + } if rctx == nil { - req.URL.Path = urlPath + req.URL.Path = sanitizedPath } else { - rctx.RoutePath = urlPath + rctx.RoutePath = sanitizedPath } } next.ServeHTTP(resp, req) From a5b865c4bff9128c910743b7d6e30a55c93bc754 Mon Sep 17 00:00:00 2001 From: Sandeep Bhat Date: Tue, 4 Oct 2022 23:30:12 +0530 Subject: [PATCH 05/10] Fix linting comments --- routers/common/middleware.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/routers/common/middleware.go b/routers/common/middleware.go index b2463ffd754bc..07f306fe35c7e 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -99,7 +99,7 @@ func stripSlashesMiddleware(next http.Handler) http.Handler { sanitizedPath = path.Clean(urlPath) if string(urlPath[len(urlPath)-1]) == "/" { - sanitizedPath = sanitizedPath + "/" + sanitizedPath += "/" } if rctx == nil { From 9d36d6b3a672d36317d881e3241756f88cf2db6d Mon Sep 17 00:00:00 2001 From: Sandeep Bhat Date: Thu, 6 Oct 2022 14:06:35 +0530 Subject: [PATCH 06/10] Add more test cases. Replace chi v1 with chi v5 --- go.mod | 1 - go.sum | 2 -- routers/common/middleware.go | 2 +- routers/common/middleware_test.go | 15 +++++++++++++++ 4 files changed, 16 insertions(+), 4 deletions(-) diff --git a/go.mod b/go.mod index c75d703dcb487..4501944a22064 100644 --- a/go.mod +++ b/go.mod @@ -33,7 +33,6 @@ require ( github.com/gliderlabs/ssh v0.3.5 github.com/go-ap/activitypub v0.0.0-20220917143152-e4e7018838c0 github.com/go-ap/jsonld v0.0.0-20220917142617-76bf51585778 - github.com/go-chi/chi v1.5.4 github.com/go-chi/chi/v5 v5.0.7 github.com/go-chi/cors v1.2.1 github.com/go-enry/go-enry/v2 v2.8.2 diff --git a/go.sum b/go.sum index 6847e69efef8e..b6d76dfe6f462 100644 --- a/go.sum +++ b/go.sum @@ -479,8 +479,6 @@ github.com/go-ap/jsonld v0.0.0-20220917142617-76bf51585778 h1:0tV3i8tE1NghMC4rXZ github.com/go-ap/jsonld v0.0.0-20220917142617-76bf51585778/go.mod h1:jyveZeGw5LaADntW+UEsMjl3IlIwk+DxlYNsbofQkGA= github.com/go-asn1-ber/asn1-ber v1.5.4 h1:vXT6d/FNDiELJnLb6hGNa309LMsrCoYFvpwHDF0+Y1A= github.com/go-asn1-ber/asn1-ber v1.5.4/go.mod h1:hEBeB/ic+5LoWskz+yKT7vGhhPYkProFKoKdwZRWMe0= -github.com/go-chi/chi v1.5.4 h1:QHdzF2szwjqVV4wmByUnTcsbIg7UGaQ0tPF2t5GcAIs= -github.com/go-chi/chi v1.5.4/go.mod h1:uaf8YgoFazUOkPBG7fxPftUylNumIev9awIWOENIuEg= github.com/go-chi/chi/v5 v5.0.1/go.mod h1:DslCQbL2OYiznFReuXYUmQ2hGd1aDpCnlMNITLSKoi8= github.com/go-chi/chi/v5 v5.0.4/go.mod h1:DslCQbL2OYiznFReuXYUmQ2hGd1aDpCnlMNITLSKoi8= github.com/go-chi/chi/v5 v5.0.7 h1:rDTPXLDHGATaeHvVlLcR4Qe0zftYethFucbjVQ1PxU8= diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 07f306fe35c7e..736a158e80090 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -17,7 +17,7 @@ import ( "code.gitea.io/gitea/modules/web/routing" "github.com/chi-middleware/proxy" - "github.com/go-chi/chi" + chi "github.com/go-chi/chi/v5" ) // Middlewares returns common middlewares diff --git a/routers/common/middleware_test.go b/routers/common/middleware_test.go index b652125748204..189f105924cc7 100644 --- a/routers/common/middleware_test.go +++ b/routers/common/middleware_test.go @@ -34,6 +34,21 @@ func TestStripSlashesMiddleware(t *testing.T) { inputPath: "https://git.data.coop//halfd/new-website.git", expectedPath: "https://git.data.coop/halfd/new-website.git", }, + { + name: "path with slashes in the middle", + inputPath: "https://git.data.coop//halfd/new-website.git", + expectedPath: "https://git.data.coop/halfd/new-website.git", + }, + { + name: "path with slashes in the end", + inputPath: "/user2//repo1/", + expectedPath: "/user2/repo1/", + }, + { + name: "path with slashes and query params", + inputPath: "/repo//migrate?service_type=3", + expectedPath: "/repo/migrate?service_type=3", + }, } for _, tt := range tests { From dfe8666521dbb9d89acf1c34b1ee5d227988e094 Mon Sep 17 00:00:00 2001 From: Sandeep Bhat Date: Fri, 7 Oct 2022 23:41:31 +0530 Subject: [PATCH 07/10] Fix inclusion of trailing slash --- routers/common/middleware.go | 26 +++++++++++++++++++------- routers/common/middleware_test.go | 2 +- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 736a158e80090..a9e4bfb51c866 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -7,7 +7,6 @@ package common import ( "fmt" "net/http" - "path" "strings" "code.gitea.io/gitea/modules/context" @@ -94,18 +93,31 @@ func stripSlashesMiddleware(next http.Handler) http.Handler { urlPath = req.URL.Path } - var sanitizedPath string + // var sanitizedPath string if len(urlPath) > 1 { - sanitizedPath = path.Clean(urlPath) + sanitizedPath := make([]rune, 0, len(urlPath)) - if string(urlPath[len(urlPath)-1]) == "/" { - sanitizedPath += "/" + prevWasSlash := false + for _, chr := range urlPath { + if chr != '/' { + sanitizedPath = append(sanitizedPath, chr) + prevWasSlash = false + continue + } + if prevWasSlash { + continue + } + sanitizedPath = append(sanitizedPath, chr) + prevWasSlash = true } + // Always remove the leading slash. + modifiedPath := strings.TrimSuffix(string(sanitizedPath), "/") + if rctx == nil { - req.URL.Path = sanitizedPath + req.URL.Path = modifiedPath } else { - rctx.RoutePath = sanitizedPath + rctx.RoutePath = modifiedPath } } next.ServeHTTP(resp, req) diff --git a/routers/common/middleware_test.go b/routers/common/middleware_test.go index 189f105924cc7..3e4a846f4e4ed 100644 --- a/routers/common/middleware_test.go +++ b/routers/common/middleware_test.go @@ -42,7 +42,7 @@ func TestStripSlashesMiddleware(t *testing.T) { { name: "path with slashes in the end", inputPath: "/user2//repo1/", - expectedPath: "/user2/repo1/", + expectedPath: "/user2/repo1", }, { name: "path with slashes and query params", From 04b5a905c23d37819f2875c65039e97b202d73c9 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 2 Mar 2023 11:30:42 +0800 Subject: [PATCH 08/10] Apply suggestions from code review --- routers/common/middleware.go | 35 ++++++++++--------------------- routers/common/middleware_test.go | 3 +-- 2 files changed, 12 insertions(+), 26 deletions(-) diff --git a/routers/common/middleware.go b/routers/common/middleware.go index e91cfedbb58a4..ea0cabe8807f2 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -93,32 +93,19 @@ func stripSlashesMiddleware(next http.Handler) http.Handler { urlPath = req.URL.Path } - // var sanitizedPath string - if len(urlPath) > 1 { - sanitizedPath := make([]rune, 0, len(urlPath)) - - prevWasSlash := false - for _, chr := range urlPath { - if chr != '/' { - sanitizedPath = append(sanitizedPath, chr) - prevWasSlash = false - continue - } - if prevWasSlash { - continue - } - sanitizedPath = append(sanitizedPath, chr) - prevWasSlash = true + sanitizedPath := &strings.Builder{} + prevWasSlash := false + for _, chr := range strings.TrimRight(urlPath, "/") { + if chr != '/' || !prevWasSlash { + sanitizedPath.WriteRune(chr) } + prevWasSlash = chr == '/' + } - // Always remove the leading slash. - modifiedPath := strings.TrimSuffix(string(sanitizedPath), "/") - - if rctx == nil { - req.URL.Path = modifiedPath - } else { - rctx.RoutePath = modifiedPath - } + if rctx == nil { + req.URL.Path = sanitizedPath.String() + } else { + rctx.RoutePath = sanitizedPath.String() } next.ServeHTTP(resp, req) }) diff --git a/routers/common/middleware_test.go b/routers/common/middleware_test.go index 3e4a846f4e4ed..b55368a21cbd2 100644 --- a/routers/common/middleware_test.go +++ b/routers/common/middleware_test.go @@ -1,6 +1,5 @@ // Copyright 2022 The Gitea Authors. All rights reserved. -// Use of this source code is governed by a MIT-style -// license that can be found in the LICENSE file. +// SPDX-License-Identifier: MIT package common import ( From e8b5930260eadd6d651a1811d5b2a24cf0a91d3a Mon Sep 17 00:00:00 2001 From: Jason Song Date: Thu, 2 Mar 2023 18:07:16 +0800 Subject: [PATCH 09/10] Update routers/common/middleware.go --- routers/common/middleware.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/routers/common/middleware.go b/routers/common/middleware.go index ea0cabe8807f2..2abdcb583d8f0 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -89,6 +89,8 @@ func stripSlashesMiddleware(next http.Handler) http.Handler { rctx := chi.RouteContext(req.Context()) if rctx != nil && rctx.RoutePath != "" { urlPath = rctx.RoutePath + } else if req.URL.RawPath != "" { + urlPath = req.URL.RawPath } else { urlPath = req.URL.Path } From e9b6d09f4085ba9506a68efd15d84d6fd0176031 Mon Sep 17 00:00:00 2001 From: Jason Song Date: Fri, 3 Mar 2023 18:21:30 +0800 Subject: [PATCH 10/10] Update routers/common/middleware_test.go --- routers/common/middleware_test.go | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-) diff --git a/routers/common/middleware_test.go b/routers/common/middleware_test.go index b55368a21cbd2..f16b9374eca71 100644 --- a/routers/common/middleware_test.go +++ b/routers/common/middleware_test.go @@ -21,22 +21,22 @@ func TestStripSlashesMiddleware(t *testing.T) { { name: "path with multiple slashes", inputPath: "https://github.com///go-gitea//gitea.git", - expectedPath: "https://github.com/go-gitea/gitea.git", + expectedPath: "/go-gitea/gitea.git", }, { name: "path with no slashes", inputPath: "https://github.com/go-gitea/gitea.git", - expectedPath: "https://github.com/go-gitea/gitea.git", + expectedPath: "/go-gitea/gitea.git", }, { name: "path with slashes in the middle", inputPath: "https://git.data.coop//halfd/new-website.git", - expectedPath: "https://git.data.coop/halfd/new-website.git", + expectedPath: "/halfd/new-website.git", }, { name: "path with slashes in the middle", inputPath: "https://git.data.coop//halfd/new-website.git", - expectedPath: "https://git.data.coop/halfd/new-website.git", + expectedPath: "/halfd/new-website.git", }, { name: "path with slashes in the end", @@ -46,13 +46,18 @@ func TestStripSlashesMiddleware(t *testing.T) { { name: "path with slashes and query params", inputPath: "/repo//migrate?service_type=3", - expectedPath: "/repo/migrate?service_type=3", + expectedPath: "/repo/migrate", + }, + { + name: "path with encoded slash", + inputPath: "/user2/%2F%2Frepo1", + expectedPath: "/user2/%2F%2Frepo1", }, } for _, tt := range tests { testMiddleware := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - assert.Equal(t, tt.expectedPath, r.URL.String()) + assert.Equal(t, tt.expectedPath, r.URL.Path) }) // pass the test middleware to validate the changes