Repository: nats-io/nats-server (go)

Issue:
Websocket: Server incorrectly sends close message status 1005 or 1006
### Observed behavior

(This issue was reported internally by @aricart)

When a client does not send any status/payload in the close message, the server detects correctly that situation but is sending back status 1005 (no status specified), which is wrong. It should simply not send any status/payload in its close message frame.
Also, server may send status 1006 (abnormal closure), which according to the specification, this status (like 1005) must not be set as a status code in a close control frame.

### Expected behavior

If a client sends a close message frame without status/payload, the server should echo the close message frame without status/payload, and the server should never send status 1006 (even if the server detects a client going away).

### Server and client version

Latest

### Host environment

N/A

### Steps to reproduce

Have a client sends a close message frame without status and observe that server is sending 1005. Also, in some situations, server would send 1006 (when it would detect a write or read error or when considering the connection stale).

Pull request description:
[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 <ivan@synadia.com>


Patch:
diff --git a/server/websocket.go b/server/websocket.go
index 7ebbaa85..664acdee 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
 	}


Test patch:
diff --git a/server/websocket_test.go b/server/websocket_test.go
index 2c8001b8..b1f0ac03 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) {