The separator a string field is escaped against is the one it is joined with

src/NosCore.Packets/Attributes/PacketIndexAttribute.cs

@@ -31,5 +31,12 @@ public PacketIndexAttribute(int index, string specialSeparator)
         public bool RemoveHeader { get; set; }
 
         public bool RemoveHash { get; set; }
+
+        /// <summary>
+        ///     Encode spaces as "^" even when nothing would break without it. The last field of a
+        ///     packet keeps its spaces by default, which is what chat lines need; a few fields
+        ///     carry text the client expects "^"-encoded regardless of where they sit.
+        /// </summary>
+        public bool EscapeSpaces { get; set; }
     }
 }

src/NosCore.Packets/NosCore.Packets.csproj

@@ -12,7 +12,7 @@
 		<RepositoryUrl>https://github.com/NosCoreIO/NosCore.Packets.git</RepositoryUrl>
 		<PackageIconUrl></PackageIconUrl>
 		<PackageTags>nostale, noscore, chickenapi, nostale private server source, nostale emulator</PackageTags>
-		<Version>21.0.0</Version>
+		<Version>21.1.0</Version>
 		<PackageRequireLicenseAcceptance>false</PackageRequireLicenseAcceptance>
 		<Description>NosCore's Packets (Nostale packets) defined over classes</Description>
 		<PackageLicenseExpression>MIT</PackageLicenseExpression>

src/NosCore.Packets/Serializer.cs

@@ -80,17 +80,33 @@ private Expression DefaultSerializer(Expression specificTypeExpression, Expressi
             return ConcatExpression(splitter, specificTypeExpression);
         }
 
-        private Expression StringSerializer(Expression exp, bool isLastIndex, bool isOptional, Expression splitter)
+        // escapeSeparator is the separator this field is joined to the packet with, which is
+        // what a value has to avoid containing. It is not the same expression as splitter: an
+        // ordinary property declares no special separator, so escaping against that one left
+        // every plain space-separated string free to split its own field in two.
+        private Expression StringSerializer(Expression exp, bool isLastIndex, bool isOptional, Expression splitter,
+            Expression escapeSeparator, bool isNested, bool escapeSpaces)
         {
-            var replace = Expression.Call(exp,
-                typeof(string).GetMethod("Replace", new[] { typeof(string), typeof(string) })!,
-                Expression.Convert(splitter, typeof(string)),
-                Expression.Constant("^", typeof(string))
-            );
+            var replaceMethod = typeof(string).GetMethod("Replace", new[] { typeof(string), typeof(string) })!;
+
+            // A nested value sits inside a field of the packet above it, so its own separator is
+            // not the only one it can break: a space in a sub-packet splits the outer field.
+            Expression escaped = Expression.Call(exp, replaceMethod,
+                Expression.Convert(escapeSeparator, typeof(string)),
+                Expression.Constant("^", typeof(string)));
+
+            if (isNested || escapeSpaces)
+            {
+                escaped = Expression.Call(escaped, replaceMethod,
+                    Expression.Constant(" ", typeof(string)),
+                    Expression.Constant("^", typeof(string)));
+            }
+
+            var replace = escaped;
 
             var nullOrEmpty = Expression.Call(null,
                 typeof(string).GetMethod("IsNullOrEmpty", new[] { typeof(string) })!,
-                Expression.Convert(splitter, typeof(string))
+                Expression.Convert(escapeSeparator, typeof(string))
             );
 
             return Expression.Condition(
@@ -99,7 +115,7 @@ private Expression StringSerializer(Expression exp, bool isLastIndex, bool isOpt
                 ConcatExpression(splitter,
                     Expression.Condition(nullOrEmpty,
                         Expression.Convert(exp, typeof(object)),
-                        Expression.Convert(isLastIndex ? exp : replace, typeof(object))))
+                        Expression.Convert(isLastIndex && !escapeSpaces ? exp : replace, typeof(object))))
             );
         }
 
@@ -195,7 +211,7 @@ private Expression ListSerializer(Expression injectedPacket, Expression specific
                     Expression.Convert(isPacketList
                         ? PacketSerializer(injectedPacket, indexAttr, param, subtype!, 0, propertySplitter, "", true)
                         : PropertySerializer(injectedPacket, indexAttr, subtype!, param, 0,
-                            Expression.Constant("")), typeof(string)), param)
+                            Expression.Constant(""), Expression.Constant("")), typeof(string)), param)
             );
 
             var listJoin = Expression.Convert(Expression.Call(
@@ -240,22 +256,26 @@ private Expression PacketSerializer(Expression injectedPacket, PacketIndexAttrib
                     isOptionalSerie = false;
                 }
 
+                var splitter = Expression.Condition(injectedPacket,
+                    Expression.Constant("^", typeof(object)),
+                    index.SpecialSeparator != null
+                        ? Expression.Constant(indexAttr.SpecialSeparator ?? string.Empty, typeof(object))
+                        : (Expression)Expression.Convert(propertySplitter, typeof(object)));
+
                 var exp = Expression.Convert(
                     PropertySerializer(injectedPacket,
                         index,
                         property.PropertyType,
                         Expression.Property(specificTypeExpression, property.Name),
                         maxIndex,
                         Expression.Condition(injectedPacket, Expression.Constant(string.Empty, typeof(object)),
-                            Expression.Constant(index.SpecialSeparator, typeof(object))))
+                            Expression.Constant(index.SpecialSeparator, typeof(object))),
+                        index.SpecialSeparator != null
+                            ? Expression.Constant(index.SpecialSeparator, typeof(object))
+                            : splitter,
+                        isFromList)
                     , typeof(object));
 
-                var splitter = Expression.Condition(injectedPacket,
-                    Expression.Constant("^", typeof(object)),
-                    index.SpecialSeparator != null
-                        ? Expression.Constant(indexAttr.SpecialSeparator ?? string.Empty, typeof(object))
-                        : (Expression)Expression.Convert(propertySplitter, typeof(object)));
-
                 var trimfirst = Expression.Condition(
                     Expression.Equal(incrementExpr, Expression.Constant(false)),
                     exp,
@@ -301,7 +321,8 @@ private Expression PacketSerializer(Expression injectedPacket, PacketIndexAttrib
         }
 
         private Expression PropertySerializer(Expression injectedPacket, PacketIndexAttribute indexAttr, Type type,
-            Expression specificTypeExpression, int maxIndex, Expression propertySplitter)
+            Expression specificTypeExpression, int maxIndex, Expression propertySplitter, Expression escapeSeparator,
+            bool isNested = false)
         {
             var useCustomSerializer = true;
             switch (type)
@@ -328,7 +349,8 @@ private Expression PropertySerializer(Expression injectedPacket, PacketIndexAttr
                 //handle string
                 case var t when t == typeof(string):
                     specificTypeExpression = StringSerializer(specificTypeExpression, indexAttr.Index == maxIndex,
-                        indexAttr.IsOptional, propertySplitter);
+                        indexAttr.IsOptional, propertySplitter, escapeSeparator, isNested,
+                        indexAttr.EscapeSpaces);
                     break;
                 //IPacket declared type
                 case var t when typeof(IPacket).IsAssignableFrom(t) && (t != typeof(IPacket)):

src/NosCore.Packets/ServerPackets/Families/GInfoPacket.cs

@@ -66,7 +66,7 @@ public class GInfoPacket : PacketBase
         /// <summary>
         ///     Should replace ' ' by '^'
         /// </summary>
-        [PacketIndex(16)]
+        [PacketIndex(16, EscapeSpaces = true)]
         public string? FamilyMessage { get; set; }
     }
 }

src/NosCore.Packets/ServerPackets/Miniland/MlInfoBrPacket.cs

@@ -27,7 +27,7 @@ public class MlInfoBrPacket : PacketBase
         [PacketIndex(4)]
         public byte Unknown2 { get; set; }
 
-        [PacketIndex(5)]
+        [PacketIndex(5, EscapeSpaces = true)]
         public string? MinilandMessage { get; set; }
     }
 }

src/NosCore.Packets/ServerPackets/Miniland/MlintroPacket.cs

@@ -12,7 +12,7 @@ namespace NosCore.Packets.ServerPackets.Miniland
     [PacketHeader("mlintro", Scope.InGame)]
     public class MlintroPacket : PacketBase
     {
-        [PacketIndex(0)]
+        [PacketIndex(0, EscapeSpaces = true)]
         public string? Intro { get; set; }
     }
 }

test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs

@@ -0,0 +1,133 @@
+//  __  _  __    __   ___ __  ___ ___
+// |  \| |/__\ /' _/ / _//__\| _ \ __|
+// | | ' | \/ |`._`.| \_| \/ | v / _|
+// |_|\__|\__/ |___/ \__/\__/|_|_\___|
+// -----------------------------------
+
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using NosCore.Packets.Attributes;
+using NosCore.Packets.Enumerations;
+using NosCore.Packets.Interfaces;
+using NosCore.Packets.ServerPackets.Mates;
+using NosCore.Packets.ServerPackets.Groups;
+using NosCore.Packets.ServerPackets.Miniland;
+using NosCore.Packets.ServerPackets.Visibility;
+using System.Collections.Generic;
+using NosCore.Shared.Enumerations;
+
+namespace NosCore.Packets.Tests
+{
+    [PacketHeader("tstsep", Scope.InGame)]
+    public class SeparatorProbePacket : PacketBase
+    {
+        [PacketIndex(0)] public int Before { get; set; }
+        [PacketIndex(1)] public string? Middle { get; set; }
+        [PacketIndex(2)] public int After { get; set; }
+        [PacketIndex(3)] public string? Last { get; set; }
+    }
+
+    [PacketHeader("tstdot", Scope.InGame)]
+    public class DottedProbePacket : PacketBase
+    {
+        [PacketIndex(0)] public int Before { get; set; }
+        [PacketIndex(1, SpecialSeparator = ".")] public string? Dotted { get; set; }
+        [PacketIndex(2)] public int After { get; set; }
+    }
+
+    // A value that contains the field separator used to split its own field in two and shift
+    // every field after it. The escaping existed but was aimed at SpecialSeparator, which an
+    // ordinary property never declares.
+    [TestClass]
+    public class StringFieldSeparatorTests
+    {
+        private static readonly ISerializer Serializer =
+            new Serializer(new[]
+            {
+                typeof(SeparatorProbePacket), typeof(DottedProbePacket),
+                typeof(ScpPacket), typeof(ScnPacket), typeof(InPacket),
+                typeof(PinitPacket), typeof(MlintroPacket), typeof(MlInfoBrPacket)
+            });
+
+        [TestMethod]
+        public void ASpaceInAMiddleFieldIsEscaped()
+        {
+            Assert.AreEqual("tstsep 1 Joyeux^Mouton 2 -",
+                Serializer.Serialize(new SeparatorProbePacket { Before = 1, Middle = "Joyeux Mouton", After = 2 }));
+        }
+
+        [TestMethod]
+        public void TheLastFieldKeepsItsSpaces()
+        {
+            // Nothing follows it, so nothing can shift - and chat lines rely on this.
+            Assert.AreEqual("tstsep 1 - 2 hello there",
+                Serializer.Serialize(new SeparatorProbePacket { Before = 1, After = 2, Last = "hello there" }));
+        }
+
+        [TestMethod]
+        public void AFieldWithItsOwnSeparatorEscapesThatSeparatorAndNotTheSpace()
+        {
+            Assert.AreEqual("tstdot 1.a^b 2",
+                Serializer.Serialize(new DottedProbePacket { Before = 1, Dotted = "a.b", After = 2 }));
+        }
+
+        [TestMethod]
+        public void ASpacedNameNoLongerSplitsTheMatePacket()
+        {
+            var wire = Serializer.Serialize(new ScpPacket
+            {
+                PetId = 1, NpcMonsterVNum = 333, Level = 15, Name = "Joyeux Mouton", IsSummonable = true
+            });
+
+            StringAssert.Contains(wire, "Joyeux^Mouton");
+            Assert.IsFalse(wire.Contains("Joyeux Mouton"));
+        }
+
+        [TestMethod]
+        public void TheSameHoldsForTheOtherPacketsThatCarryAName()
+        {
+            // ScnPacket.Name is index 36 of more, InPacket.Name is index 1 of many - neither is
+            // last, so both are escaped now. ScnPacket has said "Spaces should be replaced by ^"
+            // in a doc comment the whole time without anything enforcing it.
+            var scn = Serializer.Serialize(new ScnPacket
+            {
+                PetId = 1, NpcMonsterVNum = 333, Level = 15, Name = "Joyeux Mouton"
+            });
+            StringAssert.Contains(scn, "Joyeux^Mouton");
+            Assert.IsFalse(scn.Contains("Joyeux Mouton"));
+
+            var inp = Serializer.Serialize(new InPacket
+            {
+                VisualType = VisualType.Npc, Name = "Joyeux Mouton", VNum = "333", VisualId = 1
+            });
+            StringAssert.Contains(inp, "Joyeux^Mouton");
+            Assert.IsFalse(inp.Contains("Joyeux Mouton"));
+        }
+
+        [TestMethod]
+        public void AStringInsideASubPacketEscapesTheOuterSeparatorToo()
+        {
+            // Its own separator is "|", but the sub-packet sits inside a space-separated field,
+            // so an unescaped space here splits the packet above it.
+            Assert.AreEqual("pinit 0 0|0|1|10|Joyeux^Mouton|0|0|0|0|0|0",
+                Serializer.Serialize(new PinitPacket
+                {
+                    PinitSubPackets = new List<PinitSubPacket?>
+                    {
+                        new() { GroupPosition = 1, Name = "Joyeux Mouton", Level = 10 }
+                    }
+                }));
+        }
+
+        [TestMethod]
+        public void AFieldThatOptsInIsEscapedEvenThoughItIsLast()
+        {
+            // Nothing would break without it - the client simply expects these encoded.
+            Assert.AreEqual("mlintro Bienvenue^chez^moi",
+                Serializer.Serialize(new MlintroPacket { Intro = "Bienvenue chez moi" }));
+
+            StringAssert.Contains(
+                Serializer.Serialize(new MlInfoBrPacket { Name = "Bob", MinilandMessage = "hello there" }),
+                "hello^there");
+        }
+    }
+}