From 2df018666f13470b75d6390dae0963ec715c6605 Mon Sep 17 00:00:00 2001 From: ArdyJunata Date: Thu, 11 Jun 2026 16:23:58 +0700 Subject: [PATCH] fix: skip chmod on pre-existing dirs in SaveUploadedFile Fixes #4622 Prior to this change, SaveUploadedFile unconditionally called os.Chmod(dir, mode) after os.MkdirAll, even when the target directory already existed. This caused 'operation not permitted' errors when saving files into system-owned directories like /tmp that the current process does not own. The fix uses os.Stat before MkdirAll to detect whether the directory already exists, and only calls os.Chmod when the directory was freshly created by MkdirAll. This restores the behaviour from v1.10.1 where only os.MkdirAll was used (which correctly skips permission changes on existing dirs) while preserving the custom permission feature added in v1.12.0. Regression test added: TestSaveUploadedFileToExistingDir saves a file into os.TempDir() (a pre-existing system directory) and asserts no error is returned. --- context.go | 17 +++++++++++++++-- context_test.go | 34 ++++++++++++++++++++++++++++++++++ 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/context.go b/context.go index a2e28e5b..2ea259bd 100644 --- a/context.go +++ b/context.go @@ -716,6 +716,10 @@ func (c *Context) MultipartForm() (*multipart.Form, error) { } // SaveUploadedFile uploads the form file to specific dst. +// An optional perm argument allows specifying the permission bits for any +// newly created directories. If not provided, the default is 0750. +// Note: permissions are only applied to directories created by this call; +// pre-existing directories (e.g. /tmp) are not modified. func (c *Context) SaveUploadedFile(file *multipart.FileHeader, dst string, perm ...fs.FileMode) error { src, err := file.Open() if err != nil { @@ -728,11 +732,20 @@ func (c *Context) SaveUploadedFile(file *multipart.FileHeader, dst string, perm mode = perm[0] } dir := filepath.Dir(dst) + // Check if the directory already exists before calling MkdirAll so we + // know whether to apply chmod. We must not chmod pre-existing directories + // (e.g. /tmp) because the process may not own them. + _, statErr := os.Stat(dir) if err = os.MkdirAll(dir, mode); err != nil { return err } - if err = os.Chmod(dir, mode); err != nil { - return err + // Only apply chmod when the directory was freshly created by MkdirAll. + // If the directory already existed, os.Stat returned nil, so we skip + // the chmod to avoid "operation not permitted" on unowned directories. + if errors.Is(statErr, os.ErrNotExist) { + if err = os.Chmod(dir, mode); err != nil { + return err + } } out, err := os.Create(dst) diff --git a/context_test.go b/context_test.go index 364a92ae..241a7f1e 100644 --- a/context_test.go +++ b/context_test.go @@ -275,6 +275,40 @@ func TestSaveUploadedFileWithPermissionFailed(t *testing.T) { require.Error(t, c.SaveUploadedFile(f, "test/permission_test", mode)) } +// TestSaveUploadedFileToExistingDir is a regression test for issue #4622. +// SaveUploadedFile must not call os.Chmod on a directory that already exists, +// because the process may not own it (e.g. /tmp on Linux/macOS). +func TestSaveUploadedFileToExistingDir(t *testing.T) { + buf := new(bytes.Buffer) + mw := multipart.NewWriter(buf) + w, err := mw.CreateFormFile("file", "existing_dir_test") + require.NoError(t, err) + _, err = w.Write([]byte("existing_dir_test")) + require.NoError(t, err) + mw.Close() + + c, _ := CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest(http.MethodPost, "/", buf) + c.Request.Header.Set("Content-Type", mw.FormDataContentType()) + f, err := c.FormFile("file") + require.NoError(t, err) + + // Use os.TempDir() which is guaranteed to already exist and may not be + // owned by the current process — this is exactly the scenario from #4622. + dst := filepath.Join(os.TempDir(), "gin_save_uploaded_regression_test.txt") + t.Cleanup(func() { + _ = os.Remove(dst) + }) + + // Must not fail with "chmod ...: operation not permitted" + require.NoError(t, c.SaveUploadedFile(f, dst)) + + // Verify the file was actually written with correct content + content, err := os.ReadFile(dst) + require.NoError(t, err) + assert.Equal(t, "existing_dir_test", string(content)) +} + func TestContextReset(t *testing.T) { router := New() c := router.allocateContext(0)