Merge commit from fork

Missing ban validation in Connect Request allows TShock ban bypassing

Author: hakusaro

TShockAPI/Commands.cs

@@ -5363,7 +5363,7 @@ private static void ListConnectedPlayers(CommandArgs args)
 
 			foreach (TSPlayer ply in TShock.Players)
 			{
-				if (ply != null && ply.Active)
+				if (ply != null && ply.Active && ply.FinishedHandshake)
 				{
 					if (displayIdsRequested)
 						if (ply.Account != null)

TShockAPI/GetDataHandlers.cs

@@ -2726,6 +2726,8 @@ private static bool HandleSpawn(GetDataHandlerArgs args)
 			short numberOfDeathsPVP = args.Data.ReadInt16();
 			PlayerSpawnContext context = (PlayerSpawnContext)args.Data.ReadByte();
 
+			args.Player.FinishedHandshake = true;
+
 			if (OnPlayerSpawn(args.Player, args.Data, player, spawnx, spawny, respawnTimer, numberOfDeathsPVE, numberOfDeathsPVP, context))
 				return true;
 
@@ -2762,6 +2764,7 @@ private static bool HandleSpawn(GetDataHandlerArgs args)
 				args.Player.Dead = true;
 			else
 				args.Player.Dead = false;
+
 			return false;
 		}
 

TShockAPI/TSPlayer.cs

@@ -351,6 +351,9 @@ public int RespawnTimer
 		/// <summary>Determines if the player is disabled for not clearing their trash. A re-login is the only way to reset this.</summary>
 		public bool IsDisabledPendingTrashRemoval;
 
+		/// <summary>Determines if the player has finished the handshake (Sent all necessary packets for connection, such as Request World Data, Spawn Player, etc). A normal client would do all of this no problem.</summary>
+		public bool FinishedHandshake = false;
+
 		/// <summary>Checks to see if active throttling is happening on events by Bouncer. Rejects repeated events by malicious clients in a short window.</summary>
 		/// <returns>If the player is currently being throttled by Bouncer, or not.</returns>
 		public bool IsBouncerThrottled()

TShockAPI/TShock.cs

@@ -1376,6 +1376,8 @@ private void OnConnect(ConnectEventArgs args)
 					}
 				}
 			}
+
+			Bans.CheckBan(player);
 			Players[args.Who] = player;
 		}
 
@@ -1397,7 +1399,8 @@ private void OnJoin(JoinEventArgs args)
 				return;
 			}
 
-			Bans.CheckBan(player);
+			if (Bans.CheckBan(player))
+				return;
 		}
 
 		/// <summary>OnLeave - Called when a player leaves the server.</summary>
@@ -1437,7 +1440,7 @@ private void OnLeave(LeaveEventArgs args)
 
 			if (tsplr.ReceivedInfo)
 			{
-				if (!tsplr.SilentKickInProgress && tsplr.State >= 3)
+				if (!tsplr.SilentKickInProgress && tsplr.State >= 3 && tsplr.FinishedHandshake) //The player has left, do not broadcast any clients exploiting the behaviour of not spawning their player.
 					Utils.Broadcast(GetString("{0} has left.", tsplr.Name), Color.Yellow);
 				Log.Info(GetString("{0} disconnected.", tsplr.Name));
 
@@ -1458,6 +1461,9 @@ private void OnLeave(LeaveEventArgs args)
 				}
 			}
 
+			
+			tsplr.FinishedHandshake = false;
+
 			// Fire the OnPlayerLogout hook too, if the player was logged in and they have a TSPlayer object.
 			if (tsplr.IsLoggedIn)
 			{
@@ -1487,6 +1493,12 @@ private void OnChat(ServerChatEventArgs args)
 				return;
 			}
 
+			if (!tsplr.FinishedHandshake)
+			{
+				args.Handled = true;
+				return;
+			}
+
 			if (args.Text.Length > 500)
 			{
 				tsplr.Kick(GetString("Crash attempt via long chat packet."), true);
@@ -1703,14 +1715,14 @@ private void OnGreetPlayer(GreetPlayerEventArgs args)
 				Log.Info(GetString("{0} ({1}) from '{2}' group from '{3}' joined. ({4}/{5})", player.Name, player.IP,
 									   player.Group.Name, player.Country, TShock.Utils.GetActivePlayerCount(),
 									   TShock.Config.Settings.MaxSlots));
-				if (!player.SilentJoinInProgress)
+				if (!player.SilentJoinInProgress && player.FinishedHandshake)
 					Utils.Broadcast(GetString("{0} ({1}) has joined.", player.Name, player.Country), Color.Yellow);
 			}
 			else
 			{
 				Log.Info(GetString("{0} ({1}) from '{2}' group joined. ({3}/{4})", player.Name, player.IP,
 									   player.Group.Name, TShock.Utils.GetActivePlayerCount(), TShock.Config.Settings.MaxSlots));
-				if (!player.SilentJoinInProgress)
+				if (!player.SilentJoinInProgress && player.FinishedHandshake)
 					Utils.Broadcast(GetString("{0} has joined.", player.Name), Color.Yellow);
 			}
 

TShockAPI/Utils.cs

@@ -183,7 +183,7 @@ public void SendLogs(string log, Color color, TSPlayer excludedPlayer = null)
 		/// <returns>The number of active players on the server.</returns>
 		public int GetActivePlayerCount()
 		{
-			return TShock.Players.Count(p => null != p && p.Active);
+			return TShock.Players.Count(p => null != p && p.Active && p.FinishedHandshake);
 		}
 
 		//Random should not be generated in a method

docs/changelog.md

@@ -78,6 +78,8 @@ Use past tense when adding new entries; sign your name off when you add or chang
   * If there is no section called "Upcoming changes" below this line, please add one with `## Upcoming changes` as the first line, and then a bulleted item directly after with the first change. -->
 
 ## Upcoming changes
+* Added a variable for handshake (True upon spawn player), clients no longer notify others of their presence and cant chat if this is never set to true. (@ohayo)
+* Fixed a security issue with how bans are handled on join. (@ohayo)
 * Fixed `/dump-reference-data` mutate the command names. (#2943, @sgkoishi)
 * Added `ParryDamageBuff` (Striking Moment with Brand of the Inferno and shield) for player, updated `CursedInferno` buff for NPC (@sgkoishi, #3005)
 * Changed the use of `Player.active` to `TSPlayer.Active` for consistency. (@sgkoishi, #2939)