fix(security): prevent temporary OP from being saved (#3159)

* fix(security): prevent temporary OP from being saved

* fix: Player#setOp usage is not necessary

* fix: fixed incorrect implementation of setOpWithoutSaving for certain MC version

main/src/main/java/net/citizensnpcs/util/NMS.java

@@ -972,6 +972,10 @@ public class NMS {
         BRIDGE.setWitherInvulnerableTicks(wither, ticks);
     }
 
+    public static void setOpWithoutSaving(Player player, boolean op) {
+        BRIDGE.setOpWithoutSaving(player, op);
+    }
+
     public static boolean shouldBroadcastToPlayer(NPC npc, Supplier<Boolean> defaultResponse) {
         return npc != null && npc.data().has(NPC.Metadata.NPC_SPAWNING_IN_PROGRESS) ? false : defaultResponse.get();
     }

main/src/main/java/net/citizensnpcs/util/NMSBridge.java

@@ -298,6 +298,8 @@ public interface NMSBridge {
         wither.setInvulnerabilityTicks(ticks);
     }
 
+    public void setOpWithoutSaving(Player player, boolean op);
+
     public boolean shouldJump(Entity entity);
 
     public void shutdown();

main/src/main/java/net/citizensnpcs/util/Util.java

@@ -463,7 +463,7 @@ public class Util {
         }
         boolean wasOp = clicker.isOp();
         if (op) {
-            clicker.setOp(true);
+            NMS.setOpWithoutSaving(clicker, true);
         }
         try {
             if (bungeeServer != null) {
@@ -479,7 +479,12 @@ public class Util {
             t.printStackTrace();
         } finally {
             if (op) {
-                clicker.setOp(wasOp);
+                if (!wasOp) {
+                    // Disk I/O operation caused by Player#setOp(boolean)
+                    // is not necessary here because changes on permission
+                    // are not actually saved
+                    NMS.setOpWithoutSaving(clicker, false);
+                }
             }
         }
     }

v1_10_R1/src/main/java/net/citizensnpcs/nms/v1_10_R1/util/NMSImpl.java

@@ -18,6 +18,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.v1_10_R1.DedicatedPlayerList;
+import net.minecraft.server.v1_10_R1.DedicatedServer;
+import net.minecraft.server.v1_10_R1.OpList;
+import net.minecraft.server.v1_10_R1.OpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1316,6 +1320,21 @@ public class NMSImpl implements NMSBridge {
         handle.g(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final OpList opList = playerList.getOPs();
+        if (op) {
+            opList.add(new OpListEntry(profile, server.getPropertyManager().getInt("op-permission-level", 4), opList.b(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        player.recalculatePermissions();
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof LivingEntity))

v1_11_R1/src/main/java/net/citizensnpcs/nms/v1_11_R1/util/NMSImpl.java

@@ -18,6 +18,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.v1_11_R1.DedicatedPlayerList;
+import net.minecraft.server.v1_11_R1.DedicatedServer;
+import net.minecraft.server.v1_11_R1.OpList;
+import net.minecraft.server.v1_11_R1.OpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1369,6 +1373,21 @@ public class NMSImpl implements NMSBridge {
         handle.g(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final OpList opList = playerList.getOPs();
+        if (op) {
+            opList.add(new OpListEntry(profile, server.getPropertyManager().getInt("op-permission-level", 4), opList.b(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        player.recalculatePermissions();
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof LivingEntity))

v1_12_R1/src/main/java/net/citizensnpcs/nms/v1_12_R1/util/NMSImpl.java

@@ -19,6 +19,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.v1_12_R1.DedicatedPlayerList;
+import net.minecraft.server.v1_12_R1.DedicatedServer;
+import net.minecraft.server.v1_12_R1.OpList;
+import net.minecraft.server.v1_12_R1.OpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1377,6 +1381,21 @@ public class NMSImpl implements NMSBridge {
         handle.g(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final OpList opList = playerList.getOPs();
+        if (op) {
+            opList.add(new OpListEntry(profile, server.getPropertyManager().getInt("op-permission-level", 4), opList.b(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        player.recalculatePermissions();
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof LivingEntity))

v1_13_R2/src/main/java/net/citizensnpcs/nms/v1_13_R2/util/NMSImpl.java

@@ -18,6 +18,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.v1_13_R2.DedicatedPlayerList;
+import net.minecraft.server.v1_13_R2.DedicatedServer;
+import net.minecraft.server.v1_13_R2.OpList;
+import net.minecraft.server.v1_13_R2.OpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1413,6 +1417,22 @@ public class NMSImpl implements NMSBridge {
         handle.d(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final EntityPlayer playerHandle = ((CraftPlayer) player).getHandle();
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final OpList opList = playerList.getOPs();
+        if (op) {
+            opList.add(new OpListEntry(profile, server.getPropertyManager().getInt("op-permission-level", 4), opList.b(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        playerList.f(playerHandle);
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof LivingEntity))

v1_14_R1/src/main/java/net/citizensnpcs/nms/v1_14_R1/util/NMSImpl.java

@@ -17,6 +17,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.v1_14_R1.DedicatedPlayerList;
+import net.minecraft.server.v1_14_R1.DedicatedServer;
+import net.minecraft.server.v1_14_R1.OpList;
+import net.minecraft.server.v1_14_R1.OpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1483,6 +1487,22 @@ public class NMSImpl implements NMSBridge {
         handle.r(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final EntityPlayer playerHandle = ((CraftPlayer) player).getHandle();
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final OpList opList = playerList.getOPs();
+        if (op) {
+            opList.add(new OpListEntry(profile, server.getDedicatedServerProperties().opPermissionLevel, opList.b(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        playerList.d(playerHandle);
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof LivingEntity))

v1_15_R1/src/main/java/net/citizensnpcs/nms/v1_15_R1/util/NMSImpl.java

@@ -17,6 +17,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.v1_15_R1.DedicatedPlayerList;
+import net.minecraft.server.v1_15_R1.DedicatedServer;
+import net.minecraft.server.v1_15_R1.OpList;
+import net.minecraft.server.v1_15_R1.OpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1532,6 +1536,22 @@ public class NMSImpl implements NMSBridge {
         handle.s(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final EntityPlayer playerHandle = ((CraftPlayer) player).getHandle();
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final OpList opList = playerList.getOPs();
+        if (op) {
+            opList.add(new OpListEntry(profile, server.getDedicatedServerProperties().opPermissionLevel, opList.b(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        playerList.d(playerHandle);
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof LivingEntity))

v1_16_R3/src/main/java/net/citizensnpcs/nms/v1_16_R3/util/NMSImpl.java

@@ -17,6 +17,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.v1_16_R3.DedicatedPlayerList;
+import net.minecraft.server.v1_16_R3.DedicatedServer;
+import net.minecraft.server.v1_16_R3.OpList;
+import net.minecraft.server.v1_16_R3.OpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1552,6 +1556,22 @@ public class NMSImpl implements NMSBridge {
         handle.setInvul(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final EntityPlayer playerHandle = ((CraftPlayer) player).getHandle();
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final OpList opList = playerList.getOPs();
+        if (op) {
+            opList.add(new OpListEntry(profile, server.getDedicatedServerProperties().opPermissionLevel, opList.b(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        playerList.d(playerHandle);
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof LivingEntity))

v1_17_R1/src/main/java/net/citizensnpcs/nms/v1_17_R1/util/NMSImpl.java

@@ -16,6 +16,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.dedicated.DedicatedPlayerList;
+import net.minecraft.server.dedicated.DedicatedServer;
+import net.minecraft.server.players.ServerOpList;
+import net.minecraft.server.players.ServerOpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1549,6 +1553,22 @@ public class NMSImpl implements NMSBridge {
         handle.setInvulnerableTicks(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final ServerPlayer playerHandle = ((CraftPlayer) player).getHandle();
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final ServerOpList opList = playerList.getOps();
+        if (op) {
+            opList.add(new ServerOpListEntry(profile, server.getOperatorUserPermissionLevel(), opList.canBypassPlayerLimit(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        playerList.sendPlayerPermissionLevel(playerHandle);
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof org.bukkit.entity.LivingEntity))

v1_18_R2/src/main/java/net/citizensnpcs/nms/v1_18_R2/util/NMSImpl.java

@@ -15,6 +15,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.dedicated.DedicatedPlayerList;
+import net.minecraft.server.dedicated.DedicatedServer;
+import net.minecraft.server.players.ServerOpList;
+import net.minecraft.server.players.ServerOpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1559,6 +1563,22 @@ public class NMSImpl implements NMSBridge {
         handle.setInvulnerableTicks(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final ServerPlayer playerHandle = ((CraftPlayer) player).getHandle();
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final ServerOpList opList = playerList.getOps();
+        if (op) {
+            opList.add(new ServerOpListEntry(profile, server.getOperatorUserPermissionLevel(), opList.canBypassPlayerLimit(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        playerList.sendPlayerPermissionLevel(playerHandle);
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof org.bukkit.entity.LivingEntity))

v1_19_R3/src/main/java/net/citizensnpcs/nms/v1_19_R3/util/NMSImpl.java

@@ -20,6 +20,10 @@ import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
+import net.minecraft.server.dedicated.DedicatedPlayerList;
+import net.minecraft.server.dedicated.DedicatedServer;
+import net.minecraft.server.players.ServerOpList;
+import net.minecraft.server.players.ServerOpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1763,6 +1767,22 @@ public class NMSImpl implements NMSBridge {
         handle.setInvulnerableTicks(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final ServerPlayer playerHandle = ((CraftPlayer) player).getHandle();
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final ServerOpList opList = playerList.getOps();
+        if (op) {
+            opList.add(new ServerOpListEntry(profile, server.getOperatorUserPermissionLevel(), opList.canBypassPlayerLimit(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        playerList.sendPlayerPermissionLevel(playerHandle);
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof org.bukkit.entity.LivingEntity))

v1_20_R4/src/main/java/net/citizensnpcs/nms/v1_20_R4/util/NMSImpl.java

@@ -18,6 +18,10 @@ import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
+import net.minecraft.server.dedicated.DedicatedPlayerList;
+import net.minecraft.server.dedicated.DedicatedServer;
+import net.minecraft.server.players.ServerOpList;
+import net.minecraft.server.players.ServerOpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1804,6 +1808,22 @@ public class NMSImpl implements NMSBridge {
         }
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final ServerPlayer playerHandle = ((CraftPlayer) player).getHandle();
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final ServerOpList opList = playerList.getOps();
+        if (op) {
+            opList.add(new ServerOpListEntry(profile, server.getOperatorUserPermissionLevel(), opList.canBypassPlayerLimit(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        playerList.sendPlayerPermissionLevel(playerHandle);
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof org.bukkit.entity.LivingEntity))

v1_21_R1/src/main/java/net/citizensnpcs/nms/v1_21_R1/util/NMSImpl.java

@@ -18,6 +18,10 @@ import java.util.function.Function;
 import java.util.function.Supplier;
 import java.util.stream.Collectors;
 
+import net.minecraft.server.dedicated.DedicatedPlayerList;
+import net.minecraft.server.dedicated.DedicatedServer;
+import net.minecraft.server.players.ServerOpList;
+import net.minecraft.server.players.ServerOpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1768,6 +1772,22 @@ public class NMSImpl implements NMSBridge {
         }
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final ServerPlayer playerHandle = ((CraftPlayer) player).getHandle();
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final ServerOpList opList = playerList.getOps();
+        if (op) {
+            opList.add(new ServerOpListEntry(profile, server.getOperatorUserPermissionLevel(), opList.canBypassPlayerLimit(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        playerList.sendPlayerPermissionLevel(playerHandle);
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof org.bukkit.entity.LivingEntity))

v1_8_R3/src/main/java/net/citizensnpcs/nms/v1_8_R3/util/NMSImpl.java

@@ -19,6 +19,10 @@ import java.util.function.Consumer;
 import java.util.function.Function;
 import java.util.function.Supplier;
 
+import net.minecraft.server.v1_8_R3.DedicatedPlayerList;
+import net.minecraft.server.v1_8_R3.DedicatedServer;
+import net.minecraft.server.v1_8_R3.OpList;
+import net.minecraft.server.v1_8_R3.OpListEntry;
 import org.bukkit.Bukkit;
 import org.bukkit.Location;
 import org.bukkit.Material;
@@ -1242,6 +1246,21 @@ public class NMSImpl implements NMSBridge {
         handle.r(ticks);
     }
 
+    @Override
+    public void setOpWithoutSaving(Player player, boolean op) {
+        if (player.isOp() == op) return;
+        final GameProfile profile = ((CraftPlayer) player).getProfile();
+        final DedicatedPlayerList playerList = ((CraftServer) player.getServer()).getHandle();
+        final DedicatedServer server = playerList.getServer();
+        final OpList opList = playerList.getOPs();
+        if (op) {
+            opList.add(new OpListEntry(profile, server.getPropertyManager().getInt("op-permission-level", 4), opList.b(profile)));
+        } else {
+            opList.remove(profile);
+        }
+        player.recalculatePermissions();
+    }
+
     @Override
     public boolean shouldJump(org.bukkit.entity.Entity entity) {
         if (JUMP_FIELD == null || !(entity instanceof LivingEntity))