diff --git a/context.go b/context.go index 640e20b2..893edb28 100644 --- a/context.go +++ b/context.go @@ -726,6 +726,11 @@ func (c *Context) MultipartForm() (*multipart.Form, error) { } // SaveUploadedFile uploads the form file to specific dst. +// An optional perm argument specifies the permission bits used when creating +// the destination directory. If not provided, the default is 0750. The exact +// permission is enforced only on the destination directory and only when it is +// newly 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 { @@ -738,11 +743,19 @@ func (c *Context) SaveUploadedFile(file *multipart.FileHeader, dst string, perm mode = perm[0] } dir := filepath.Dir(dst) + // Record whether the destination directory exists before MkdirAll, so we + // only chmod a directory we just created. Chmod'ing a pre-existing directory + // the process does not own (e.g. /tmp) fails with "operation not permitted" + // (#4622). A non-ErrNotExist stat error also skips chmod and lets MkdirAll + // surface the underlying failure. + _, statErr := os.Stat(dir) if err = os.MkdirAll(dir, mode); err != nil { return err } - if err = os.Chmod(dir, mode); err != nil { - return err + 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 d93e470f..e8d305e4 100644 --- a/context_test.go +++ b/context_test.go @@ -274,6 +274,50 @@ func TestSaveUploadedFileWithPermissionFailed(t *testing.T) { require.Error(t, c.SaveUploadedFile(f, dst, 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), where chmod +// fails with "operation not permitted". This asserts the behavioral contract +// directly — a pre-existing directory's permissions are left unchanged — so it +// catches the regression on every platform, including environments (root/CI, +// user-owned $TMPDIR) where chmod on the temp dir would otherwise succeed. +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) + + // A pre-existing directory owned by this process, set to a known mode. + dir := t.TempDir() + require.NoError(t, os.Chmod(dir, 0o700)) + + // Pass a perm that differs from the directory's current mode. The fix must + // not apply it to the pre-existing directory; the old code chmod'd it + // unconditionally, which also failed outright on unowned dirs like /tmp. + dst := filepath.Join(dir, "existing_dir_test.txt") + require.NoError(t, c.SaveUploadedFile(f, dst, 0o755)) + + // The pre-existing directory's permissions must be unchanged. + info, err := os.Stat(dir) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0o700), info.Mode().Perm(), + "permissions of a pre-existing directory must not be modified") + + // The file must still be written with the 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)