Skip to content

Commit 4823c8b

Browse files
committed
fix: safe type assertions in CloseNotify() and Hijack()
When gin is used with http.TimeoutHandler or any other middleware that wraps http.ResponseWriter with a type that does not implement http.Hijacker or http.CloseNotifier, the direct type assertions in Hijack() and CloseNotify() cause a runtime panic. Replace with checked assertions following the same pattern already used in Flush(). CloseNotify() returns nil when unsupported (http.CloseNotifier is deprecated since Go 1.11). Hijack() returns a descriptive error, consistent with the existing errHijackAlreadyWritten pattern. Fixes #4460
1 parent d3ffc99 commit 4823c8b

2 files changed

Lines changed: 67 additions & 15 deletions

File tree

response_writer.go

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,10 @@ const (
1717
defaultStatus = http.StatusOK
1818
)
1919

20-
var errHijackAlreadyWritten = errors.New("gin: response body already written")
20+
var (
21+
errHijackAlreadyWritten = errors.New("gin: response body already written")
22+
errHijackNotSupported = errors.New("gin: underlying ResponseWriter does not support hijacking")
23+
)
2124

2225
// ResponseWriter ...
2326
type ResponseWriter interface {
@@ -109,20 +112,25 @@ func (w *responseWriter) Written() bool {
109112

110113
// Hijack implements the http.Hijacker interface.
111114
func (w *responseWriter) Hijack() (net.Conn, *bufio.ReadWriter, error) {
112-
// Allow hijacking before any data is written (size == -1) or after headers are written (size == 0),
113-
// but not after body data is written (size > 0). For compatibility with websocket libraries (e.g., github.com/coder/websocket)
114115
if w.size > 0 {
115116
return nil, nil, errHijackAlreadyWritten
116117
}
118+
hijacker, ok := w.ResponseWriter.(http.Hijacker)
119+
if !ok {
120+
return nil, nil, errHijackNotSupported
121+
}
117122
if w.size < 0 {
118123
w.size = 0
119124
}
120-
return w.ResponseWriter.(http.Hijacker).Hijack()
125+
return hijacker.Hijack()
121126
}
122127

123128
// CloseNotify implements the http.CloseNotifier interface.
124129
func (w *responseWriter) CloseNotify() <-chan bool {
125-
return w.ResponseWriter.(http.CloseNotifier).CloseNotify()
130+
if cn, ok := w.ResponseWriter.(http.CloseNotifier); ok {
131+
return cn.CloseNotify()
132+
}
133+
return nil
126134
}
127135

128136
// Flush implements the http.Flusher interface.

response_writer_test.go

Lines changed: 54 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -113,17 +113,13 @@ func TestResponseWriterHijack(t *testing.T) {
113113
writer.reset(testWriter)
114114
w := ResponseWriter(writer)
115115

116-
assert.Panics(t, func() {
117-
_, _, err := w.Hijack()
118-
require.NoError(t, err)
119-
})
120-
assert.True(t, w.Written())
121-
122-
assert.Panics(t, func() {
123-
w.CloseNotify()
124-
})
116+
// Hijack on a non-hijacker writer returns an error without panicking.
117+
_, _, err := w.Hijack()
118+
assert.Error(t, err)
119+
// Capability check happens before state mutation, so Written() stays false on failure.
120+
assert.False(t, w.Written())
125121

126-
w.Flush()
122+
assert.NotPanics(t, func() { w.Flush() })
127123
}
128124

129125
type mockHijacker struct {
@@ -315,3 +311,51 @@ func TestPusherWithoutPusher(t *testing.T) {
315311
pusher := w.Pusher()
316312
assert.Nil(t, pusher, "Expected pusher to be nil")
317313
}
314+
315+
// mockNonHijackerWriter implements only http.ResponseWriter.
316+
// It intentionally does NOT implement http.Hijacker, http.Flusher, or http.CloseNotifier.
317+
type mockNonHijackerWriter struct {
318+
headers http.Header
319+
}
320+
321+
func (m *mockNonHijackerWriter) Header() http.Header {
322+
if m.headers == nil {
323+
m.headers = make(http.Header)
324+
}
325+
return m.headers
326+
}
327+
328+
func (m *mockNonHijackerWriter) Write(b []byte) (int, error) {
329+
return len(b), nil
330+
}
331+
332+
func (m *mockNonHijackerWriter) WriteHeader(statusCode int) {}
333+
334+
func TestResponseWriterOptionalInterfaceFallbacks(t *testing.T) {
335+
w := &mockNonHijackerWriter{}
336+
rw := &responseWriter{}
337+
rw.reset(w)
338+
339+
t.Run("Flush does not panic without Flusher", func(t *testing.T) {
340+
assert.NotPanics(t, func() { rw.Flush() })
341+
})
342+
343+
t.Run("CloseNotify returns nil without CloseNotifier", func(t *testing.T) {
344+
var ch <-chan bool
345+
assert.NotPanics(t, func() { ch = rw.CloseNotify() })
346+
assert.Nil(t, ch)
347+
})
348+
349+
t.Run("Hijack returns error without Hijacker", func(t *testing.T) {
350+
rw.reset(w) // reset state so Written() starts false
351+
assert.NotPanics(t, func() {
352+
conn, buf, err := rw.Hijack()
353+
assert.Nil(t, conn)
354+
assert.Nil(t, buf)
355+
assert.Error(t, err)
356+
assert.Contains(t, err.Error(), "does not support hijacking")
357+
})
358+
// Capability check happens before state mutation, so Written() stays false on failure.
359+
assert.False(t, rw.Written())
360+
})
361+
}

0 commit comments

Comments
 (0)