fix: address the findings from a full-repo adversarial review

buildsystem-api/src/main/java/de/eintosti/buildsystem/api/world/access/WorldSetting.java

@@ -33,17 +33,17 @@ public enum WorldSetting {
     /**
      * Whether blocks may be broken in the world.
      */
-    BLOCK_BREAKING("buildsystem.bypass.settings", WorldDataKey.BLOCK_BREAKING),
+    BLOCK_BREAKING(bypassSettings(), WorldDataKey.BLOCK_BREAKING),
 
     /**
      * Whether blocks may be placed in the world.
      */
-    BLOCK_PLACEMENT("buildsystem.bypass.settings", WorldDataKey.BLOCK_PLACEMENT),
+    BLOCK_PLACEMENT(bypassSettings(), WorldDataKey.BLOCK_PLACEMENT),
 
     /**
      * Whether blocks may be interacted with in the world.
      */
-    BLOCK_INTERACTIONS("buildsystem.bypass.settings", WorldDataKey.BLOCK_INTERACTIONS);
+    BLOCK_INTERACTIONS(bypassSettings(), WorldDataKey.BLOCK_INTERACTIONS);
 
     private final String bypassPermission;
     private final WorldDataKey<Boolean> key;
@@ -53,6 +53,13 @@ public enum WorldSetting {
         this.key = key;
     }
 
+    /**
+     * The single source of truth for the permission node shared by every {@link WorldSetting}.
+     */
+    private static String bypassSettings() {
+        return "buildsystem.bypass.settings";
+    }
+
     /**
      * Gets the permission node that lets a player modify the world even when this setting would otherwise deny it.
      *

buildsystem-core/build.gradle.kts

@@ -240,7 +240,6 @@ bukkit {
         }
         register("buildsystem.create") {
             children = listOf(
-                "buildsystem.create.private",
                 "buildsystem.create.type.normal",
                 "buildsystem.create.type.flat",
                 "buildsystem.create.type.nether",
@@ -250,6 +249,11 @@ bukkit {
             description = "Permission for creating world types."
             default = BukkitPluginDescription.Permission.Default.TRUE
         }
+        register("buildsystem.create.category") {
+            description =
+                "Create a world in a navigator category. Category ids are dynamic; grant buildsystem.create.category.<id> to allow a specific category."
+            default = BukkitPluginDescription.Permission.Default.OP
+        }
         register("buildsystem.create.template") {
             description = "Select a template when creating a world. Template names are dynamic; deny buildsystem.create.template.<name> to restrict a specific template."
             default = BukkitPluginDescription.Permission.Default.TRUE

buildsystem-core/src/main/java/de/eintosti/buildsystem/BuildSystemPlugin.java

@@ -119,16 +119,19 @@ public void onDisable() {
 
         reloadConfigData(false);
         saveConfig();
-        try {
-            saveBuildConfig().join();
-        } catch (CompletionException e) {
-            getLogger().severe("Error while waiting for saves: " + e.getCause());
-        }
 
+        // Cancelling only stops future ticks, not one already running,
+        // so this must happen before the join() below
         if (this.configSaveTask != null) {
             this.configSaveTask.cancel();
         }
 
+        try {
+            saveBuildConfig().join();
+        } catch (CompletionException e) {
+            getLogger().severe("Error while waiting for saves: %s".formatted(e.getCause()));
+        }
+
         // Shut the shared background pool down only after the final saves above have completed.
         services.scheduler().shutdown();
 
@@ -149,26 +152,42 @@ private void performUpdateCheck() {
             return;
         }
 
-        updateChecker.requestUpdateCheck().whenComplete((result, e) -> {
-            if (result.requiresUpdate()) {
-                Bukkit.getConsoleSender()
-                        .sendMessage(ChatColor.YELLOW + "[BuildSystem] Great! a new update is available: "
-                                + ChatColor.GREEN + "v" + result.getNewestVersion());
-                Bukkit.getConsoleSender()
-                        .sendMessage(ChatColor.YELLOW + " ➥ Your current version: " + ChatColor.RED
-                                + this.getDescription().getVersion());
-                return;
-            }
-
-            UpdateChecker.UpdateReason reason = result.getReason();
-            switch (reason) {
-                case COULD_NOT_CONNECT, INVALID_JSON, UNAUTHORIZED_QUERY, UNKNOWN_ERROR, UNSUPPORTED_VERSION_SCHEME ->
-                    Bukkit.getConsoleSender()
-                            .sendMessage(ChatColor.RED
-                                    + "[BuildSystem] Could not check for a new version of BuildSystem. Reason: "
-                                    + reason);
-            }
-        });
+        updateChecker
+                .requestUpdateCheck()
+                .whenCompleteAsync(
+                        (result, e) -> {
+                            if (result == null) {
+                                return;
+                            }
+
+                            if (result.requiresUpdate()) {
+                                Bukkit.getConsoleSender()
+                                        .sendMessage("%s[BuildSystem] Great! a new update is available: %sv%s"
+                                                .formatted(
+                                                        ChatColor.YELLOW, ChatColor.GREEN, result.getNewestVersion()));
+                                Bukkit.getConsoleSender()
+                                        .sendMessage("%s ➥ Your current version: %s%s"
+                                                .formatted(
+                                                        ChatColor.YELLOW,
+                                                        ChatColor.RED,
+                                                        this.getDescription().getVersion()));
+                                return;
+                            }
+
+                            UpdateChecker.UpdateReason reason = result.getReason();
+                            switch (reason) {
+                                case COULD_NOT_CONNECT,
+                                        INVALID_JSON,
+                                        UNAUTHORIZED_QUERY,
+                                        UNKNOWN_ERROR,
+                                        UNSUPPORTED_VERSION_SCHEME ->
+                                    Bukkit.getConsoleSender()
+                                            .sendMessage(
+                                                    "%s[BuildSystem] Could not check for a new version of BuildSystem. Reason: %s"
+                                                            .formatted(ChatColor.RED, reason));
+                            }
+                        },
+                        services.scheduler().mainThread());
     }
 
     private void createTemplateFolder() {

buildsystem-core/src/main/java/de/eintosti/buildsystem/command/subcommand/worlds/AddBuilderSubCommand.java

@@ -63,16 +63,13 @@ public AddBuilderSubCommand(
     public void execute(Player player, String worldName, String[] args) {
         BuildWorld buildWorld =
                 worldService.getWorldStorage().getBuildWorld(player.getWorld().getName());
-        if (buildWorld != null
-                && !buildWorld
-                        .getPermissions()
-                        .canPerformCommand(player, getArgument().getPermission())) {
-            messages.sendPermissionError(player);
+        if (buildWorld == null) {
+            messages.sendMessage(player, "worlds_addbuilder_unknown_world");
             return;
         }
 
-        if (buildWorld == null) {
-            messages.sendMessage(player, "worlds_addbuilder_unknown_world");
+        if (!hasAddBuilderPermission(player, buildWorld)) {
+            messages.sendPermissionError(player);
             return;
         }
 
@@ -146,12 +143,27 @@ private void applyBuilder(
     }
 
     public void getAddBuilderInput(Player player, BuildWorld buildWorld, boolean closeInventory) {
+        if (!hasAddBuilderPermission(player, buildWorld)) {
+            messages.sendPermissionError(player);
+            return;
+        }
+
         prompts.prompt(player).title("enter_player_name").request(input -> {
             String builderName = input.trim();
             addBuilder(player, buildWorld, builderName, closeInventory);
         });
     }
 
+    /**
+     * Whether {@code player} may add builders to {@code buildWorld}, per the argument's permission node. Shared by the
+     * command entry point and the GUI prompt so both are gated by the same check.
+     */
+    private boolean hasAddBuilderPermission(Player player, BuildWorld buildWorld) {
+        return buildWorld
+                .getPermissions()
+                .canPerformCommand(player, getArgument().getPermission());
+    }
+
     @Override
     public List<String> complete(Player player, String[] args) {
         if (args.length != 2) {

buildsystem-core/src/main/java/de/eintosti/buildsystem/command/subcommand/worlds/SetPermissionSubCommand.java

@@ -89,6 +89,13 @@ public void getPermissionInput(Player player, BuildWorld buildWorld, boolean clo
             XSound.ENTITY_PLAYER_LEVELUP.play(player);
             messages.sendMessage(player, "worlds_setpermission_set", Placeholders.of("%world%", buildWorld.getName()));
 
+            // A folder override means the stored permission is not the one being enforced.
+            String effective = buildWorld.getData().get(WorldDataKey.PERMISSION);
+            if (!permission.equals(effective)) {
+                messages.sendMessage(
+                        player, "worlds_setpermission_overridden", Placeholders.of("%permission%", effective));
+            }
+
             if (closeInventory) {
                 player.closeInventory();
             } else {

buildsystem-core/src/main/java/de/eintosti/buildsystem/command/subcommand/worlds/SetProjectSubCommand.java

@@ -64,12 +64,19 @@ public void execute(Player player, String worldName, String[] args) {
 
     public void getProjectInput(Player player, BuildWorld buildWorld, boolean closeInventory) {
         prompts.prompt(player).title("enter_world_project").request(input -> {
-            buildWorld.getData().set(WorldDataKey.PROJECT, input.trim());
+            String project = input.trim();
+            buildWorld.getData().set(WorldDataKey.PROJECT, project);
             settingsService.forceUpdateSidebar(buildWorld);
 
             XSound.ENTITY_PLAYER_LEVELUP.play(player);
             messages.sendMessage(player, "worlds_setproject_set", Placeholders.of("%world%", buildWorld.getName()));
 
+            // The world's folder can override this value, in which case the stored project is not the one shown.
+            String effective = buildWorld.getData().get(WorldDataKey.PROJECT);
+            if (!project.equals(effective)) {
+                messages.sendMessage(player, "worlds_setproject_overridden", Placeholders.of("%project%", effective));
+            }
+
             if (closeInventory) {
                 player.closeInventory();
             } else {

buildsystem-core/src/main/java/de/eintosti/buildsystem/config/PluginConfig.java

@@ -77,6 +77,14 @@ public record World(
          */
         public record VoidBlock(boolean enabled, Material material) {}
 
+        /**
+         * Fallback world-creation limits, applied only to players holding no
+         * {@code buildsystem.create.<visibility>.<amount>} permission node, so a permission grant always wins. Counted
+         * per player and per visibility; {@code -1} means unlimited.
+         *
+         * @param publicWorlds Default maximum number of public worlds one player may create
+         * @param privateWorlds Default maximum number of private worlds one player may create
+         */
         public record Limits(int publicWorlds, int privateWorlds) {}
 
         public record Defaults(
@@ -133,7 +141,17 @@ public record Sftp(
                     @Nullable String username,
                     @Nullable String password,
                     @Nullable String path)
-                    implements StorageSettings {}
+                    implements StorageSettings {
+
+                /**
+                 * Overridden so the password never appears in a log or pasted support output.
+                 */
+                @Override
+                public String toString() {
+                    return "Sftp[host=%s, port=%d, username=%s, password=%s, path=%s]"
+                            .formatted(host, port, username, password == null ? null : "***", path);
+                }
+            }
 
             public record S3(
                     @Nullable String url,
@@ -142,7 +160,23 @@ public record S3(
                     @Nullable String region,
                     @Nullable String bucket,
                     @Nullable String path)
-                    implements StorageSettings {}
+                    implements StorageSettings {
+
+                /**
+                 * Overridden so the access key and secret key never appear in a log or pasted support output.
+                 */
+                @Override
+                public String toString() {
+                    return "S3[url=%s, accessKey=%s, secretKey=%s, region=%s, bucket=%s, path=%s]"
+                            .formatted(
+                                    url,
+                                    accessKey == null ? null : "***",
+                                    secretKey == null ? null : "***",
+                                    region,
+                                    bucket,
+                                    path);
+                }
+            }
 
             public record AutoBackup(boolean enabled, boolean onlyActiveWorlds, int interval) {}
         }

buildsystem-core/src/main/java/de/eintosti/buildsystem/config/migration/ConfigMigrationManager.java

@@ -18,9 +18,14 @@
 package de.eintosti.buildsystem.config.migration;
 
 import de.eintosti.buildsystem.BuildSystemPlugin;
+import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.StandardCopyOption;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.logging.Level;
 import java.util.logging.Logger;
 import org.jspecify.annotations.NullMarked;
 
@@ -68,22 +73,60 @@ private void registerMigration(int fromVersion, Migration migration) {
      */
     public void migrate() {
         Logger logger = plugin.getLogger();
+        int fromVersion = plugin.getConfig().getInt("version", 1);
+        if (fromVersion >= LATEST_VERSION) {
+            logger.info("Config is at the latest version: %d".formatted(fromVersion));
+            return;
+        }
+
+        if (!backupConfig(fromVersion, logger)) {
+            return;
+        }
 
         while (plugin.getConfig().getInt("version", 1) < LATEST_VERSION) {
             int currentVersion = plugin.getConfig().getInt("version", 1);
             Migration migration = migrations.get(currentVersion);
             if (migration == null) {
                 throw new IllegalStateException(
-                        "Missing migration from version " + currentVersion + " to " + (currentVersion + 1));
+                        "Missing migration from version %d to %d".formatted(currentVersion, currentVersion + 1));
             }
 
-            logger.info("Migrating from version " + currentVersion + " to " + (currentVersion + 1) + "...");
+            logger.info("Migrating from version %d to %d...".formatted(currentVersion, currentVersion + 1));
             migration.migrate(plugin.getConfig());
             plugin.getConfig().set("version", currentVersion + 1);
             plugin.getConfig().setComments("version", List.of("Internal, do not change manually!"));
             plugin.saveConfig();
         }
 
-        logger.info("Config is at the latest version: " + plugin.getConfig().getInt("version", 1));
+        logger.info("Config is at the latest version: %d"
+                .formatted(plugin.getConfig().getInt("version", 1)));
+    }
+
+    /**
+     * Copies {@code config.yml} to a sibling {@code config.yml.v<fromVersion>.bak} before the first migration
+     * mutates it in place.
+     *
+     * @return {@code true} if the backup succeeded (or the config file does not exist yet), {@code false} if
+     *     migration must be aborted because the pre-migration state could not be preserved
+     */
+    private boolean backupConfig(int fromVersion, Logger logger) {
+        File configFile = new File(plugin.getDataFolder(), "config.yml");
+        if (!configFile.exists()) {
+            return true;
+        }
+
+        File backup = new File(plugin.getDataFolder(), "config.yml.v%d.bak".formatted(fromVersion));
+        try {
+            Files.copy(configFile.toPath(), backup.toPath(), StandardCopyOption.REPLACE_EXISTING);
+        } catch (IOException e) {
+            logger.log(
+                    Level.SEVERE,
+                    "Failed to back up config.yml to %s; aborting migration".formatted(backup.getName()),
+                    e);
+            return false;
+        }
+
+        logger.info("Backed up config.yml to %s".formatted(backup.getName()));
+        return true;
     }
 }

buildsystem-core/src/main/java/de/eintosti/buildsystem/listener/player/PlayerJoinListener.java

@@ -216,16 +216,21 @@ private void performUpdateCheck(Player player) {
             return;
         }
 
-        updateChecker.requestUpdateCheck().whenComplete((result, e) -> {
-            if (result.requiresUpdate()) {
-                StringBuilder stringBuilder = new StringBuilder();
-                messages.getStringList("update_available", player)
-                        .forEach(line -> stringBuilder
-                                .append(line.replace("%new_version%", result.getNewestVersion())
-                                        .replace("%current_version%", updateChecker.getCurrentVersion()))
-                                .append("\n"));
-                player.sendMessage(stringBuilder.toString());
-            }
-        });
+        updateChecker
+                .requestUpdateCheck()
+                .whenCompleteAsync(
+                        (result, e) -> {
+                            if (result == null || !result.requiresUpdate()) {
+                                return;
+                            }
+
+                            Placeholders placeholders = Placeholders.of()
+                                    .add("%new_version%", result.getNewestVersion())
+                                    .add("%current_version%", updateChecker.getCurrentVersion())
+                                    .build();
+                            player.sendMessage(String.join(
+                                    "\n", messages.getStringList("update_available", player, placeholders)));
+                        },
+                        scheduler.mainThread());
     }
 }