Skip to content

Commit

Permalink
[FIXED] Websocket: handling of close message frame without status
Browse files Browse the repository at this point in the history 
The server was incorrectly sending back a close message frame with
status 1005 when not receiving a status in the close message frame
from the client. This was wrong. This status was to be used internally
to keep track that no status was received and therefore the server
should not send a status back.

Also, the server was sending status 1006 in some error conditions,
while the spec also states that this status must not be used to
set the status, instead it was to internally keep track that there
was an error condition.

Resolves #6259

Signed-off-by: Ivan Kozlovic <ivan@synadia.com>
  • Loading branch information
@kozlovic @neilalexander
kozlovic authored and neilalexander committed Dec 16, 2024
1 parent f2a78cf commit 9739629
Show file tree
Hide file tree
Showing 2 changed files with 51 additions and 35 deletions.
24 changes: 18 additions & 6 deletions server/websocket.go
View file
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 @@ -458,9 +457,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 @@ -647,10 +658,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
Original file line number Diff line number Diff line change
Expand Up @@ -647,19 +647,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 @@ -685,7 +690,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 @@ -695,12 +704,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 @@ -778,7 +789,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 @@ -795,14 +805,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 @@ -812,13 +823,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 @@ -1015,9 +1019,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

0 comments on commit 9739629

Please sign in to comment.