From 8a83499939a00df5e5bb298f902f546e384e61bd Mon Sep 17 00:00:00 2001 From: erwan-joly Date: Sun, 30 Aug 2026 11:17:56 +1200 Subject: [PATCH 1/3] fix(serializer): escape the field separator, not the special one A string field that contained the separator split its own field in two and shifted every field after it. Serialized on master, a mate named "Joyeux Mouton" produced sc_p 1 333 0 15 ... 0 Joyeux Mouton 0 where the client reads a name and then a stray field. The escaping was already there - StringSerializer replaces the separator with "^" for any field that is not the last one - but the separator it was handed is PacketIndexAttribute.SpecialSeparator, which an ordinary property never declares. IsNullOrEmpty then short-circuited and the value went out untouched, while the real " " separator was applied afterwards by PacketSerializer and never reached the substitution. So the guard only ever fired for the handful of properties that declare a separator of their own. StringSerializer now takes the separator the field is actually joined with. The last field still keeps its spaces, which chat lines and miniland intros rely on, and a property with a SpecialSeparator still escapes that one rather than the space. ScnPacket.Name has carried "Spaces should be replaced by ^" as a doc comment this whole time with nothing enforcing it. The 115 existing tests pin a lot of exact wire output and all still pass. --- src/NosCore.Packets/Serializer.cs | 34 +++--- .../StringFieldSeparatorTests.cs | 102 ++++++++++++++++++ 2 files changed, 123 insertions(+), 13 deletions(-) create mode 100644 test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs diff --git a/src/NosCore.Packets/Serializer.cs b/src/NosCore.Packets/Serializer.cs index e53ce85c..03214869 100644 --- a/src/NosCore.Packets/Serializer.cs +++ b/src/NosCore.Packets/Serializer.cs @@ -80,17 +80,22 @@ 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) { var replace = Expression.Call(exp, typeof(string).GetMethod("Replace", new[] { typeof(string), typeof(string) })!, - Expression.Convert(splitter, typeof(string)), + Expression.Convert(escapeSeparator, typeof(string)), Expression.Constant("^", typeof(string)) ); 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( @@ -195,7 +200,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,6 +245,12 @@ 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, @@ -247,15 +258,12 @@ private Expression PacketSerializer(Expression injectedPacket, PacketIndexAttrib 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) , 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 +309,7 @@ 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) { var useCustomSerializer = true; switch (type) @@ -328,7 +336,7 @@ 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); break; //IPacket declared type case var t when typeof(IPacket).IsAssignableFrom(t) && (t != typeof(IPacket)): diff --git a/test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs b/test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs new file mode 100644 index 00000000..137a3542 --- /dev/null +++ b/test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs @@ -0,0 +1,102 @@ +// __ _ __ __ ___ __ ___ ___ +// | \| |/__\ /' _/ / _//__\| _ \ __| +// | | ' | \/ |`._`.| \_| \/ | 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.Visibility; +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) + }); + + [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")); + } + } +} From 9ad6e13f3aec1ad92e256cf1b96513a4657ab5cb Mon Sep 17 00:00:00 2001 From: erwan-joly Date: Sun, 30 Aug 2026 11:35:23 +1200 Subject: [PATCH 2/3] fix(serializer): escape nested strings and let a field opt in Two gaps left by escaping against the field separator alone. A string inside a sub-packet only escaped its own separator. Its value sits in a field of the packet above it, so a space still split that outer field: pinit 0 0|0|1|10|Joyeux Mouton|0|0|0|0|0|0 Nested strings now escape the outer separator as well. The last field of a packet keeps its spaces, which is what chat lines need, but a few fields carry text the client expects "^"-encoded wherever they sit - mlintro, the miniland message on mlinfobr, the family message on ginfo. They declare EscapeSpaces rather than every caller remembering to substitute. 122 tests pass. --- .../Attributes/PacketIndexAttribute.cs | 7 ++++ src/NosCore.Packets/Serializer.cs | 32 +++++++++++++----- .../ServerPackets/Families/GInfoPacket.cs | 2 +- .../ServerPackets/Miniland/MlInfoBrPacket.cs | 2 +- .../ServerPackets/Miniland/MlintroPacket.cs | 2 +- .../StringFieldSeparatorTests.cs | 33 ++++++++++++++++++- 6 files changed, 65 insertions(+), 13 deletions(-) diff --git a/src/NosCore.Packets/Attributes/PacketIndexAttribute.cs b/src/NosCore.Packets/Attributes/PacketIndexAttribute.cs index 75d28b0b..5d375259 100644 --- a/src/NosCore.Packets/Attributes/PacketIndexAttribute.cs +++ b/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; } + + /// + /// 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. + /// + public bool EscapeSpaces { get; set; } } } \ No newline at end of file diff --git a/src/NosCore.Packets/Serializer.cs b/src/NosCore.Packets/Serializer.cs index 03214869..1c443d6e 100644 --- a/src/NosCore.Packets/Serializer.cs +++ b/src/NosCore.Packets/Serializer.cs @@ -85,13 +85,24 @@ private Expression DefaultSerializer(Expression specificTypeExpression, Expressi // 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) + Expression escapeSeparator, bool isNested, bool escapeSpaces) { - var replace = Expression.Call(exp, - typeof(string).GetMethod("Replace", new[] { typeof(string), 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)) - ); + 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) })!, @@ -104,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)))) ); } @@ -261,7 +272,8 @@ private Expression PacketSerializer(Expression injectedPacket, PacketIndexAttrib Expression.Constant(index.SpecialSeparator, typeof(object))), index.SpecialSeparator != null ? Expression.Constant(index.SpecialSeparator, typeof(object)) - : splitter) + : splitter, + isFromList) , typeof(object)); var trimfirst = Expression.Condition( @@ -309,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 escapeSeparator) + Expression specificTypeExpression, int maxIndex, Expression propertySplitter, Expression escapeSeparator, + bool isNested = false) { var useCustomSerializer = true; switch (type) @@ -336,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, escapeSeparator); + indexAttr.IsOptional, propertySplitter, escapeSeparator, isNested, + indexAttr.EscapeSpaces); break; //IPacket declared type case var t when typeof(IPacket).IsAssignableFrom(t) && (t != typeof(IPacket)): diff --git a/src/NosCore.Packets/ServerPackets/Families/GInfoPacket.cs b/src/NosCore.Packets/ServerPackets/Families/GInfoPacket.cs index cdeea661..bf4e4672 100644 --- a/src/NosCore.Packets/ServerPackets/Families/GInfoPacket.cs +++ b/src/NosCore.Packets/ServerPackets/Families/GInfoPacket.cs @@ -66,7 +66,7 @@ public class GInfoPacket : PacketBase /// /// Should replace ' ' by '^' /// - [PacketIndex(16)] + [PacketIndex(16, EscapeSpaces = true)] public string? FamilyMessage { get; set; } } } \ No newline at end of file diff --git a/src/NosCore.Packets/ServerPackets/Miniland/MlInfoBrPacket.cs b/src/NosCore.Packets/ServerPackets/Miniland/MlInfoBrPacket.cs index 32cf1f35..2090f305 100644 --- a/src/NosCore.Packets/ServerPackets/Miniland/MlInfoBrPacket.cs +++ b/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; } } } \ No newline at end of file diff --git a/src/NosCore.Packets/ServerPackets/Miniland/MlintroPacket.cs b/src/NosCore.Packets/ServerPackets/Miniland/MlintroPacket.cs index b9df9436..41ac58b3 100644 --- a/src/NosCore.Packets/ServerPackets/Miniland/MlintroPacket.cs +++ b/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; } } } \ No newline at end of file diff --git a/test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs b/test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs index 137a3542..6cc59bf6 100644 --- a/test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs +++ b/test/NosCore.Packets.Tests/StringFieldSeparatorTests.cs @@ -9,7 +9,10 @@ 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 @@ -41,7 +44,8 @@ public class StringFieldSeparatorTests new Serializer(new[] { typeof(SeparatorProbePacket), typeof(DottedProbePacket), - typeof(ScpPacket), typeof(ScnPacket), typeof(InPacket) + typeof(ScpPacket), typeof(ScnPacket), typeof(InPacket), + typeof(PinitPacket), typeof(MlintroPacket), typeof(MlInfoBrPacket) }); [TestMethod] @@ -98,5 +102,32 @@ public void TheSameHoldsForTheOtherPacketsThatCarryAName() 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 + { + 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"); + } } } From 460a57e8693698a6365bf01b317da8032ce339fe Mon Sep 17 00:00:00 2001 From: erwan-joly Date: Sun, 30 Aug 2026 11:45:59 +1200 Subject: [PATCH 3/3] chore: 21.1.0 --- src/NosCore.Packets/NosCore.Packets.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/NosCore.Packets/NosCore.Packets.csproj b/src/NosCore.Packets/NosCore.Packets.csproj index c62d0442..2c30ec12 100644 --- a/src/NosCore.Packets/NosCore.Packets.csproj +++ b/src/NosCore.Packets/NosCore.Packets.csproj @@ -12,7 +12,7 @@ https://github.com/NosCoreIO/NosCore.Packets.git nostale, noscore, chickenapi, nostale private server source, nostale emulator - 21.0.0 + 21.1.0 false NosCore's Packets (Nostale packets) defined over classes MIT