Handling Instances Reported By Client

Help and Feedback Code Review
#1

So i’ve read that I have to make sure any data I get from the client is valid. I’m making a simple building system where a player clicks on a BillboardGui with an Attachment as its Adornee in order to build a piece on said Attachment. The Gui fires a remote event with three parameters. Player, attachment, and piece they want to build (String). Am I handling the data correctly?

clientBuildEvent.OnServerEvent:Connect(function(plr, attach, piece)
if not plr or not plr:IsA("Player") then
	print("plr invalid")
	return
end
if not attach.Parent.Owner.Value == plr then
	print(plr.Name.." doesn't own attachment they want to build on")
	return
end
if not attach or not(attach:IsA("Attachment")) or not attach:IsDescendantOf(workspace) then
	print("attachment invalid")
	return
end
if type(piece) ~= "string" or ((piece ~= "placeStair") and (piece ~= "placeFloor") ) then
	print("piece invalid")
	return
end
--do stuff with the parameters
end)
2 Likes
#2

This seems good so far, but some things to note:

The first parameter (the player) is guaranteed by Roblox to always be a player. You don’t need to check this.
For the second parameter, when you do not x == y you can just do x ~= y.
I think the third looks pretty good.
For the last, you don’t need to check if it’s of type string. Simply checking that piece ~= "placeStair" and piece ~= "placeFloor" does the trick, because a non-string will never equal a string.

3 Likes
#3

I have a question about the :IsA method. Can a client make an instance that defines the IsA method as something malicious and then have it run on the server? Or does it not work like that.

#4

No, functions can’t be sent over the network. Tables that have fields the same as instances can, but they won’t have any methods. The remotes devhub page probably had the limitations listed.

1 Like
#5

Another change I would make is putting (what is currently) the third check before (what is currently) the second check.

It makes more sense to check if attach is a valid object before accessing its properties (although code-wise this doesn’t matter because if attach isn’t a valid object the check will just error and nothing will come of the request).

Additionally, if you wanted to, you could check whether the second parameter is an Instance before calling IsA.

1 Like