Skip to content

context.SaveUploadedFile: Only run Chmod if mode needs change #4482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
krombel wants to merge 1 commit into
base: master
Choose a base branch
from
Open

context.SaveUploadedFile: Only run Chmod if mode needs change #4482

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -720,7 +720,12 @@ func (c *Context) SaveUploadedFile(file *multipart.FileHeader, dst string, perm
if err = os.MkdirAll(dir, mode); err != nil {
return err
}
if err = os.Chmod(dir, mode); err != nil {
// Only run Chmod if mode is not as configured
info, err := os.Stat(dir)
if err == nil && info.Mode().Perm() != mode {
err = os.Chmod(dir, mode)
}
if err != nil {
return err
}
Comment on lines +723 to 730
Copy link

@anfaas1618 anfaas1618 Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Only run Chmod if mode is not as configured
info, err := os.Stat(dir)
if err == nil && info.Mode().Perm() != mode {
err = os.Chmod(dir, mode)
}
if err != nil {
return err
}
// Only run Chmod if mode is not as configured
if info, statErr := os.Stat(dir); statErr == nil && info.Mode().Perm() != mode {
if err := os.Chmod(dir, mode); err != nil {
return err
}
} else if statErr != nil {
return statErr
}

since this info is used in same block of error , can we scope it better,

Copy link
Author

@krombel krombel Dec 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way to not cause codecov/patch to fail...
I don't know how to see the details on codecov now but in this run it failed an my approach now did not cause this error
https://github.com/gin-gonic/gin/runs/59003196111
My original patch was 9ea8851 which is quite similar to your approach. Codecov failed on 9ea8851#diff-552f47512a00afe5fc6850cc9ddc830a6daeca162750e50aab4ed549685e0253R731 so the return err decreased codecov => fail

Sadly I do not know how to increase codecoverage in this code area through testing so either I accept your change and a failing codecov/patch check is acceptable or this approach is...

Copy link

@anfaas1618 anfaas1618 Jan 3, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did it this way to not cause codecov/patch to fail... I don't know how to see the details on codecov now but in this run it failed an my approach now did not cause this error https://github.com/gin-gonic/gin/runs/59003196111 My original patch was 9ea8851 which is quite similar to your approach. Codecov failed on 9ea8851#diff-552f47512a00afe5fc6850cc9ddc830a6daeca162750e50aab4ed549685e0253R731 so the return err decreased codecov => fail

Sadly I do not know how to increase codecoverage in this code area through testing so either I accept your change and a failing codecov/patch check is acceptable or this approach is...

func TestSaveUploadedFileWithModeDifferent(t *testing.T) {
	buf := new(bytes.Buffer)
	mw := multipart.NewWriter(buf)
	w, _ := mw.CreateFormFile("file", "test")
	w.Write([]byte("test"))
	mw.Close()
	c, _ := CreateTestContext(httptest.NewRecorder())
	c.Request, _ = http.NewRequest(http.MethodPost, "/", buf)
	c.Request.Header.Set("Content-Type", mw.FormDataContentType())
	f, _ := c.FormFile("file")

	testFile := filepath.Join("test_dir2", "test")
	t.Cleanup(func() {
		os.Remove(testFile)
		os.Remove("test_dir2")
	})

	os.MkdirAll("test_dir2", 0o700)
	assert.NoError(t, c.SaveUploadedFile(f, testFile, 0o755))
}

can you try this test in context_test , also test against 750 if possible, and check coverage

Copy link
Author

@krombel krombel Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so far so good. My issue was, that I was not able to trigger the "return err" path so intentionally causing the case, that an error is thrown, which would cause that code path to be triggered and therefore coverage to not decrease

Copy link

@anfaas1618 anfaas1618 Jan 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you add the test cases for it and clear out the coverage part, the diff is behind ⚠️ Report is 229 commits behind head on master.

but will need to add test for changes done .


Expand Down