From cf157ab3600e53d51f3ab67259b6a429e912f509 Mon Sep 17 00:00:00 2001 From: Gusted Date: Sun, 9 Feb 2025 11:50:43 +0100 Subject: [PATCH] fix: always set stripped slashes on http request - The middleware that takes care of normalizing '//user2/////repo1' to `/user2/repo1` would only set the normalized value to the Chi (Forgejo's http router) `RoutePath` field, so Chi would correctly do the routing. However not all components in Forgejo (like Forgejo's `context` module) rely on Chi to get this updated path and some still rely on the value of `(http.Request).URL.Path`, so always set the normalized value to the http request. - Adjusted unit test. - Resolves forgejo/forgejo#6822 - The related issue was caused by https://codeberg.org/forgejo/forgejo/src/commit/751a3da979db06adc99cc64fac6aad9505fd5a96/services/context/context.go#L115 using the value of the http request on not that was set in the Chi context. --- routers/common/middleware.go | 7 +++---- routers/common/middleware_test.go | 25 ++++++++++++++++++++----- services/context/repo.go | 2 +- 3 files changed, 24 insertions(+), 10 deletions(-) diff --git a/routers/common/middleware.go b/routers/common/middleware.go index ebc4d62d03..8d136ef2c7 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -95,10 +95,9 @@ func stripSlashesMiddleware(next http.Handler) http.Handler { prevWasSlash = chr == '/' } - if rctx == nil { - req.URL.Path = sanitizedPath.String() - } else { - rctx.RoutePath = sanitizedPath.String() + req.URL.Path = sanitizedPath.String() + if rctx != nil { + rctx.RoutePath = req.URL.Path } next.ServeHTTP(resp, req) }) diff --git a/routers/common/middleware_test.go b/routers/common/middleware_test.go index f16b9374ec..0e111e1261 100644 --- a/routers/common/middleware_test.go +++ b/routers/common/middleware_test.go @@ -7,6 +7,9 @@ import ( "net/http/httptest" "testing" + "code.gitea.io/gitea/modules/web" + + chi "github.com/go-chi/chi/v5" "github.com/stretchr/testify/assert" ) @@ -43,6 +46,11 @@ func TestStripSlashesMiddleware(t *testing.T) { inputPath: "/user2//repo1/", expectedPath: "/user2/repo1", }, + { + name: "path with slashes in the beginning", + inputPath: "https://codeberg.org//user2/repo1/", + expectedPath: "/user2/repo1", + }, { name: "path with slashes and query params", inputPath: "/repo//migrate?service_type=3", @@ -56,15 +64,22 @@ func TestStripSlashesMiddleware(t *testing.T) { } for _, tt := range tests { - testMiddleware := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r := web.NewRoute() + r.Use(stripSlashesMiddleware) + + called := false + r.Get("*", func(w http.ResponseWriter, r *http.Request) { assert.Equal(t, tt.expectedPath, r.URL.Path) + + rctx := chi.RouteContext(r.Context()) + assert.Equal(t, tt.expectedPath, rctx.RoutePath) + + called = true }) - // 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) + r.ServeHTTP(httptest.NewRecorder(), req) + assert.True(t, called) } } diff --git a/services/context/repo.go b/services/context/repo.go index ff03844c03..8b8359fb70 100644 --- a/services/context/repo.go +++ b/services/context/repo.go @@ -1058,7 +1058,7 @@ func RepoRefByType(refType RepoRefType, ignoreNotExistErr ...bool) func(*Context if refType == RepoRefLegacy { // redirect from old URL scheme to new URL scheme - prefix := strings.TrimPrefix(setting.AppSubURL+strings.ToLower(strings.TrimSuffix(ctx.Req.URL.Path, ctx.Params("*"))), strings.ToLower(ctx.Repo.RepoLink)) + prefix := strings.TrimPrefix(setting.AppSubURL+strings.ToLower(strings.TrimSuffix(ctx.Req.URL.Path, ctx.PathParamRaw("*"))), strings.ToLower(ctx.Repo.RepoLink)) ctx.Redirect(path.Join( ctx.Repo.RepoLink,