diff --git a/context.go b/context.go index a2e28e5b..893edb28 100644 --- a/context.go +++ b/context.go @@ -141,6 +141,16 @@ func (c *Context) Copy() *Context { cp.Params = make([]Param, len(cParams)) copy(cp.Params, cParams) + if c.Errors != nil { + cp.Errors = make(errorMsgs, len(c.Errors)) + copy(cp.Errors, c.Errors) + } + + if c.Accepted != nil { + cp.Accepted = make([]string, len(c.Accepted)) + copy(cp.Accepted, c.Accepted) + } + return &cp } @@ -716,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 { @@ -728,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 364a92ae..e8d305e4 100644 --- a/context_test.go +++ b/context_test.go @@ -248,13 +248,11 @@ func TestSaveUploadedFileWithPermission(t *testing.T) { require.NoError(t, err) assert.Equal(t, "permission_test", f.Filename) var mode fs.FileMode = 0o755 - require.NoError(t, c.SaveUploadedFile(f, "permission_test", mode)) - t.Cleanup(func() { - assert.NoError(t, os.Remove("permission_test")) - }) - info, err := os.Stat(filepath.Dir("permission_test")) + dst := filepath.Join(t.TempDir(), "subdir", "permission_test") + require.NoError(t, c.SaveUploadedFile(f, dst, mode)) + info, err := os.Stat(filepath.Dir(dst)) require.NoError(t, err) - assert.Equal(t, info.Mode().Perm(), mode) + assert.Equal(t, mode, info.Mode().Perm()) } func TestSaveUploadedFileWithPermissionFailed(t *testing.T) { @@ -272,7 +270,52 @@ func TestSaveUploadedFileWithPermissionFailed(t *testing.T) { require.NoError(t, err) assert.Equal(t, "permission_test", f.Filename) var mode fs.FileMode = 0o644 - require.Error(t, c.SaveUploadedFile(f, "test/permission_test", mode)) + dst := filepath.Join(t.TempDir(), "test", "permission_test") + 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) { @@ -689,6 +732,50 @@ func TestContextCopy(t *testing.T) { assert.Equal(t, cp.fullPath, c.fullPath) } +func TestContextCopyCopiesErrors(t *testing.T) { + c, _ := CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest(http.MethodGet, "/", nil) + _ = c.Error(errors.New("first error")) + _ = c.Error(errors.New("second error")) + + cp := c.Copy() + + // copied context has the same errors + assert.Len(t, cp.Errors, 2) + assert.Equal(t, c.Errors[0].Error(), cp.Errors[0].Error()) + assert.Equal(t, c.Errors[1].Error(), cp.Errors[1].Error()) + + // mutations on the copy do not affect the original + _ = cp.Error(errors.New("third error")) + assert.Len(t, c.Errors, 2) + assert.Len(t, cp.Errors, 3) +} + +func TestContextCopyCopiesAccepted(t *testing.T) { + c, _ := CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest(http.MethodGet, "/", nil) + c.SetAccepted("application/json", "text/html") + + cp := c.Copy() + + assert.Equal(t, c.Accepted, cp.Accepted) + + // mutations on the copy do not affect the original + cp.SetAccepted("text/plain") + assert.Equal(t, []string{"application/json", "text/html"}, c.Accepted) + assert.Equal(t, []string{"text/plain"}, cp.Accepted) +} + +func TestContextCopyNilErrorsAndAccepted(t *testing.T) { + c, _ := CreateTestContext(httptest.NewRecorder()) + c.Request, _ = http.NewRequest(http.MethodGet, "/", nil) + + cp := c.Copy() + + assert.Nil(t, cp.Errors) + assert.Nil(t, cp.Accepted) +} + func TestContextHandlerName(t *testing.T) { c, _ := CreateTestContext(httptest.NewRecorder()) c.handlers = HandlersChain{func(c *Context) {}, handlerNameTest} diff --git a/go.mod b/go.mod index 3f234252..4421641e 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( github.com/mattn/go-isatty v0.0.20 github.com/modern-go/reflect2 v1.0.2 github.com/pelletier/go-toml/v2 v2.2.4 - github.com/quic-go/quic-go v0.59.0 + github.com/quic-go/quic-go v0.60.0 github.com/stretchr/testify v1.11.1 github.com/ugorji/go/codec v1.3.1 go.mongodb.org/mongo-driver/v2 v2.5.0 diff --git a/go.sum b/go.sum index 3dec0af2..a83985ba 100644 --- a/go.sum +++ b/go.sum @@ -50,10 +50,12 @@ github.com/pelletier/go-toml/v2 v2.2.4 h1:mye9XuhQ6gvn5h28+VilKrrPoQVanw5PMw/TB0 github.com/pelletier/go-toml/v2 v2.2.4/go.mod h1:2gIqNv+qfxSVS7cM2xJQKtLSTLUE9V8t9Stt+h56mCY= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= +github.com/quic-go/go-ossfuzz-seeds v0.1.0 h1:APacT+iIaNF6fd8AGEiN3bT/Jtkd2jz4v4TzM7MFjy0= +github.com/quic-go/go-ossfuzz-seeds v0.1.0/go.mod h1:3IOHRbJIc+L6YKMwfDtJAM9Vj9k0YY4muhuyUYk5tbk= github.com/quic-go/qpack v0.6.0 h1:g7W+BMYynC1LbYLSqRt8PBg5Tgwxn214ZZR34VIOjz8= github.com/quic-go/qpack v0.6.0/go.mod h1:lUpLKChi8njB4ty2bFLX2x4gzDqXwUpaO1DP9qMDZII= -github.com/quic-go/quic-go v0.59.0 h1:OLJkp1Mlm/aS7dpKgTc6cnpynnD2Xg7C1pwL6vy/SAw= -github.com/quic-go/quic-go v0.59.0/go.mod h1:upnsH4Ju1YkqpLXC305eW3yDZ4NfnNbmQRCMWS58IKU= +github.com/quic-go/quic-go v0.60.0 h1:xcQioE8OM66UQLeUMHltK1CCcOu3JbVB4JAQdDQSB+0= +github.com/quic-go/quic-go v0.60.0/go.mod h1:wpKpjmPpftl30sL6pFh7REVpjbcCVy4zt2vDyK1TuJk= github.com/rogpeppe/go-internal v1.10.0 h1:TMyTOH3F/DB16zRVcYyreMH6GnZZrwQVAoYjRBZyWFQ= github.com/rogpeppe/go-internal v1.10.0/go.mod h1:UQnix2H7Ngw/k4C5ijL5+65zddjncjaFoBhdsK/akog= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= diff --git a/recovery.go b/recovery.go index bbf1d565..2124fe5f 100644 --- a/recovery.go +++ b/recovery.go @@ -106,7 +106,12 @@ func secureRequestDump(r *http.Request) string { return strings.Join(lines, "\r\n") } -func defaultHandleRecovery(c *Context, _ any) { +func defaultHandleRecovery(c *Context, err any) { + e, ok := err.(error) + if !ok { + e = fmt.Errorf("%v", err) + } + c.Error(e) //nolint: errcheck c.AbortWithStatus(http.StatusInternalServerError) } diff --git a/recovery_test.go b/recovery_test.go index 028c4ad6..e5211790 100644 --- a/recovery_test.go +++ b/recovery_test.go @@ -5,6 +5,7 @@ package gin import ( + "errors" "net" "net/http" "os" @@ -152,6 +153,49 @@ func TestPanicWithAbortHandler(t *testing.T) { assert.NotContains(t, out, "panic recovered") } +func TestPanicInHandlerRecordsError(t *testing.T) { + tests := []struct { + name string + recoveredErr any + expectedErr string + }{ + { + name: "string panic", + recoveredErr: "Oops, Houston, we have a problem", + expectedErr: "Oops, Houston, we have a problem", + }, + { + name: "error panic", + recoveredErr: errors.New("recovered error"), + expectedErr: "recovered error", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + router := New() + + var recoveredErrors errorMsgs + router.Use(func(c *Context) { + c.Next() + recoveredErrors = c.Errors + }) + router.Use(RecoveryWithWriter(nil)) + router.GET("/recovery", func(_ *Context) { + panic(tt.recoveredErr) + }) + + w := PerformRequest(router, http.MethodGet, "/recovery") + + assert.Equal(t, http.StatusInternalServerError, w.Code) + if assert.Len(t, recoveredErrors, 1) { + assert.EqualError(t, recoveredErrors[0], tt.expectedErr) + assert.Equal(t, ErrorTypePrivate, recoveredErrors[0].Type) + } + }) + } +} + func TestCustomRecoveryWithWriter(t *testing.T) { errBuffer := new(strings.Builder) buffer := new(strings.Builder) diff --git a/response_writer_test.go b/response_writer_test.go index 83d3fc8b..03417375 100644 --- a/response_writer_test.go +++ b/response_writer_test.go @@ -15,10 +15,33 @@ import ( "github.com/stretchr/testify/require" ) -// TODO -// func (w *responseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { -// func (w *responseWriter) CloseNotify() <-chan bool { -// func (w *responseWriter) Flush() { +// TestResponseWriterFlushWithFlusher verifies Flush() calls the underlying Flusher. +func TestResponseWriterFlushWithFlusher(t *testing.T) { + testWriter := httptest.NewRecorder() + writer := &responseWriter{ResponseWriter: testWriter} + writer.Flush() + assert.True(t, testWriter.Flushed) +} + +// TestResponseWriterFlushWithNonFlusher verifies Flush() is a no-op +// when the underlying ResponseWriter does not implement http.Flusher. +// Guards against the panic reported in https://github.com/gin-gonic/gin/issues/4460 +func TestResponseWriterFlushWithNonFlusher(t *testing.T) { + nonFlusher := &nonFlusherWriter{header: http.Header{}} + writer := &responseWriter{ResponseWriter: nonFlusher} + require.NotPanics(t, func() { + writer.Flush() + }) +} + +// nonFlusherWriter is a minimal http.ResponseWriter that does NOT implement http.Flusher. +type nonFlusherWriter struct { + header http.Header +} + +func (w *nonFlusherWriter) Header() http.Header { return w.header } +func (w *nonFlusherWriter) Write(b []byte) (int, error) { return len(b), nil } +func (w *nonFlusherWriter) WriteHeader(code int) {} var ( _ ResponseWriter = &responseWriter{}