diff --git a/Plugin/unraid/api/confform.php b/Plugin/unraid/api/confform.php index 8321294..2691266 100644 --- a/Plugin/unraid/api/confform.php +++ b/Plugin/unraid/api/confform.php @@ -122,7 +122,7 @@ if ($_SERVER['REQUEST_METHOD'] === 'POST') { } } - $results = vv_conf_write_changes($changes); + $results = vv_conf_write_changes($changes, $rejected); // Propagate master.conf to partner hosts when the owner edits it (mirrors rawconf.php). $push = []; @@ -131,7 +131,18 @@ if ($_SERVER['REQUEST_METHOD'] === 'POST') { vv_push_setup_state(); } - echo json_encode(['ok' => !in_array(false, $results, true), 'files' => $results, 'push' => $push]); + // A refused change never reaches a file, so it leaves no false in $results — a save whose + // only change was refused used to answer ok:true and show the operator their old value back + // with no explanation. Refusals are failures here and they are named: the whole point of the + // path guard is that someone learns their edit would have widened a delete target. + echo json_encode(['ok' => !in_array(false, $results, true) && !$rejected, + 'files' => $results, + 'rejected' => $rejected, + 'error' => $rejected + ? 'Refused: ' . implode(', ', + array_map(fn($r) => $r['key'] . ' (' . $r['reason'] . ')', $rejected)) + : null, + 'push' => $push]); exit; } diff --git a/Plugin/unraid/include/ai_repair.php b/Plugin/unraid/include/ai_repair.php index 48616ff..258df16 100644 --- a/Plugin/unraid/include/ai_repair.php +++ b/Plugin/unraid/include/ai_repair.php @@ -139,6 +139,20 @@ function vv_ai_finding_may_autofix(array $f): bool { if (empty($f['proven'])) return false; if (($f['proposed'] ?? null) === null) return false; if (vv_ai_conf_is_toggle((string)($f['conf_key'] ?? ''))) return false; + + // A third condition, for paths only. "Proven" means a probe answered, and every probe this + // has is a network probe — nothing in it can answer a filesystem question, so a path + // proposal reaches here carrying a proof that is about something else entirely. Requiring + // the directory to exist is the equivalent evidence, and it is the difference between + // pointing a cleanup at a real share and pointing it at a typo that will be created empty + // by the first script to write there. + // + // vv_conf_path_write_ok() still runs inside the writer underneath this. That one refuses + // what is dangerous; this one refuses what is merely unproven, which is a bar only the + // unattended path has to clear. + $proposed = (string)$f['proposed']; + if ($proposed !== '' && $proposed[0] === '/' && !file_exists($proposed)) return false; + return true; } diff --git a/Plugin/unraid/include/confform.php b/Plugin/unraid/include/confform.php index be8521c..44b8277 100644 --- a/Plugin/unraid/include/confform.php +++ b/Plugin/unraid/include/confform.php @@ -383,6 +383,69 @@ function vv_conf_value_safe(string $value): bool { return !preg_match('/\$\(|`|<\(|>\(/', $value); } +// ── Path values ────────────────────────────────────────────────────────────────────────────── +// A conf path is not just a string: several scripts delete inside one. arr_download_orphan_cleaner +// runs rm -rf on entries under a download dir, rsync.sh runs --delete against a profile's +// destination, and the transcode ramdisk tears down its own mount point. Those are correct +// against /mnt/user/Movies and catastrophic against /mnt/user, and nothing between the form and +// the file could previously tell those apart — bash -n proves the file parses, and the read-back +// proves the value arrived, but both are perfectly happy with a value that will erase a share. +// +// Depth is not the test. /tv, /movies and "/kids movies" are all real values in host1.conf: they +// are paths inside the arr containers, and a rule requiring two segments would refuse the conf +// this host already runs. What separates a safe path edit from a dangerous one is not how deep +// the new value is but which direction it moved — widening a path to its own ancestor is the +// edit that turns a targeted cleanup into a sweep, and it is meaningful at every depth. +// +// Scalars only. Arrays are spliced in verbatim by a different branch of the writer, and the arr +// root-folder maps that live in them deserve their own pass; what matters here is that every +// value the repair subsystem writes is a scalar, so its whole surface is covered. +// +// Returns null when the write is allowed, or the reason it is not. +function vv_conf_path_write_ok(string $key, string $new, ?string $old): ?string { + $isPath = fn(?string $v): bool => $v !== null && $v !== '' && $v[0] === '/'; + + // A ${VAR} reference is the established idiom for composing paths here and cannot be + // resolved statically, so it is left to the read-back the way it always was. + if ($new !== '' && $new[0] === '$') return null; + + // Not a path write at all unless one side of it is a path. + if (!$isPath($new) && !$isPath($old)) return null; + + // A newline inside double quotes is legal bash, so this is one of the few malformations + // bash -n cannot catch. A path carrying one arrives at rsync or rm as two arguments. + if (preg_match('/[\r\n\x00]/', $new)) return 'newline-in-path'; + + // Blanking a path is the classic: rm -rf "$DIR"/* with an empty DIR is rm -rf /*. The raw + // conf editor is the way to empty one deliberately — this writer is the automated path. + if ($isPath($old) && $new === '') return 'path-cleared'; + + // Was a path, is now something that resolves against whatever directory the script happens + // to be in. There is no safe answer to "delete inside Movies" when Movies is relative. + if ($isPath($old) && !$isPath($new)) return 'path-became-relative'; + + if (!$isPath($new)) return null; + + // '..' as a whole segment. Not a substring match — a share legitimately named "..old" is not + // traversal, and a rule that cannot tell them apart teaches people to work around it. + foreach (explode('/', $new) as $seg) { + if ($seg === '..') return 'path-traversal'; + } + + $norm = fn(string $p): string => rtrim(preg_replace('#/+#', '/', $p), '/'); + $n = $norm($new); + if ($n === '') return 'path-is-root'; + + // The rule this whole function exists for. An ancestor is matched at the segment boundary so + // /mnt/user/Movies2 is not read as a parent of /mnt/user/Movies. + if ($isPath($old)) { + $o = $norm((string)$old); + if ($o !== $n && str_starts_with($o . '/', $n . '/')) return 'path-broadened'; + } + + return null; +} + // 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. @@ -459,18 +522,37 @@ function vv_conf_audit(string $file, string $key, string $outcome, string $detai // 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 { +// +// $rejected collects what was refused before any file was touched. A rejected change is dropped +// from the batch, so a request whose only change is refused produces no results at all — and +// "no results" and "nothing went wrong" were indistinguishable to a caller checking for false. +// The settings form reported a save that had silently not happened. Callers that care pass this +// and say so; the two that write assoc arrays are unaffected and do not. +function vv_conf_write_changes(array $changes, array &$rejected = []): array { + $rejected = []; + $refuse = function (array $c, string $reason) use (&$rejected): void { + vv_conf_audit((string) $c['file'], (string) $c['key'], 'rejected', 'reason=' . $reason); + $rejected[] = ['file' => (string) $c['file'], 'key' => (string) $c['key'], 'reason' => $reason]; + }; + $byFile = []; foreach ($changes as $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'); + $refuse($c, 'malformed-key'); continue; } if (!vv_conf_value_safe((string) ($c['value'] ?? ''))) { - vv_conf_audit((string) $c['file'], (string) $c['key'], 'rejected', 'reason=command-substitution'); + $refuse($c, 'command-substitution'); continue; } + // Compared against the value the conf holds now, which is what makes "broadened" a + // question this can answer at all. Scalars only — see vv_conf_path_write_ok(). + if (($c['type'] ?? 'scalar') === 'scalar') { + $bad = vv_conf_path_write_ok((string) $c['key'], (string) ($c['value'] ?? ''), + vv_conf_vars()[$c['key']] ?? null); + if ($bad !== null) { $refuse($c, $bad); continue; } + } $byFile[$c['file']][] = $c; }