Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIXED] Websocket: handling of close message frame without status #6260

Merged
merged 1 commit into from
Dec 14, 2024
Merged

[FIXED] Websocket: handling of close message frame without status #6260

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 18 additions & 6 deletions server/websocket.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@ const (
wsCloseStatusProtocolError = 1002
wsCloseStatusUnsupportedData = 1003
wsCloseStatusNoStatusReceived = 1005
wsCloseStatusAbnormalClosure = 1006
wsCloseStatusInvalidPayloadData = 1007
wsCloseStatusPolicyViolation = 1008
wsCloseStatusMessageTooBig = 1009
Expand Down Expand Up @@ -462,9 +461,21 @@ func (c *client) wsHandleControlFrame(r *wsReadInfo, frameType wsOpCode, nc io.R
}
}
}
clm := wsCreateCloseMessage(status, body)
// If the status indicates that nothing was received, then we don't
// send anything back.
// From https://datatracker.ietf.org/doc/html/rfc6455#section-7.4
// it says that code 1005 is a reserved value and MUST NOT be set as a
// status code in a Close control frame by an endpoint. It is
// designated for use in applications expecting a status code to indicate
// that no status code was actually present.
var clm []byte
if status != wsCloseStatusNoStatusReceived {
clm = wsCreateCloseMessage(status, body)
}
c.wsEnqueueControlMessage(wsCloseMessage, clm)
nbPoolPut(clm) // wsEnqueueControlMessage has taken a copy.
if len(clm) > 0 {
nbPoolPut(clm) // wsEnqueueControlMessage has taken a copy.
}
// Return io.EOF so that readLoop will close the connection as ClientClosed
// after processing pending buffers.
return pos, io.EOF
Expand Down Expand Up @@ -651,10 +662,11 @@ func (c *client) wsEnqueueCloseMessage(reason ClosedState) {
status = wsCloseStatusProtocolError
case MaxPayloadExceeded:
status = wsCloseStatusMessageTooBig
case ServerShutdown:
case WriteError, ReadError, StaleConnection, ServerShutdown:
// We used to have WriteError, ReadError and StaleConnection result in
// code 1006, which the spec says that it must not be used to set the
// status in the close message. So using this one instead.
status = wsCloseStatusGoingAway
case WriteError, ReadError, StaleConnection:
status = wsCloseStatusAbnormalClosure
default:
status = wsCloseStatusInternalSrvError
}
Expand Down
62 changes: 33 additions & 29 deletions server/websocket_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -646,19 +646,24 @@ func TestWSReadPongFrame(t *testing.T) {

func TestWSReadCloseFrame(t *testing.T) {
for _, test := range []struct {
name string
payload []byte
name string
noStatus bool
payload []byte
}{
{"without payload", nil},
{"with payload", []byte("optional payload")},
{"status without payload", false, nil},
{"status with payload", false, []byte("optional payload")},
{"no status no payload", true, nil},
} {
t.Run(test.name, func(t *testing.T) {
c, ri, tr := testWSSetupForRead()
// a close message has a status in 2 bytes + optional payload
payload := make([]byte, 2+len(test.payload))
binary.BigEndian.PutUint16(payload[:2], wsCloseStatusNormalClosure)
if len(test.payload) > 0 {
copy(payload[2:], test.payload)
var payload []byte
if !test.noStatus {
// a close message has a status in 2 bytes + optional payload
payload = make([]byte, 2+len(test.payload))
binary.BigEndian.PutUint16(payload[:2], wsCloseStatusNormalClosure)
if len(test.payload) > 0 {
copy(payload[2:], test.payload)
}
}
close := testWSCreateClientMsg(wsCloseMessage, 1, true, false, payload)
// Have a normal frame prior to close to make sure that wsRead returns
Expand All @@ -684,7 +689,11 @@ func TestWSReadCloseFrame(t *testing.T) {
if n := len(nb); n == 0 {
t.Fatalf("Expected buffers, got %v", n)
}
if expected := 2 + 2 + len(test.payload); expected != len(nb[0]) {
if test.noStatus {
if expected := 2; expected != len(nb[0]) {
t.Fatalf("Expected buffer to be %v bytes long, got %v", expected, len(nb[0]))
}
} else if expected := 2 + 2 + len(test.payload); expected != len(nb[0]) {
t.Fatalf("Expected buffer to be %v bytes long, got %v", expected, len(nb[0]))
}
b := nb[0][0]
Expand All @@ -694,12 +703,14 @@ func TestWSReadCloseFrame(t *testing.T) {
if b&byte(wsCloseMessage) == 0 {
t.Fatalf("Should have been a CLOSE, it wasn't: %v", b)
}
if status := binary.BigEndian.Uint16(nb[0][2:4]); status != wsCloseStatusNormalClosure {
t.Fatalf("Expected status to be %v, got %v", wsCloseStatusNormalClosure, status)
}
if len(test.payload) > 0 {
if !bytes.Equal(nb[0][4:], test.payload) {
t.Fatalf("Unexpected content: %s", nb[0][4:])
if !test.noStatus {
if status := binary.BigEndian.Uint16(nb[0][2:4]); status != wsCloseStatusNormalClosure {
t.Fatalf("Expected status to be %v, got %v", wsCloseStatusNormalClosure, status)
}
if len(test.payload) > 0 {
if !bytes.Equal(nb[0][4:], test.payload) {
t.Fatalf("Unexpected content: %s", nb[0][4:])
}
}
}
})
Expand Down Expand Up @@ -777,7 +788,6 @@ func TestWSCloseFrameWithPartialOrInvalid(t *testing.T) {

// Now test close with invalid status size (1 instead of 2 bytes)
c, ri, tr = testWSSetupForRead()
payload[0] = 100
binary.BigEndian.PutUint16(payload, wsCloseStatusNormalClosure)
closeMsg = testWSCreateClientMsg(wsCloseMessage, 1, true, false, payload[:1])

Expand All @@ -794,14 +804,15 @@ func TestWSCloseFrameWithPartialOrInvalid(t *testing.T) {
if n := len(bufs); n != 0 {
t.Fatalf("Unexpected buffer returned: %v", n)
}
// A CLOSE should have been queued with the payload of the original close message.
// Since no status was received, the server will send a close frame without
// status code nor payload.
c.mu.Lock()
nb, _ = c.collapsePtoNB()
c.mu.Unlock()
if n := len(nb); n == 0 {
t.Fatalf("Expected buffers, got %v", n)
}
if expected := 2 + 2; expected != len(nb[0]) {
if expected := 2; expected != len(nb[0]) {
t.Fatalf("Expected buffer to be %v bytes long, got %v", expected, len(nb[0]))
}
b = nb[0][0]
Expand All @@ -811,13 +822,6 @@ func TestWSCloseFrameWithPartialOrInvalid(t *testing.T) {
if b&byte(wsCloseMessage) == 0 {
t.Fatalf("Should have been a CLOSE, it wasn't: %v", b)
}
// Since satus was not valid, we should get wsCloseStatusNoStatusReceived
if status := binary.BigEndian.Uint16(nb[0][2:4]); status != wsCloseStatusNoStatusReceived {
t.Fatalf("Expected status to be %v, got %v", wsCloseStatusNoStatusReceived, status)
}
if len(nb[0][:]) != 4 {
t.Fatalf("Unexpected content: %s", nb[0][2:])
}
}

func TestWSReadGetErrors(t *testing.T) {
Expand Down Expand Up @@ -1014,9 +1018,9 @@ func TestWSEnqueueCloseMsg(t *testing.T) {
{BadClientProtocolVersion, wsCloseStatusProtocolError},
{MaxPayloadExceeded, wsCloseStatusMessageTooBig},
{ServerShutdown, wsCloseStatusGoingAway},
{WriteError, wsCloseStatusAbnormalClosure},
{ReadError, wsCloseStatusAbnormalClosure},
{StaleConnection, wsCloseStatusAbnormalClosure},
{WriteError, wsCloseStatusGoingAway},
{ReadError, wsCloseStatusGoingAway},
{StaleConnection, wsCloseStatusGoingAway},
{ClosedState(254), wsCloseStatusInternalSrvError},
} {
t.Run(test.reason.String(), func(t *testing.T) {
Expand Down
Loading