Client's buffer for non-101 response body issue optimization proposition

[Yurhigz]

What type of PR is this? (check all applicable)

• Refactor
• Feature
• Bug Fix
• Optimization
• Documentation Update
• Go Version Update
• Dependency Update

Description

This PR addresses the buffer size limitation discussed in issue #994 (made by thorrez) by increasing maxErrorResponseSize from 1024 to 4096 bytes when handling non-101 WebSocket upgrade responses.
When a WebSocket upgrade request fails (returns a status code other than 101), the client reads a portion of the error response body to help application debugging. I wanted to increase the buffer size up to 4096 bytes from 1024 bytes. This limit increases allows larger error responses while still protecting against malicious intents (excessively large bodies).
I added 3 cases under the TestRespOnBadHandshake to verify correct behavior :

• small error ( < 4096 bytes)
• large error ( > 4096 bytes)
• edge case ( = 4096 bytes)

Related Tickets & Documents

• Related Issue [FEATURE] Option to control client's buffer for non-101 response body #994

Added/updated tests?

• Yes
• No, and this is why: please replace this line with details on why tests
  have not been included
• I need help with writing tests

Run verifications and test

• make verify is passing
• make test is passing

[BeatieWolfe]

A simpler solution is to leave most of the code as is, but to get the size of the error response buffer from new Dialer field MaxErrorBodySize int. If the Dialer. MaxErrorBodySize is zero, then use the original buffer size of 1024.

The Dialer setting allows apps to adjust the value as needed. The 4096 in this PR may not be large enough.

Feel free to ignore everything I wrote as I am not a maintainer.

[BeatieWolfe]

The location of this variable declaration breaks the documentation (the DialContext documentation is no longer associated with the method). Move the var declaration.

While you are at it, change it to a const.

[Yurhigz]

Yup indeed you're right there !

[BeatieWolfe]

Delete the conversion to int64. The conversion is not required if maxErrorResponseSize is changed from a variable to a constant.

[BeatieWolfe]

io.ReadAll never returns io.EOF. Use if err != nil {.

[BeatieWolfe]

Eliminate the duplicated test by using table driven test.

for _, t := range []struct{ size, expect int }{{maxErrorResponseSize + 100, maxErrorResponseSize}, {maxErrorResponseSize, maxErrorResponseSize}} {
        body := bytes.Repeat([]byte{'a'}, t.size)
        ...
		if len(p) != t.expected {
			t.Fatalf("body size=%d, want %d for original body size %d", len(p), t.expected, t.size)
		}
 }

[Yurhigz]

A simpler solution is to leave most of the code as is, but to get the size of the error response buffer from new Dialer field MaxErrorBodySize int. If the Dialer. MaxErrorBodySize is zero, then use the original buffer size of 1024.

The Dialer setting allows apps to adjust the value as needed. The 4096 in this PR may not be large enough.

Feel free to ignore everything I wrote as I am not a maintainer.

Well I thought about adding the MaxErrorBodySize as a setting within the Dialer Struct. But we must make sure it can’t read an arbitrarily large / unbounded error body, otherwise a malicious or just poorly configured server could return a huge response body and cause excessive memory usage or even a denial-of-service. That’s why I was thinking a hard cap (or a read timeout) would still be necessary.

[BeatieWolfe]

But we must make sure it can’t read an arbitrarily large / unbounded error body

With my proposal, the maximum number of bytes read is neither arbitrary nor unbounded — it’s explicitly limited. The maximum is defined by the application, or defaults to 1024 bytes if not specified.

otherwise a malicious

What specific threat are you referring to here? Are you suggesting a scenario where an attacker has modified the application’s source code?

just poorly configured

It’s not the responsibility of the package to prevent an application from misconfiguring itself. Its role is to behave predictably within the constraints defined by the application.

That’s why I was thinking a hard cap (or a read timeout)

The current code has a read timeout.

[Yurhigz]

Thank you for the feedback, I'll provide a better version by adding an element to the dial struct as you mentioned. With a default value at 0 which define a buffer size of 1024.