From b5118f65a202f7625ee5d5fdc376f59c5cc8a872 Mon Sep 17 00:00:00 2001 From: keitosuwahara Date: Sun, 13 Jul 2025 22:57:28 +0900 Subject: [PATCH 1/5] Prevention of Hijack() runtime panics --- response_writer.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/response_writer.go b/response_writer.go index 753a0b09ab..ab2f5fec68 100644 --- a/response_writer.go +++ b/response_writer.go @@ -6,6 +6,7 @@ package gin import ( "bufio" + "errors" "io" "net" "net/http" @@ -16,6 +17,8 @@ const ( defaultStatus = http.StatusOK ) +var errHijackAlreadyWritten = errors.New("gin: response already written") + // ResponseWriter ... type ResponseWriter interface { http.ResponseWriter @@ -106,6 +109,9 @@ func (w *responseWriter) Written() bool { // Hijack implements the http.Hijacker interface. func (w *responseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) { + if w.Written() { + return nil, nil, errHijackAlreadyWritten + } if w.size < 0 { w.size = 0 } From be39a2b9bee66dfe1c7405285e6b38ed351faae6 Mon Sep 17 00:00:00 2001 From: keitosuwahara Date: Sun, 13 Jul 2025 23:23:41 +0900 Subject: [PATCH 2/5] added test of Hijack() --- response_writer_test.go | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) diff --git a/response_writer_test.go b/response_writer_test.go index 259b8fa820..fe4fe25e85 100644 --- a/response_writer_test.go +++ b/response_writer_test.go @@ -5,6 +5,8 @@ package gin import ( + "bufio" + "net" "net/http" "net/http/httptest" "testing" @@ -124,6 +126,45 @@ func TestResponseWriterHijack(t *testing.T) { w.Flush() } +type mockHijacker struct { + *httptest.ResponseRecorder + hijacked bool +} + +func (m *mockHijacker) Hijack() (net.Conn, *bufio.ReadWriter, error) { + m.hijacked = true + return nil, nil, nil +} + +func TestResponseWriterHijackAfterWrite(t *testing.T) { + // Test case 1: Hijack before writing + hijacker := &mockHijacker{ResponseRecorder: httptest.NewRecorder()} + writer := &responseWriter{} + writer.reset(hijacker) + w := ResponseWriter(writer) + + _, _, err := w.Hijack() + require.NoError(t, err) + assert.True(t, hijacker.hijacked, "Hijack should be called") + assert.True(t, w.Written(), "Written() should be true after Hijack") + + // Test case 2: Hijack after writing + hijacker2 := &mockHijacker{ResponseRecorder: httptest.NewRecorder()} + writer2 := &responseWriter{} + writer2.reset(hijacker2) + w2 := ResponseWriter(writer2) + + _, err = w2.Write([]byte("test")) + require.NoError(t, err) + assert.True(t, w2.Written(), "Written() should be true after Write") + + // Now, try to hijack + _, _, err = w2.Hijack() + require.Error(t, err) + assert.Equal(t, errHijackAlreadyWritten, err, "Hijack after write should return errHijackAlreadyWritten") + assert.False(t, hijacker2.hijacked, "Hijack should not be called after write") +} + func TestResponseWriterFlush(t *testing.T) { testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { writer := &responseWriter{} From 5cd431d6f64fbe864d1b7ff349d73aef690943ea Mon Sep 17 00:00:00 2001 From: keitosuwahara Date: Mon, 21 Jul 2025 22:26:48 +0900 Subject: [PATCH 3/5] fix review --- response_writer_test.go | 69 +++++++++++++++++++++++++---------------- 1 file changed, 42 insertions(+), 27 deletions(-) diff --git a/response_writer_test.go b/response_writer_test.go index fe4fe25e85..d68ce5eefe 100644 --- a/response_writer_test.go +++ b/response_writer_test.go @@ -131,39 +131,54 @@ type mockHijacker struct { hijacked bool } +// Hijack implements the http.Hijacker interface. It just records that it was called. func (m *mockHijacker) Hijack() (net.Conn, *bufio.ReadWriter, error) { m.hijacked = true return nil, nil, nil } func TestResponseWriterHijackAfterWrite(t *testing.T) { - // Test case 1: Hijack before writing - hijacker := &mockHijacker{ResponseRecorder: httptest.NewRecorder()} - writer := &responseWriter{} - writer.reset(hijacker) - w := ResponseWriter(writer) - - _, _, err := w.Hijack() - require.NoError(t, err) - assert.True(t, hijacker.hijacked, "Hijack should be called") - assert.True(t, w.Written(), "Written() should be true after Hijack") - - // Test case 2: Hijack after writing - hijacker2 := &mockHijacker{ResponseRecorder: httptest.NewRecorder()} - writer2 := &responseWriter{} - writer2.reset(hijacker2) - w2 := ResponseWriter(writer2) - - _, err = w2.Write([]byte("test")) - require.NoError(t, err) - assert.True(t, w2.Written(), "Written() should be true after Write") - - // Now, try to hijack - _, _, err = w2.Hijack() - require.Error(t, err) - assert.Equal(t, errHijackAlreadyWritten, err, "Hijack after write should return errHijackAlreadyWritten") - assert.False(t, hijacker2.hijacked, "Hijack should not be called after write") -} + tests := []struct { + name string + action func(w ResponseWriter) error // Action to perform before hijacking + expectHijack bool + expectWritten bool + expectError error + }{ + { + name: "hijack before write should succeed", + action: func(w ResponseWriter) error { return nil }, + expectHijack: true, + expectWritten: true, // Hijack itself marks the writer as written + expectError: nil, + }, + { + name: "hijack after write should fail", + action: func(w ResponseWriter) error { + _, err := w.Write([]byte("test")) + return err + }, + expectHijack: false, + expectWritten: true, + expectError: errHijackAlreadyWritten, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + hijacker := &mockHijacker{ResponseRecorder: httptest.NewRecorder()} + writer := &responseWriter{} + writer.reset(hijacker) + w := ResponseWriter(writer) + + require.NoError(t, tc.action(w), "unexpected error during pre-hijack action") + + _, _, hijackErr := w.Hijack() + assert.ErrorIs(t, hijackErr, tc.expectError, "unexpected error from Hijack()") + assert.Equal(t, tc.expectHijack, hijacker.hijacked, "unexpected hijacker.hijacked state") + assert.Equal(t, tc.expectWritten, w.Written(), "unexpected w.Written() state") + }) + }} func TestResponseWriterFlush(t *testing.T) { testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From faae1ff70bf508741dc6333ebabcfff01c0d5e49 Mon Sep 17 00:00:00 2001 From: keitosuwahara Date: Mon, 21 Jul 2025 22:32:14 +0900 Subject: [PATCH 4/5] fix lint error --- response_writer_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/response_writer_test.go b/response_writer_test.go index d68ce5eefe..b3cb9e724e 100644 --- a/response_writer_test.go +++ b/response_writer_test.go @@ -174,11 +174,12 @@ func TestResponseWriterHijackAfterWrite(t *testing.T) { require.NoError(t, tc.action(w), "unexpected error during pre-hijack action") _, _, hijackErr := w.Hijack() - assert.ErrorIs(t, hijackErr, tc.expectError, "unexpected error from Hijack()") + require.ErrorIs(t, hijackErr, tc.expectError, "unexpected error from Hijack()") assert.Equal(t, tc.expectHijack, hijacker.hijacked, "unexpected hijacker.hijacked state") assert.Equal(t, tc.expectWritten, w.Written(), "unexpected w.Written() state") }) - }} + } +} func TestResponseWriterFlush(t *testing.T) { testServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { From a21f5aca7785c896dec53f6431fc54ca14c6d6b5 Mon Sep 17 00:00:00 2001 From: keitosuwahara Date: Fri, 1 Aug 2025 22:22:55 +0900 Subject: [PATCH 5/5] added check assertion of Wrrten() condition before calling Hijack() --- response_writer_test.go | 43 +++++++++++++++++++++++++++-------------- 1 file changed, 28 insertions(+), 15 deletions(-) diff --git a/response_writer_test.go b/response_writer_test.go index b3cb9e724e..ef19841860 100644 --- a/response_writer_test.go +++ b/response_writer_test.go @@ -139,18 +139,20 @@ func (m *mockHijacker) Hijack() (net.Conn, *bufio.ReadWriter, error) { func TestResponseWriterHijackAfterWrite(t *testing.T) { tests := []struct { - name string - action func(w ResponseWriter) error // Action to perform before hijacking - expectHijack bool - expectWritten bool - expectError error + name string + action func(w ResponseWriter) error // Action to perform before hijacking + expectWrittenBeforeHijack bool + expectHijackSuccess bool + expectWrittenAfterHijack bool + expectError error }{ { - name: "hijack before write should succeed", - action: func(w ResponseWriter) error { return nil }, - expectHijack: true, - expectWritten: true, // Hijack itself marks the writer as written - expectError: nil, + name: "hijack before write should succeed", + action: func(w ResponseWriter) error { return nil }, + expectWrittenBeforeHijack: false, + expectHijackSuccess: true, + expectWrittenAfterHijack: true, // Hijack itself marks the writer as written + expectError: nil, }, { name: "hijack after write should fail", @@ -158,9 +160,10 @@ func TestResponseWriterHijackAfterWrite(t *testing.T) { _, err := w.Write([]byte("test")) return err }, - expectHijack: false, - expectWritten: true, - expectError: errHijackAlreadyWritten, + expectWrittenBeforeHijack: true, + expectHijackSuccess: false, + expectWrittenAfterHijack: true, + expectError: errHijackAlreadyWritten, }, } @@ -171,12 +174,22 @@ func TestResponseWriterHijackAfterWrite(t *testing.T) { writer.reset(hijacker) w := ResponseWriter(writer) + // Check initial state + assert.False(t, w.Written(), "should not be written initially") + + // Perform pre-hijack action require.NoError(t, tc.action(w), "unexpected error during pre-hijack action") + // Check state before hijacking + assert.Equal(t, tc.expectWrittenBeforeHijack, w.Written(), "unexpected w.Written() state before hijack") + + // Attempt to hijack _, _, hijackErr := w.Hijack() + + // Check results require.ErrorIs(t, hijackErr, tc.expectError, "unexpected error from Hijack()") - assert.Equal(t, tc.expectHijack, hijacker.hijacked, "unexpected hijacker.hijacked state") - assert.Equal(t, tc.expectWritten, w.Written(), "unexpected w.Written() state") + assert.Equal(t, tc.expectHijackSuccess, hijacker.hijacked, "unexpected hijacker.hijacked state") + assert.Equal(t, tc.expectWrittenAfterHijack, w.Written(), "unexpected w.Written() state after hijack") }) } }