Improving my server-side checks to be organized, effective, and less redundant

Help and Feedback Scripting Support
#1

I’ve always been one to heavily lock down my remotes on the server to prevent cheating. The only issue is my checks start to get long and almost redundant, so I’ve tried to keep it to what I need. Even then, if you had a remote where one of the parameters is a part then you’d have your checks base off of thinking it is a part instance. But, if a cheater passes in another instance, the script would have a high change of erroring. Even though it still wouldn’t allow cheaters to do anything, that still annoys me.

So, in my quest to shorten and simplify the process of making server-side checks, I’ve done some research into ways to make it much cleaner. I still have some questions though. Side note: Appologies for any typos or grammar mistakes; I typed this up pretty quickly.

CollectionService Checks

I’ve done some testing with how CollectionService works across the server and client. I can confirm that checks added by the server to an instance cannot in any way be changed by the client. Here is the test I conducted:

I created a part with a server added CollectionService tag to it to see if the client could pass it into a remote and somehow modify the tags it has, even if the part was created in the client.

Server-side code:

local CollectionService = game:GetService("CollectionService")

CollectionService:AddTag(workspace.Part, "Test")

game.ReplicatedStorage.RemoteEvent.OnServerEvent:Connect(function(player, part)
	print(CollectionService:GetTags(part))
end)

Client-side code: 
local CollectionService = game:GetService("CollectionService")

wait(5)
print("sending real part")
game.ReplicatedStorage.RemoteEvent:FireServer(workspace.Part)

wait(5)
print("sending fake part")
local part = Instance.new("Part", workspace)
CollectionService:AddTag(part, "Test")
game.ReplicatedStorage.RemoteEvent:FireServer(part)

wait(5)
print("sending modified real part")
CollectionService:AddTag(workspace.Part, "Totally Real Tag")
game.ReplicatedStorage.RemoteEvent:FireServer(workspace.Part)

wait(5)
print("sending another modified real part")
CollectionService:RemoveTag(workspace.Part, "Test")
game.ReplicatedStorage.RemoteEvent:FireServer(workspace.Part)

Output: 
  sending real part
  {
    [1] = "Test"
  }

  sending fake part
  {}

  sending modified real part
  {
    [1] = "Test"
  }

  sending another modified real part
  {
    [1] = "Test"
  }

It’s clear to see here that clients can’t change anything with instance tags. Supposedly, if something that you were planning to check on the server has a CollectionService tag that will always be there then you can check to see if it’s there to fully verify that the instance is valid (this doesn’t mean there would be a tag on everything in the game). So, is this a good way of verifying what the client passed in?

PCall Checks

At the beginning of the post, I talked about how it was annoying that a cheater could cause the server code to error. So, I have supposedly came up with a system that could solve that, in combination with an organized way of managing server-side checks.

Server-side code:

function DoServerCheck(checks)
	local checksPassed = false
	
	local success = pcall(function()
		checksPassed = checks()
	end)
	
	return success and checksPassed
end

game.ReplicatedStorage.RemoteEvent.OnServerEvent:Connect(function(player, part)
	local remoteValid = DoServerCheck(function()
		local checksToPass = 1
		local checksPassed = 0
		
		if part.Color == Color3.new(1, 0, 0) and part.Name == "RedPart" then
			checksPassed += 1
		end
		
		return checksToPass <= checksPassed
	end)
	
	print(remoteValid)
end)

Client-side code: 
wait(5)
print("sending real part")
game.ReplicatedStorage.RemoteEvent:FireServer(workspace.RedPart)

wait(5)
print("sending fake part that doesnt meet the conditions")
game.ReplicatedStorage.RemoteEvent:FireServer(workspace.GreenPart)

wait(5)
print("sending something that would cause issues")
game.ReplicatedStorage.RemoteEvent:FireServer(Instance.new("PointLight"))

Output: 
  sending real part
  true

  sending fake part that doesnt meet the conditions
  false

  sending something that would cause issues
  false

This system seems to hold up and is somewhat organized. It allows the server-side checks to be put together in an organized way and as an added bonus makes sure it can’t throw errors. Is there a better way to make a system like this? Is this system even a good idea to begin with?

This concludes my findings, but I just want to confirm. Am I on the right track here? All these methods seem to hold up pretty well and seem effective, but I’m definitely not an expert when it comes to these checks.

Thanks for reading and any help is appreciated.

1 Like
#2

CollectionService
The CollectionService checks are redundant because any parts created on the client aren’t replicated to the server. So, it’s just impossible to send parts from the client to the server. It says on the documentation nil will be passed instead. It’s similar with collectionService, nothing is replicated from the client. However, the tags from the server are replicated to the client.

Validate and Sanitize
Firstly, it is a good idea. It seems rather odd how you done it though. Firstly, no need to run protected call. You can prevent errors by simply checking the type of the value sent. So, something like this:

local RED = Color.new(1, 0, 0) -- constant

function(value)game.ReplicatedStorage.RemoteEvent.OnServerEvent:Connect(function(player, part)
	local remoteValid = false

	-- Try every way to validate the argument. First, is it even an instance?
	if typeof(Part) == "Instance" then
		-- Is it a part?
		if Part:IsA("Part") then
			if Part.Color == RED and Part.Name == "RedPart" then
				-- All of the conditions satisfied means it is valid.
				remoteValid = true
			end
		end
	end

	print(remoteValid)
end)

No need to wrap it in a pcall, so no need for the external function. Now, nested if-statements aren’t cool I suppose. Actually looks like a lot of boilerplate code that can be put into a function if wanted. Just not sure how to yet.

1 Like
#3

It would appear that I didn’t really look into the specifics of passing through instances into remotes. I guess my intent of validation went a bit too far.

For validation and sanitization, I’m still a little unsure. The goal of the system I had setup is to have an organized way of validating what the client sent. What you mentioned is what I’m doing now in my games. I dislike that system because 1. Yes, nested if-statements are annoying to me 2. I have the goal to put as minimal checks as possible.

Look at it this way: if you wrap it in a pcall, you don’t have to have any checks for if the sent values are nil or the correct type (for example, an instance). With a pcall, I would be able to leave out many checks that might be necessary without a pcall to avoid errors. Because, if the pcall is unsuccessful, I know that something unintended or wrong happened and I know to not run the rest of the code.

#4

Something scary I read recently - you do need that typeof(part) == "Instance, because exploiters can send tables that look superficially like instances. The illusion shatters really quick when you try using methods (e.g. fakepart:IsA(“Part”) will error because you can’t send functions over remotes), but code that never runs methods and simply reads properties can be fooled big time.

Let’s say there’s a super zany and comically misguided remote event for handling purchases. It takes a product name, and an IntValue instance in the player’s leaderstats to pay with.
Normally, if the IntValue has enough Value to buy the product, then the product is purchased, and Value is removed from the IntValue.
But an attacker can send a table like {Value = 99999999}. If you reduce the Value of this, you’ve just done zip and nada.
The galaxy-brained developer may rule out exploiters paying with other’s wallets or sending these fake instances by checking whether money.Parent.Parent == player. Now the exploiter can send the table {Value = 9999999, Parent = {Parent = player}}

1 Like
#5

Wow after reading the second paragraph that really something else, haha. The dedication people put into breaking games is impressive, but it only means we have to be more impressive to counter it.

I guess the simple way of countering this is just adding one check at the beginning of all instance related parameters, yes?

#6

Pretty much. It’s dumb, but if you don’t have too many remotes (or rather, too many different things that can be done with remotes… don’t pipe everything thru one remote), then you don’t have to do this drudgery too much.

Also:

Yes, nested if-statements are annoying to me

Try using and. It is really just an if statement, and it too can get hard to manage, but at least it can be made to look nice…

whatever:Connect(function(player, part, times)
	-- part must be a red part named RedPart
	-- times must be an integer
	assert( typeof(part) == "Instance"
		and typeof(times) == "number"
		and part:IsA("Part")
		and part.Color == RED
		and part.Name == "RedPart"
	, "Invalid parameters")
	-- you can probably replace assert with a function that
	-- also takes the player in its args and, if first arg is false
	-- (like assert per usual), marks it as suspicious
	-- (and then errors to stop the function from continuing)
	
	-- ...
end)
1 Like