Merge pull request #1487 from tgstation/MrStonedOne-patch-1 [TGSDeploy]

Fix exploit allowing for the reading of discord connection strings.

build/Version.props

@@ -3,7 +3,7 @@
   <!-- Integration tests will ensure they match across the board -->
   <Import Project="ControlPanelVersion.props" />
   <PropertyGroup>
-    <TgsCoreVersion>5.12.0</TgsCoreVersion>
+    <TgsCoreVersion>5.12.1</TgsCoreVersion>
     <TgsConfigVersion>4.6.0</TgsConfigVersion>
     <TgsApiVersion>9.10.2</TgsApiVersion>
     <TgsApiLibraryVersion>10.4.1</TgsApiLibraryVersion>

src/Tgstation.Server.Host/Controllers/ChatController.cs

@@ -220,7 +220,7 @@ namespace Tgstation.Server.Host.Controllers
 							.OrderBy(x => x.Id))),
 				chatBot =>
 				{
-					if (connectionStrings)
+					if (!connectionStrings)
 						chatBot.ConnectionString = null;
 
 					return Task.CompletedTask;

tests/Tgstation.Server.Tests/Live/Instance/ChatTest.cs

@@ -7,6 +7,7 @@ using Microsoft.VisualStudio.TestTools.UnitTesting;
 
 using Tgstation.Server.Api.Models;
 using Tgstation.Server.Api.Models.Request;
+using Tgstation.Server.Api.Rights;
 using Tgstation.Server.Client;
 using Tgstation.Server.Client.Components;
 
@@ -30,7 +31,11 @@ namespace Tgstation.Server.Tests.Live.Instance
 			var ircTask = RunIrc(cancellationToken);
 			await RunDiscord(cancellationToken);
 			await ircTask;
+
+			var listTest = RunListTest(cancellationToken);
 			await RunLimitTests(cancellationToken);
+
+			await listTest;
 		}
 
 		async Task RunIrc(CancellationToken cancellationToken)
@@ -190,6 +195,41 @@ namespace Tgstation.Server.Tests.Live.Instance
 			Assert.AreEqual(0, nowBots.Count);
 		}
 
+		async Task RunListTest(CancellationToken cancellationToken)
+		{
+			// regression test for GHSA-rv76-495p-g7cp
+			// test starts with all perms
+			var permsClient = instanceClient.CreateClient(metadata).PermissionSets;
+			var ourInstancePermissionSetTask = permsClient.Read(cancellationToken);
+
+			var ourIPS = await ourInstancePermissionSetTask;
+			Assert.IsTrue(ourIPS.ChatBotRights.Value.HasFlag(ChatBotRights.ReadConnectionString));
+
+			var results = await chatClient.List(null, cancellationToken);
+
+			Assert.IsTrue(results.Count > 0);
+			Assert.IsTrue(results.All(chatBot => chatBot.ConnectionString != null));
+
+			var result = await chatClient.GetId(results[0], cancellationToken);
+
+			Assert.IsNotNull(result.ConnectionString);
+
+			await permsClient.Update(new InstancePermissionSetRequest
+			{
+				PermissionSetId = ourIPS.PermissionSetId,
+				ChatBotRights = ourIPS.ChatBotRights.Value & (~ChatBotRights.ReadConnectionString),
+			}, cancellationToken);
+
+			results = await chatClient.List(null, cancellationToken);
+
+			Assert.IsTrue(results.Count > 0);
+			Assert.IsTrue(results.All(chatBot => chatBot.ConnectionString == null));
+
+			result = await chatClient.GetId(results[0], cancellationToken);
+
+			Assert.IsNull(result.ConnectionString);
+		}
+
 		async Task RunLimitTests(CancellationToken cancellationToken)
 		{
 			await ApiAssert.ThrowsException<ConflictException>(() => chatClient.Create(new ChatBotCreateRequest