mirror of
https://github.com/gin-gonic/gin.git
synced 2026-06-23 18:13:47 +08:00
fix(context): skip chmod on pre-existing dirs in SaveUploadedFile (#4702)
* 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. * test: make SaveUploadedFile #4622 regression test platform-independent The regression test relied on os.Chmod failing on a pre-existing directory, which only happens for a non-owner, non-root process. When tests run as root (common in containers/CI) or with a user-owned $TMPDIR, the buggy chmod succeeds, so the test passed even against the unfixed code. Assert the actual contract instead: a pre-existing directory's permissions are left unchanged. This catches the regression deterministically on every platform. Also tighten the SaveUploadedFile doc/comments: the requested perm is enforced only on the newly created destination directory, not on every directory in the path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Bo-Yi Wu <appleboy.tw@gmail.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
da1e108614
commit
d9307dbcbb
17
context.go
17
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)
|
||||
|
||||
@ -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)
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user