Address code review round 2: Python regex for function extraction, bash 4.1+ comment, find -print0 for manifest, mktemp 0600 comment

app/sovran_systemsos_web/scripts/sovran-hub-backup.sh

@@ -277,6 +277,7 @@ log "Starting backup process…"
 # stale RUNNING status after a Hub restart).
 
 LOCK_FILE="/var/lock/sovran-hub-backup.lock"
+# Note: exec {LOCK_FD}>>file requires bash 4.1+ (NixOS provides bash 5.x).
 exec {LOCK_FD}>>"$LOCK_FILE" 2>/dev/null || \
   fail "Cannot open lock file: $LOCK_FILE. Ensure /var/lock is writable."
 flock --nonblock "$LOCK_FD" 2>/dev/null || \
@@ -443,6 +444,8 @@ _create_archive_impl() {
   local archive_path="$BACKUP_DIR/$archive_name"
   local partial_path="${archive_path}.partial"
   local diag_tmp
+  # mktemp creates files with mode 0600 (owner-only) by default, so tar
+  # diagnostics (which may include file paths) are not readable by other users.
   diag_tmp="$(mktemp /tmp/sovran-tar-diag.XXXXXX)"
   PARTIAL_FILES+=("$partial_path" "$diag_tmp")
 
@@ -857,7 +860,8 @@ CHECKSUM_FILE="$BACKUP_DIR/SHA256SUMS.txt"
   echo "(/run/media/Second_Drive) and are reconstructable/internal-backup data."
   echo ""
   echo "Artifact listing:"
-  find "$BACKUP_DIR" -mindepth 1 -maxdepth 2 -type f ! -name 'INCOMPLETE' ! -name 'BACKUP_COMPLETE' | sort
+  find "$BACKUP_DIR" -mindepth 1 -maxdepth 2 -type f \
+    ! -name 'INCOMPLETE' ! -name 'BACKUP_COMPLETE' -print0 | sort -z | tr '\0' '\n'
 } > "$MANIFEST_FILE"
 
 # ── Generate checksums for all backup artifacts ─────────────────

app/tests/test_manual_backup_workflow.py

@@ -1,5 +1,6 @@
 import asyncio
 import os
+import re
 import subprocess
 import sys
 import tempfile
@@ -14,6 +15,18 @@ SUPPORT_JS = REPO_ROOT / "app" / "sovran_systemsos_web" / "static" / "js" / "sup
 NIX_HUB_FILE = REPO_ROOT / "modules" / "core" / "sovran-hub.nix"
 
 
+def _extract_bash_function(script_path: Path, func_name: str) -> str:
+    """Extract a bash function definition from a shell script using Python regex.
+    Returns the complete function definition as a string that can be inlined into
+    test scripts, avoiding eval of awk output and the associated risks."""
+    source = script_path.read_text()
+    pattern = r"^" + re.escape(func_name) + r"\(\) \{.*?^}"
+    match = re.search(pattern, source, re.MULTILINE | re.DOTALL)
+    if match is None:
+        raise ValueError(f"Function '{func_name}' not found in {script_path}")
+    return match.group(0)
+
+
 class ManualBackupWorkflowTests(unittest.TestCase):
     def test_backup_script_uses_tar_archives_with_checksums_and_exclusions(self):
         source = BACKUP_SCRIPT.read_text()
@@ -337,19 +350,22 @@ class ManualBackupWorkflowTests(unittest.TestCase):
 
     def test_allowlist_accepts_file_changed_message(self):
         """Behavioral: _is_home_warning_allowlisted (from the actual backup script)
-        returns true for the exact GNU tar 'file changed as we read it' message (LC_ALL=C)."""
-        script = f"""#!/usr/bin/env bash
-# Load the production function directly from the backup script to avoid logic drift
-eval "$(awk '/^_is_home_warning_allowlisted[(][)]/,/^[}}]/' {BACKUP_SCRIPT})"
-msgs=(
-  "tar: /home/user/firefox.db: file changed as we read it"
-  "tar: /home/user/.local/share/app: file removed before we read it"
+        returns true for the exact GNU tar 'file changed as we read it' message (LC_ALL=C).
+        The function definition is extracted from the production script via Python regex
+        to ensure the test exercises the real allowlist without eval-of-awk risks."""
+        func_def = _extract_bash_function(BACKUP_SCRIPT, "_is_home_warning_allowlisted")
+        script = (
+            "#!/usr/bin/env bash\n"
+            + func_def + "\n"
+            + 'msgs=(\n'
+            + '  "tar: /home/user/firefox.db: file changed as we read it"\n'
+            + '  "tar: /home/user/.local/share/app: file removed before we read it"\n'
+            + ")\n"
+            + 'for msg in "${msgs[@]}"; do\n'
+            + '  _is_home_warning_allowlisted "$msg" || { echo "BLOCKED: $msg"; exit 1; }\n'
+            + '  echo "ALLOWED: $msg"\n'
+            + "done\n"
         )
-for msg in "${{msgs[@]}}"; do
-  _is_home_warning_allowlisted "$msg" || {{ echo "BLOCKED: $msg"; exit 1; }}
-  echo "ALLOWED: $msg"
-done
-"""
         with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=False) as f:
             f.write(script)
             script_path = f.name
@@ -365,21 +381,22 @@ done
         """Behavioral: _is_home_warning_allowlisted (from the actual backup script)
         returns false for permission errors, I/O errors, write errors, and other
         fatal conditions."""
-        script = f"""#!/usr/bin/env bash
-# Load the production function directly from the backup script to avoid logic drift
-eval "$(awk '/^_is_home_warning_allowlisted[(][)]/,/^[}}]/' {BACKUP_SCRIPT})"
-msgs=(
-  "tar: /home/user/file: Permission denied"
-  "tar: /dev/sdb: Cannot read: Input/output error"
-  "tar: Error is not recoverable: exiting now"
-  "Write error"
-  "tar: /home/user: Cannot stat: No such file or directory"
+        func_def = _extract_bash_function(BACKUP_SCRIPT, "_is_home_warning_allowlisted")
+        script = (
+            "#!/usr/bin/env bash\n"
+            + func_def + "\n"
+            + 'msgs=(\n'
+            + '  "tar: /home/user/file: Permission denied"\n'
+            + '  "tar: /dev/sdb: Cannot read: Input/output error"\n'
+            + '  "tar: Error is not recoverable: exiting now"\n'
+            + '  "Write error"\n'
+            + '  "tar: /home/user: Cannot stat: No such file or directory"\n'
+            + ")\n"
+            + 'for msg in "${msgs[@]}"; do\n'
+            + '  _is_home_warning_allowlisted "$msg" && { echo "ALLOWED_BUT_SHOULD_NOT: $msg"; exit 1; }\n'
+            + '  echo "CORRECTLY_BLOCKED: $msg"\n'
+            + "done\n"
         )
-for msg in "${{msgs[@]}}"; do
-  _is_home_warning_allowlisted "$msg" && {{ echo "ALLOWED_BUT_SHOULD_NOT: $msg"; exit 1; }}
-  echo "CORRECTLY_BLOCKED: $msg"
-done
-"""
         with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=False) as f:
             f.write(script)
             script_path = f.name