From 6a60125d805bcecba5d18f26902c37da6103366b Mon Sep 17 00:00:00 2001 From: Khalil Date: Sun, 20 Sep 2026 09:07:35 +0200 Subject: [PATCH 1/3] fix(exchange): confirm/cancel packets are dropped once a trade is open RequestExchangeType.Confirmed/Cancelled are only ever sent while InExchange is already true, but ExchangeRequestPacket's declared scope is InGame only (missing InTrade), so WorldPacketHandlingStrategy's generic scope gate silently rejected every confirm and cancel before the handler ran. Special-case those two request types past the gate. Separately, ExchangeRequestPacketHandler's Confirmed case had its success/failure branches inverted: a validated exchange (Success, null packet dict) took the branch that dereferences the packet dict, throwing on every legitimately successful confirm. Co-Authored-By: Claude Sonnet 5 --- .../Networking/ClientSession/WorldPacketHandlingStrategy.cs | 6 +++++- .../Exchange/ExchangeRequestPacketHandler.cs | 2 +- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/NosCore.GameObject/Networking/ClientSession/WorldPacketHandlingStrategy.cs b/src/NosCore.GameObject/Networking/ClientSession/WorldPacketHandlingStrategy.cs index 6a841b84b..43bec716e 100644 --- a/src/NosCore.GameObject/Networking/ClientSession/WorldPacketHandlingStrategy.cs +++ b/src/NosCore.GameObject/Networking/ClientSession/WorldPacketHandlingStrategy.cs @@ -9,6 +9,7 @@ using NosCore.GameObject.Infastructure; using NosCore.Networking.SessionRef; using NosCore.Packets.Attributes; +using NosCore.Packets.ClientPackets.Exchanges; using NosCore.Packets.ClientPackets.Infrastructure; using NosCore.Packets.ClientPackets.UI; using NosCore.Packets.Enumerations; @@ -153,7 +154,10 @@ private bool ValidateScope(IPacket packet, ClientSession session, PacketHeaderAt return true; } - if (session.HasSelectedCharacter && (attr.Scopes & Scope.InTrade) == 0 && session.Character.InExchangeOrShop) + var isExchangeLifecycleAction = packet is ExchangeRequestPacket + { RequestType: RequestExchangeType.Confirmed or RequestExchangeType.Cancelled }; + if (session.HasSelectedCharacter && (attr.Scopes & Scope.InTrade) == 0 && session.Character.InExchangeOrShop + && !isExchangeLifecycleAction) { logger.LogWarning(logLanguage[LogLanguageKey.PLAYER_IN_SHOP], packet.Header); return false; diff --git a/src/NosCore.PacketHandlers/Exchange/ExchangeRequestPacketHandler.cs b/src/NosCore.PacketHandlers/Exchange/ExchangeRequestPacketHandler.cs index d57d0198f..e5c2961b8 100644 --- a/src/NosCore.PacketHandlers/Exchange/ExchangeRequestPacketHandler.cs +++ b/src/NosCore.PacketHandlers/Exchange/ExchangeRequestPacketHandler.cs @@ -184,7 +184,7 @@ await clientSession.SendPacketAsync(new InfoiPacket var success = exchangeService.ValidateExchange(clientSession, exchangeTarget); - if (success.Item1 == ExchangeResultType.Success) + if (success.Item1 == ExchangeResultType.Failure) { foreach (var infoPacket in success.Item2!) { From 56fbb81c1d2c821ebd84a8c89647667707f75364 Mon Sep 17 00:00:00 2001 From: Khalil Date: Sun, 20 Sep 2026 09:53:58 +0200 Subject: [PATCH 2/3] fix(exchange): item offers force-close trades and merges misreport the receiver's amount ExcListPacket.SubPackets deserializes with a minimum of 3 default-valued fillers regardless of how many real sub-packets were sent, because the trailing list has no wire count prefix. Amount has a declared [Range(1, ...)], so a filler (Amount == 0) can never be a genuine offer - same phantom-entry shape as QstlistPacket/FinitPacket, filtered in ExcListPacketHandler the same way those are filtered client-side. Separately, ExchangeRequestPacketHandler's Requested case had no guard against a player targeting their own VisualId, letting a self-invite be sent, accepted, and opened. And ExchangeService.ProcessExchange built the receiving side's inventory notification from the sender's own item object instead of the item actually added to the receiver's inventory. Partial transfers mutate the sender's item in place when removing the offered amount, so the receiver was notified with the sender's post-removal remaining balance rather than their own real (possibly merged) total. The persisted inventory was always correct; only the notification packet was wrong. Co-Authored-By: Claude Sonnet 5 --- .../ExchangeService/ExchangeService.cs | 2 +- .../Exchange/ExcListPacketHandler.cs | 13 +++++- .../Exchange/ExchangeRequestPacketHandler.cs | 2 +- .../ExchangeService/ExchangeServiceTests.cs | 27 +++++++++++ .../Exchange/ExcListPacketHandlerTests.cs | 45 +++++++++++++++++++ .../ExchangeRequestPacketHandlerTests.cs | 35 +++++++++++++++ 6 files changed, 120 insertions(+), 4 deletions(-) diff --git a/src/NosCore.GameObject/Services/ExchangeService/ExchangeService.cs b/src/NosCore.GameObject/Services/ExchangeService/ExchangeService.cs index 7aceeb252..ff2a08691 100644 --- a/src/NosCore.GameObject/Services/ExchangeService/ExchangeService.cs +++ b/src/NosCore.GameObject/Services/ExchangeService/ExchangeService.cs @@ -333,7 +333,7 @@ bool IsFullTransfer items.Add(new KeyValuePair(transfer.SessionId, (sourceItem ?? transfer.OriginalItem).GeneratePocketChange((PocketType)transfer.OriginalItem.Type, transfer.OriginalItem.Slot))); items.Add(new KeyValuePair(transfer.TargetId, - transfer.OriginalItem.GeneratePocketChange((PocketType)addedItem.Item.Type, addedItem.Item.Slot))); + addedItem.Item.GeneratePocketChange((PocketType)addedItem.Item.Type, addedItem.Item.Slot))); } return items; diff --git a/src/NosCore.PacketHandlers/Exchange/ExcListPacketHandler.cs b/src/NosCore.PacketHandlers/Exchange/ExcListPacketHandler.cs index 881bb64e6..67a3cfe7d 100644 --- a/src/NosCore.PacketHandlers/Exchange/ExcListPacketHandler.cs +++ b/src/NosCore.PacketHandlers/Exchange/ExcListPacketHandler.cs @@ -17,6 +17,7 @@ using NosCore.Shared.I18N; using Microsoft.Extensions.Logging; using System.Collections.Generic; +using System.Linq; using System.Threading.Tasks; namespace NosCore.PacketHandlers.Exchange @@ -39,10 +40,18 @@ public override async Task ExecuteAsync(ExcListPacket packet, ClientSession clie (s.VisualId == exchangeService.GetTargetId(clientSession.Character.VisualId)) && (s.MapInstanceId == clientSession.Character.MapInstanceId), out var target); - if ((packet.SubPackets!.Count > 0) && hasTarget) + // ExcListPacket.SubPackets is a trailing list with no explicit wire count prefix; the + // deserializer pads it to a minimum of 3 entries with default(ExcListSubPacket) fillers + // regardless of how many real sub-packets were sent. Amount has a declared [Range(1, ...)], + // so a filler (Amount == 0) can never be a genuine offer - same phantom-entry shape as + // QstlistPacket/FinitPacket, filtered the same way. + var realSubPackets = packet.SubPackets?.Where(value => value is { Amount: >= 1 }).ToList() + ?? new List(); + + if ((realSubPackets.Count > 0) && hasTarget) { byte i = 0; - foreach (var value in packet.SubPackets) + foreach (var value in realSubPackets) { var item = clientSession.Character.InventoryService.LoadBySlotAndType(value!.Slot, (NoscorePocketType)value.PocketType); diff --git a/src/NosCore.PacketHandlers/Exchange/ExchangeRequestPacketHandler.cs b/src/NosCore.PacketHandlers/Exchange/ExchangeRequestPacketHandler.cs index e5c2961b8..7225188c5 100644 --- a/src/NosCore.PacketHandlers/Exchange/ExchangeRequestPacketHandler.cs +++ b/src/NosCore.PacketHandlers/Exchange/ExchangeRequestPacketHandler.cs @@ -67,7 +67,7 @@ await clientSession.SendPacketAsync(new Infoi2Packet return; } - if (!hasTarget || target.ExchangeBlocked) + if (!hasTarget || target.ExchangeBlocked || (target.VisualId == clientSession.Character.VisualId)) { await clientSession.SendPacketAsync(new Infoi2Packet { diff --git a/test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs b/test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs index b8ecb7d69..676648738 100644 --- a/test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs +++ b/test/NosCore.GameObject.Tests/Services/ExchangeService/ExchangeServiceTests.cs @@ -19,6 +19,7 @@ using NosCore.Packets.ClientPackets.Inventory; using NosCore.Packets.Enumerations; using NosCore.Packets.Interfaces; +using NosCore.Packets.ServerPackets.Inventory; using NosCore.Packets.ServerPackets.UI; using NosCore.Tests.Shared; using Microsoft.Extensions.Logging.Abstractions; @@ -359,5 +360,31 @@ public void AnOfferedItemThatLeftTheInventoryDoesNotReachTheOtherSide() Assert.AreEqual(0, itemList.Count, "a vanished offer must not transfer"); Assert.AreEqual(0, receiver.Count, "the receiver must not gain a duplicate"); } + + [TestMethod] + public void ProcessExchangeNotifiesReceiverWithTheirActualMergedAmount() + { + IInventoryService giver = + new GameObject.Services.InventoryService.InventoryService(new List { new Item { VNum = 1012, Type = NoscorePocketType.Main } }, + WorldConfiguration!, NullLogger.Instance); + IInventoryService receiver = + new GameObject.Services.InventoryService.InventoryService(new List { new Item { VNum = 1012, Type = NoscorePocketType.Main } }, + WorldConfiguration!, NullLogger.Instance); + + var offered = giver.AddItemToPocket(InventoryItemInstance.Create(ItemProvider!.Create(1012, 5), 0))!.First(); + receiver.AddItemToPocket(InventoryItemInstance.Create(ItemProvider.Create(1012, 4), 0)); + + ExchangeProvider!.OpenExchange(1, 2); + // Offering less than the full stack (2 of 5) forces the partial-transfer branch, + // which mutates the giver's own item object in place - this used to leak into the + // receiver's notification packet. + ExchangeProvider.AddItems(1, offered, 2); + + var itemList = ExchangeProvider.ProcessExchange(1, 2, giver, receiver); + + var receiverPacket = itemList.First(s => s.Key == 2).Value; + var receiverAmount = receiverPacket.IvnSubPackets!.First()!.RareAmount; + Assert.AreEqual((short)6, receiverAmount, "the receiver must be notified of their true merged total (4 + 2), not the giver's remaining balance"); + } } } diff --git a/test/NosCore.PacketHandlers.Tests/Exchange/ExcListPacketHandlerTests.cs b/test/NosCore.PacketHandlers.Tests/Exchange/ExcListPacketHandlerTests.cs index bf436eae1..0376559d6 100644 --- a/test/NosCore.PacketHandlers.Tests/Exchange/ExcListPacketHandlerTests.cs +++ b/test/NosCore.PacketHandlers.Tests/Exchange/ExcListPacketHandlerTests.cs @@ -104,6 +104,28 @@ public async Task SettingValidGoldShouldUpdateExchange() .ExecuteAsync(); } + [TestMethod] + public async Task PhantomZeroAmountSubPacketsShouldNotCloseGoldOnlyOffer() + { + await new Spec("Phantom zero-amount sub-packets should not close a gold-only offer") + .Given(CharacterHasGold) + .And(ExchangeIsOpenWithTarget) + .WhenAsync(SettingGoldWithPhantomSubPackets) + .Then(ExchangeShouldNotBeClosed) + .ExecuteAsync(); + } + + [TestMethod] + public async Task PhantomZeroAmountSubPacketsShouldNotPreventGoldFromBeingSet() + { + await new Spec("Phantom zero-amount sub-packets should not prevent gold from being set") + .Given(CharacterHasGold) + .And(ExchangeIsOpenWithTarget) + .WhenAsync(SettingGoldWithPhantomSubPackets) + .Then(ExchangeGoldShouldBeSet) + .ExecuteAsync(); + } + private void CharacterHasNoGold() { Session.Character.Gold = 0; @@ -179,11 +201,34 @@ await Handler.ExecuteAsync(new ExcListPacket }, Session); } + private async Task SettingGoldWithPhantomSubPackets() + { + // Shape observed on the real wire: a gold-only offer still deserializes with a + // minimum of 3 default-valued sub-packets (Amount == 0), which must be filtered + // out rather than treated as real offered items. + await Handler.ExecuteAsync(new ExcListPacket + { + Gold = 5000, + BankGold = 0, + SubPackets = new List + { + new() { Slot = 0, PocketType = PocketType.Equipment, Amount = 0 }, + new() { Slot = 0, PocketType = PocketType.Equipment, Amount = 0 }, + new() { Slot = 0, PocketType = PocketType.Equipment, Amount = 0 } + } + }, Session); + } + private void ExchangeGoldShouldNotBeSet() { ExchangeService.Verify(x => x.SetGold(It.IsAny(), It.IsAny(), It.IsAny()), Times.Never); } + private void ExchangeShouldNotBeClosed() + { + ExchangeService.Verify(x => x.CloseExchange(It.IsAny(), It.IsAny()), Times.Never); + } + private void ExchangeGoldShouldBeSet() { var characterId = Session.Character.CharacterId; diff --git a/test/NosCore.PacketHandlers.Tests/Exchange/ExchangeRequestPacketHandlerTests.cs b/test/NosCore.PacketHandlers.Tests/Exchange/ExchangeRequestPacketHandlerTests.cs index b749483af..8f80a00c1 100644 --- a/test/NosCore.PacketHandlers.Tests/Exchange/ExchangeRequestPacketHandlerTests.cs +++ b/test/NosCore.PacketHandlers.Tests/Exchange/ExchangeRequestPacketHandlerTests.cs @@ -86,6 +86,26 @@ public async Task RequestingExchangeWhenTargetBlocksExchangeShouldFail() .ExecuteAsync(); } + [TestMethod] + public async Task RequestingExchangeWithSelfShouldFail() + { + await new Spec("Requesting exchange with self should fail") + .Given(TargetIsSelf) + .WhenAsync(RequestingExchangeWithSelf) + .Then(ShouldReceiveBlockingTradesMessage) + .ExecuteAsync(); + } + + [TestMethod] + public async Task RequestingExchangeWithSelfShouldNotOpenExchange() + { + await new Spec("Requesting exchange with self should not open an exchange") + .Given(TargetIsSelf) + .WhenAsync(RequestingExchangeWithSelf) + .Then(NoExchangeShouldBeOpened) + .ExecuteAsync(); + } + [TestMethod] public async Task RequestingExchangeWithBlacklistedPlayerShouldFail() { @@ -128,6 +148,12 @@ private void TargetBlocksExchange() ExchangeService.Setup(x => x.CheckExchange(It.IsAny())).Returns(false); } + private void TargetIsSelf() + { + Session.Character.MapInstance = TestHelpers.Instance.MapInstanceAccessorService.GetBaseMapById(1)!; + ExchangeService.Setup(x => x.CheckExchange(It.IsAny())).Returns(false); + } + private void TargetIsBlacklisted() { Session.Character.MapInstance = TestHelpers.Instance.MapInstanceAccessorService.GetBaseMapById(1)!; @@ -161,6 +187,15 @@ await Handler.ExecuteAsync(new ExchangeRequestPacket }, Session); } + private async Task RequestingExchangeWithSelf() + { + await Handler.ExecuteAsync(new ExchangeRequestPacket + { + RequestType = RequestExchangeType.Requested, + VisualId = Session.Character.VisualId + }, Session); + } + private async Task CancellingExchange() { await Handler.ExecuteAsync(new ExchangeRequestPacket From a8e8876de0570899125d3d17c1e5cd179b210fbe Mon Sep 17 00:00:00 2001 From: erwan-joly Date: Tue, 13 Oct 2026 19:25:28 +1300 Subject: [PATCH 3/3] fix(exchange): rely on NosCore.Packets 21.1.3 for the req_exc scope and exc_list parsing 21.1.3 lets req_exc through while a trade is open and parses exc_list as one item per offered sub-packet, so the scope-gate exception and the zero-amount filter come out. A gold-only offer arrives with no sub-packet list at all, which the null check now covers instead of throwing. Co-Authored-By: Khalil Co-Authored-By: Claude Opus 5.5 --- Directory.Packages.props | 2 +- .../WorldPacketHandlingStrategy.cs | 6 +---- .../Exchange/ExcListPacketHandler.cs | 13 ++-------- .../Exchange/ExcListPacketHandlerTests.cs | 24 +++++++------------ 4 files changed, 12 insertions(+), 33 deletions(-) diff --git a/Directory.Packages.props b/Directory.Packages.props index f718476a5..2e3b14f1e 100644 --- a/Directory.Packages.props +++ b/Directory.Packages.props @@ -43,7 +43,7 @@ - + diff --git a/src/NosCore.GameObject/Networking/ClientSession/WorldPacketHandlingStrategy.cs b/src/NosCore.GameObject/Networking/ClientSession/WorldPacketHandlingStrategy.cs index 43bec716e..6a841b84b 100644 --- a/src/NosCore.GameObject/Networking/ClientSession/WorldPacketHandlingStrategy.cs +++ b/src/NosCore.GameObject/Networking/ClientSession/WorldPacketHandlingStrategy.cs @@ -9,7 +9,6 @@ using NosCore.GameObject.Infastructure; using NosCore.Networking.SessionRef; using NosCore.Packets.Attributes; -using NosCore.Packets.ClientPackets.Exchanges; using NosCore.Packets.ClientPackets.Infrastructure; using NosCore.Packets.ClientPackets.UI; using NosCore.Packets.Enumerations; @@ -154,10 +153,7 @@ private bool ValidateScope(IPacket packet, ClientSession session, PacketHeaderAt return true; } - var isExchangeLifecycleAction = packet is ExchangeRequestPacket - { RequestType: RequestExchangeType.Confirmed or RequestExchangeType.Cancelled }; - if (session.HasSelectedCharacter && (attr.Scopes & Scope.InTrade) == 0 && session.Character.InExchangeOrShop - && !isExchangeLifecycleAction) + if (session.HasSelectedCharacter && (attr.Scopes & Scope.InTrade) == 0 && session.Character.InExchangeOrShop) { logger.LogWarning(logLanguage[LogLanguageKey.PLAYER_IN_SHOP], packet.Header); return false; diff --git a/src/NosCore.PacketHandlers/Exchange/ExcListPacketHandler.cs b/src/NosCore.PacketHandlers/Exchange/ExcListPacketHandler.cs index 67a3cfe7d..abbd0b57b 100644 --- a/src/NosCore.PacketHandlers/Exchange/ExcListPacketHandler.cs +++ b/src/NosCore.PacketHandlers/Exchange/ExcListPacketHandler.cs @@ -17,7 +17,6 @@ using NosCore.Shared.I18N; using Microsoft.Extensions.Logging; using System.Collections.Generic; -using System.Linq; using System.Threading.Tasks; namespace NosCore.PacketHandlers.Exchange @@ -40,18 +39,10 @@ public override async Task ExecuteAsync(ExcListPacket packet, ClientSession clie (s.VisualId == exchangeService.GetTargetId(clientSession.Character.VisualId)) && (s.MapInstanceId == clientSession.Character.MapInstanceId), out var target); - // ExcListPacket.SubPackets is a trailing list with no explicit wire count prefix; the - // deserializer pads it to a minimum of 3 entries with default(ExcListSubPacket) fillers - // regardless of how many real sub-packets were sent. Amount has a declared [Range(1, ...)], - // so a filler (Amount == 0) can never be a genuine offer - same phantom-entry shape as - // QstlistPacket/FinitPacket, filtered the same way. - var realSubPackets = packet.SubPackets?.Where(value => value is { Amount: >= 1 }).ToList() - ?? new List(); - - if ((realSubPackets.Count > 0) && hasTarget) + if ((packet.SubPackets?.Count > 0) && hasTarget) { byte i = 0; - foreach (var value in realSubPackets) + foreach (var value in packet.SubPackets) { var item = clientSession.Character.InventoryService.LoadBySlotAndType(value!.Slot, (NoscorePocketType)value.PocketType); diff --git a/test/NosCore.PacketHandlers.Tests/Exchange/ExcListPacketHandlerTests.cs b/test/NosCore.PacketHandlers.Tests/Exchange/ExcListPacketHandlerTests.cs index 0376559d6..b4582f9ae 100644 --- a/test/NosCore.PacketHandlers.Tests/Exchange/ExcListPacketHandlerTests.cs +++ b/test/NosCore.PacketHandlers.Tests/Exchange/ExcListPacketHandlerTests.cs @@ -105,23 +105,23 @@ public async Task SettingValidGoldShouldUpdateExchange() } [TestMethod] - public async Task PhantomZeroAmountSubPacketsShouldNotCloseGoldOnlyOffer() + public async Task GoldOnlyOfferShouldNotCloseExchange() { - await new Spec("Phantom zero-amount sub-packets should not close a gold-only offer") + await new Spec("Gold only offer should not close exchange") .Given(CharacterHasGold) .And(ExchangeIsOpenWithTarget) - .WhenAsync(SettingGoldWithPhantomSubPackets) + .WhenAsync(OfferingOnlyGold) .Then(ExchangeShouldNotBeClosed) .ExecuteAsync(); } [TestMethod] - public async Task PhantomZeroAmountSubPacketsShouldNotPreventGoldFromBeingSet() + public async Task GoldOnlyOfferShouldSetGold() { - await new Spec("Phantom zero-amount sub-packets should not prevent gold from being set") + await new Spec("Gold only offer should set gold") .Given(CharacterHasGold) .And(ExchangeIsOpenWithTarget) - .WhenAsync(SettingGoldWithPhantomSubPackets) + .WhenAsync(OfferingOnlyGold) .Then(ExchangeGoldShouldBeSet) .ExecuteAsync(); } @@ -201,21 +201,13 @@ await Handler.ExecuteAsync(new ExcListPacket }, Session); } - private async Task SettingGoldWithPhantomSubPackets() + private async Task OfferingOnlyGold() { - // Shape observed on the real wire: a gold-only offer still deserializes with a - // minimum of 3 default-valued sub-packets (Amount == 0), which must be filtered - // out rather than treated as real offered items. await Handler.ExecuteAsync(new ExcListPacket { Gold = 5000, BankGold = 0, - SubPackets = new List - { - new() { Slot = 0, PocketType = PocketType.Equipment, Amount = 0 }, - new() { Slot = 0, PocketType = PocketType.Equipment, Amount = 0 }, - new() { Slot = 0, PocketType = PocketType.Equipment, Amount = 0 } - } + SubPackets = null }, Session); }