fix(websocket): replace invalid UTF-8 instead of dropping the session

internal/connections/connectiondetails.go

@@ -8,6 +8,7 @@ import (
 	"sync"
 	"sync/atomic"
 	"time"
+	"unicode/utf8"
 
 	"github.com/GoMudEngine/GoMud/internal/mudlog"
 	"github.com/GoMudEngine/GoMud/internal/term"
@@ -296,16 +297,38 @@ func (cd *ConnectionDetails) Write(p []byte) (n int, err error) {
 			return 0, nil
 		}
 
-		err := cd.wsConn.WriteMessage(websocket.TextMessage, p)
+		// A text frame must be valid UTF-8 or the client closes the connection
+		// (1007), which is what the first-byte check above is guarding one case of.
+		// Anything else that splits a rune - a width-aware wrap that counts bytes,
+		// for instance - would drop the session mid-sentence, so replace the bad
+		// bytes and log rather than hand the client something it must reject.
+		payload, replaced := validUTF8Payload(p)
+		if replaced {
+			mudlog.Error("conn.Write", "error", "Invalid UTF-8 in websocket payload; replaced", "bytes", p)
+		}
+
+		err := cd.wsConn.WriteMessage(websocket.TextMessage, payload)
 		if err != nil {
 			return 0, err
 		}
+		// Report the caller's length: the write consumed all of p, whatever the
+		// replacement did to the byte count.
 		return len(p), nil
 	}
 
 	return cd.conn.Write(p)
 }
 
+// validUTF8Payload returns a websocket-safe copy of p, reporting whether any
+// invalid byte had to be replaced. A valid payload is returned untouched so the
+// common path allocates nothing.
+func validUTF8Payload(p []byte) (payload []byte, replaced bool) {
+	if utf8.Valid(p) {
+		return p, false
+	}
+	return []byte(strings.ToValidUTF8(string(p), string(utf8.RuneError))), true
+}
+
 func (cd *ConnectionDetails) Read(p []byte) (n int, err error) {
 
 	if cd.sshChannel != nil {

internal/connections/connectiondetails_utf8_test.go

@@ -0,0 +1,44 @@
+package connections
+
+import (
+	"strings"
+	"testing"
+	"unicode/utf8"
+)
+
+// A websocket text frame must be valid UTF-8, so a payload that splits a rune
+// closes the connection with 1007 rather than showing the player anything.
+func TestValidUTF8Payload(t *testing.T) {
+	bar := strings.Repeat("░", 25)
+
+	tests := []struct {
+		name     string
+		in       []byte
+		replaced bool
+	}{
+		{name: "ascii", in: []byte("You see a rusty sword."), replaced: false},
+		{name: "multi-byte runes", in: []byte(bar), replaced: false},
+		{name: "empty", in: []byte{}, replaced: false},
+		// The tail of a progress bar cut after the first byte of a 3-byte rune.
+		{name: "rune split at the end", in: []byte(bar)[:len(bar)-2], replaced: true},
+		// Orphaned continuation bytes, i.e. the other half of that cut.
+		{name: "orphan continuation bytes", in: []byte{0x96, 0x91, 'h', 'i'}, replaced: true},
+		{name: "lone 0xff", in: []byte{'a', 0xff, 'b'}, replaced: true},
+	}
+
+	for _, test := range tests {
+		t.Run(test.name, func(t *testing.T) {
+			payload, replaced := validUTF8Payload(test.in)
+
+			if replaced != test.replaced {
+				t.Fatalf("replaced = %v, want %v", replaced, test.replaced)
+			}
+			if !utf8.Valid(payload) {
+				t.Fatalf("payload is still not valid UTF-8: %q", payload)
+			}
+			if !test.replaced && string(payload) != string(test.in) {
+				t.Fatalf("valid payload was altered: got %q, want %q", payload, test.in)
+			}
+		})
+	}
+}