jmsperu commented on PR #12900: URL: https://github.com/apache/cloudstack/pull/12900#issuecomment-4762925426
Thanks for the review @DaanHoogland (and the Copilot pass). Pushed `5b82495` addressing the open points: - **Command injection / password in process list** (`backupDatabase`): `mysqldump` now runs without a shell (`ProcessBuilder` arg list) and reads credentials from a `0600` `--defaults-extra-file`, so values from `db.properties` can't be shell-interpreted and the password no longer appears in the process list. The dump is gzipped in-process. - **`ArrayIndexOutOfBoundsException` on negative retention** (`cleanupOldBackups`): clamped to `0`. - **Symlink-following delete** (`deleteDirectory`): rewritten with `Files.walkFileTree` (no symlink following) so a symlink inside the NAS tree can't cause deletion outside the backup path. - **Concurrent backups across management servers** (`NASBackupProvider.submitTask` / `runInContext`): now guarded by a cluster-wide `GlobalLock` (one MS at a time); acquisition is an overridable seam so it stays unit-testable without the DB lock. - `dir.mkdirs()` now only runs when the directory doesn't already exist. Also extended `InfrastructureBackupTaskTest` with retention/delete coverage (negative-retention clamp, keeps-newest/deletes-oldest, and symlink-escape safety — the last two fail against the previous code). The refactor into a shared `backupDirectory` helper is already in the current revision. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
