Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add getUserInfo function at convar (fixes #1996) #1999

Merged
merged 11 commits into from
Jan 30, 2025

Conversation

Cheatoid
Copy link
Contributor

This is a PR where getUserInfo is implemented at convar library.

@Cheatoid
Copy link
Contributor Author

Cheatoid commented Jan 25, 2025

My bad, based this branch off builtins... fixed.

@ax255
Copy link
Contributor

ax255 commented Jan 25, 2025

Is this really needed since you can already network convar value from people client to server yourself ? This could allow even if someone has disabled convar permission on their client to bypass it and get an client userinfo convar value from the server realm ignoring the client permission.

@Cheatoid
Copy link
Contributor Author

Is this really needed since you can already network convar value from people client to server yourself ?

You could do that.

This could allow even if someone has disabled convar permission on their client to bypass it and get an client userinfo convar value from the server realm ignoring the client permission.

No? When calling getUserInfo from server-side it would retrieve the userinfo variable of the chip owner ("only me"). The client-side permission exists because I thought it would be a good addition.

@ax255
Copy link
Contributor

ax255 commented Jan 25, 2025

Looking at your code the user info perm only exist in client side so the getUserInfo used in server side will not know about this permission, also in SF client permission are not networked to server so even if it did had that permission severside it wouldn't be able to tell if the client has the permission off.

@Cheatoid
Copy link
Contributor Author

Cheatoid commented Jan 25, 2025

That's the design choice being made here, no permission for owner-only (server-side).
Because if you are placing down the chip, I figured it is not much of a risk since you know those values off console.
The permission exists on client-side to prevent other chips from accessing your userinfo convar.

@ax255
Copy link
Contributor

ax255 commented Jan 25, 2025

The is the thing, server side will run regardless of the client permission, so even if someone has their convar or userinfo permission disabled you can bypass it by using this function on server side to retrieve client side convar.

@Cheatoid
Copy link
Contributor Author

There is no "someone", it is you.

@adamnejm
Copy link
Collaborator

@ax255 instance.player is always the chip's owner.

@adamnejm
Copy link
Collaborator

adamnejm commented Jan 25, 2025

Is the IsValid check really needed? I'd just make the function error instead of returning nil only if the player is invalid if that's necessary. That way signature would only be string return.

@adamnejm
Copy link
Collaborator

adamnejm commented Jan 25, 2025

Also, since the convar lib is now shared, maybe just make 2 separate CL and SV functions instead of checking the realm twice in one? It's not that much repetition...

@ax255
Copy link
Contributor

ax255 commented Jan 25, 2025

Oh my bad that was dumb, indentation on mobile cutting thing off made me not read this thoroughly, then I guess it's good I this state. Maybe this function is not even needed to be in client side since it will always return for the local player and you can use the regular convar lib for it.

@Cheatoid
Copy link
Contributor Author

Is the IsValid check really needed?

I suppose it is necessary when using superuser (or in case chip owner has disconnected and such)?
But it is worth mentioning, I haven't tested this in-game at all, yet.

@adamnejm
Copy link
Collaborator

Right, I'd switch to instance.Types.Player.GetPlayer and make it error though ;p

@Cheatoid
Copy link
Contributor Author

I'd switch to instance.Types.Player.GetPlayer

Not sure why, and what's the difference exactly? But, also that would require adding initialize hook and passing a Player object?

@Cheatoid Cheatoid marked this pull request as ready for review January 25, 2025 04:54
@adamnejm
Copy link
Collaborator

adamnejm commented Jan 25, 2025

It would handle the validity check / erroring for you. Adding initialize is not a big deal?
Also, the SERVER check could be removed, instance.player is set on the clientside:

function convar_library.getUserInfo(name)
	checkluatype(name, TYPE_STRING)
	if CLIENT then checkpermission(instance, name, "convar") end
	return Ply_GetInfo(getply(instance.player), name)
end

* Actually type check before check permission is better

* Use `Player.GetPlayer` approach instead
@adamnejm
Copy link
Collaborator

Well not done yet, I forgot that getply is only for wrapped ents instance.player is GLua's player. Your validity check was correct. sorry

@Cheatoid
Copy link
Contributor Author

Confirmed in-game, broken. Throws Entity is not valid.
Reverting...

@Cheatoid
Copy link
Contributor Author

For those who want convar.getUserInfoNum, just define it in SF:

local tonumber = tonumber
local convar_getUserInfo = convar.getUserInfo
convar.getUserInfoNum = function(name)
    return tonumber(convar_getUserInfo(name)) or 0
end

lua/starfall/libs_sh/convar.lua Outdated Show resolved Hide resolved
@thegrb93 thegrb93 merged commit 082e95a into thegrb93:master Jan 30, 2025
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants