From de25110493f0615d5fe5f4d613ad6c803a6141c5 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 18 Jul 2026 18:08:33 +0000 Subject: [PATCH] Address code review round 2: Python regex for function extraction, bash 4.1+ comment, find -print0 for manifest, mktemp 0600 comment --- .../scripts/sovran-hub-backup.sh | 6 +- app/tests/test_manual_backup_workflow.py | 73 ++++++++++++------- 2 files changed, 50 insertions(+), 29 deletions(-) diff --git a/app/sovran_systemsos_web/scripts/sovran-hub-backup.sh b/app/sovran_systemsos_web/scripts/sovran-hub-backup.sh index 9604589..24dbe6e 100755 --- a/app/sovran_systemsos_web/scripts/sovran-hub-backup.sh +++ b/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 ───────────────── diff --git a/app/tests/test_manual_backup_workflow.py b/app/tests/test_manual_backup_workflow.py index 8d6fbaf..f5c1dc8 100644 --- a/app/tests/test_manual_backup_workflow.py +++ b/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" -) -for msg in "${{msgs[@]}}"; do - _is_home_warning_allowlisted "$msg" || {{ echo "BLOCKED: $msg"; exit 1; }} - echo "ALLOWED: $msg" -done -""" + 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" + ) 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" -) -for msg in "${{msgs[@]}}"; do - _is_home_warning_allowlisted "$msg" && {{ echo "ALLOWED_BUT_SHOULD_NOT: $msg"; exit 1; }} - echo "CORRECTLY_BLOCKED: $msg" -done -""" + 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" + ) with tempfile.NamedTemporaryFile(mode="w", suffix=".sh", delete=False) as f: f.write(script) script_path = f.name