From b2df405cc0355dcffe202bc4040fae316940d427 Mon Sep 17 00:00:00 2001
From: "Dylan K. Taylor" <dktapps@pmmp.io>
Date: Wed, 29 Nov 2023 16:31:59 +0000
Subject: [PATCH] NetworkSession: Send less information to clients on error
 disconnects in particular, the information from VerifyLoginTask shouldn't be
 sent to clients, as it could contain sensitive information. This change only
 affects disconnection screens. The server log shows the same amount of
 information as before (though formatted differently in some cases).

---
 src/network/mcpe/NetworkSession.php           | 20 ++++++++++++++-----
 .../mcpe/handler/LoginPacketHandler.php       |  6 ++++--
 .../handler/ResourcePacksPacketHandler.php    |  6 ++++--
 src/network/mcpe/raklib/RakLibInterface.php   |  7 +++++--
 4 files changed, 28 insertions(+), 11 deletions(-)

diff --git a/src/network/mcpe/NetworkSession.php b/src/network/mcpe/NetworkSession.php
index 16baee0aee..1a4c85255e 100644
--- a/src/network/mcpe/NetworkSession.php
+++ b/src/network/mcpe/NetworkSession.php
@@ -236,8 +236,10 @@ class NetworkSession{
 			$this->onPlayerCreated(...),
 			function() : void{
 				//TODO: this should never actually occur... right?
-				$this->logger->error("Failed to create player");
-				$this->disconnectWithError(KnownTranslationFactory::pocketmine_disconnect_error_internal());
+				$this->disconnectWithError(
+					reason: "Failed to create player",
+					disconnectScreenMessage: KnownTranslationFactory::pocketmine_disconnect_error_internal()
+				);
 			}
 		);
 	}
@@ -675,8 +677,13 @@ class NetworkSession{
 		}, $reason);
 	}
 
-	public function disconnectWithError(Translatable|string $reason) : void{
-		$this->disconnect(KnownTranslationFactory::pocketmine_disconnect_error($reason, implode("-", str_split(bin2hex(random_bytes(6)), 4))));
+	public function disconnectWithError(Translatable|string $reason, Translatable|string|null $disconnectScreenMessage = null) : void{
+		$errorId = implode("-", str_split(bin2hex(random_bytes(6)), 4));
+
+		$this->disconnect(
+			reason: KnownTranslationFactory::pocketmine_disconnect_error($reason, $errorId)->prefix(TextFormat::RED),
+			disconnectScreenMessage: KnownTranslationFactory::pocketmine_disconnect_error($disconnectScreenMessage ?? $reason, $errorId),
+		);
 	}
 
 	public function disconnectIncompatibleProtocol(int $protocolVersion) : void{
@@ -735,7 +742,10 @@ class NetworkSession{
 		}
 
 		if($error !== null){
-			$this->disconnectWithError(KnownTranslationFactory::pocketmine_disconnect_invalidSession($error));
+			$this->disconnectWithError(
+				reason: KnownTranslationFactory::pocketmine_disconnect_invalidSession($error),
+				disconnectScreenMessage: KnownTranslationFactory::pocketmine_disconnect_error_authentication()
+			);
 
 			return;
 		}
diff --git a/src/network/mcpe/handler/LoginPacketHandler.php b/src/network/mcpe/handler/LoginPacketHandler.php
index 9ac82bc5fe..26e2bf0283 100644
--- a/src/network/mcpe/handler/LoginPacketHandler.php
+++ b/src/network/mcpe/handler/LoginPacketHandler.php
@@ -73,8 +73,10 @@ class LoginPacketHandler extends PacketHandler{
 		try{
 			$skin = $this->session->getTypeConverter()->getSkinAdapter()->fromSkinData(ClientDataToSkinDataHelper::fromClientData($clientData));
 		}catch(\InvalidArgumentException | InvalidSkinException $e){
-			$this->session->getLogger()->debug("Invalid skin: " . $e->getMessage());
-			$this->session->disconnectWithError(KnownTranslationFactory::disconnectionScreen_invalidSkin());
+			$this->session->disconnectWithError(
+				reason: "Invalid skin: " . $e->getMessage(),
+				disconnectScreenMessage: KnownTranslationFactory::disconnectionScreen_invalidSkin()
+			);
 
 			return true;
 		}
diff --git a/src/network/mcpe/handler/ResourcePacksPacketHandler.php b/src/network/mcpe/handler/ResourcePacksPacketHandler.php
index 08ce53efe2..3d413ee5a1 100644
--- a/src/network/mcpe/handler/ResourcePacksPacketHandler.php
+++ b/src/network/mcpe/handler/ResourcePacksPacketHandler.php
@@ -85,8 +85,10 @@ class ResourcePacksPacketHandler extends PacketHandler{
 	}
 
 	private function disconnectWithError(string $error) : void{
-		$this->session->getLogger()->error("Error downloading resource packs: " . $error);
-		$this->session->disconnectWithError(KnownTranslationFactory::disconnectionScreen_resourcePack());
+		$this->session->disconnectWithError(
+			reason: "Error downloading resource packs: " . $error,
+			disconnectScreenMessage: KnownTranslationFactory::disconnectionScreen_resourcePack()
+		);
 	}
 
 	public function handleResourcePackClientResponse(ResourcePackClientResponsePacket $packet) : bool{
diff --git a/src/network/mcpe/raklib/RakLibInterface.php b/src/network/mcpe/raklib/RakLibInterface.php
index d189cf0fd3..17aa87e839 100644
--- a/src/network/mcpe/raklib/RakLibInterface.php
+++ b/src/network/mcpe/raklib/RakLibInterface.php
@@ -219,11 +219,14 @@ class RakLibInterface implements ServerEventListener, AdvancedNetworkInterface{
 				$session->handleEncoded($buf);
 			}catch(PacketHandlingException $e){
 				$logger = $session->getLogger();
-				$logger->error("Bad packet: " . $e->getMessage());
 
+				$session->disconnectWithError(
+					reason: "Bad packet: " . $e->getMessage(),
+					disconnectScreenMessage: KnownTranslationFactory::pocketmine_disconnect_error_badPacket()
+				);
 				//intentionally doesn't use logException, we don't want spammy packet error traces to appear in release mode
 				$logger->debug(implode("\n", Utils::printableExceptionInfo($e)));
-				$session->disconnectWithError(KnownTranslationFactory::pocketmine_disconnect_error_badPacket());
+
 				$this->interface->blockAddress($address, 5);
 			}catch(\Throwable $e){
 				//record the name of the player who caused the crash, to make it easier to find the reproducing steps