diff --git a/server/websocket.go b/server/websocket.go index 49881b2c1e..69e6e1a9a7 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 @@ -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 @@ -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 } diff --git a/server/websocket_test.go b/server/websocket_test.go index 368a9fe8d6..e4fa70e75a 100644 --- a/server/websocket_test.go +++ b/server/websocket_test.go @@ -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 @@ -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] @@ -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:]) + } } } }) @@ -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]) @@ -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] @@ -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) { @@ -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) {