Refactor "route" related code, fix Safari cookie bug (#24330)

Fix #24176

Clean some misuses of route package, clean some legacy FIXMEs

---------

Co-authored-by: Giteabot <[email protected]>

Author: wxiaoguang

modules/context/api.go

@@ -343,6 +343,7 @@ func RepoRefForAPI(next http.Handler) http.Handler {
 			}
 			ctx.Repo.Commit = commit
 			ctx.Repo.TreePath = ctx.Params("*")
+			next.ServeHTTP(w, req)
 			return
 		}
 

modules/context/context.go

@@ -446,13 +446,33 @@ func (ctx *Context) JSON(status int, content interface{}) {
 	}
 }
 
+func removeSessionCookieHeader(w http.ResponseWriter) {
+	cookies := w.Header()["Set-Cookie"]
+	w.Header().Del("Set-Cookie")
+	for _, cookie := range cookies {
+		if strings.HasPrefix(cookie, setting.SessionConfig.CookieName+"=") {
+			continue
+		}
+		w.Header().Add("Set-Cookie", cookie)
+	}
+}
+
 // Redirect redirects the request
 func (ctx *Context) Redirect(location string, status ...int) {
 	code := http.StatusSeeOther
 	if len(status) == 1 {
 		code = status[0]
 	}
 
+	if strings.Contains(location, "://") || strings.HasPrefix(location, "//") {
+		// Some browsers (Safari) have buggy behavior for Cookie + Cache + External Redirection, eg: /my-path => https://other/path
+		// 1. the first request to "/my-path" contains cookie
+		// 2. some time later, the request to "/my-path" doesn't contain cookie (caused by Prevent web tracking)
+		// 3. Gitea's Sessioner doesn't see the session cookie, so it generates a new session id, and returns it to browser
+		// 4. then the browser accepts the empty session, then the user is logged out
+		// So in this case, we should remove the session cookie from the response header
+		removeSessionCookieHeader(ctx.Resp)
+	}
 	http.Redirect(ctx.Resp, ctx.Req, location, code)
 }
 

modules/context/context_test.go

@@ -0,0 +1,40 @@
+// Copyright 2023 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package context
+
+import (
+	"net/http"
+	"testing"
+
+	"code.gitea.io/gitea/modules/setting"
+
+	"github.com/stretchr/testify/assert"
+)
+
+type mockResponseWriter struct {
+	header http.Header
+}
+
+func (m *mockResponseWriter) Header() http.Header {
+	return m.header
+}
+
+func (m *mockResponseWriter) Write(bytes []byte) (int, error) {
+	panic("implement me")
+}
+
+func (m *mockResponseWriter) WriteHeader(statusCode int) {
+	panic("implement me")
+}
+
+func TestRemoveSessionCookieHeader(t *testing.T) {
+	w := &mockResponseWriter{}
+	w.header = http.Header{}
+	w.header.Add("Set-Cookie", (&http.Cookie{Name: setting.SessionConfig.CookieName, Value: "foo"}).String())
+	w.header.Add("Set-Cookie", (&http.Cookie{Name: "other", Value: "bar"}).String())
+	assert.Len(t, w.Header().Values("Set-Cookie"), 2)
+	removeSessionCookieHeader(w)
+	assert.Len(t, w.Header().Values("Set-Cookie"), 1)
+	assert.Contains(t, "other=bar", w.Header().Get("Set-Cookie"))
+}

modules/web/handler.go

@@ -8,7 +8,6 @@ import (
 	"fmt"
 	"net/http"
 	"reflect"
-	"strings"
 
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/web/routing"
@@ -131,16 +130,22 @@ func hasResponseBeenWritten(argsIn []reflect.Value) bool {
 // toHandlerProvider converts a handler to a handler provider
 // A handler provider is a function that takes a "next" http.Handler, it can be used as a middleware
 func toHandlerProvider(handler any) func(next http.Handler) http.Handler {
-	if hp, ok := handler.(func(next http.Handler) http.Handler); ok {
-		return hp
-	}
-
 	funcInfo := routing.GetFuncInfo(handler)
 	fn := reflect.ValueOf(handler)
 	if fn.Type().Kind() != reflect.Func {
 		panic(fmt.Sprintf("handler must be a function, but got %s", fn.Type()))
 	}
 
+	if hp, ok := handler.(func(next http.Handler) http.Handler); ok {
+		return func(next http.Handler) http.Handler {
+			h := hp(next) // this handle could be dynamically generated, so we can't use it for debug info
+			return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
+				routing.UpdateFuncInfo(req.Context(), funcInfo)
+				h.ServeHTTP(resp, req)
+			})
+		}
+	}
+
 	provider := func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(respOrig http.ResponseWriter, req *http.Request) {
 			// wrap the response writer to check whether the response has been written
@@ -175,26 +180,3 @@ func toHandlerProvider(handler any) func(next http.Handler) http.Handler {
 	provider(nil).ServeHTTP(nil, nil) // do a pre-check to make sure all arguments and return values are supported
 	return provider
 }
-
-// MiddlewareWithPrefix wraps a handler function at a prefix, and make it as a middleware
-// TODO: this design is incorrect, the asset handler should not be a middleware
-func MiddlewareWithPrefix(pathPrefix string, middleware func(handler http.Handler) http.Handler, handlerFunc http.HandlerFunc) func(next http.Handler) http.Handler {
-	funcInfo := routing.GetFuncInfo(handlerFunc)
-	handler := http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
-		routing.UpdateFuncInfo(req.Context(), funcInfo)
-		handlerFunc(resp, req)
-	})
-	return func(next http.Handler) http.Handler {
-		return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
-			if !strings.HasPrefix(req.URL.Path, pathPrefix) {
-				next.ServeHTTP(resp, req)
-				return
-			}
-			if middleware != nil {
-				middleware(handler).ServeHTTP(resp, req)
-			} else {
-				handler.ServeHTTP(resp, req)
-			}
-		})
-	}
-}

modules/web/route.go

@@ -44,23 +44,13 @@ type Route struct {
 // NewRoute creates a new route
 func NewRoute() *Route {
 	r := chi.NewRouter()
-	return &Route{
-		R:              r,
-		curGroupPrefix: "",
-		curMiddlewares: []interface{}{},
-	}
+	return &Route{R: r}
 }
 
 // Use supports two middlewares
 func (r *Route) Use(middlewares ...interface{}) {
-	if r.curGroupPrefix != "" {
-		// FIXME: this behavior is incorrect, should use "With" instead
-		r.curMiddlewares = append(r.curMiddlewares, middlewares...)
-	} else {
-		// FIXME: another misuse, the "Use" with empty middlewares is called after "Mount"
-		for _, m := range middlewares {
-			r.R.Use(toHandlerProvider(m))
-		}
+	for _, m := range middlewares {
+		r.R.Use(toHandlerProvider(m))
 	}
 }
 
@@ -116,9 +106,7 @@ func (r *Route) Methods(method, pattern string, h []any) {
 
 // Mount attaches another Route along ./pattern/*
 func (r *Route) Mount(pattern string, subR *Route) {
-	middlewares := make([]interface{}, len(r.curMiddlewares))
-	copy(middlewares, r.curMiddlewares)
-	subR.Use(middlewares...)
+	subR.Use(r.curMiddlewares...)
 	r.R.Mount(r.getPattern(pattern), subR.R)
 }
 

routers/common/middleware.go

@@ -15,24 +15,23 @@ import (
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/web/routing"
 
+	"gitea.com/go-chi/session"
 	"github.com/chi-middleware/proxy"
 	chi "github.com/go-chi/chi/v5"
 )
 
-// Middlewares returns common middlewares
-func Middlewares() []func(http.Handler) http.Handler {
-	handlers := []func(http.Handler) http.Handler{
-		func(next http.Handler) http.Handler {
-			return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
-				// First of all escape the URL RawPath to ensure that all routing is done using a correctly escaped URL
-				req.URL.RawPath = req.URL.EscapedPath()
+// ProtocolMiddlewares returns HTTP protocol related middlewares
+func ProtocolMiddlewares() (handlers []any) {
+	handlers = append(handlers, func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(resp http.ResponseWriter, req *http.Request) {
+			// First of all escape the URL RawPath to ensure that all routing is done using a correctly escaped URL
+			req.URL.RawPath = req.URL.EscapedPath()
 
-				ctx, _, finished := process.GetManager().AddTypedContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI), process.RequestProcessType, true)
-				defer finished()
-				next.ServeHTTP(context.NewResponse(resp), req.WithContext(cache.WithCacheContext(ctx)))
-			})
-		},
-	}
+			ctx, _, finished := process.GetManager().AddTypedContext(req.Context(), fmt.Sprintf("%s: %s", req.Method, req.RequestURI), process.RequestProcessType, true)
+			defer finished()
+			next.ServeHTTP(context.NewResponse(resp), req.WithContext(cache.WithCacheContext(ctx)))
+		})
+	})
 
 	if setting.ReverseProxyLimit > 0 {
 		opt := proxy.NewForwardedHeadersOptions().
@@ -112,3 +111,17 @@ func stripSlashesMiddleware(next http.Handler) http.Handler {
 		next.ServeHTTP(resp, req)
 	})
 }
+
+func Sessioner() func(next http.Handler) http.Handler {
+	return session.Sessioner(session.Options{
+		Provider:       setting.SessionConfig.Provider,
+		ProviderConfig: setting.SessionConfig.ProviderConfig,
+		CookieName:     setting.SessionConfig.CookieName,
+		CookiePath:     setting.SessionConfig.CookiePath,
+		Gclifetime:     setting.SessionConfig.Gclifetime,
+		Maxlifetime:    setting.SessionConfig.Maxlifetime,
+		Secure:         setting.SessionConfig.Secure,
+		SameSite:       setting.SessionConfig.SameSite,
+		Domain:         setting.SessionConfig.Domain,
+	})
+}

routers/init.go

@@ -177,20 +177,15 @@ func GlobalInitInstalled(ctx context.Context) {
 func NormalRoutes(ctx context.Context) *web.Route {
 	ctx, _ = templates.HTMLRenderer(ctx)
 	r := web.NewRoute()
-	for _, middle := range common.Middlewares() {
-		r.Use(middle)
-	}
+	r.Use(common.ProtocolMiddlewares()...)
 
 	r.Mount("/", web_routers.Routes(ctx))
 	r.Mount("/api/v1", apiv1.Routes(ctx))
 	r.Mount("/api/internal", private.Routes())
 
 	if setting.Packages.Enabled {
-		// Add endpoints to match common package manager APIs
-
 		// This implements package support for most package managers
 		r.Mount("/api/packages", packages_router.CommonRoutes(ctx))
-
 		// This implements the OCI API (Note this is not preceded by /api but is instead /v2)
 		r.Mount("/v2", packages_router.ContainerRoutes(ctx))
 	}

routers/install/routes.go

@@ -19,8 +19,6 @@ import (
 	"code.gitea.io/gitea/routers/common"
 	"code.gitea.io/gitea/routers/web/healthcheck"
 	"code.gitea.io/gitea/services/forms"
-
-	"gitea.com/go-chi/session"
 )
 
 type dataStore map[string]interface{}
@@ -30,7 +28,6 @@ func (d *dataStore) GetData() map[string]interface{} {
 }
 
 func installRecovery(ctx goctx.Context) func(next http.Handler) http.Handler {
-	_, rnd := templates.HTMLRenderer(ctx)
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {
 			defer func() {
@@ -69,6 +66,7 @@ func installRecovery(ctx goctx.Context) func(next http.Handler) http.Handler {
 					if !setting.IsProd {
 						store["ErrorMsg"] = combinedErr
 					}
+					_, rnd := templates.HTMLRenderer(ctx)
 					err = rnd.HTML(w, http.StatusInternalServerError, "status/500", templates.BaseVars().Merge(store))
 					if err != nil {
 						log.Error("%v", err)
@@ -83,34 +81,22 @@ func installRecovery(ctx goctx.Context) func(next http.Handler) http.Handler {
 
 // Routes registers the installation routes
 func Routes(ctx goctx.Context) *web.Route {
-	r := web.NewRoute()
-	for _, middle := range common.Middlewares() {
-		r.Use(middle)
-	}
-
-	r.Use(web.MiddlewareWithPrefix("/assets/", nil, public.AssetsHandlerFunc("/assets/")))
-
-	r.Use(session.Sessioner(session.Options{
-		Provider:       setting.SessionConfig.Provider,
-		ProviderConfig: setting.SessionConfig.ProviderConfig,
-		CookieName:     setting.SessionConfig.CookieName,
-		CookiePath:     setting.SessionConfig.CookiePath,
-		Gclifetime:     setting.SessionConfig.Gclifetime,
-		Maxlifetime:    setting.SessionConfig.Maxlifetime,
-		Secure:         setting.SessionConfig.Secure,
-		SameSite:       setting.SessionConfig.SameSite,
-		Domain:         setting.SessionConfig.Domain,
-	}))
+	base := web.NewRoute()
+	base.Use(common.ProtocolMiddlewares()...)
+	base.RouteMethods("/assets/*", "GET, HEAD", public.AssetsHandlerFunc("/assets/"))
 
+	r := web.NewRoute()
+	r.Use(common.Sessioner())
 	r.Use(installRecovery(ctx))
 	r.Use(Init(ctx))
 	r.Get("/", Install) // it must be on the root, because the "install.js" use the window.location to replace the "localhost" AppURL
 	r.Post("/", web.Bind(forms.InstallForm{}), SubmitInstall)
 	r.Get("/post-install", InstallDone)
 	r.Get("/api/healthz", healthcheck.Check)
-
 	r.NotFound(installNotFound)
-	return r
+
+	base.Mount("", r)
+	return base
 }
 
 func installNotFound(w http.ResponseWriter, req *http.Request) {

routers/install/routes_test.go

@@ -11,11 +11,14 @@ import (
 )
 
 func TestRoutes(t *testing.T) {
+	// TODO: this test seems not really testing the handlers
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
-	routes := Routes(ctx)
-	assert.NotNil(t, routes)
-	assert.EqualValues(t, "/", routes.R.Routes()[0].Pattern)
-	assert.Nil(t, routes.R.Routes()[0].SubRoutes)
-	assert.Len(t, routes.R.Routes()[0].Handlers, 2)
+	base := Routes(ctx)
+	assert.NotNil(t, base)
+	r := base.R.Routes()[1]
+	routes := r.SubRoutes.Routes()[0]
+	assert.EqualValues(t, "/", routes.Pattern)
+	assert.Nil(t, routes.SubRoutes)
+	assert.Len(t, routes.Handlers, 2)
 }

routers/web/base.go

@@ -59,12 +59,7 @@ func storageHandler(storageSetting setting.Storage, prefix string, objStore stor
 					return
 				}
 
-				http.Redirect(
-					w,
-					req,
-					u.String(),
-					http.StatusTemporaryRedirect,
-				)
+				http.Redirect(w, req, u.String(), http.StatusTemporaryRedirect)
 			})
 		}
 
@@ -122,9 +117,9 @@ func (d *dataStore) GetData() map[string]interface{} {
 	return *d
 }
 
-// Recovery returns a middleware that recovers from any panics and writes a 500 and a log if so.
+// RecoveryWith500Page returns a middleware that recovers from any panics and writes a 500 and a log if so.
 // This error will be created with the gitea 500 page.
-func Recovery(ctx goctx.Context) func(next http.Handler) http.Handler {
+func RecoveryWith500Page(ctx goctx.Context) func(next http.Handler) http.Handler {
 	_, rnd := templates.HTMLRenderer(ctx)
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) {