-
-
Notifications
You must be signed in to change notification settings - Fork 21
GH-1297 Add fly mode in vanish. #1297
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: master
Are you sure you want to change the base?
Conversation
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.
Code Review
This pull request adds a 'fly mode' feature to vanish. The implementation is mostly correct, but there is a significant vulnerability in the FlyController that allows players to gain flight in survival mode. I've provided a detailed explanation and a suggested fix for this issue across two comments. The changes to the config and interface are fine.
| @EventHandler(ignoreCancelled = true) | ||
| void onDisable(DisableVanishEvent event) { | ||
| Player player = event.getPlayer(); | ||
|
|
||
| if (player.getGameMode() == GameMode.CREATIVE || player.getGameMode() == GameMode.SPECTATOR) { | ||
| this.previousFlightStates.remove(player.getUniqueId()); | ||
| return; | ||
| } | ||
|
|
||
| FlightState state = this.previousFlightStates.remove(player.getUniqueId()); | ||
| if (state == null) { | ||
| return; | ||
| } | ||
|
|
||
| player.setAllowFlight(state.hadAllowFlight()); | ||
| player.setFlying(state.wasFlying()); | ||
| } |
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 current logic allows for a flight exploit where a player can enable vanish in creative mode, switch to survival, and retain flight after disabling vanish. By using the GameMode stored when vanish was enabled (as per my other comment), you can prevent this. The new logic should check if the player has switched from a gamemode that allows flight to one that doesn't, and if so, explicitly disable flight.
@EventHandler(ignoreCancelled = true)
void onDisable(DisableVanishEvent event) {
Player player = event.getPlayer();
FlightState state = this.previousFlightStates.remove(player.getUniqueId());
if (state == null) {
return;
}
GameMode currentMode = player.getGameMode();
if (currentMode == GameMode.CREATIVE || currentMode == GameMode.SPECTATOR) {
return;
}
GameMode originalMode = state.gameMode();
if (originalMode == GameMode.CREATIVE || originalMode == GameMode.SPECTATOR) {
player.setAllowFlight(false);
player.setFlying(false);
return;
}
player.setAllowFlight(state.hadAllowFlight());
player.setFlying(state.wasFlying());
}| this.previousFlightStates.computeIfAbsent( | ||
| player.getUniqueId(), | ||
| id -> new FlightState(player.getAllowFlight(), player.isFlying()) | ||
| ); |
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.
To fix a potential flight exploit, it's necessary to also store the player's GameMode when they enter vanish. This will be used when they exit vanish to determine if their flight status should be restored.
You will also need to update the FlightState record on line 68 to include a GameMode field:
private record FlightState(boolean hadAllowFlight, boolean wasFlying, GameMode gameMode) {}| this.previousFlightStates.computeIfAbsent( | |
| player.getUniqueId(), | |
| id -> new FlightState(player.getAllowFlight(), player.isFlying()) | |
| ); | |
| this.previousFlightStates.computeIfAbsent( | |
| player.getUniqueId(), | |
| id -> new FlightState(player.getAllowFlight(), player.isFlying(), player.getGameMode()) | |
| ); |
| class FlyController implements Listener { | ||
|
|
||
| private final VanishSettings settings; | ||
| private final Map<UUID, FlightState> previousFlightStates = new ConcurrentHashMap<>(); |
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.
move to separate class?
No description provided.