From 16d48a4a56680bc9672f9dbf160032304114bfe0 Mon Sep 17 00:00:00 2001 From: James Puleo Date: Sat, 26 Mar 2022 16:44:54 -0400 Subject: [PATCH 1/3] Fixed `HandlePlayerAddBuff` data handler always being marked as Handled This would cause all `PlayerAddBuff` packets to always be rejected, causing desync, and general annoyance, as it meant any PvP items that applied buffs never worked. --- TShockAPI/GetDataHandlers.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/TShockAPI/GetDataHandlers.cs b/TShockAPI/GetDataHandlers.cs index 4101e5d4..0770eda0 100644 --- a/TShockAPI/GetDataHandlers.cs +++ b/TShockAPI/GetDataHandlers.cs @@ -3466,8 +3466,7 @@ namespace TShockAPI if (OnPlayerBuff(args.Player, args.Data, id, type, time)) return true; - args.Player.SendData(PacketTypes.PlayerAddBuff, "", id); - return true; + return false; } private static bool HandleUpdateNPCHome(GetDataHandlerArgs args) From 23fd7acd79f023a7c71c47d9798fd4bd2c62ff1c Mon Sep 17 00:00:00 2001 From: James Puleo Date: Sat, 26 Mar 2022 16:48:58 -0400 Subject: [PATCH 2/3] Improve `OnPlayerBuff` logic to properly handle buffs from other players Previously, we checked if the target player was null, before checking if their ID was out of bounds, so the check was moved to be first. We now check if the buff being applied is within bounds. We introduce `AddPlayerBuffWhitelist` to replace `WhitelistBuffMaxTime`, which allows us to specify a maximum amount of ticks a buff can be applied to another player for, and if it can be applied without the target being in PvP. If a buff is not within this array, it is *not* allowed to be applied by other players. When rejecting from `OnPlayerBuff`, we send a `PlayerBuff` instead of `PlayerAddBuff`, to sync the current buffs of the target, without syncing the rejected one. --- TShockAPI/Bouncer.cs | 274 +++++++++++++++++++++++++++++++---- TShockAPI/GetDataHandlers.cs | 12 -- 2 files changed, 244 insertions(+), 42 deletions(-) diff --git a/TShockAPI/Bouncer.cs b/TShockAPI/Bouncer.cs index 469b6ee9..efe21554 100644 --- a/TShockAPI/Bouncer.cs +++ b/TShockAPI/Bouncer.cs @@ -44,6 +44,27 @@ namespace TShockAPI internal Handlers.LandGolfBallInCupHandler LandGolfBallInCupHandler { get; private set; } internal Handlers.SyncTilePickingHandler SyncTilePickingHandler { get; private set; } + /// + /// A class that represents the limits for a particular buff when a client applies it with PlayerAddBuff. + /// + internal class BuffLimit + { + /// + /// How many ticks at the maximum a player can apply this to another player for. + /// + public int MaxTicks { get; set; } + /// + /// Can this buff be added without the receiver being hostile (PvP) + /// + public bool CanBeAddedWithoutHostile { get; set; } + /// + /// Can this buff only be applied to the sender? + /// + public bool CanOnlyBeAppliedToSender { get; set; } + } + + internal static BuffLimit[] PlayerAddBuffWhitelist; + /// /// Represents a place style corrector. /// @@ -231,6 +252,172 @@ namespace TShockAPI return actualItemPlaceStyle; } }); + + #region PlayerAddBuff Whitelist + + PlayerAddBuffWhitelist = new BuffLimit[Main.maxBuffTypes]; + PlayerAddBuffWhitelist[BuffID.Poisoned] = new BuffLimit + { + MaxTicks = 60 * 60 + }; + PlayerAddBuffWhitelist[BuffID.OnFire] = new BuffLimit + { + MaxTicks = 60 * 20 + }; + PlayerAddBuffWhitelist[BuffID.Confused] = new BuffLimit + { + MaxTicks = 60 * 4 + }; + PlayerAddBuffWhitelist[BuffID.CursedInferno] = new BuffLimit + { + MaxTicks = 60 * 7 + }; + PlayerAddBuffWhitelist[BuffID.Wet] = new BuffLimit + { + MaxTicks = 60 * 30, + // The Water Gun can be shot at other players and inflict Wet while not in PvP + CanBeAddedWithoutHostile = true + }; + PlayerAddBuffWhitelist[BuffID.Ichor] = new BuffLimit + { + MaxTicks = 60 * 20 + }; + PlayerAddBuffWhitelist[BuffID.Venom] = new BuffLimit + { + MaxTicks = 60 * 30 + }; + PlayerAddBuffWhitelist[BuffID.GelBalloonBuff] = new BuffLimit + { + MaxTicks = 60 * 30, + // The Sparkle Slime Balloon inflicts this while not in PvP + CanBeAddedWithoutHostile = true + }; + PlayerAddBuffWhitelist[BuffID.Frostburn] = new BuffLimit + { + MaxTicks = 60 * 8 + }; + PlayerAddBuffWhitelist[BuffID.Campfire] = new BuffLimit + { + MaxTicks = 2, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true, + }; + PlayerAddBuffWhitelist[BuffID.Sunflower] = new BuffLimit + { + MaxTicks = 2, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true, + }; + PlayerAddBuffWhitelist[BuffID.WaterCandle] = new BuffLimit + { + MaxTicks = 2, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true, + }; + PlayerAddBuffWhitelist[BuffID.BeetleEndurance1] = new BuffLimit + { + MaxTicks = 5, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true, + }; + PlayerAddBuffWhitelist[BuffID.BeetleEndurance2] = new BuffLimit + { + MaxTicks = 5, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true, + }; + PlayerAddBuffWhitelist[BuffID.BeetleEndurance3] = new BuffLimit + { + MaxTicks = 5, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true, + }; + PlayerAddBuffWhitelist[BuffID.BeetleMight1] = new BuffLimit + { + MaxTicks = 5, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true, + }; + PlayerAddBuffWhitelist[BuffID.BeetleMight2] = new BuffLimit + { + MaxTicks = 5, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true, + }; + PlayerAddBuffWhitelist[BuffID.BeetleMight3] = new BuffLimit + { + MaxTicks = 5, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true, + }; + PlayerAddBuffWhitelist[BuffID.SolarShield1] = new BuffLimit + { + MaxTicks = 5, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = false, + }; + PlayerAddBuffWhitelist[BuffID.SolarShield2] = new BuffLimit + { + MaxTicks = 5, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = false, + }; + PlayerAddBuffWhitelist[BuffID.SolarShield3] = new BuffLimit + { + MaxTicks = 5, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = false, + }; + PlayerAddBuffWhitelist[BuffID.MonsterBanner] = new BuffLimit + { + MaxTicks = 2, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true + }; + PlayerAddBuffWhitelist[BuffID.HeartLamp] = new BuffLimit + { + MaxTicks = 2, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true + }; + PlayerAddBuffWhitelist[BuffID.PeaceCandle] = new BuffLimit + { + MaxTicks = 2, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true + }; + PlayerAddBuffWhitelist[BuffID.StarInBottle] = new BuffLimit + { + MaxTicks = 2, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true + }; + PlayerAddBuffWhitelist[BuffID.CatBast] = new BuffLimit + { + MaxTicks = 2, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true + }; + PlayerAddBuffWhitelist[BuffID.OnFire3] = new BuffLimit + { + MaxTicks = 60 * 5, + CanBeAddedWithoutHostile = false, + CanOnlyBeAppliedToSender = false + }; + PlayerAddBuffWhitelist[BuffID.HeartyMeal] = new BuffLimit + { + MaxTicks = 60 * 7, + CanBeAddedWithoutHostile = true, + CanOnlyBeAppliedToSender = true + }; + PlayerAddBuffWhitelist[BuffID.Frostburn2] = new BuffLimit + { + MaxTicks = 60 * 7, + CanBeAddedWithoutHostile = false, + CanOnlyBeAppliedToSender = false + }; + + #endregion Whitelist } internal void OnGetSection(object sender, GetDataHandlers.GetSectionEventArgs args) @@ -1640,9 +1827,24 @@ namespace TShockAPI int type = args.Type; int time = args.Time; + if (id >= Main.maxPlayers) + { + TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected player cap from {0}", args.Player.Name); + args.Handled = true; + return; + } + if (TShock.Players[id] == null) { - TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected null check"); + TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected null check from {0}", args.Player.Name); + args.Handled = true; + return; + } + + if (type >= Main.maxBuffTypes) + { + TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected invalid buff type {0}", args.Player.Name); + args.Player.SendData(PacketTypes.PlayerBuff, "", id); args.Handled = true; return; } @@ -1650,31 +1852,7 @@ namespace TShockAPI if (args.Player.IsBeingDisabled()) { TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected disabled from {0}", args.Player.Name); - args.Player.SendData(PacketTypes.PlayerAddBuff, "", id); - args.Handled = true; - return; - } - - if (id >= Main.maxPlayers) - { - TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected player cap from {0}", args.Player.Name); - args.Player.SendData(PacketTypes.PlayerAddBuff, "", id); - args.Handled = true; - return; - } - - if (!TShock.Players[id].TPlayer.hostile || !Main.pvpBuff[type]) - { - TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected hostile/pvp from {0}", args.Player.Name); - args.Player.SendData(PacketTypes.PlayerAddBuff, "", id); - args.Handled = true; - return; - } - - if (!args.Player.IsInRange(TShock.Players[id].TileX, TShock.Players[id].TileY, 50)) - { - TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected range check from {0}", args.Player.Name); - args.Player.SendData(PacketTypes.PlayerAddBuff, "", id); + args.Player.SendData(PacketTypes.PlayerBuff, "", id); args.Handled = true; return; } @@ -1682,15 +1860,51 @@ namespace TShockAPI if (args.Player.IsBouncerThrottled()) { TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected throttled from {0}", args.Player.Name); - args.Player.SendData(PacketTypes.PlayerAddBuff, "", id); + args.Player.SendData(PacketTypes.PlayerBuff, "", id); args.Handled = true; return; } - if (WhitelistBuffMaxTime[type] > 0 && time <= WhitelistBuffMaxTime[type]) + var targetPlayer = TShock.Players[id]; + var buffLimit = PlayerAddBuffWhitelist[type]; + + if (!args.Player.IsInRange(targetPlayer.TileX, targetPlayer.TileY, 50)) { - TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected buff time whitelists from {0}", args.Player.Name); - args.Handled = false; + TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected range check from {0}", args.Player.Name); + args.Player.SendData(PacketTypes.PlayerBuff, "", id); + args.Handled = true; + return; + } + + if (buffLimit == null) + { + TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected non-whitelisted buff {0}", args.Player.Name); + args.Player.SendData(PacketTypes.PlayerBuff, "", id); + args.Handled = true; + return; + } + + if (buffLimit.CanOnlyBeAppliedToSender && id != args.Player.Index) + { + TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected applied to non-sender from {0}", args.Player.Name); + args.Player.SendData(PacketTypes.PlayerBuff, "", id); + args.Handled = true; + return; + } + + if (!buffLimit.CanBeAddedWithoutHostile && !targetPlayer.TPlayer.hostile) + { + TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected hostile/pvp from {0}", args.Player.Name); + args.Player.SendData(PacketTypes.PlayerBuff, "", id); + args.Handled = true; + return; + } + + if (time <= 0 || time > buffLimit.MaxTicks) + { + TShock.Log.ConsoleDebug("Bouncer / OnPlayerBuff rejected time too long from {0}", args.Player.Name); + args.Player.SendData(PacketTypes.PlayerBuff, "", id); + args.Handled = true; return; } } diff --git a/TShockAPI/GetDataHandlers.cs b/TShockAPI/GetDataHandlers.cs index 0770eda0..e1d34934 100644 --- a/TShockAPI/GetDataHandlers.cs +++ b/TShockAPI/GetDataHandlers.cs @@ -79,20 +79,8 @@ namespace TShockAPI { private static Dictionary GetDataHandlerDelegates; - public static int[] WhitelistBuffMaxTime; - public static void InitGetDataHandler() { - #region Blacklists - - WhitelistBuffMaxTime = new int[Main.maxBuffTypes]; - WhitelistBuffMaxTime[20] = 600; - WhitelistBuffMaxTime[0x18] = 1200; - WhitelistBuffMaxTime[0x1f] = 120; - WhitelistBuffMaxTime[0x27] = 420; - - #endregion Blacklists - GetDataHandlerDelegates = new Dictionary { { PacketTypes.PlayerInfo, HandlePlayerInfo }, From b34bfbd605c0ac854ffa7fd088b07a5e007fff4b Mon Sep 17 00:00:00 2001 From: James Puleo Date: Sat, 26 Mar 2022 16:54:10 -0400 Subject: [PATCH 3/3] Update `CHANGELOG.md` with `PlayerAddBuff` Bouncer changes --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index d651ce6d..abd107a5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -32,6 +32,12 @@ This is the rolling changelog for TShock for Terraria. Use past tense when addin * Fixed painting wall/tile being rejected from hand of creation. (@Rozen4334) * Added a second `Utils.TryParseTime` method for parsing large, positive time spans. (@punchready) * Fixed `/tempgroup` breaking on durations greater than roughly 24 days. (@punchready) +* Fixed `HandlePlayerAddBuff` data handler always being marked as `Handled`, and therefore never allowing the `PlayerAddBuff` to be sent to anyone. (@drunderscore) +* Improved `OnPlayerBuff` logic to properly handle players adding buffs to other players. (@drunderscore) + * Check if the target ID is within bounds as the first thing to check. + * Check if the buff type being applied is within bounds. + * Introduce `AddPlayerBuffWhitelist` (replacing `WhitelistBuffMaxTime`), which allows us to specify the maximum amount of ticks a buff can be applied for, and if it can be applied without the target being in PvP. + * When rejecting from `OnPlayerBuff`, instead of sending a `PlayerAddBuff` packet with the rejected buff (essentially a no-op, as the sender implicitly applies the buff to the target, and causes desync as the buff was rejected), send a `PlayerBuff` to re-sync the target's buffs, without the buff we just rejected. ## TShock 4.5.18 * Fixed `TSPlayer.GiveItem` not working if the player is in lava. (@gohjoseph)