fix: automatic backup implemented security fixes + validation

Author: lvca

server/src/main/java/com/arcadedb/server/backup/AutoBackupSchedulerPlugin.java

@@ -26,6 +26,10 @@
 import com.arcadedb.server.event.ServerEventLog;
 
 import java.io.File;
+import java.io.IOException;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Set;
 import java.util.logging.Level;
 
@@ -88,40 +92,19 @@ public void startService() {
       return;
     }
 
-    // Validate and resolve backup directory
+    // Validate and resolve backup directory using consolidated security validation
     String backupDirectory = backupConfig.getBackupDirectory();
-    final java.nio.file.Path backupPath = java.nio.file.Paths.get(backupDirectory);
+    final Path serverRoot = Paths.get(server.getRootPath()).toAbsolutePath().normalize();
 
-    // Reject absolute paths for security
-    if (backupPath.isAbsolute()) {
-      throw new IllegalArgumentException(
-          "Backup directory must be a relative path, not absolute: " + backupDirectory);
-    }
+    // Validate and get the resolved, secure path
+    final Path resolvedPath = validateAndResolveBackupPath(backupDirectory, serverRoot);
+    backupDirectory = resolvedPath.toString();
 
-    // Reject path traversal attempts
-    if (backupDirectory.contains("..")) {
-      throw new IllegalArgumentException(
-          "Backup directory cannot contain path traversal (..): " + backupDirectory);
-    }
-
-    // Resolve relative path against server root
-    backupDirectory = java.nio.file.Paths.get(server.getRootPath(), backupDirectory).toString();
-
-    // Final validation: ensure resolved path is within server root
-    final java.nio.file.Path resolvedPath = java.nio.file.Paths.get(backupDirectory).toAbsolutePath().normalize();
-    final java.nio.file.Path serverRoot = java.nio.file.Paths.get(server.getRootPath()).toAbsolutePath().normalize();
-
-    if (!resolvedPath.startsWith(serverRoot)) {
-      throw new IllegalArgumentException(
-          "Backup directory must be within server root path for security reasons: " + backupDirectory);
-    }
-
-    // Ensure backup directory exists
-    final File backupDir = new File(backupDirectory);
-    if (!backupDir.exists()) {
-      if (!backupDir.mkdirs()) {
-        throw new RuntimeException("Failed to create backup directory: " + backupDirectory);
-      }
+    // Ensure backup directory exists - use Files.createDirectories to avoid TOCTOU
+    try {
+      Files.createDirectories(resolvedPath);
+    } catch (final IOException e) {
+      throw new RuntimeException("Failed to create backup directory: " + backupDirectory, e);
     }
 
     // Initialize retention manager
@@ -270,4 +253,60 @@ public void reloadConfiguration() {
 
     LogManager.instance().log(this, Level.INFO, "Auto-backup configuration reloaded");
   }
+
+  /**
+   * Validates and resolves a backup directory path with comprehensive security checks.
+   * <p>
+   * Security checks performed:
+   * 1. Reject absolute paths
+   * 2. Normalize path and reject if it escapes via ..
+   * 3. Resolve against server root
+   * 4. Verify final path is within server root
+   * 5. Check for symlinks that could escape the root
+   *
+   * @param backupDir  The backup directory path from configuration
+   * @param serverRoot The server root directory (absolute, normalized)
+   * @return The validated, resolved absolute path
+   * @throws IllegalArgumentException if the path fails security validation
+   */
+  public static Path validateAndResolveBackupPath(final String backupDir, final Path serverRoot) {
+    if (backupDir == null || backupDir.isEmpty())
+      throw new IllegalArgumentException("Backup directory cannot be empty");
+
+    final Path inputPath = Paths.get(backupDir);
+
+    // 1. Reject absolute paths
+    if (inputPath.isAbsolute())
+      throw new IllegalArgumentException(
+          "Backup directory must be a relative path, not absolute: " + backupDir);
+
+    // 2. Normalize and check for path traversal
+    final Path normalizedInput = inputPath.normalize();
+    if (normalizedInput.startsWith(".."))
+      throw new IllegalArgumentException(
+          "Backup directory cannot escape server root via path traversal: " + backupDir);
+
+    // 3. Resolve against server root
+    final Path resolvedPath = serverRoot.resolve(normalizedInput).normalize();
+
+    // 4. Verify resolved path is within server root
+    if (!resolvedPath.startsWith(serverRoot))
+      throw new IllegalArgumentException(
+          "Backup directory must be within server root path: " + backupDir);
+
+    // 5. If path exists, check for symlinks that could escape the root
+    if (Files.exists(resolvedPath)) {
+      try {
+        final Path realPath = resolvedPath.toRealPath();
+        if (!realPath.startsWith(serverRoot))
+          throw new IllegalArgumentException(
+              "Backup directory symlink resolves outside server root: " + backupDir);
+      } catch (final IOException e) {
+        throw new IllegalArgumentException(
+            "Cannot resolve backup directory path: " + backupDir, e);
+      }
+    }
+
+    return resolvedPath;
+  }
 }

server/src/main/java/com/arcadedb/server/backup/BackupRetentionManager.java

@@ -104,34 +104,52 @@ public int applyRetention(final String databaseName) {
 
     // Delete files not in the keep set
     int deletedCount = 0;
+    int failedCount = 0;
     for (final BackupFileInfo info : backupFiles) {
       if (!filesToKeep.contains(info.file)) {
-        if (info.file.delete()) {
+        try {
+          java.nio.file.Files.delete(info.file.toPath());
           deletedCount++;
           LogManager.instance().log(this, Level.INFO,
               "Deleted old backup: %s", info.file.getName());
-        } else {
+        } catch (final java.io.IOException e) {
+          failedCount++;
           LogManager.instance().log(this, Level.WARNING,
-              "Failed to delete old backup: %s", info.file.getName());
+              "Failed to delete old backup '%s': %s", info.file.getName(), e.getMessage());
         }
       }
     }
 
+    if (failedCount > 0) {
+      LogManager.instance().log(this, Level.WARNING,
+          "Retention for database '%s' completed with %d deletion failures", databaseName, failedCount);
+    }
+
     LogManager.instance().log(this, Level.INFO,
         "Retention applied for database '%s': kept %d, deleted %d backups",
         databaseName, filesToKeep.size(), deletedCount);
 
     return deletedCount;
   }
 
+  // Maximum number of backup files to process to prevent unbounded memory usage
+  private static final int MAX_BACKUP_FILES_TO_PROCESS = 10000;
+
   /**
    * Gets all backup files for a database directory, sorted by timestamp.
+   * Limited to MAX_BACKUP_FILES_TO_PROCESS to prevent unbounded memory usage.
    */
   private List<BackupFileInfo> getBackupFiles(final File dbBackupDir) {
     final File[] files = dbBackupDir.listFiles(BACKUP_FILE_FILTER);
     if (files == null || files.length == 0)
       return Collections.emptyList();
 
+    if (files.length > MAX_BACKUP_FILES_TO_PROCESS) {
+      LogManager.instance().log(this, Level.WARNING,
+          "Database backup directory contains %d files, processing only the most recent %d",
+          files.length, MAX_BACKUP_FILES_TO_PROCESS);
+    }
+
     final List<BackupFileInfo> backupFiles = new ArrayList<>();
     for (final File file : files) {
       final LocalDateTime timestamp = parseBackupTimestamp(file.getName());
@@ -141,6 +159,13 @@ private List<BackupFileInfo> getBackupFiles(final File dbBackupDir) {
 
     // Sort by timestamp (oldest first)
     backupFiles.sort(Comparator.comparing(info -> info.timestamp));
+
+    // Limit to most recent files if too many
+    if (backupFiles.size() > MAX_BACKUP_FILES_TO_PROCESS) {
+      final int startIndex = backupFiles.size() - MAX_BACKUP_FILES_TO_PROCESS;
+      return new ArrayList<>(backupFiles.subList(startIndex, backupFiles.size()));
+    }
+
     return backupFiles;
   }
 

server/src/main/java/com/arcadedb/server/backup/BackupScheduler.java

@@ -22,6 +22,8 @@
 import com.arcadedb.server.ArcadeDBServer;
 
 import java.time.LocalDateTime;
+import java.util.ArrayList;
+import java.util.List;
 import java.util.Map;
 import java.util.concurrent.*;
 import java.util.logging.Level;
@@ -153,7 +155,8 @@ private void scheduleNextCronExecution(final String databaseName, final BackupTa
         task.run();
       } finally {
         // Schedule the next execution only if still running and parser is still registered
-        if (running && cronParsers.containsKey(databaseName)) {
+        // Use atomic get to avoid race condition between containsKey and get
+        if (running) {
           final CronScheduleParser currentParser = cronParsers.get(databaseName);
           if (currentParser != null)
             scheduleNextCronExecution(databaseName, task, currentParser);
@@ -202,8 +205,9 @@ public void triggerImmediateBackup(final String databaseName, final DatabaseBack
   public void stop() {
     running = false;
 
-    // Cancel all scheduled tasks
-    for (final ScheduledFuture<?> future : scheduledTasks.values())
+    // Cancel all scheduled tasks - create copy to avoid ConcurrentModificationException
+    final List<ScheduledFuture<?>> tasksToCancel = new ArrayList<>(scheduledTasks.values());
+    for (final ScheduledFuture<?> future : tasksToCancel)
       future.cancel(false);
     scheduledTasks.clear();
     cronParsers.clear();

server/src/main/java/com/arcadedb/server/backup/BackupTask.java

@@ -25,8 +25,9 @@
 import com.arcadedb.server.event.ServerEventLog;
 import com.arcadedb.server.ha.HAServer;
 
-import java.io.File;
+import java.lang.reflect.Constructor;
 import java.lang.reflect.InvocationTargetException;
+import java.lang.reflect.Method;
 import java.time.LocalDateTime;
 import java.time.LocalTime;
 import java.time.format.DateTimeFormatter;
@@ -40,6 +41,15 @@
 public class BackupTask implements Runnable {
   private static final DateTimeFormatter BACKUP_TIMESTAMP_FORMAT = DateTimeFormatter.ofPattern("yyyyMMdd-HHmmss");
 
+  // Cached reflection objects for better performance
+  private static volatile Class<?>                     backupClass;
+  private static volatile Constructor<?>               backupConstructor;
+  private static volatile Method                       setDirectoryMethod;
+  private static volatile Method                       setVerboseLevelMethod;
+  private static volatile Method                       backupDatabaseMethod;
+  private static volatile boolean                      reflectionInitialized;
+  private static final    Object                       REFLECTION_LOCK = new Object();
+
   private final ArcadeDBServer         server;
   private final String                 databaseName;
   private final DatabaseBackupConfig   config;
@@ -150,48 +160,79 @@ private boolean isWithinTimeWindow() {
 
   /**
    * Performs the actual backup using the integration Backup class.
+   * <p>
+   * Note: The backup mechanism in ArcadeDB reads from immutable pages and handles
+   * consistency internally. The transaction check is a safety warning but does not
+   * block new transactions - the backup is designed to be non-blocking.
    */
   private String performBackup() throws Exception {
     final Database database = server.getDatabase(databaseName);
 
-    // Check for active transaction - never automatically rollback as it could cause data loss
-    if (database.isTransactionActive() && ((DatabaseInternal) database).getTransaction().hasChanges()) {
-      throw new BackupException("Cannot perform backup for database '" + databaseName +
-          "': active transaction with pending changes detected. Please commit or rollback the transaction manually.");
+    // Check for active transaction - warn but don't block
+    // ArcadeDB backup is designed to work on immutable pages, so this is informational
+    if (database.isTransactionActive()) {
+      final DatabaseInternal dbInternal = (DatabaseInternal) database;
+      if (dbInternal.getTransaction().hasChanges()) {
+        LogManager.instance().log(this, Level.WARNING,
+            "Backup for database '%s' starting with active transaction - uncommitted changes will not be included",
+            databaseName);
+      }
     }
 
     // Generate backup filename
     final String timestamp = LocalDateTime.now().format(BACKUP_TIMESTAMP_FORMAT);
     final String backupFileName = databaseName + "-backup-" + timestamp + ".zip";
 
-    // Prepare backup directory for this database
-    final String dbBackupDir = java.nio.file.Paths.get(backupDirectory, databaseName).toString();
-    final File backupDirFile = new File(dbBackupDir);
-    if (!backupDirFile.exists()) {
-      if (!backupDirFile.mkdirs()) {
-        throw new BackupException("Failed to create backup directory for database '" + databaseName + "': " + dbBackupDir);
-      }
+    // Prepare backup directory for this database - use Files.createDirectories to avoid TOCTOU
+    final java.nio.file.Path dbBackupPath = java.nio.file.Paths.get(backupDirectory, databaseName);
+    try {
+      java.nio.file.Files.createDirectories(dbBackupPath);
+    } catch (final java.io.IOException e) {
+      throw new BackupException("Failed to create backup directory for database '" + databaseName + "': " + dbBackupPath, e);
     }
+    final String dbBackupDir = dbBackupPath.toString();
 
+    // Perform backup using cached reflection for better performance
     try {
-      final Class<?> clazz = Class.forName("com.arcadedb.integration.backup.Backup");
-      final Object backup = clazz.getConstructor(Database.class, String.class)
-          .newInstance(database, backupFileName);
+      initializeReflection();
 
-      clazz.getMethod("setDirectory", String.class).invoke(backup, dbBackupDir);
-      clazz.getMethod("setVerboseLevel", Integer.TYPE).invoke(backup, 1);
+      final Object backup = backupConstructor.newInstance(database, backupFileName);
+      setDirectoryMethod.invoke(backup, dbBackupDir);
+      setVerboseLevelMethod.invoke(backup, 1);
 
-      final String backupFile = (String) clazz.getMethod("backupDatabase").invoke(backup);
-      return backupFile;
+      return (String) backupDatabaseMethod.invoke(backup);
 
-    } catch (final ClassNotFoundException | NoSuchMethodException | IllegalAccessException | InstantiationException e) {
+    } catch (final IllegalAccessException | InstantiationException e) {
       throw new BackupException("Backup libs not found in classpath. Make sure arcadedb-integration module is " +
           "included.", e);
     } catch (final InvocationTargetException e) {
       throw new BackupException("Error performing backup for database '" + databaseName + "'", e.getTargetException());
     }
   }
 
+  /**
+   * Initializes the cached reflection objects for the Backup class.
+   * Uses double-checked locking for thread-safe lazy initialization.
+   */
+  private static void initializeReflection() throws BackupException {
+    if (!reflectionInitialized) {
+      synchronized (REFLECTION_LOCK) {
+        if (!reflectionInitialized) {
+          try {
+            backupClass = Class.forName("com.arcadedb.integration.backup.Backup");
+            backupConstructor = backupClass.getConstructor(Database.class, String.class);
+            setDirectoryMethod = backupClass.getMethod("setDirectory", String.class);
+            setVerboseLevelMethod = backupClass.getMethod("setVerboseLevel", Integer.TYPE);
+            backupDatabaseMethod = backupClass.getMethod("backupDatabase");
+            reflectionInitialized = true;
+          } catch (final ClassNotFoundException | NoSuchMethodException e) {
+            throw new BackupException("Backup libs not found in classpath. Make sure arcadedb-integration module is included.", e);
+          }
+        }
+      }
+    }
+  }
+
   public String getDatabaseName() {
     return databaseName;
   }

server/src/main/java/com/arcadedb/server/backup/CronScheduleParser.java

@@ -139,10 +139,11 @@ public LocalDateTime getNextExecutionTime(final LocalDateTime from) {
     LocalDateTime next = from.plusSeconds(1).withNano(0);
 
     // Find next matching time by incrementing fields
-    int iterations = 0;
-    final int maxIterations = 366 * 24 * 60 * 60; // One year of seconds max
+    // Use a reasonable limit: 4 years should cover any valid CRON expression
+    // This prevents potential infinite loops from impossible CRON expressions
+    final LocalDateTime maxDate = from.plusYears(4);
 
-    while (iterations++ < maxIterations) {
+    while (next.isBefore(maxDate)) {
       // Check month
       if (!months.get(next.getMonthValue())) {
         next = next.plusMonths(1).withDayOfMonth(1).withHour(0).withMinute(0).withSecond(0);
@@ -184,7 +185,9 @@ public LocalDateTime getNextExecutionTime(final LocalDateTime from) {
       return next;
     }
 
-    throw new IllegalStateException("Could not find next execution time for CRON expression: " + expression);
+    throw new IllegalStateException(
+        "Could not find next execution time within 4 years for CRON expression '" + expression +
+            "'. The expression may be invalid or specify an impossible schedule.");
   }
 
   /**