Give conf writes a backup, a lock and a read-back before they count

These files are gitignored, so a bad write had nothing to go back to; the escaping order
also stored any value containing a quote truncated but still parseable, which only a
read-back can catch.

.gitignore

@@ -3,7 +3,10 @@
 # Templates (*.template) are safe and remain tracked.
 Configurations/host*.conf
 Configurations/master.conf
-Configurations/*.bak
+# *.bak alone missed conf_upgrade's real output — it writes host1.conf.bak-20260802, which does
+# not end in .bak — so those sat untracked rather than ignored, one `git add -A` from being
+# pushed. The glob has to cover the suffix.
+Configurations/*.bak*
 .vscode
 
 # ── Personal scratch notes — dev-only, never pushed ───────────────────────────

Deployment/master.conf.template

@@ -144,6 +144,8 @@
     AI_DATA_DIR="${DATA_DIR}/ai"
     CACHE_BACKUP_DIR="${DATA_DIR}/cache"
     LOG_ARCHIVE_DIR="${DATA_DIR}/logs"
+    BACKUP_DIR="${DATA_DIR}/Backups"
+    CONF_BACKUP_DIR="${BACKUP_DIR}/Confs"        # 0700 — holds credentials
     PERSISTENT_CONF_CACHE="${CACHE_BACKUP_DIR}/conf"
     ARR_CACHE_BACKUP_DIR="${CACHE_BACKUP_DIR}/arr"
 
@@ -1659,6 +1661,13 @@
 
 # ==============================================================================================
 # ── AI / RAG ──────────────────────────────────────────────────────────────────────────────────
+# ━━━ Conf Backups ━━━
+# Every write through the plugin copies the conf aside first, to CONF_BACKUP_DIR with an
+# ISO-8601 stamp. The confs are gitignored, so that directory is the whole recovery path — there
+# is no history to revert to. A backup that cannot be taken cancels the write.
+# Oldest are pruned past this count, per conf file.
+    CONF_BACKUP_RETAIN=30
+
 # ==============================================================================================
 # Varaverk works exactly as well with AI off as with it on. Nothing below is required for any
 # script to function — every feature that can lean on AI has a complete non-AI path, and the

Plugin/unraid/include/confform.php

@@ -18,7 +18,27 @@
 //
 //   Structure only; values are not validated.
 //       Consistent with conf_upgrade.sh, this reconciles shape and leaves correctness to the
-//       consuming script.
+//       consuming script. Shape is enforced — the file must parse, source, and read back the
+//       value that was asked for. Whether 300 is a sensible timeout is still not this file's
+//       question.
+//
+//   One guarded write path, not one set of guards per caller.
+//       vv_conf_edit() owns the lock, the backup, the validation and the audit line, and every
+//       conf write in the plugin goes through it — the Settings form, both raw editors, the
+//       flag toggles, orchestrator membership, script moves and reorders, the rsync window
+//       arrays, the docker folder map and first-run setup. vv_write_conf_raw() has exactly one
+//       caller left, inside vv_conf_edit() itself.
+//
+//       A caller that reaches past it gets tmp + rename and nothing else: no backup, no bash -n,
+//       no read-back, no audit line. That is how this started — seven files each with their own
+//       partial idea of what a safe conf write was, two of them carrying a copy-pasted bash -n
+//       block that failed open.
+//
+//   The rewrite happens inside the lock, or it proves nothing moved.
+//       $mutate receives the current contents, so a caller that can rebuild from them has no
+//       window at all. Callers that must assemble the result first — the ones driven by a form
+//       payload — compare against what they read and return null if it no longer matches,
+//       which abandons the write rather than reverting a concurrent edit.
 //
 // OPERATIONAL SAFEGUARDS
 //   An unmatched section yields no fields rather than a wrong write.
@@ -30,6 +50,33 @@
 //       Each field carries the exact line it came from, so a write cannot land outside the
 //       subsection it was read from.
 //
+//   A value is shell source code, so command substitution is refused outright.
+//       Scalars are written inside double quotes and array values are spliced in verbatim, so
+//       $(...) or a backtick in a value runs as root in every script that sources the conf. No
+//       conf in this repo uses either, so rejecting them costs nothing. $VAR and ${VAR} stay
+//       legal — AI_DATA_DIR="${DATA_DIR}/ai" is the established idiom, and a reference resolves
+//       to a value where a substitution runs a program.
+//
+//   Nothing is written without a backup in hand.
+//       The previous contents are copied to CONF_BACKUP_DIR/<file>.<stamp> first, and a backup
+//       that cannot be taken cancels the write. The confs are gitignored, so that directory is
+//       the entire recovery story — there is no history to revert to.
+//
+//   Validated, then verified, then rolled back on failure.
+//       bash -n proves the candidate parses; sourcing the installed file and reading the keys
+//       back proves the values survived quoting. A scalar that does not read back as the value
+//       requested restores the backup. A conf that parses cleanly and holds the wrong string is
+//       the failure a syntax check cannot see.
+//
+//   Held under an exclusive lock for the whole read-modify-write.
+//       Two concurrent savers would otherwise read the same original, and the second rename
+//       would discard the first one's change without either reporting a failure.
+//
+//   Every outcome is logged, and secrets are logged by name only.
+//       LOG_DIR/conf_changes.log records applied, rejected, failed and rolled-back alike. A
+//       credential-shaped key logs value=<redacted> — the log proves a change happened, it is
+//       not a second copy of the secret.
+//
 // EXPORTS
 //   vv_conf_has_sections()     does this script have an editable conf section
 //   vv_conf_parse_subsection() fields within one named subsection
@@ -38,7 +85,10 @@
 //   vv_conf_write_changes()    apply edits back to the conf file
 //
 // CONFIGURATION
-//   CONF_DIR   master.conf and host*.conf are the read and write targets
+//   CONF_DIR             master.conf and host*.conf are the read and write targets
+//   CONF_BACKUP_DIR      DATA_DIR/Backups/Confs — pre-write copies, 0700
+//   LOG_DIR              conf_changes.log is written here
+//   CONF_BACKUP_RETAIN   backups kept per conf file (default 30)
 // ═══════════════════════════════════════════════════════════════════════════════════════════════
 require_once __DIR__ . '/config.php';
 
@@ -307,19 +357,132 @@ function vv_conf_fields_for_script(string $id): array {
     return $groups;
 }
 
+// A conf key must be a plain shell identifier. Every downstream use — the replacement regex,
+// the source-verification subshell, the audit line — treats the key as trusted text, so it is
+// validated once here rather than escaped differently in three places.
+function vv_conf_key_valid(string $key): bool {
+    return (bool) preg_match('/^[A-Za-z_][A-Za-z0-9_]*$/', $key);
+}
+
+// Credential-shaped keys are logged by name only. The audit log is the record that a change
+// happened, not a second copy of the secret that changed.
+function vv_conf_key_is_secret(string $key): bool {
+    return (bool) preg_match('/(PASS|PASSWORD|SECRET|TOKEN|API_KEY|APIKEY|_KEY)$|(PASS|PASSWORD|SECRET|TOKEN|APIKEY)/i', $key);
+}
+
+// Scalar values are written inside double quotes and array values are spliced in verbatim, so
+// a value is shell source code the moment any script reads the conf. Command substitution in a
+// value therefore executes on every load — in every script, as root. No conf in this repo uses
+// it, so rejecting it costs nothing and closes the path.
+//
+// $VAR and ${VAR} stay legal on purpose: AI_DATA_DIR="${DATA_DIR}/ai" is the established idiom
+// here, and a reference resolves to a value where a substitution runs a program.
+function vv_conf_value_safe(string $value): bool {
+    return !preg_match('/\$\(|`|<\(|>\(/', $value);
+}
+
+// Under DATA_DIR, not beside the confs. data/ is the one on-disk root and is gitignored whole,
+// so a backup here cannot become a tracked file the way Configurations/*.bak did. 0700 because
+// these are verbatim copies of files holding every credential on the host.
+function vv_conf_backup_dir(): string {
+    if (!is_dir(CONF_BACKUP_DIR)) @mkdir(CONF_BACKUP_DIR, 0700, true);
+    return CONF_BACKUP_DIR;
+}
+
+// Copy the current conf aside before it is touched. Returns the backup path, or null when no
+// backup could be taken — which the caller treats as a reason not to write, because the whole
+// recovery story for these files is this directory.
+function vv_conf_backup(string $filename): ?string {
+    $src = CONF_DIR . '/' . $filename;
+    if (!is_file($src)) return null;
+
+    $dest = vv_conf_backup_dir() . '/' . $filename . '.' . date('Y-m-d\THis');
+    // Same second, second change: keep both rather than silently overwrite the older one.
+    if (file_exists($dest)) {
+        $n = 1;
+        while (file_exists($dest . '.' . $n)) $n++;
+        $dest .= '.' . $n;
+    }
+    if (!@copy($src, $dest)) return null;
+    @chmod($dest, 0600);
+
+    vv_conf_prune_backups($filename);
+    return $dest;
+}
+
+// Retain the newest CONF_BACKUP_RETAIN backups per conf file. Pruned by filename, which sorts
+// chronologically because the stamp is ISO-8601 — no stat() per candidate.
+function vv_conf_prune_backups(string $filename): void {
+    $keep = (int) (vv_conf_vars()['CONF_BACKUP_RETAIN'] ?? 30);
+    if ($keep < 1) $keep = 30;
+
+    $found = glob(vv_conf_backup_dir() . '/' . $filename . '.*') ?: [];
+    if (count($found) <= $keep) return;
+
+    sort($found);
+    foreach (array_slice($found, 0, count($found) - $keep) as $old) @unlink($old);
+}
+
+// Source a candidate conf in a subshell and read back the keys that were just written.
+// bash -n proves the file parses; this proves the values survived quoting and arrived as
+// intended. Returns null when the file could not be sourced at all.
+function vv_conf_read_back(string $path, array $keys): ?array {
+    $script = 'source ' . escapeshellarg($path) . ' >/dev/null 2>&1 || exit 90; ';
+    foreach ($keys as $k) {
+        if (!vv_conf_key_valid($k)) continue;
+        $script .= 'printf "%s\t%s\n" ' . escapeshellarg($k) . ' "${' . $k . '-}"; ';
+    }
+
+    $out = []; $rc = 0;
+    exec('bash -c ' . escapeshellarg($script) . ' 2>/dev/null', $out, $rc);
+    if ($rc !== 0) return null;
+
+    $vals = [];
+    foreach ($out as $line) {
+        $parts = explode("\t", $line, 2);
+        if (count($parts) === 2) $vals[$parts[0]] = $parts[1];
+    }
+    return $vals;
+}
+
+// One line per conf change, best-effort and never able to block the write itself.
+function vv_conf_audit(string $file, string $key, string $outcome, string $detail = ''): void {
+    $line = date('Y-m-d H:i:s')
+          . " file={$file} key={$key} outcome={$outcome}"
+          . ($detail !== '' ? " {$detail}" : '')
+          . ' ip=' . ($_SERVER['REMOTE_ADDR'] ?? 'cli')
+          . "\n";
+    @file_put_contents(LOG_DIR . '/conf_changes.log', $line, FILE_APPEND | LOCK_EX);
+}
+
 // Write a batch of field changes back to their respective conf files.
 // Each change: {file, key, value, type}
 function vv_conf_write_changes(array $changes): array {
     $byFile = [];
     foreach ($changes as $c) {
-        if (!empty($c['file']) && !empty($c['key'])) $byFile[$c['file']][] = $c;
+        if (empty($c['file']) || empty($c['key'])) continue;
+        if (!vv_conf_key_valid($c['key'])) {
+            vv_conf_audit((string) $c['file'], (string) $c['key'], 'rejected', 'reason=malformed-key');
+            continue;
+        }
+        if (!vv_conf_value_safe((string) ($c['value'] ?? ''))) {
+            vv_conf_audit((string) $c['file'], (string) $c['key'], 'rejected', 'reason=command-substitution');
+            continue;
+        }
+        $byFile[$c['file']][] = $c;
     }
 
     $results = [];
     foreach ($byFile as $file => $fileChanges) {
-        $raw = vv_read_conf_raw($file);
-        if ($raw === '') { $results[$file] = false; continue; }
+        $results[$file] = vv_conf_write_file($file, $fileChanges);
+    }
+    return $results;
+}
 
+// Read-modify-write for one conf file. The surgical replacement lives here; the guards that
+// make installing it safe live in vv_conf_install(), which every conf writer shares.
+function vv_conf_write_file(string $file, array $fileChanges): bool {
+    return vv_conf_edit($file, function (string $raw) use ($fileChanges): ?string {
         foreach ($fileChanges as $c) {
             $qKey  = preg_quote($c['key'], '/');
             $value = $c['value'];
@@ -328,7 +491,10 @@ function vv_conf_write_changes(array $changes): array {
             if ($type === 'scalar') {
                 $raw = preg_replace_callback(
                     '/^(\s*' . $qKey . '\s*=\s*)("(?:[^"\\\\]|\\\\.)*"|\'(?:[^\'\\\\]|\\\\.)*\'|[^#\n]*?)(\s*(?:#[^\n]*)?)$/m',
-                    fn($m) => $m[1] . '"' . str_replace(['"', '\\'], ['\\"', '\\\\'], $value) . '"' . $m[3],
+                    // Backslashes first. Escaping quotes first meant the backslash just inserted
+                    // was itself doubled on the next pass — " became \\" — which closed the
+                    // string early and stored a truncated value that still parsed cleanly.
+                    fn($m) => $m[1] . '"' . str_replace(['\\', '"'], ['\\\\', '\\"'], $value) . '"' . $m[3],
                     $raw
                 ) ?? $raw;
 
@@ -354,23 +520,154 @@ function vv_conf_write_changes(array $changes): array {
                 ) ?? $raw;
             }
         }
-        // Only the scalar path escapes its value; the array, array_single and assoc_array
-        // paths splice the caller's text into the file verbatim, and the type comes from the
-        // request. Every script sources these files, so the result is parsed before it is
-        // allowed to replace a working conf.
-        $results[$file] = vv_conf_syntax_ok($raw) && vv_write_conf_raw($file, $raw);
+
+        return $raw;
+
+    // Scalars are verified because their intended value is known exactly. The array types splice
+    // caller-supplied text whose sourced form is legitimately not equal to what was written, so
+    // for those a clean source is the whole assertion.
+    }, vv_conf_expected_scalars($fileChanges), array_column($fileChanges, 'key'));
+}
+
+// The intended end state for the keys that can be checked against one.
+function vv_conf_expected_scalars(array $fileChanges): array {
+    $expect = [];
+    foreach ($fileChanges as $c) {
+        if (($c['type'] ?? 'scalar') === 'scalar') $expect[$c['key']] = (string) $c['value'];
     }
-    return $results;
+    return $expect;
+}
+
+// The one guarded way to install a changed conf. $mutate receives the current contents and
+// returns the rewritten ones, or null to abort without touching anything. Every conf writer in
+// the plugin goes through here, so the lock, the backup, the validation and the audit trail are
+// written once and cannot be forgotten by a new caller.
+//
+//   $expect       key => intended value, verified by sourcing the installed file
+//   $subjects     names for the audit line, when there is no key to assert (a toggled member)
+//   $allowCreate  write a conf that does not exist yet. Off by default: for every caller except
+//                 first-run setup, a missing target means the filename is wrong, and creating it
+//                 would leave a stray conf that shadows nothing and is sourced by nobody.
+function vv_conf_edit(string $file, callable $mutate, array $expect = [], array $subjects = [],
+                      bool $allowCreate = false): bool {
+    $path     = CONF_DIR . '/' . $file;
+    $subjects = $subjects ?: (array_keys($expect) ?: ['-']);
+
+    $audit = function (string $outcome, string $detail = '') use ($file, $subjects): void {
+        foreach ($subjects as $s) vv_conf_audit($file, (string) $s, $outcome, $detail);
+    };
+
+    // Held across the whole read-modify-write. Two concurrent savers would otherwise each read
+    // the same original, and the second rename would silently discard the first one's change.
+    $lockFh = @fopen(CONF_DIR . '/.conf-write.lock', 'c');
+    if ($lockFh === false || !flock($lockFh, LOCK_EX)) {
+        if ($lockFh) fclose($lockFh);
+        $audit('failed', 'reason=lock');
+        return false;
+    }
+
+    try {
+        // A conf that does not exist yet is a create, not an edit — first-run setup writes
+        // host*.conf before there is anything to read. There is no prior content to lose, so
+        // there is nothing to back up, and undoing a failed create means removing the file.
+        $exists = is_file($path);
+        if (!$exists && !$allowCreate) { $audit('failed', 'reason=unreadable'); return false; }
+        $raw = $exists ? vv_read_conf_raw($file) : '';
+        if ($exists && $raw === '')    { $audit('failed', 'reason=unreadable'); return false; }
+
+        $before = $exists ? (vv_conf_read_back($path, $subjects) ?? []) : [];
+
+        $new = $mutate($raw);
+        if ($new === null)             { $audit('failed', 'reason=no-match'); return false; }
+        if ($exists && $new === $raw)  { $audit('no-change');                 return true;  }
+
+        $backup = null;
+        if ($exists) {
+            $backup = vv_conf_backup($file);
+            if ($backup === null) {
+                // No recovery path for this write means the write does not happen. These files
+                // are gitignored, so a backup not taken cannot be reconstructed afterwards.
+                $audit('failed', 'reason=backup');
+                return false;
+            }
+        }
+
+        $undo = function () use ($backup, $path): void {
+            if ($backup !== null) @copy($backup, $path); else @unlink($path);
+        };
+
+        if (!vv_conf_syntax_ok($new))        { $audit('rejected', 'reason=syntax'); return false; }
+        if (!vv_write_conf_raw($file, $new)) { $audit('failed',   'reason=write');  return false; }
+
+        // bash -n proved the candidate parses. This proves the installed file still sources and
+        // that each value arrived intact — a quoting bug produces a file that parses perfectly
+        // and holds the wrong string, which is the failure the syntax check cannot see.
+        $after = vv_conf_read_back($path, $subjects);
+        if ($after === null) {
+            $undo();
+            $audit('rolled-back', 'reason=source-failed');
+            return false;
+        }
+
+        foreach ($expect as $key => $want) {
+            if (($after[$key] ?? null) !== $want) {
+                $undo();
+                vv_conf_audit($file, $key, 'rolled-back', 'reason=value-mismatch');
+                return false;
+            }
+        }
+
+        foreach ($subjects as $s) {
+            $s = (string) $s;
+            // A subject that is not a conf key — a script id, or a marker for a whole-file save
+            // — has no value to read back, so there is no before and after to report.
+            if (!vv_conf_key_valid($s))          { vv_conf_audit($file, $s, 'applied'); continue; }
+            if (vv_conf_key_is_secret($s))       { vv_conf_audit($file, $s, 'applied', 'value=<redacted>'); continue; }
+            vv_conf_audit($file, $s, 'applied',
+                'from=' . vv_conf_audit_val($before[$s] ?? '') . ' to=' . vv_conf_audit_val($after[$s] ?? ''));
+        }
+
+        return true;
+
+    } finally {
+        flock($lockFh, LOCK_UN);
+        fclose($lockFh);
+    }
+}
+
+// Audit values are single-line and bounded. A conf value can be a 40-line array; the log is a
+// record of what changed, and an unbounded splat of it makes the log unreadable at the moment
+// it is actually needed.
+function vv_conf_audit_val(string $v): string {
+    $v = preg_replace('/\s+/', ' ', trim($v));
+    if (strlen($v) > 120) $v = substr($v, 0, 117) . '...';
+    return '"' . $v . '"';
 }
 
 // bash -n against a private temp copy. Returns true when the content parses as a sourceable
 // conf, false otherwise — never writes anything itself.
 function vv_conf_syntax_ok(string $content): bool {
+    return vv_conf_syntax_error($content) === null;
+}
+
+// The same check, with bash's own complaint when it fails — the raw editors show it to whoever
+// is typing, where "conf does not parse" alone would mean hunting the line by hand. $label
+// replaces the temp path in the message so the reader sees their own filename.
+//
+// Fails closed. This used to pass when the temp file could not be created, which was defensible
+// while every write came from a human clicking Save on a form. The assistant writes through here
+// too, so an unverified conf is not installed — a refused write is recoverable, a conf that no
+// script can source is a system-wide outage.
+function vv_conf_syntax_error(string $content, string $label = 'conf'): ?string {
     $tmp = tempnam(sys_get_temp_dir(), 'vvconf');
-    if ($tmp === false) return true;   // cannot check — do not block the write
+    if ($tmp === false) return 'cannot verify: no writable temp directory';
+
     file_put_contents($tmp, $content);
     $out = []; $rc = 0;
     exec('bash -n ' . escapeshellarg($tmp) . ' 2>&1', $out, $rc);
     @unlink($tmp);
-    return $rc === 0;
+    if ($rc === 0) return null;
+
+    $msg = implode(' ', array_filter(array_map('trim', $out)));
+    return str_replace($tmp, $label, $msg ?: 'conf does not parse');
 }

Plugin/unraid/include/config.php

@@ -117,6 +117,8 @@ define('STATE_DIR',        DATA_DIR . '/state');
 define('AI_DATA_DIR',      DATA_DIR . '/ai');
 define('CACHE_BACKUP_DIR', DATA_DIR . '/cache');
 define('LOG_ARCHIVE_DIR',  DATA_DIR . '/logs');
+define('BACKUP_DIR',       DATA_DIR . '/Backups');
+define('CONF_BACKUP_DIR',  BACKUP_DIR . '/Confs');
 define('LOG_DIR',     '/var/log/varaverk');
 // User-authored custom scripts (scheduler page "+ Create Script") — kept outside the git
 // repo entirely, alongside the User Scripts plugin's own storage. Any *.sh file placed

data/README.md

@@ -19,6 +19,8 @@ data/
 ├── cache/    persistent backups of the tmpfs caches — and only those
 │   ├── arr/    *_tracked_cache.json, restored into tmpfs on demand
 │   └── conf/   partner host*.conf snapshot (0700 — holds credentials)
+├── Backups/  point-in-time copies kept so a bad write can be undone
+│   └── Confs/  pre-write copies of this host's own confs (0700 — holds credentials)
 └── logs/     retained log output
 ```
 
@@ -31,6 +33,8 @@ variable is what a storage-mode migration rewrites.
 | `state/` | `STATE_DIR` | ALL state files must use this. No `/tmp`, no repo root. |
 | `ai/` | `AI_DATA_DIR` | |
 | `cache/` | `CACHE_BACKUP_DIR` | `ARR_CACHE_BACKUP_DIR`, `PERSISTENT_CONF_CACHE` sit under it |
+| `Backups/` | `BACKUP_DIR` | parent only; each kind of backup gets a subdirectory |
+| `Backups/Confs/` | `CONF_BACKUP_DIR` | retained per `CONF_BACKUP_RETAIN`; **not** a cache — see below |
 | `logs/` | `LOG_ARCHIVE_DIR` | live logging still goes to `LOG_DIR` (`/var/log/varaverk`) |
 
 ---
@@ -74,6 +78,11 @@ Ask what happens if it is deleted.
 called — `lidarr_art_miss_cache.tsv` has "cache" in its name and lives in `db/` for exactly that
 reason.
 
+`Backups/` is the same trap from the other side: it has "backup" in its name but backs up nothing
+that exists elsewhere. The confs are gitignored, so a pre-write copy under `Backups/Confs/` is the
+only prior version of that file anywhere. Deleting it loses something permanently, which is why it
+is a root of its own and not a subdirectory of `cache/`.
+
 ---
 
 ## History