diff --git a/modules/setting/server.go b/modules/setting/server.go index cedca32da9..a865e942a6 100644 --- a/modules/setting/server.go +++ b/modules/setting/server.go @@ -370,6 +370,10 @@ func loadServerFrom(rootCfg ConfigProvider) { } } + // TODO: GOLANG-HTTP-TMPDIR: Some Golang packages (like "http") use os.TempDir() to create temporary files when uploading files. + // So ideally we should set the TMPDIR environment variable to make them use our managed temp directory. + // But there is no clear place to set it currently, for example: when running "install" page, the AppDataPath is not ready yet, then AppDataTempDir won't work + EnableGzip = sec.Key("ENABLE_GZIP").MustBool() EnablePprof = sec.Key("ENABLE_PPROF").MustBool(false) PprofDataPath = sec.Key("PPROF_DATA_PATH").MustString(filepath.Join(AppWorkPath, "data/tmp/pprof")) diff --git a/modules/web/routemock.go b/modules/web/routemock.go index e85b0db738..68d19475e9 100644 --- a/modules/web/routemock.go +++ b/modules/web/routemock.go @@ -46,11 +46,15 @@ func RouterMockPoint(pointName string) func(next http.Handler) http.Handler { // // Then the mock function will be executed as a middleware at the mock point. // It only takes effect in testing mode (setting.IsInTesting == true). -func RouteMock(pointName string, h any) { +func RouteMock(pointName string, h any) func() { if _, ok := routeMockPoints[pointName]; !ok { panic("route mock point not found: " + pointName) } + old := routeMockPoints[pointName] routeMockPoints[pointName] = toHandlerProvider(h) + return func() { + routeMockPoints[pointName] = old + } } // RouteMockReset resets all mock points (no mock anymore) diff --git a/modules/web/router.go b/modules/web/router.go index 5812ff69d4..5374f82a23 100644 --- a/modules/web/router.go +++ b/modules/web/router.go @@ -55,7 +55,7 @@ func NewRouter() *Router { // Use supports two middlewares func (r *Router) Use(middlewares ...any) { for _, m := range middlewares { - if m != nil { + if !isNilOrFuncNil(m) { r.chiRouter.Use(toHandlerProvider(m)) } } diff --git a/routers/common/middleware.go b/routers/common/middleware.go index bfa258b976..6bf430d361 100644 --- a/routers/common/middleware.go +++ b/routers/common/middleware.go @@ -72,8 +72,13 @@ func RequestContextHandler() func(h http.Handler) http.Handler { req = req.WithContext(cache.WithCacheContext(ctx)) ds.SetContextValue(httplib.RequestContextKey, req) ds.AddCleanUp(func() { - if req.MultipartForm != nil { - _ = req.MultipartForm.RemoveAll() // remove the temp files buffered to tmp directory + // TODO: GOLANG-HTTP-TMPDIR: Golang saves the uploaded files to temp directory (TMPDIR) when parsing multipart-form. + // The "req" might have changed due to the new "req.WithContext" calls + // For example: in NewBaseContext, a new "req" with context is created, and the multipart-form is parsed there. + // So we always use the latest "req" from the data store. + ctxReq := ds.GetContextValue(httplib.RequestContextKey).(*http.Request) + if ctxReq.MultipartForm != nil { + _ = ctxReq.MultipartForm.RemoveAll() // remove the temp files buffered to tmp directory } }) next.ServeHTTP(respWriter, req) diff --git a/routers/web/web.go b/routers/web/web.go index 89a570dce0..6890789321 100644 --- a/routers/web/web.go +++ b/routers/web/web.go @@ -227,6 +227,8 @@ func ctxDataSet(args ...any) func(ctx *context.Context) { } } +const RouterMockPointBeforeWebRoutes = "before-web-routes" + // Routes returns all web routes func Routes() *web.Router { routes := web.NewRouter() @@ -285,7 +287,7 @@ func Routes() *web.Router { webRoutes := web.NewRouter() webRoutes.Use(mid...) - webRoutes.Group("", func() { registerWebRoutes(webRoutes) }, common.BlockExpensive(), common.QoS()) + webRoutes.Group("", func() { registerWebRoutes(webRoutes) }, common.BlockExpensive(), common.QoS(), web.RouterMockPoint(RouterMockPointBeforeWebRoutes)) routes.Mount("", webRoutes) return routes } diff --git a/services/context/base.go b/services/context/base.go index de839ede81..8bd66bed09 100644 --- a/services/context/base.go +++ b/services/context/base.go @@ -43,8 +43,10 @@ type Base struct { Locale translation.Locale } +var ParseMultipartFormMaxMemory = int64(32 << 20) + func (b *Base) ParseMultipartForm() bool { - err := b.Req.ParseMultipartForm(32 << 20) + err := b.Req.ParseMultipartForm(ParseMultipartFormMaxMemory) if err != nil { // TODO: all errors caused by client side should be ignored (connection closed). if !errors.Is(err, io.EOF) && !errors.Is(err, io.ErrUnexpectedEOF) { diff --git a/tests/integration/attachment_test.go b/tests/integration/attachment_test.go index 44aaee09f8..18efde7214 100644 --- a/tests/integration/attachment_test.go +++ b/tests/integration/attachment_test.go @@ -7,17 +7,23 @@ import ( "bytes" "image" "image/png" + "io/fs" "mime/multipart" "net/http" + "os" "strings" "testing" repo_model "code.gitea.io/gitea/models/repo" "code.gitea.io/gitea/modules/storage" "code.gitea.io/gitea/modules/test" + "code.gitea.io/gitea/modules/web" + route_web "code.gitea.io/gitea/routers/web" + "code.gitea.io/gitea/services/context" "code.gitea.io/gitea/tests" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func testGeneratePngBytes() []byte { @@ -52,14 +58,38 @@ func testCreateIssueAttachment(t *testing.T, session *TestSession, csrf, repoURL return obj["uuid"] } -func TestCreateAnonymousAttachment(t *testing.T) { +func TestAttachments(t *testing.T) { defer tests.PrepareTestEnv(t)() + t.Run("CreateAnonymousAttachment", testCreateAnonymousAttachment) + t.Run("CreateUser2IssueAttachment", testCreateUser2IssueAttachment) + t.Run("UploadAttachmentDeleteTemp", testUploadAttachmentDeleteTemp) + t.Run("GetAttachment", testGetAttachment) +} + +func testUploadAttachmentDeleteTemp(t *testing.T) { + session := loginUser(t, "user2") + countTmpFile := func() int { + // TODO: GOLANG-HTTP-TMPDIR: Golang saves the uploaded file to os.TempDir() when it exceeds the max memory limit. + files, err := fs.Glob(os.DirFS(os.TempDir()), "multipart-*") //nolint:usetesting // Golang's "http" package's behavior + require.NoError(t, err) + return len(files) + } + var tmpFileCountDuringUpload int + defer test.MockVariableValue(&context.ParseMultipartFormMaxMemory, 1)() + defer web.RouteMock(route_web.RouterMockPointBeforeWebRoutes, func(resp http.ResponseWriter, req *http.Request) { + tmpFileCountDuringUpload = countTmpFile() + })() + _ = testCreateIssueAttachment(t, session, GetUserCSRFToken(t, session), "user2/repo1", "image.png", testGeneratePngBytes(), http.StatusOK) + assert.Equal(t, 1, tmpFileCountDuringUpload, "the temp file should exist when uploaded size exceeds the parse form's max memory") + assert.Equal(t, 0, countTmpFile(), "the temp file should be deleted after upload") +} + +func testCreateAnonymousAttachment(t *testing.T) { session := emptyTestSession(t) testCreateIssueAttachment(t, session, GetAnonymousCSRFToken(t, session), "user2/repo1", "image.png", testGeneratePngBytes(), http.StatusSeeOther) } -func TestCreateIssueAttachment(t *testing.T) { - defer tests.PrepareTestEnv(t)() +func testCreateUser2IssueAttachment(t *testing.T) { const repoURL = "user2/repo1" session := loginUser(t, "user2") uuid := testCreateIssueAttachment(t, session, GetUserCSRFToken(t, session), repoURL, "image.png", testGeneratePngBytes(), http.StatusOK) @@ -90,8 +120,7 @@ func TestCreateIssueAttachment(t *testing.T) { MakeRequest(t, req, http.StatusOK) } -func TestGetAttachment(t *testing.T) { - defer tests.PrepareTestEnv(t)() +func testGetAttachment(t *testing.T) { adminSession := loginUser(t, "user1") user2Session := loginUser(t, "user2") user8Session := loginUser(t, "user8")