general hardening fixes, duplication prevention#4
general hardening fixes, duplication prevention#4multiman155 wants to merge 1 commit intoMCCitiesNetwork:mainfrom
Conversation
There was a problem hiding this comment.
I like the idea here but i think this can still go wrong
if rentRegion succeeds and the placeholder lookup fails after that this still comes back as null and refunds the player even though the lease already
changed
feels like the write result needs to be handled separate from the placeholder lookup so we do not refund after a successful commit
looks like ExtendCommand has the same pattern too
There was a problem hiding this comment.
A lot to unpack in this PR, there is too much going on. Please break this PR up into multiple PRs.
- Do not bundle the CI/workflow changes with logic changes
- Split the sign command sanitizing logic into another PR (this is a welcome change, just too muddled up in here)
- Fixes to the unit test I will fix separately (since I can't fix by merging the PR right now as-is), so please revert those
- In general, a preference was made to have worldguard always be the authoritative source of truth when it comes to permission checks, I've commented on the places I think changes were made incorrectly/do not reflect that preference to be fixed accordingly.
- The economy changes should be reasonable, notably its hard to understand because theres so much going on.
There was a problem hiding this comment.
I think its generally unnecessary to add checks for valid region logic here. The job of this command is to manage agent invites and it should stay in that scope. Furthermore, we may extend the scope of agents to rentals in the future anyway.
| case RealtyLogicImpl.RemoveSanctionedAuctioneerResult.NoFreeholdContract() -> | ||
| sender.sendMessage(messages.messageFor(MessageKeys.AGENT_INVITE_NO_FREEHOLD, | ||
| Placeholder.unparsed("region", regionId))); | ||
| case RealtyLogicImpl.RemoveSanctionedAuctioneerResult.NotTitleHolder() -> |
There was a problem hiding this comment.
The point here is the world guard is the authoritative source of "who is the owner". So if there is somehow a desync, worldguard will prevail.
|
|
||
| private void executeCancel(@NotNull CommandContext<CommandSourceStack> ctx) { | ||
| CommandSender sender = ctx.sender().getSender(); | ||
| if (!(sender instanceof Player player)) { |
There was a problem hiding this comment.
This command should be executable by console.
| sender.sendMessage(messages.messageFor(MessageKeys.AUCTION_NOT_SANCTIONED, | ||
| Placeholder.unparsed("region", regionId))); | ||
| case RealtyLogicImpl.CancelAuctionResult.NoFreeholdContract ignored -> | ||
| sender.sendMessage(messages.messageFor(MessageKeys.AUCTION_NO_FREEHOLD_CONTRACT, |
There was a problem hiding this comment.
Again same as before, i think this command should not be validating whether it is a freehold contract or not, it's job is to handle auctions and that should be the end the scope of the checks.
| case RealtyLogicImpl.RenewLeaseholdResult.NoLeaseholdContract ignored -> | ||
| sender.sendMessage(messages.messageFor(MessageKeys.EXTEND_NO_LEASEHOLD_CONTRACT, | ||
| Placeholder.unparsed("region", regionId))); | ||
| case RealtyLogicImpl.RenewLeaseholdResult.NotTenant ignored -> |
There was a problem hiding this comment.
Worldguard is the authoritative source of truth
| } | ||
| String regionId = region.region().getId(); | ||
| UUID worldId = region.world().getUID(); | ||
| if (sender instanceof Player player |
There was a problem hiding this comment.
Worldguard is authoritative, so these checks did not need to be moved into the database layer.
| } | ||
| String regionId = region.region().getId(); | ||
| UUID worldId = region.world().getUID(); | ||
| if (sender instanceof Player player |
There was a problem hiding this comment.
Worldguard is authoritative, so these checks did not need to be moved into the database layer.
| } | ||
| String regionId = region.region().getId(); | ||
| UUID worldId = region.world().getUID(); | ||
| if (sender instanceof Player player |
There was a problem hiding this comment.
Worldguard is authoritative, so these checks did not need to be moved into the database layer.
| sender.sendMessage(messages.messageFor(MessageKeys.UNRENT_NO_LEASEHOLD_CONTRACT, | ||
| Placeholder.unparsed("region", regionId))); | ||
| } | ||
| case RealtyLogicImpl.UnrentResult.NotTenant ignored -> { |
There was a problem hiding this comment.
Worldguard is authoritative
| } | ||
| String regionId = region.region().getId(); | ||
| UUID worldId = region.world().getUID(); | ||
| if (sender instanceof Player player |
There was a problem hiding this comment.
Worldguard is authoritative, so these checks did not need to be moved into the database layer.
3 participants
This PR makes the plugin safer and harder to break by mistake.
The biggest fix is for
rentandextend. Before this change, the code could take money from one player and give it to another player before it knew for sure that the database update worked. If the database step failed, that could create extra money by accident. Now the code waits until the database change succeeds before finishing the payment, and it refunds correctly if something goes wrong.This PR also moves more permission checks into the main shared logic of the plugin. Before, some checks only happened in the command code, which meant there were ways those protections could be missed. Now the important rules about who is allowed to change ownership, landlord, tenant, prices, and agent data are enforced in the core logic itself.
It also makes sign-triggered commands safer. Sign commands are now cleaned and checked before they run, so blank, broken, multi-line, or malformed commands are skipped instead of being executed.
On top of that, this fixes the database test setup after the leasehold table rename, adds more tests for security-sensitive cases, and adds GitHub security automation with dependency review, CodeQL, and Dependabot.
Verification:
./gradlew.bat test