diff --git a/context.go b/context.go index 5174033e..8014d4bc 100644 --- a/context.go +++ b/context.go @@ -728,11 +728,16 @@ func (c *Context) SaveUploadedFile(file *multipart.FileHeader, dst string, perm mode = perm[0] } dir := filepath.Dir(dst) + _, 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 chmod newly created directories to avoid "operation not permitted" + // errors on pre-existing directories we may not own (e.g., /tmp). + if os.IsNotExist(statErr) { + 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 ef60379d..2a3ce70f 100644 --- a/context_test.go +++ b/context_test.go @@ -274,6 +274,28 @@ func TestSaveUploadedFileWithPermissionFailed(t *testing.T) { require.Error(t, c.SaveUploadedFile(f, "test/permission_test", mode)) } +func TestSaveUploadedFileToExistingDir(t *testing.T) { + buf := new(bytes.Buffer) + mw := multipart.NewWriter(buf) + w, err := mw.CreateFormFile("file", "test") + require.NoError(t, err) + _, err = w.Write([]byte("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) + + // Save to a pre-existing directory. Previously this would fail with + // "chmod : operation not permitted" when the directory was not + // owned by the current process (e.g., /tmp). + dir := t.TempDir() + dst := filepath.Join(dir, "uploaded.txt") + require.NoError(t, c.SaveUploadedFile(f, dst)) +} + func TestContextReset(t *testing.T) { router := New() c := router.allocateContext(0) diff --git a/response_writer.go b/response_writer.go index 9035e6f1..305322b6 100644 --- a/response_writer.go +++ b/response_writer.go @@ -17,7 +17,10 @@ const ( defaultStatus = http.StatusOK ) -var errHijackAlreadyWritten = errors.New("gin: response body already written") +var ( + errHijackAlreadyWritten = errors.New("gin: response body already written") + errHijackNotSupported = errors.New("gin: underlying ResponseWriter does not implement http.Hijacker") +) // ResponseWriter ... type ResponseWriter interface { @@ -117,12 +120,21 @@ func (w *responseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { if w.size < 0 { w.size = 0 } - return w.ResponseWriter.(http.Hijacker).Hijack() + if hijacker, ok := w.ResponseWriter.(http.Hijacker); ok { + return hijacker.Hijack() + } + return nil, nil, errHijackNotSupported } // CloseNotify implements the http.CloseNotifier interface. +// +// Deprecated: the CloseNotifier interface predates Go's context package. +// New code should use Request.Context instead. func (w *responseWriter) CloseNotify() <-chan bool { - return w.ResponseWriter.(http.CloseNotifier).CloseNotify() + if cn, ok := w.ResponseWriter.(http.CloseNotifier); ok { + return cn.CloseNotify() + } + return nil } // Flush implements the http.Flusher interface. diff --git a/response_writer_test.go b/response_writer_test.go index dfc1d2c6..02dbe4dd 100644 --- a/response_writer_test.go +++ b/response_writer_test.go @@ -113,15 +113,12 @@ func TestResponseWriterHijack(t *testing.T) { writer.reset(testWriter) w := ResponseWriter(writer) - assert.Panics(t, func() { - _, _, err := w.Hijack() - require.NoError(t, err) - }) + _, _, err := w.Hijack() + require.ErrorIs(t, err, errHijackNotSupported) assert.True(t, w.Written()) - assert.Panics(t, func() { - w.CloseNotify() - }) + ch := w.CloseNotify() + assert.Nil(t, ch) w.Flush() } @@ -315,3 +312,43 @@ func TestPusherWithoutPusher(t *testing.T) { pusher := w.Pusher() assert.Nil(t, pusher, "Expected pusher to be nil") } + +// mockCloseNotifier is an http.ResponseWriter that implements http.CloseNotifier. +type mockCloseNotifier struct { + *httptest.ResponseRecorder +} + +func (m *mockCloseNotifier) CloseNotify() <-chan bool { + return make(chan bool) +} + +func TestCloseNotifyWithCloseNotifier(t *testing.T) { + rw := &mockCloseNotifier{ResponseRecorder: httptest.NewRecorder()} + w := &responseWriter{} + w.reset(rw) + + ch := w.CloseNotify() + assert.NotNil(t, ch, "Expected CloseNotify channel to be non-nil") +} + +func TestCloseNotifyWithoutCloseNotifier(t *testing.T) { + // httptest.NewRecorder does not implement http.CloseNotifier + rw := httptest.NewRecorder() + w := &responseWriter{} + w.reset(rw) + + ch := w.CloseNotify() + assert.Nil(t, ch, "Expected CloseNotify channel to be nil when underlying writer does not support it") +} + +func TestHijackWithoutHijacker(t *testing.T) { + // httptest.NewRecorder does not implement http.Hijacker + rw := httptest.NewRecorder() + w := &responseWriter{} + w.reset(rw) + + conn, buf, err := w.Hijack() + assert.Nil(t, conn) + assert.Nil(t, buf) + require.ErrorIs(t, err, errHijackNotSupported) +}