From ab56ffe50179fc58ddee5a5d0c085645a971b7eb Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Fri, 13 Dec 2024 16:46:36 -0700 Subject: [PATCH] [FIXED] Websocket: handling of close message frame without status 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 --- server/websocket.go | 24 ++++++++++++---- server/websocket_test.go | 62 +++++++++++++++++++++------------------- 2 files changed, 51 insertions(+), 35 deletions(-) diff --git a/server/websocket.go b/server/websocket.go index 7ebbaa85b79..664acdee80c 100644 --- a/server/websocket.go +++ b/server/websocket.go @@ -67,7 +67,6 @@ const ( wsCloseStatusProtocolError = 1002 wsCloseStatusUnsupportedData = 1003 wsCloseStatusNoStatusReceived = 1005 - wsCloseStatusAbnormalClosure = 1006 wsCloseStatusInvalidPayloadData = 1007 wsCloseStatusPolicyViolation = 1008 wsCloseStatusMessageTooBig = 1009 @@ -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 @@ -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 } diff --git a/server/websocket_test.go b/server/websocket_test.go index 2c8001b8099..b1f0ac03744 100644 --- a/server/websocket_test.go +++ b/server/websocket_test.go @@ -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 @@ -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] @@ -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:]) + } } } }) @@ -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]) @@ -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] @@ -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) { @@ -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) {