Skip to content

fix: safe type assertions in CloseNotify() and Hijack()#4639

Open
jassus213 wants to merge 2 commits intogin-gonic:masterfrom
jassus213:fix/response-writer-safe-assertions
Open

fix: safe type assertions in CloseNotify() and Hijack()#4639
jassus213 wants to merge 2 commits intogin-gonic:masterfrom
jassus213:fix/response-writer-safe-assertions

Conversation

@jassus213
Copy link
Copy Markdown

Problem

Similar to #4460 (fixed for Flush() in #4479), Hijack() and CloseNotify() perform direct type assertions that panic when the underlying writer doesn't implement the interface.

When gin is used with http.TimeoutHandler or any middleware that wraps http.ResponseWriter with a type that doesn't implement http.Hijacker or http.CloseNotifier, calling Hijack() or CloseNotify() causes a runtime panic:

// panics if underlying writer is not http.CloseNotifier                                                                                                                                    
return w.ResponseWriter.(http.CloseNotifier).CloseNotify()                       
                                                          
// panics if underlying writer is not http.Hijacker       
return w.ResponseWriter.(http.Hijacker).Hijack()                                                                                                                                            

Fix

Replace direct assertions with checked assertions - the same pattern already used in Flush():

  • CloseNotify() returns nil when unsupported (http.CloseNotifier is deprecated since Go 1.11; callers should use Request.Context().Done())
  • Hijack() returns errHijackNotSupported, consistent with the existing errHijackAlreadyWritten pattern

The capability check in Hijack() now runs before mutating w.size, so Written() stays false on the error path.

Tests

Added TestResponseWriterOptionalInterfaceFallbacks with a mockNonHijackerWriter (implements only http.ResponseWriter) verifying that Flush(), CloseNotify(), and Hijack() all degrade gracefully without panicking.

Closes #4638.

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 gin-gonic#4460
@codecov
Copy link
Copy Markdown

codecov Bot commented Apr 22, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 98.43%. Comparing base (3dc1cd6) to head (1f5552f).
⚠️ Report is 275 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4639      +/-   ##
==========================================
- Coverage   99.21%   98.43%   -0.79%     
==========================================
  Files          42       48       +6     
  Lines        3182     3761     +579     
==========================================
+ Hits         3157     3702     +545     
- Misses         17       50      +33     
- Partials        8        9       +1     
Flag Coverage Δ
?
--ldflags="-checklinkname=0" -tags sonic 98.42% <100.00%> (?)
-tags go_json 98.36% <100.00%> (?)
-tags nomsgpack 98.41% <100.00%> (?)
go-1.18 ?
go-1.19 ?
go-1.20 ?
go-1.21 ?
go-1.25 98.43% <100.00%> (?)
go-1.26 98.43% <100.00%> (?)
macos-latest 98.43% <100.00%> (-0.79%) ⬇️
ubuntu-latest 98.43% <100.00%> (-0.79%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unsafe type assertions in Hijack() and CloseNotify() cause panic with http.TimeoutHandler

1 participant