Prevention of Hijack() runtime panics

[suwakei]

Proposal

The current implementation of the Hijack() method allows it to be called even after the response body has already been written to the client;
Go's internal implementation of http.Server is designed so that calling Hijack () after Write() or WriteHeader() has been called is designed to return an error,
and the current Gin implementation differs from this.

Because of this difference,
some implementations of the underlying http.ResponseWriter may experience unexpected panics if Hijack() is called at an unintended time.

Therefore, we suggest changing the Hijack() method to check at the beginning of the method to see if the response has already been written
and return a clear and descriptive error if it has.

Changes

Definition of errHijackAlreadyWritten:

• Define an explicit error variable to return when Hijack() fails.

Update Hijack() method:

• At the beginning of the method, call w.Written() to see if the response has already been written.
• If it has already been written, return an errHijackAlreadyWritten error and stop further execution.

[codecov]

Codecov Report

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

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4295      +/-   ##
==========================================
- Coverage   99.21%   98.92%   -0.29%     
==========================================
  Files          42       44       +2     
  Lines        3182     3440     +258     
==========================================
+ Hits         3157     3403     +246     
- Misses         17       26       +9     
- Partials        8       11       +3     

Flag	Coverage Δ
	?
--ldflags="-checklinkname=0" -tags sonic	98.85% <100.00%> (?)
-tags go_json	98.85% <100.00%> (?)
-tags nomsgpack	98.90% <100.00%> (?)
go-1.18	?
go-1.19	?
go-1.20	?
go-1.21	?
go-1.23	98.92% <100.00%> (?)
go-1.24	98.92% <100.00%> (?)
macos-latest	98.92% <100.00%> (-0.29%)	⬇️
ubuntu-latest	98.92% <100.00%> (-0.29%)	⬇️

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.

[suwakei]

All lines introduced in this PR are covered by tests.

The reported -0.29% decrease in coverage is not caused by these changes,
but rather due to a difference in the base reference (commit 3dc1cd6).
If there's anything you'd like me to improve or test further, feel free to let me know!

[suwakei]

Hi! Just following up to see if there's anything I can improve in this PR 🙇

[suwakei]

Thank you for your review, I have corrected the points you pointed out.

1. Memory Management:

Opinion: This point is correct as a general reminder, but it is not a problem in this code. mockHijacker is created as a local variable in the TestResponseWriterHijackAfterWrite function.
I believe that there is no need to worry about memory leaks because the memory is automatically released at the end of the function.

2. Concurrency Considerations:

Opinion: In the current test code, mockHijacker is not used in concurrency,
I don't think it needs to be supported at the moment.

3. Error Handling in Hijack Method:

Opinion: mockHijacker is a mock for testing, and since the return value is not used in the current test, an implementation that returns nil is fine.
However, to increase code readability, I will specify in the comments that this is an intentional implementation.

	hijacked bool
}
// response_writer_test.go
++ // 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

4. Testing Edge Cases:

Opinion: It is important for the robustness of the code to test that Hijack works as expected even if responseWriter.Write fails. Looking at the implementation of responseWriter.Write,
first WriteHeaderNow() is called and w.size is updated from noWritten(-1) to 0. This causes Written() to be true. This process takes place before the actual w.ResponseWriter.Write(data) succeeds or fails.
Thus, even if Write fails, subsequent Hijack() should return errHijackAlreadyWritten because Written() is already true. Add a test case to guarantee this behavior.

5. Remove Duplicate Type Definition:

Opinion:Looking at the code in response_writer_test.go, there is only one definition of mockHijacker.
This is most likely a misrecognition by the AI tool when parsing the difference (PATCH). It is safe to ignore this point.

6. Group Related Assertions:

Describe the text in place of documentation in the name field of the test.
(See also 11. Documentation:)

7. Assertion Messages:

Fixed style consistency.

8. Consider Logging:

Adding logging to a simple mock like this one may be excessive.

9. Enhance Test Case Comments:

Describe the text in place of documentation in the name field of the test.
(See also 11. Documentation:)

10. Parameterize Test Cases:

Fixed.
It will be easier to add new test cases like those pointed out in “4. Testing of Edge Cases” in the future It will be easier to add new test cases like those pointed out in "4.

11. Documentation:

This is related to “9. Enhanced test case comments” and “10. Parameterization of test cases”. We believe that the name field (e.g., “hijack before write should succeed”) that is attached to each entry in a table-driven test serves as documentation as it is.

[suwakei]

Hi maintainers 👋
Just following up on this PR — happy to make any changes if needed. Please let me know if there's anything blocking review. Thanks!

[Copilot]

Pull Request Overview

This PR prevents runtime panics by adding a safety check to the Hijack() method to ensure it cannot be called after the response has already been written, aligning with Go's standard HTTP server behavior.

• Adds a pre-write check in Hijack() method that returns an error if the response has already been written
• Introduces a specific error type errHijackAlreadyWritten for this failure scenario
• Includes comprehensive test coverage for both valid and invalid hijack scenarios

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
response_writer.go	Adds error check and error variable to prevent hijacking after response is written
response_writer_test.go	Adds test cases and mock hijacker to verify the new safety behavior

Comments suppressed due to low confidence (2)

response_writer_test.go:152

• The test expects expectWritten: true for the "hijack before write" case, but according to the comment, this is because "Hijack itself marks the writer as written". However, the new implementation returns early if already written, so it's unclear if hijacking actually marks the writer as written when it succeeds. This test case should verify the actual behavior rather than assume it.

			expectWritten: true, // Hijack itself marks the writer as written

response_writer_test.go:174

• The test should verify the Written() state before calling Hijack() to ensure the test setup is correct. Currently, it only checks the state after hijacking, which doesn't validate that the pre-conditions are properly established.

			require.NoError(t, tc.action(w), "unexpected error during pre-hijack action")

[suwakei]

Thanks for the feedback! I've updated the tests to:

• Verify the Written() state before and after Hijack() as suggested
• Ensure that test expectations align with the actual behavior

Let me know if there's anything else I should update. 🙌