Merge pull request #250 from aether-framework/bugfix/190-191-193-194-196-197-198-207-208-221-mixed-fixes

Improve validation and error handling in CLI commands

Author: Splatcrafter

aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/bootstrap/BootstrapLoader.java

@@ -25,6 +25,8 @@
 import com.google.common.base.Preconditions;
 import de.splatgames.aether.datafixers.api.bootstrap.DataFixerBootstrap;
 import org.jetbrains.annotations.NotNull;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
 
 import java.lang.reflect.Constructor;
 import java.util.ServiceLoader;
@@ -69,6 +71,8 @@
  */
 public final class BootstrapLoader {
 
+    private static final Logger LOG = LoggerFactory.getLogger(BootstrapLoader.class);
+
     /**
      * Private constructor to prevent instantiation.
      *
@@ -111,6 +115,11 @@ private BootstrapLoader() {
     public static DataFixerBootstrap load(@NotNull final String className) {
         Preconditions.checkNotNull(className, "className must not be null");
 
+        // Security note: Class.forName() will load and instantiate any class on the classpath
+        // that implements DataFixerBootstrap. Only use bootstrap classes from trusted sources.
+        LOG.warn("Loading bootstrap class '{}' via reflection. "
+                + "Ensure this class is from a trusted source.", className);
+
         try {
             final Class<?> clazz = Class.forName(className);
 

aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/MigrateCommand.java

@@ -434,6 +434,16 @@ public class MigrateCommand implements Callable<Integer> {
     @Override
     public Integer call() {
         try {
+            // 0. Validate version range
+            if (this.toVersion < 0) {
+                System.err.println("Error: --to version must be non-negative, got: " + this.toVersion);
+                return 1;
+            }
+            if (this.fromVersion != null && this.fromVersion < 0) {
+                System.err.println("Error: --from version must be non-negative, got: " + this.fromVersion);
+                return 1;
+            }
+
             // 1. Load bootstrap
             final DataFixerBootstrap bootstrap = BootstrapLoader.load(this.bootstrapClass);
 
@@ -545,8 +555,17 @@ private <T> MigrationResult processFile(
 
         final Instant startTime = Instant.now();
 
-        // Read input
-        final String content = Files.readString(inputFile.toPath());
+        // Check file size to prevent OOM on very large files
+        final long fileSize = Files.size(inputFile.toPath());
+        if (fileSize > 100 * 1024 * 1024) {
+            throw new IOException("File exceeds maximum size (100MB): " + inputFile);
+        }
+
+        // Read input (strip UTF-8 BOM if present)
+        String content = Files.readString(inputFile.toPath());
+        if (content.startsWith("\uFEFF")) {
+            content = content.substring(1);
+        }
         final T data = handler.parse(content);
 
         // Determine source version
@@ -638,7 +657,14 @@ private void writeOutput(@NotNull final File inputFile, @NotNull final String co
         Preconditions.checkNotNull(inputFile, "inputFile must not be null");
         Preconditions.checkNotNull(content, "content must not be null");
         if (this.output != null) {
-            // Write to specified output
+            // Validate output path: reject path traversal via unnormalized segments
+            final Path outputPath = this.output.toPath();
+            if (!outputPath.normalize().equals(outputPath)
+                    || outputPath.toString().contains("..")) {
+                throw new IOException(
+                        "Output path contains path traversal: " + this.output);
+            }
+
             if (this.output.isDirectory()) {
                 final Path outPath = this.output.toPath().resolve(inputFile.getName());
                 Files.writeString(outPath, content);
@@ -648,17 +674,25 @@ private void writeOutput(@NotNull final File inputFile, @NotNull final String co
                 throw new IllegalArgumentException(
                         "Output must be a directory when multiple input files are specified");
             }
-        } else if (this.inputFiles.size() == 1 && this.output == null) {
+        } else if (this.inputFiles.size() == 1) {
             // Single file with no output: stdout
             System.out.println(content);
         } else {
-            // Multiple files: in-place with backup
-            if (this.backup) {
-                final Path backupPath = inputFile.toPath().resolveSibling(
-                        inputFile.getName() + ".bak");
-                Files.copy(inputFile.toPath(), backupPath, StandardCopyOption.REPLACE_EXISTING);
+            // Multiple files: in-place with atomic write via temp file
+            final Path tempPath = Files.createTempFile(
+                    inputFile.toPath().getParent(), "migrate_", ".tmp");
+            try {
+                Files.writeString(tempPath, content);
+                if (this.backup) {
+                    final Path backupPath = inputFile.toPath().resolveSibling(
+                            inputFile.getName() + ".bak");
+                    Files.move(inputFile.toPath(), backupPath, StandardCopyOption.REPLACE_EXISTING);
+                }
+                Files.move(tempPath, inputFile.toPath(), StandardCopyOption.REPLACE_EXISTING);
+            } catch (final IOException e) {
+                Files.deleteIfExists(tempPath);
+                throw e;
             }
-            Files.writeString(inputFile.toPath(), content);
         }
     }
 

aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/command/ValidateCommand.java

@@ -35,6 +35,7 @@
 import picocli.CommandLine.Parameters;
 
 import java.io.File;
+import java.io.IOException;
 import java.nio.file.Files;
 import java.util.List;
 import java.util.concurrent.Callable;
@@ -232,6 +233,11 @@ public class ValidateCommand implements Callable<Integer> {
      */
     @Override
     public Integer call() {
+        if (this.toVersion < 0) {
+            System.err.println("Error: --to version must be non-negative, got: " + this.toVersion);
+            return 1;
+        }
+
         try {
             final DataFixerBootstrap bootstrap = BootstrapLoader.load(this.bootstrapClass);
             final DataVersion targetVersion = new DataVersion(this.toVersion);
@@ -314,7 +320,14 @@ private <T> ValidationResult validateFile(
             final DataVersion targetVersion
     ) {
         try {
-            final String content = Files.readString(file.toPath());
+            final long fileSize = Files.size(file.toPath());
+            if (fileSize > 100 * 1024 * 1024) {
+                throw new IOException("File exceeds maximum size (100MB): " + file);
+            }
+            String content = Files.readString(file.toPath());
+            if (content.startsWith("\uFEFF")) {
+                content = content.substring(1);
+            }
             final T data = handler.parse(content);
 
             final DataVersion fileVersion = VersionExtractor.extract(

aether-datafixers-cli/src/main/java/de/splatgames/aether/datafixers/cli/format/FormatRegistry.java

@@ -158,7 +158,7 @@ private FormatRegistry() {
      * @param handler the format handler to register, must not be {@code null}
      * @see FormatHandler#formatId()
      */
-    public static void register(@NotNull final FormatHandler<?> handler) {
+    public static synchronized void register(@NotNull final FormatHandler<?> handler) {
         Preconditions.checkNotNull(handler, "handler must not be null");
 
         final String id = handler.formatId();

aether-datafixers-spring-boot-starter/src/main/java/de/splatgames/aether/datafixers/spring/actuator/DataFixerEndpoint.java

@@ -245,7 +245,9 @@ public DataFixersSummary summary() {
     public DomainDetails domainDetails(@Selector final String domain) {
         final AetherDataFixer fixer = this.registry.get(domain);
         if (fixer == null) {
-            return null; // Spring will return 404
+            // Returning null from @ReadOperation with @Selector produces HTTP 404
+            // (Spring Boot Actuator convention for missing resources)
+            return null;
         }
 
         try {

aether-datafixers-testkit/src/main/java/de/splatgames/aether/datafixers/testkit/assertion/DataResultAssert.java

@@ -326,7 +326,7 @@ public DataResultAssert<A> hasErrorMessageMatching(@NotNull final String regex)
     @NotNull
     public AbstractObjectAssert<?, A> extractingValue() {
         this.isSuccess();
-        return Assertions.assertThat(this.actual.result().orElse(null));
+        return Assertions.assertThat(this.actual.result().orElseThrow());
     }
 
     /**

aether-datafixers-testkit/src/main/java/de/splatgames/aether/datafixers/testkit/assertion/TypedAssert.java

@@ -215,9 +215,16 @@ public TypedAssert<A> hasValueInstanceOf(@NotNull final Class<?> expectedClass)
     @NotNull
     public <T> DynamicAssert<T> encodedWith(@NotNull final DynamicOps<T> ops) {
         isNotNull();
-        final Dynamic<T> encoded = this.actual.encode(ops)
-                .getOrThrow(msg -> new AssertionError("Failed to encode Typed value: " + msg));
-        return new DynamicAssert<>(encoded);
+        try {
+            final Dynamic<T> encoded = this.actual.encode(ops)
+                    .getOrThrow(msg -> new AssertionError("Failed to encode Typed value: " + msg));
+            return new DynamicAssert<>(encoded);
+        } catch (final AssertionError e) {
+            throw e;
+        } catch (final Exception e) {
+            // Preserve the original exception stack trace as the cause
+            throw new AssertionError("Failed to encode Typed value: " + e.getMessage(), e);
+        }
     }
 
     // ==================== Extraction ====================