Extra slash in URL makes git clone fail #9262

@silverwind commented on GitHub (Jul 23, 2022):

This does work on GitHub, so I guess we can also allow one extra / at start of the path.

@silverwind commented on GitHub (Jul 23, 2022): This does work on GitHub, so I guess we can also allow one extra `/` at start of the path.

GiteaMirror commented

@zeripath commented on GitHub (Jul 23, 2022):

What's the reason behind wanting to do this?

@zeripath commented on GitHub (Jul 23, 2022): What's the reason behind wanting to do this?

GiteaMirror commented

@silverwind commented on GitHub (Jul 23, 2022):

To follow the robustness principle in this case. Extra slashes are the result of common misconfigurations but the intend of such requests is still clear enough imho that we can accept it.

@silverwind commented on GitHub (Jul 23, 2022): To follow the [robustness principle](https://en.wikipedia.org/wiki/Robustness_principle) in this case. Extra slashes are the result of common misconfigurations but the intend of such requests is still clear enough imho that we can accept it.

GiteaMirror commented

@reynir commented on GitHub (Jul 23, 2022):

Yes, the software I'm using uses a different git implementation, and something somewhere in the tall stack adds an extra slash in the beginning. I am trying to figure where this happens so I can either report it or fix it myself.

I found this behavior of gitea a bit surprising because often web servers normalize the paths so extra slashes don't break things. It can be nice when you're building a path in a script that you can be a bit sloppy and just put a slash between each path segment not worrying if either segment contains a slash.

@reynir commented on GitHub (Jul 23, 2022): Yes, the software I'm using uses a different git implementation, and something somewhere in the tall stack adds an extra slash in the beginning. I am trying to figure where this happens so I can either report it or fix it myself. I found this behavior of gitea a bit surprising because often web servers normalize the paths so extra slashes don't break things. It can be nice when you're building a path in a script that you can be a bit sloppy and just put a slash between each path segment not worrying if either segment contains a slash.

GiteaMirror commented

@silverwind commented on GitHub (Jul 23, 2022):

web servers normalize the paths so extra slashes don't break things

Yes, in fact, nginx has a default behaviour where it merges any // to /. I'm not saying we should do this everywhere like nginx does, but at the start of the path seems reasonable.

@silverwind commented on GitHub (Jul 23, 2022): > web servers normalize the paths so extra slashes don't break things Yes, in fact, nginx has a [default behaviour](http://nginx.org/en/docs/http/ngx_http_core_module.html#merge_slashes) where it merges any `//` to `/`. I'm not saying we should do this everywhere like nginx does, but at the start of the path seems reasonable.

GiteaMirror commented

@Gusted commented on GitHub (Jul 23, 2022):

This does work on GitHub, so I guess we can also allow one extra / at start of the path.

FYI, Github & Gitlab allows any arbitrary number of extra leading slashes, even better, it allows any number of slashes, for every valid slash position. Well I still consider this user fault, I personally won't put up a PR for this. But feel free to use this patch and create a PR for this:

This is just to strip leading/trailing slashes and still have working behavior:

diff --git a/routers/common/middleware.go b/routers/common/middleware.go
index 6ea1e1dfb..78e83abc6 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/v5"
 )
 
 // Middlewares returns common middlewares
@@ -48,7 +48,38 @@ func Middlewares() []func(http.Handler) http.Handler {
                handlers = append(handlers, proxy.ForwardedHeaders(opt))
        }
 
-       handlers = append(handlers, middleware.StripSlashes)
+       // Strip leading and trailing slashes.
+       handlers = append(handlers, func(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 {
+                               // Strip trailing slashes.
+                               if strings.HasSuffix(path, "/") {
+                                       path = strings.TrimRight(path, "/")
+                               }
+
+                               // Strip double leading slashes to a single slash.
+                               if strings.HasPrefix(path, "//") {
+                                       path = "/" + strings.TrimLeft(path, "/")
+                               }
+
+                               if rctx == nil {
+                                       req.URL.Path = path
+                               } else {
+                                       rctx.RoutePath = path
+                               }
+                       }
+                       next.ServeHTTP(resp, req)
+               })
+       })

However if someone wants to go ahead and argue that also considering every other slash position to allow for multiple slashes, feel free to use this patch and create a PR:

diff --git a/routers/common/middleware.go b/routers/common/middleware.go
index 6ea1e1dfb..64aa3603f 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/v5"
 )
 
 // Middlewares returns common middlewares
@@ -48,7 +48,48 @@ func Middlewares() []func(http.Handler) http.Handler {
                handlers = append(handlers, proxy.ForwardedHeaders(opt))
        }
 
-       handlers = append(handlers, middleware.StripSlashes)
+       // Strip leading and trailing slashes.
+       handlers = append(handlers, func(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 {
+                               newPath := make([]rune, 0, len(path))
+
+                               prevWasSlash := false
+                               // Loop trough all characters and de-duplicate any multiple slashes to a single slash.
+                               for _, chr := range path {
+                                       if chr != '/' {
+                                               newPath = append(newPath, chr)
+                                               prevWasSlash = false
+                                               continue
+                                       }
+                                       if prevWasSlash {
+                                               continue
+                                       }
+                                       newPath = append(newPath, chr)
+                                       prevWasSlash = true
+                               }
+
+                               // Always remove the leading slash.
+                               modifiedPath := strings.TrimSuffix(string(newPath), "/")
+
+                               if rctx == nil {
+                                       req.URL.Path = modifiedPath
+                               } else {
+                                       rctx.RoutePath = modifiedPath
+                               }
+                       }
+                       next.ServeHTTP(resp, req)
+               })
+       })
 
        if !setting.DisableRouterLog {
                handlers = append(handlers, routing.NewLoggerHandler())

@Gusted commented on GitHub (Jul 23, 2022): > This does work on GitHub, so I guess we can also allow one extra `/` at start of the path. FYI, Github & Gitlab allows any arbitrary number of extra leading slashes, even better, it allows any number of slashes, for every valid slash position. Well I still consider this user fault, I personally won't put up a PR for this. But feel free to use this patch and create a PR for this: This is just to strip leading/trailing slashes and still have working behavior: <details> ```diff diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 6ea1e1dfb..78e83abc6 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/v5" ) // Middlewares returns common middlewares @@ -48,7 +48,38 @@ func Middlewares() []func(http.Handler) http.Handler { handlers = append(handlers, proxy.ForwardedHeaders(opt)) } - handlers = append(handlers, middleware.StripSlashes) + // Strip leading and trailing slashes. + handlers = append(handlers, func(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 { + // Strip trailing slashes. + if strings.HasSuffix(path, "/") { + path = strings.TrimRight(path, "/") + } + + // Strip double leading slashes to a single slash. + if strings.HasPrefix(path, "//") { + path = "/" + strings.TrimLeft(path, "/") + } + + if rctx == nil { + req.URL.Path = path + } else { + rctx.RoutePath = path + } + } + next.ServeHTTP(resp, req) + }) + }) ``` </details> However if someone wants to go ahead and argue that also considering every other slash position to allow for multiple slashes, feel free to use this patch and create a PR: <details> ```diff diff --git a/routers/common/middleware.go b/routers/common/middleware.go index 6ea1e1dfb..64aa3603f 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/v5" ) // Middlewares returns common middlewares @@ -48,7 +48,48 @@ func Middlewares() []func(http.Handler) http.Handler { handlers = append(handlers, proxy.ForwardedHeaders(opt)) } - handlers = append(handlers, middleware.StripSlashes) + // Strip leading and trailing slashes. + handlers = append(handlers, func(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 { + newPath := make([]rune, 0, len(path)) + + prevWasSlash := false + // Loop trough all characters and de-duplicate any multiple slashes to a single slash. + for _, chr := range path { + if chr != '/' { + newPath = append(newPath, chr) + prevWasSlash = false + continue + } + if prevWasSlash { + continue + } + newPath = append(newPath, chr) + prevWasSlash = true + } + + // Always remove the leading slash. + modifiedPath := strings.TrimSuffix(string(newPath), "/") + + if rctx == nil { + req.URL.Path = modifiedPath + } else { + rctx.RoutePath = modifiedPath + } + } + next.ServeHTTP(resp, req) + }) + }) if !setting.DisableRouterLog { handlers = append(handlers, routing.NewLoggerHandler()) ``` </details>

GiteaMirror commented

@sandyydk commented on GitHub (Oct 3, 2022):

@Gusted Can I pick this up?

@sandyydk commented on GitHub (Oct 3, 2022): @Gusted Can I pick this up?

GiteaMirror commented

@Gusted commented on GitHub (Oct 3, 2022):

@sandyydk Feel free to use the patches I've provided. No need to ask.

@Gusted commented on GitHub (Oct 3, 2022): @sandyydk Feel free to use the patches I've provided. No need to ask.

GiteaMirror commented

@sandyydk commented on GitHub (Oct 3, 2022):

Sure thanks 😀

On Mon, Oct 3, 2022, 11:26 PM Gusted @.***> wrote:

@sandyydk https://github.com/sandyydk Feel free to use the patches I've
provided. No need to ask.

—
Reply to this email directly, view it on GitHub
https://github.com/go-gitea/gitea/issues/20462#issuecomment-1265823827,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/ABIORRYCQ5YBXSGC4KMKIRTWBMM6NANCNFSM54NZ5N5A
.
You are receiving this because you were mentioned.Message ID:
@.***>

@sandyydk commented on GitHub (Oct 3, 2022): Sure thanks 😀 On Mon, Oct 3, 2022, 11:26 PM Gusted ***@***.***> wrote: > @sandyydk <https://github.com/sandyydk> Feel free to use the patches I've > provided. No need to ask. > > — > Reply to this email directly, view it on GitHub > <https://github.com/go-gitea/gitea/issues/20462#issuecomment-1265823827>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/ABIORRYCQ5YBXSGC4KMKIRTWBMM6NANCNFSM54NZ5N5A> > . > You are receiving this because you were mentioned.Message ID: > ***@***.***> >

GiteaMirror commented

@stuzer05 commented on GitHub (Feb 26, 2023):

@reynir this has been fixed, please close this issue

@stuzer05 commented on GitHub (Feb 26, 2023): @reynir this has been fixed, please close this issue

GiteaMirror commented

@reynir commented on GitHub (Feb 26, 2023):

Where was this fixed?

@reynir commented on GitHub (Feb 26, 2023): Where was this fixed?

GiteaMirror commented

@stuzer05 commented on GitHub (Feb 26, 2023):

Where was this fixed?

Cannot point that out exactly, but on main branch that works:

stuzer05@stuzer05:~/Downloads$ git clone http://localhost:3000/stuzer05/test_rights.git/
Cloning into 'test_rights'...
remote: Enumerating objects: 9, done.
remote: Counting objects: 100% (9/9), done.
remote: Compressing objects: 100% (4/4), done.
remote: Total 9 (delta 0), reused 0 (delta 0), pack-reused 0
Receiving objects: 100% (9/9), done.

@stuzer05 commented on GitHub (Feb 26, 2023): > Where was this fixed? Cannot point that out exactly, but on main branch that works: ```bash stuzer05@stuzer05:~/Downloads$ git clone http://localhost:3000/stuzer05/test_rights.git/ Cloning into 'test_rights'... remote: Enumerating objects: 9, done. remote: Counting objects: 100% (9/9), done. remote: Compressing objects: 100% (4/4), done. remote: Total 9 (delta 0), reused 0 (delta 0), pack-reused 0 Receiving objects: 100% (9/9), done. ```

GiteaMirror commented