diff --git a/internal/connections/connectiondetails.go b/internal/connections/connectiondetails.go index 1753b8682..620605293 100644 --- a/internal/connections/connectiondetails.go +++ b/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 { diff --git a/internal/connections/connectiondetails_utf8_test.go b/internal/connections/connectiondetails_utf8_test.go new file mode 100644 index 000000000..d269cc6c2 --- /dev/null +++ b/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) + } + }) + } +}