Extra slash in URL makes git clone fail

[reynir]

Description

Cloning a repository with a double slash fails:

$ git clone https://git.data.coop//halfd/new-website.git
Cloning into 'new-website'...
remote: Not found.
fatal: repository 'https://git.data.coop//halfd/new-website.git/' not found

Gitea Version

1.16.8

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

Using the docker image.

Database

SQLite

[silverwind]

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

[zeripath]

What's the reason behind wanting to do this?

[silverwind]

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.

[reynir]

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.

[silverwind]

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.

[Gusted]

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())

[sandyydk]

@Gusted Can I pick this up?

[Gusted]

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

[sandyydk]

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 <#20462 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABIORRYCQ5YBXSGC4KMKIRTWBMM6NANCNFSM54NZ5N5A> . You are receiving this because you were mentioned.Message ID: ***@***.***>

[stuzer05]

@reynir this has been fixed, please close this issue

[reynir]

Where was this fixed?

[stuzer05]

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.

[reynir]

Thank you for testing. Could you try as well with a double leading slash in the path? I. e. git clone http://localhost:3000//stuzer05/test_rights.git as that's is what I originally reported.

[stuzer05]

Thank you for testing. Could you try as well with a double leading slash in the path? I. e. git clone http://localhost:3000//stuzer05/test_rights.git as that's is what I originally reported.

Currently doesn't work, made a pr

[wxiaoguang]

OK, as a bystander, I think there are some misunderstanding or miscommunication during this issue's history.

First, thank you all for making Gitea better.

The current situation is:

• This issue is not resolved yet
• There was already a PR trying to fix this problem: Support sanitising the URL by removing extra slashes in the URL #21333
• That PR already got some reviews, but there was no final conclusion yet.
• A new PR Double slash in URL makes git clone fail #23163

In my mind:

• It's better to focus in Support sanitising the URL by removing extra slashes in the URL #21333
• Call TOC to make a final decision: go or no-go, with detailed explanations
• ps: If it's agreed to fix the double-slash, I think all double-slashes could be fixed together like 21333 (not only just fixing this issue), to make "//org//repo//..." work, like GitHub. Actually, GitHub supports any amount of slashes: https://github.com/go-gitea/gitea////////////tree/main/////.github