Let a password containing a $ actually save, and say why when a save is refused

The value was written as typed, bash expanded it when the read-back sourced the file, and
the guard rolled the whole write back with nothing on screen but "save failed" — which is
also what an empty value, a trailing space, and a stale API-key check had been doing.

Plugin/unraid/System_Essentials/unraid_api_key_renew.sh

@@ -135,7 +135,14 @@ if [[ -n "$KEY" ]]; then
     PREVIEW="${KEY:0:8}...${KEY: -4}"
     # Always sync registry key → conf, even if the key was already there.
     # Conf gets wiped on git pull / conf regeneration without touching the registry.
-    CONF_HAS_KEY=$(grep -oP "(?<=^\s*${VAR_NAME}=\")[^\"]*" "$CONF_FILE" 2>/dev/null || true)
+    # Sourced, not pattern-matched. This was a grep -oP with a variable-length lookbehind, which
+    # Unraid's grep is ugrep and rejects outright — "length of lookbehind assertion is not
+    # limited", rc 2, swallowed by the || true. CONF_HAS_KEY was therefore always empty, the
+    # comparison below never matched, and this script logged "conf is stale — syncing" and
+    # rewrote the same key into the conf every fifteen minutes since it was written.
+    # Sourcing also means the value is read the way bash reads it, escapes and all.
+    CONF_HAS_KEY=$(bash -c 'source "$1" >/dev/null 2>&1 || exit 0; printf "%s" "${!2-}"' \
+                   _ "$CONF_FILE" "$VAR_NAME" 2>/dev/null || true)
     if [[ "$CONF_HAS_KEY" == "$KEY" ]]; then
         echo "API key valid ✅ — $VAR_NAME = $PREVIEW"
         log "Key in registry and conf — no action needed"

Plugin/unraid/include/confform.php

@@ -504,18 +504,30 @@ function _vv_conf_parse_field_range(array $lines, int $start, int $end, string $
             continue;
         }
 
-        // Scalar: KEY="value" or KEY=value
+        // Scalar: KEY="value", KEY='value' or KEY=value.
-        if (preg_match('/^\s*([A-Z_][A-Z0-9_]*)\s*=\s*"([^"]*)"(?:\s+#\s*(.+))?$/', $line, $m)) {
+        //
-            $fields[] = ['key' => $m[1], 'value' => $m[2],
+        // One branch, and it unquotes rather than capturing between quotes. Two regexes did this
-                         'type' => 'scalar', 'desc' => ($m[3] ?? '') ?: $desc, 'file' => $filename,
+        // — one for the quoted form, one for the bare form — and neither understood an escape, so
-                         '_lines' => $descLines];
+        // a value containing \" fell through the first (the capture ended at the escaped quote and
-            continue;
+        // the tail no longer matched) into the second, which handed the form the value with its
-        }
+        // surrounding quotes still attached. Saving that back wrapped it in another pair.
-        if (preg_match('/^\s*([A-Z_][A-Z0-9_]*)\s*=\s*([^(\n#]*?)(?:\s+#\s*(.+))?$/', $line, $m)) {
+        //
-            $val = trim($m[2]);
+        // A secret now carries \$ and \` for exactly the reason vv_conf_quote_scalar() explains,
-            if ($val === '') continue;
+        // which made the same class of bug reachable by typing an ordinary password: the form
+        // would have shown the backslashes and re-escaped them on every save.
+        if (preg_match('/^\s*([A-Z_][A-Z0-9_]*)\s*=\s*(.*)$/', $line, $m)) {
+            $rest = $m[2];
+            // No value at all, or nothing but a comment. \s* above already ate the leading space,
+            // so a # in first position cannot be part of the value — unlike one further in, which
+            // is an ordinary character to bash and to the unquote below.
+            if ($rest === '' || $rest[0] === '#') continue;
+            $val  = vv_conf_unquote($rest, $off);
+            // Whatever the value did not consume. The comment is found here rather than by a
+            // second pattern over the whole line, so a # inside a quoted password can never be
+            // mistaken for the start of one.
+            $cmt  = preg_match('/^\s+#\s*(.+)$/', substr($rest, $off), $cm) ? trim($cm[1]) : '';
             $fields[] = ['key' => $m[1], 'value' => $val,
-                         'type' => 'scalar', 'desc' => ($m[3] ?? '') ?: $desc, 'file' => $filename,
+                         'type' => 'scalar', 'desc' => $cmt ?: $desc, 'file' => $filename,
                          '_lines' => $descLines];
         }
     }
@@ -729,7 +741,13 @@ 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 . '-}"; ';
+        // \001 terminates the value. PHP's exec() strips trailing whitespace from every line it
+        // returns, so an empty value produced a line with nothing after the tab, explode() found
+        // one part instead of two, and the key vanished from the result entirely — which the
+        // caller reads as "not equal to what I wrote" and rolls the write back. Clearing any
+        // field was therefore impossible, and a value ending in a space came back trimmed and
+        // failed the same way. A non-whitespace sentinel survives the trim; it is stripped below.
+        $script .= 'printf "%s\t%s\001\n" ' . escapeshellarg($k) . ' "${' . $k . '-}"; ';
     }
 
     $out = []; $rc = 0;
@@ -739,11 +757,24 @@ function vv_conf_read_back(string $path, array $keys): ?array {
     $vals = [];
     foreach ($out as $line) {
         $parts = explode("\t", $line, 2);
-        if (count($parts) === 2) $vals[$parts[0]] = $parts[1];
+        if (count($parts) !== 2) continue;
+        $v = $parts[1];
+        if (str_ends_with($v, "\001")) $v = substr($v, 0, -1);
+        $vals[$parts[0]] = $v;
     }
     return $vals;
 }
 
+// Why the last vv_conf_edit() gave up, for a caller that has only a bool to report with. The
+// audit log always held the reason — value-mismatch, syntax, backup, lock — while the operator
+// got "save failed" and no way to tell a rolled-back write from a locked file. A save that does
+// nothing and will not say why is the hardest kind of bug to be handed.
+function vv_conf_last_error(?string $set = null): string {
+    static $last = '';
+    if ($set !== null) $last = $set;
+    return $last;
+}
+
 // 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')
@@ -776,7 +807,14 @@ function vv_conf_write_changes(array $changes, array &$rejected = []): array {
             $refuse($c, 'malformed-key');
             continue;
         }
-        if (!vv_conf_value_safe((string) ($c['value'] ?? ''))) {
+        // The command-substitution guard exists because a conf value is shell source the moment a
+        // script sources the file. A scalar secret has its $ and ` escaped by
+        // vv_conf_quote_scalar(), so $( and ` are literal characters there and cannot execute —
+        // the guard has nothing left to guard and would only be refusing legitimate passwords.
+        // Narrow on purpose: scalar and secret both, because the exemption is sound only for the
+        // escaping that function gives exactly that combination.
+        $literal = ($c['type'] ?? 'scalar') === 'scalar' && vv_conf_key_is_secret((string) $c['key']);
+        if (!$literal && !vv_conf_value_safe((string) ($c['value'] ?? ''))) {
             $refuse($c, 'command-substitution');
             continue;
         }
@@ -793,10 +831,48 @@ function vv_conf_write_changes(array $changes, array &$rejected = []): array {
     $results = [];
     foreach ($byFile as $file => $fileChanges) {
         $results[$file] = vv_conf_write_file($file, $fileChanges);
+        if ($results[$file] === false)
+            $rejected[] = ['file' => $file, 'key' => implode(', ', array_column($fileChanges, 'key')),
+                           'reason' => vv_conf_last_error() ?: 'write failed'];
     }
     return $results;
 }
 
+// How a scalar is written back, and it is not one rule for everything.
+//
+// Everything is double-quoted, including secrets. The difference is whether $ and ` are escaped.
+//
+// A conf value is shell source the moment a script sources the file, so bash expands any bare $.
+// That is deliberate for most keys — AI_DATA_DIR="${DATA_DIR}/ai" is the idiom here, and a
+// password is the exact opposite: never a reference, always the literal characters typed.
+//
+// A password containing a $ was silently unsavable. It was written as typed, bash expanded $ign
+// to nothing when the read-back sourced the file, the value no longer matched what was intended,
+// and the guard correctly rolled the whole write back — leaving a Save button that did nothing
+// and an audit line nobody had reason to look at.
+//
+// Escaping rather than single-quoting, deliberately. Single quotes would also work and read more
+// clearly, but the conf is parsed by bash tooling as well as sourced by it, and some of that
+// tooling matches KEY="..." literally — unraid_api_key_renew.sh reads and rewrites
+// HOST*_UNRAID_API_KEY with exactly that pattern, every fifteen minutes, and would have found no
+// match, taken the update branch anyway and reported success. Staying inside double quotes keeps
+// every one of those consumers working and confines this change to the two characters that
+// actually needed it.
+function vv_conf_quote_scalar(string $key, string $value): string {
+    // 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.
+    $from = ['\\', '"'];
+    $to   = ['\\\\', '\\"'];
+    if (vv_conf_key_is_secret($key)) {
+        // \$ and \` are literal inside double quotes. vv_conf_unquote() undoes both, so PHP and
+        // bash read the same password back.
+        $from[] = '$';  $to[] = '\\$';
+        $from[] = '`';  $to[] = '\\`';
+    }
+    return '"' . str_replace($from, $to, $value) . '"';
+}
+
 // 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 {
@@ -807,12 +883,10 @@ function vv_conf_write_file(string $file, array $fileChanges): bool {
             $type  = $c['type'] ?? 'scalar';
 
             if ($type === 'scalar') {
+                $quoted = vv_conf_quote_scalar($c['key'], $value);
                 $raw = preg_replace_callback(
                     '/^(\s*' . $qKey . '\s*=\s*)("(?:[^"\\\\]|\\\\.)*"|\'(?:[^\'\\\\]|\\\\.)*\'|[^#\n]*?)(\s*(?:#[^\n]*)?)$/m',
-                    // Backslashes first. Escaping quotes first meant the backslash just inserted
+                    fn($m) => $m[1] . $quoted . $m[3],
-                    // 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;
 
@@ -875,7 +949,9 @@ function vv_conf_edit(string $file, callable $mutate, array $expect = [], array
 
     $audit = function (string $outcome, string $detail = '') use ($file, $subjects): void {
         foreach ($subjects as $s) vv_conf_audit($file, (string) $s, $outcome, $detail);
+        vv_conf_last_error($outcome . ($detail !== '' ? ' ' . $detail : ''));
     };
+    vv_conf_last_error('');
 
     // 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.
@@ -933,6 +1009,7 @@ function vv_conf_edit(string $file, callable $mutate, array $expect = [], array
             if (($after[$key] ?? null) !== $want) {
                 $undo();
                 vv_conf_audit($file, $key, 'rolled-back', 'reason=value-mismatch');
+                vv_conf_last_error("rolled-back reason=value-mismatch key=$key");
                 return false;
             }
         }

Plugin/unraid/include/config.php

@@ -818,16 +818,72 @@ const VV_CONF_ARRAY_BODY  = '(?:([^)\n]*)\)|\n(.*?)^[ \t]*\)[ \t]*$)';
 // one, so a commented-out-to-off switch read as on. Roughly half of master.conf's toggles carry
 // an inline comment.
 //
-// The quoted forms are extracted before that, because inside quotes a # is data, not a comment —
+// Quotes are honoured before that, because inside them a # is data, not a comment — a password
-// a password or a colour would otherwise be truncated at the first hash. Unquoted, the comment
+// or a colour would otherwise be truncated at the first hash. Unquoted, the comment must be
-// must be introduced by whitespace, matching bash: FOO=#fff and FOO=bar#baz both assign literally,
+// introduced by whitespace, matching bash: FOO=#fff and FOO=bar#baz both assign literally, since
-// since # only opens a comment at the start of a word.
+// # only opens a comment at the start of a word.
+//
+// The three regexes that did this are now one pass in vv_conf_unquote(), which handles escapes
+// and adjacent quoted runs as well. See there for why that stopped being optional.
 function vv_parse_conf_scalar(string $raw, string $key): string {
     if (!preg_match('/^\s*' . preg_quote($key, '/') . '\s*=\s*(.*)$/m', $raw, $m)) return '';
-    $v = ltrim($m[1]);
+    return vv_conf_unquote(ltrim($m[1]));
-    if (preg_match('/^"([^"\n]*)"/', $v, $q)) return $q[1];
+}
-    if (preg_match("/^'([^'\n]*)'/", $v, $q)) return $q[1];
+
-    return trim(preg_replace('/\s+#.*$/', '', $v));
+// Unquote one bash word the way bash does, because the regexes this replaced did not and the
+// conf is read by both. Three separate regexes each handled one quoting style in isolation and
+// none of them handled an escape or two quoted runs in a row, so a value carrying a quote or a
+// backslash parsed differently here than it did when a script sourced the same line. Nothing in
+// the conf held one, which is the only reason it never showed.
+//
+// It matters now because a secret is written with its $ and ` escaped, so a password containing
+// either is stored as "a\$b". The old regex captured everything between the quotes verbatim and
+// would have shown a\$b — a backslash the operator never typed, in a field they are about to
+// copy a credential out of, while every bash script that sourced the same line held a$b.
+//
+// Not a shell. No expansion of any kind, so ${DATA_DIR}/ai stays the literal text it is today —
+// vv_conf_vars() owns resolving references, and doing it here would turn a display value into a
+// different string than the one on disk.
+// $end receives the offset where the word stopped, so a caller that also wants the trailing
+// comment knows where the value ended without re-deriving it with a second regex that would
+// disagree about quoting — which is exactly how the form came to show a # from inside a password
+// as the start of a comment.
+function vv_conf_unquote(string $v, ?int &$end = null): string {
+    $out = ''; $i = 0; $n = strlen($v);
+    while ($i < $n) {
+        $ch = $v[$i];
+        if ($ch === "'") {
+            // No escapes inside single quotes — the closing quote is the next one, always.
+            $j = strpos($v, "'", $i + 1);
+            if ($j === false) { $out .= substr($v, $i + 1); $i = $n; break; }
+            $out .= substr($v, $i + 1, $j - $i - 1);
+            $i    = $j + 1;
+        } elseif ($ch === '"') {
+            $i++;
+            while ($i < $n && $v[$i] !== '"') {
+                // Only these four are escapes inside double quotes. A backslash before anything
+                // else is a literal backslash, which is why \d in a regex value survives.
+                if ($v[$i] === '\\' && $i + 1 < $n && strpos('\\"$`', $v[$i + 1]) !== false) {
+                    $out .= $v[$i + 1]; $i += 2;
+                } else {
+                    $out .= $v[$i];     $i++;
+                }
+            }
+            $i++;
+        } elseif ($ch === '\\' && $i + 1 < $n) {
+            $out .= $v[$i + 1]; $i += 2;
+        } elseif ($ch === ' ' || $ch === "\t" || $ch === "\r" || $ch === "\n") {
+            // End of the word. Everything after it is another word or a comment, and an
+            // unquoted conf value is one word by construction. \r is in the set because a conf
+            // saved with CRLF endings would otherwise carry one into every unquoted value.
+            break;
+        } else {
+            $out .= $ch; $i++;
+        }
+    }
+    // Clamped: an unterminated double quote runs $i one past the end.
+    $end = min($i, $n);
+    return $out;
 }
 
 // Parse a key=value state file (e.g. fallback_state.db, partnership_state.db).