-
-
Notifications
You must be signed in to change notification settings - Fork 803
Re-enabling the superadmin group #1767
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
base: main
Are you sure you want to change the base?
Conversation
…ly unnecessary to 'disable'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR restores the ability to use the "superadmin" group by removing restrictions that were previously forcing users with this group back to the "admin" group. The change removes conditional checks that prevented the superadmin group from being used.
- Removed the superadmin-to-admin downgrade logic in the setgroup command
- Removed the superadmin-to-admin downgrade logic when loading player data
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| [core]/es_extended/server/modules/commands.lua | Removes the check that prevented setting players to the "superadmin" group via the /setgroup command |
| [core]/es_extended/server/main.lua | Removes the check that downgraded "superadmin" to "admin" when loading player data from the database |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| userData.group = "admin" | ||
| print("[^3WARNING^7] ^5Superadmin^7 detected, setting group to ^5admin^7") | ||
| else | ||
|
|
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line contains only whitespace and should be removed. The conditional logic can be simplified by directly assigning result.group to userData.group without the empty line.
| print("[^3WARNING^7] ^5Superadmin^7 detected, setting group to ^5admin^7") | ||
| else | ||
|
|
||
| userData.group = result.group |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The indentation of this line is inconsistent with the surrounding code structure. It should be indented at the same level as the if/else block it belongs to, not nested deeper.
| userData.group = result.group | |
| userData.group = result.group |
|
|
||
| userData.group = result.group |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By removing the logic that downgraded result.group == "superadmin" to "admin", any users.group value of "superadmin" is now loaded directly into userData.group and passed to CreateExtendedPlayer, which ultimately leads to add_principal identifier.<license> group.superadmin. If an attacker can influence the users.group column (e.g., via a vulnerable admin panel, misconfigured tooling, or partial DB compromise), they can grant themselves full FiveM/ACE group.superadmin rights instead of being limited to group.admin as before. To maintain a safety boundary between regular ESX admins and server superadmins, keep superadmin values rejected or downgraded here and manage group.superadmin membership only via trusted out-of-band ACL configuration.
| userData.group = result.group | |
| -- Prevent database-controlled escalation to superadmin. | |
| -- Superadmin ACE rights should be managed only via trusted ACL configuration. | |
| if result.group == "superadmin" then | |
| userData.group = "admin" | |
| else | |
| userData.group = result.group | |
| end |
| function(xPlayer, args) | ||
| if not args.playerId then | ||
| args.playerId = xPlayer.source | ||
| end |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing the guard that remapped args.group == "superadmin" to "admin" means /setgroup can now assign the superadmin group, and args.playerId.setGroup(args.group) will call add_principal identifier.<license> group.superadmin under the hood. On typical FiveM/ACE setups, group.superadmin has far broader privileges than group.admin, so any ESX admin who can run /setgroup (or an attacker who compromises such an account) can escalate to full server-level superadmin. To avoid this privilege escalation, keep superadmin assignments restricted (e.g., block superadmin here or enforce that only the console or an out-of-band ACL can grant group.superadmin).
| end | |
| end | |
| if args.group == "superadmin" then | |
| args.group = "admin" | |
| end |
Description
Restores the ability to setgroup players into the 'superadmin' group.
Motivation
Gives users the ability to freely use this group again, while restricting it is completely useless.
Implementation Details
Just removed the conditions where the core would force the user back to the original admin group.
PR Checklist