From 063515401d757700b330bc5ad3885455e96248b6 Mon Sep 17 00:00:00 2001
From: Abdelkarim Mateos Sanchez <abdelkarim.mateos@castris.com>
Date: Mon, 11 May 2026 12:35:20 +0200
Subject: [PATCH] Fix expire phase failing on snapshots without owner write bit
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

fn_expire_backup called fn_rm_dir directly without ever sanitizing
permissions. When a snapshot aged into expiration containing files
or directories without u+w (typical of ISPConfig .php-fcgi-starter at
0500, web1/ at 0550, or /etc/ssl/private/ at 0700), `rm -rf` failed
with "Permission denied" and the entire backup run aborted.

Real-world trigger on stor01-ovh, user kvm476:
  rm: cannot remove '.../2026-01-19-124749/var/www/php-fcgi-scripts/
  web1/.php-fcgi-starter': Permission denied

Two changes:

1. fn_fix_permissions: widen the find filter from the narrow
   `-perm 0111` / `-perm 000` to `! -perm -u=w`. This catches every
   dir/file where the owner lacks write, including 0400, 0500, 0550,
   0700, 0710 — all common in /var/www, /root, /etc/ssl/private.
   Also collapse the duplicated SUDO branches into a single $sudo_cmd
   prefix.

2. fn_expire_backup: invoke fn_fix_permissions on the snapshot before
   fn_rm_dir, guarded so it only runs on local destinations (the find
   in fn_fix_permissions does not go through fn_run_cmd / SSH).

Added tests/06_expire_perms.bats with two regression cases:
- ISPConfig-style 0500 file inside 0550 dir
- 0400 private key inside 0700 dir

All 56 bats tests pass.
---
 rsync_tmbackup.sh          | 51 +++++++++++++--------------
 tests/06_expire_perms.bats | 72 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 96 insertions(+), 27 deletions(-)
 create mode 100644 tests/06_expire_perms.bats

diff --git a/rsync_tmbackup.sh b/rsync_tmbackup.sh
index 51f518d..e8b6dc8 100755
--- a/rsync_tmbackup.sh
+++ b/rsync_tmbackup.sh
@@ -118,6 +118,12 @@ fn_expire_backup() {
         fi
 
         fn_log_info "Expiring $1"
+        # Sanitize permissions before deletion: snapshots may contain files/dirs
+        # without owner write bit (e.g. ISPConfig .php-fcgi-starter at 0500,
+        # web1/ at 0550) that would make `rm -rf` fail. Only safe on local dest.
+        if [ -z "$SSH_DEST_FOLDER_PREFIX" ]; then
+                fn_fix_permissions "$1"
+        fi
         fn_rm_dir "$1"
 }
 
@@ -299,38 +305,29 @@ fn_df_t() {
         fn_run_cmd "df -T '${1}'"
 }
 
-# Function to set permissions on the last used directory
+# Function to set permissions on a backup directory tree.
+# Covers any dir or file lacking owner write (u+w) — this includes the legacy
+# narrow cases (0111, 000) plus 0500/0550/0700/0710 typical of ISPConfig,
+# /root/, /etc/ssl/private/, etc.
 fn_fix_permissions() {
     local base_dir="$1"
+    local sudo_cmd=""
+
+    if [ "$SUDO" = true ]; then
+        sudo_cmd="sudo"
+    fi
 
     fn_log_info "Fixing permissions in directory: $base_dir"
 
-    if [ "$SUDO" = true ]; then
-        if ! find "$base_dir" -type d -perm 0111 -exec sudo chmod 700 {} \;; then
-            fn_log_error "Failed to set permissions on some directories"
-            exit 1
-        fi
-        if ! find "$base_dir" -type d -perm 000 -exec sudo chmod 700 {} \;; then
-            fn_log_error "Failed to set permissions on some directories"
-            exit 1
-        fi
-        if ! find "$base_dir" -type f -perm 000 -exec sudo chmod 600 {} \;; then
-            fn_log_error "Failed to set permissions on some files"
-            exit 1
-        fi
-    else
-        if ! find "$base_dir" -type d -perm 0111 -exec chmod 700 {} \;; then
-            fn_log_error "Failed to set permissions on some directories"
-            exit 1
-        fi
-        if ! find "$base_dir" -type d -perm 000 -exec chmod 700 {} \;; then
-            fn_log_error "Failed to set permissions on some directories"
-            exit 1
-        fi
-        if ! find "$base_dir" -type f -perm 000 -exec chmod 600 {} \;; then
-            fn_log_error "Failed to set permissions on some files"
-            exit 1
-        fi
+    # Any dir without u+w → grant u+rwX so we can traverse and remove
+    if ! find "$base_dir" -type d ! -perm -u=w -exec $sudo_cmd chmod u+rwX {} +; then
+        fn_log_error "Failed to set permissions on some directories"
+        exit 1
+    fi
+    # Any file without u+w → grant u+rw
+    if ! find "$base_dir" -type f ! -perm -u=w -exec $sudo_cmd chmod u+rw {} +; then
+        fn_log_error "Failed to set permissions on some files"
+        exit 1
     fi
 
     fn_log_info "Permissions fixed successfully"
diff --git a/tests/06_expire_perms.bats b/tests/06_expire_perms.bats
new file mode 100644
index 0000000..29c41fa
--- /dev/null
+++ b/tests/06_expire_perms.bats
@@ -0,0 +1,72 @@
+#!/usr/bin/env bats
+#
+# Regression tests for expire phase when snapshots contain files/dirs
+# without owner write bit (e.g. ISPConfig .php-fcgi-starter at mode 0500,
+# web1/ directories at 0550). Before the fix, `rm -rf` in fn_expire_backup
+# would fail with "Permission denied" on these snapshots.
+#
+
+load 'test_helper'
+
+setup() {
+    setup_test_environment
+    create_sample_files "$TEST_SOURCE" 2
+}
+
+teardown() {
+    # Restore perms so teardown's rm -rf works even if a test failed mid-way
+    if [ -n "$TEST_TEMP_DIR" ] && [ -d "$TEST_TEMP_DIR" ]; then
+        chmod -R u+rwX "$TEST_TEMP_DIR" 2>/dev/null || true
+    fi
+    teardown_test_environment
+}
+
+@test "expire removes snapshot containing 0500 file (ISPConfig .php-fcgi-starter case)" {
+    # Backup 1 (will become the one to expire)
+    run "$SCRIPT_PATH" -m 1 "$TEST_SOURCE" "$TEST_DEST"
+    assert_success
+
+    local oldest
+    oldest=$(find "$TEST_DEST" -maxdepth 1 -type d -name "????-??-??-??????" | head -1)
+    [ -n "$oldest" ]
+
+    # Inject the problematic perms inside the first snapshot
+    mkdir -p "$oldest/var/www/php-fcgi-scripts/web1"
+    echo '#!/bin/bash' > "$oldest/var/www/php-fcgi-scripts/web1/.php-fcgi-starter"
+    chmod 0500 "$oldest/var/www/php-fcgi-scripts/web1/.php-fcgi-starter"
+    chmod 0550 "$oldest/var/www/php-fcgi-scripts/web1"
+
+    sleep 1
+
+    # Backup 2 with -m 1 must expire backup 1 — this is where the bug fires
+    echo "trigger" >> "$TEST_SOURCE/file_1.txt"
+    run "$SCRIPT_PATH" -m 1 "$TEST_SOURCE" "$TEST_DEST"
+    assert_success
+
+    assert_dir_not_exists "$oldest"
+
+    local count
+    count=$(count_backups "$TEST_DEST")
+    [ "$count" -eq 1 ]
+}
+
+@test "expire removes snapshot with 0400 file and 0700 parent dir" {
+    run "$SCRIPT_PATH" -m 1 "$TEST_SOURCE" "$TEST_DEST"
+    assert_success
+
+    local oldest
+    oldest=$(find "$TEST_DEST" -maxdepth 1 -type d -name "????-??-??-??????" | head -1)
+
+    mkdir -p "$oldest/etc/ssl/private"
+    echo 'fake-key' > "$oldest/etc/ssl/private/server.key"
+    chmod 0400 "$oldest/etc/ssl/private/server.key"
+    chmod 0700 "$oldest/etc/ssl/private"
+
+    sleep 1
+
+    echo "trigger" >> "$TEST_SOURCE/file_1.txt"
+    run "$SCRIPT_PATH" -m 1 "$TEST_SOURCE" "$TEST_DEST"
+    assert_success
+
+    assert_dir_not_exists "$oldest"
+}