From 0101aef51a77dcc1d5f392d12eead0e2feb49af2 Mon Sep 17 00:00:00 2001 From: Gmer4Lfe Date: Fri, 14 Aug 2026 23:39:12 -0400 Subject: [PATCH] Let a password containing a $ actually save, and say why when a save is refused MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../System_Essentials/unraid_api_key_renew.sh | 9 +- Plugin/unraid/include/confform.php | 113 +++++++++++++++--- Plugin/unraid/include/config.php | 72 +++++++++-- 3 files changed, 167 insertions(+), 27 deletions(-) diff --git a/Plugin/unraid/System_Essentials/unraid_api_key_renew.sh b/Plugin/unraid/System_Essentials/unraid_api_key_renew.sh index e4b0452..56dfc85 100755 --- a/Plugin/unraid/System_Essentials/unraid_api_key_renew.sh +++ b/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" diff --git a/Plugin/unraid/include/confform.php b/Plugin/unraid/include/confform.php index 2809bb2..97b1e61 100644 --- a/Plugin/unraid/include/confform.php +++ b/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 - if (preg_match('/^\s*([A-Z_][A-Z0-9_]*)\s*=\s*"([^"]*)"(?:\s+#\s*(.+))?$/', $line, $m)) { - $fields[] = ['key' => $m[1], 'value' => $m[2], - 'type' => 'scalar', 'desc' => ($m[3] ?? '') ?: $desc, 'file' => $filename, - '_lines' => $descLines]; - continue; - } - if (preg_match('/^\s*([A-Z_][A-Z0-9_]*)\s*=\s*([^(\n#]*?)(?:\s+#\s*(.+))?$/', $line, $m)) { - $val = trim($m[2]); - if ($val === '') continue; + // Scalar: KEY="value", KEY='value' or KEY=value. + // + // One branch, and it unquotes rather than capturing between quotes. Two regexes did this + // — one for the quoted form, one for the bare form — and neither understood an escape, so + // a value containing \" fell through the first (the capture ended at the escaped quote and + // 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. + // + // A secret now carries \$ and \` for exactly the reason vv_conf_quote_scalar() explains, + // 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 - // 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], + fn($m) => $m[1] . $quoted . $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; } } diff --git a/Plugin/unraid/include/config.php b/Plugin/unraid/include/config.php index 98d7cc6..ace4e06 100644 --- a/Plugin/unraid/include/config.php +++ b/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 — -// a password or a colour would otherwise be truncated at the first hash. Unquoted, the comment -// must be introduced by whitespace, matching bash: FOO=#fff and FOO=bar#baz both assign literally, -// since # only opens a comment at the start of a word. +// Quotes are honoured before that, because inside them a # is data, not a comment — a password +// or a colour would otherwise be truncated at the first hash. Unquoted, the comment must be +// introduced by whitespace, matching bash: FOO=#fff and FOO=bar#baz both assign literally, since +// # 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]); - if (preg_match('/^"([^"\n]*)"/', $v, $q)) return $q[1]; - if (preg_match("/^'([^'\n]*)'/", $v, $q)) return $q[1]; - return trim(preg_replace('/\s+#.*$/', '', $v)); + return vv_conf_unquote(ltrim($m[1])); +} + +// 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).