diff --git a/Plugin/unraid/README-unraid.md b/Plugin/unraid/README-unraid.md index 57d2f2c..05b8747 100644 --- a/Plugin/unraid/README-unraid.md +++ b/Plugin/unraid/README-unraid.md @@ -114,34 +114,64 @@ be the reason a page fails to load. ## ━━━ WHAT GUARDS THE API LAYER ━━━ -Every endpoint under `api/` is protected by exactly one thing: **the Unraid WebGUI session.** -Anything that can reach `/plugins/varaverk/api/*.php` with a valid session can do everything -this plugin can do — stop the array, power off the host, write `master.conf` and push it to -every partner, create an lldap user, write and schedule a root-run script. +Two layers, both supplied by the platform, neither implemented in this plugin. -That is the same trust level as the rest of the WebGUI, and it is the intended model. It is -written down here because two things about it are easy to assume and wrong. +**1. nginx session auth.** `/etc/nginx/nginx.conf` applies `satisfy any; allow unix:; deny all; +auth_request /auth-request.php;` to everything it serves, `/plugins/` included. A request +without a valid WebGUI session never reaches PHP — it is redirected to the login page. -**No endpoint validates a CSRF token.** `pages/partnership.php` and `pages/scheduler.php` send -Unraid's `csrf_token` with their POSTs, which reads like the token is checked somewhere. It is -not — no file in `api/` looks at it. A request that arrives with a logged-in session cookie is -honoured whatever caused the browser to send it. Adding validation is a worthwhile hardening -pass, but it is a real change: every caller has to send the token before any endpoint requires -it, or the UI breaks silently on whichever page was missed. Do it deliberately, in one pass, -with the pages open — not opportunistically while touching one endpoint. +**2. Unraid's CSRF prepend.** `/etc/php.ini` sets +`auto_prepend_file=/usr/local/emhttp/webGui/include/local_prepend.php`, which runs before the +first line of any endpoint and, for **every POST**, requires a valid token as either a +`csrf_token` body field or an `X-CSRF-Token` header. On a mismatch it logs to syslog and +`exit`s. php-fpm inherits this — there is no pool override in `/etc/php-fpm.d/www.conf`. -**`api/webhook.php` is the exception that authenticates nothing at all.** It exists to receive -Sonarr/Radarr/Lidarr download events, which arrive from a container rather than a browser. -`master.conf` carries a `WEBHOOK_SECRET` and the standalone Node listener on `WEBHOOK_PORT` -validates it — this WebGUI-hosted path does not. Injection is not the risk (the path is -validated and escaped); triggering work is. Closing it means adding a secret check here **and** -updating the webhook URL in each arr's settings, in that order. +Anything that clears both can do everything this plugin can do: stop the array, power off the +host, write `master.conf` and push it to every partner, create an lldap user, write and +schedule a root-run script. That is the same trust level as the rest of the WebGUI, and it is +the intended model. -Two further endpoints are worth knowing about because they read wider than the rest: -`api/api_test.php` returns an API key prefix and the live GraphQL schema, and -`api/import_script.php`'s browse action and `api/manual_sync.php`'s browse actions list -directory names anywhere on either host. All three are read-only and none return file contents, -but they are the ones to look at first if the session boundary ever moves. +### The two consequences that actually matter + +**GET is not covered.** The prepend inspects POSTs only. So any action that *changes something* +must be POST — over GET it would run with no token check at all. Four actions were GET and have +been converted: `create_api_key.php`, `arrs.php?action=refresh_remote`, `cert.php?action=run` +and `setup.php?action=ssh_generate`. **When adding an endpoint the rule is simply: if it changes +state, it is POST.** Read-only GETs (`monitor`, `board`, `checklist`, the browse pickers, +`api_test.php`) are bounded by the session alone, which is correct for reads. + +**The webGUI's token injector is jQuery-only.** `$.ajaxPrefilter` in dynamix's +`HeadInlineJS.php` appends `csrf_token` to jQuery POSTs. Varaverk's pages use native `fetch()`, +which it does not touch — so a `fetch()` POST carries no token and the prepend kills it +silently: `csrf_terminate()` exits with an empty body, `r.json()` throws on the empty response, +and the page's own `.catch()` swallows it. There is no error anywhere except a syslog line. + +`Varaverk.page` therefore installs a small `window.fetch` shim, inline and above the tab +content, that attaches `X-CSRF-Token` to same-origin `/plugins/varaverk/` requests. The header +form is deliberate: it works for `FormData`, `URLSearchParams` and raw JSON bodies alike, so no +call site has to know about it and a new one cannot forget it. A cross-origin page cannot set +a custom header without a preflight it will fail, which is what makes it a real defence rather +than a formality. + +**Do not construct POSTs outside `fetch()`** — a raw `XMLHttpRequest` or a generated form — +without adding the token yourself; the shim only wraps `fetch`. A plain `
` +is already covered by a different platform mechanism: dynamix's `BodyInlineJS.php` appends a +hidden `csrf_token` input to every form on the page, which is what makes `VaraverkSettings.page` +work without any of this. + +### api/webhook.php is dead code + +It receives arr download events — but an arr has no WebGUI session and no CSRF token, so it is +blocked by *both* layers above and cannot be called at all. The live path is +`Arrs_Stack/webhook_listener.js` on `WEBHOOK_PORT`, running outside nginx entirely and +validating `WEBHOOK_SECRET` itself. `api/webhook.php` cannot be made to work by adding a secret +check — it would have to be served from outside `/plugins/` first. Removing it is the other +reasonable option. + +Two endpoints read wider than the rest and are worth knowing about if the session boundary ever +moves: `api/api_test.php` returns an API key prefix and the live GraphQL schema, and the browse +actions in `api/import_script.php` and `api/manual_sync.php` list directory names anywhere on +either host. All are read-only and none return file contents. --- diff --git a/Plugin/unraid/Varaverk.page b/Plugin/unraid/Varaverk.page index 1c2fc5a..49da25a 100644 --- a/Plugin/unraid/Varaverk.page +++ b/Plugin/unraid/Varaverk.page @@ -8,7 +8,55 @@ $docroot = $docroot ?? $_SERVER['DOCUMENT_ROOT'] ?: '/usr/local/emhttp'; $pluginDir = "$docroot/plugins/$plugin"; require_once "$pluginDir/include/config.php"; +?> + + + re-run the partner cache writer for one host +// GET cached (300s) full payload +// POST action=refresh_remote host=host re-run the partner cache writer for one host +// (POST, so Unraid's CSRF guard applies) // // RESPONSE // normal vv_arrs_all() verbatim — local node live, remote nodes cached with cache_age @@ -70,9 +71,17 @@ require_once dirname(__DIR__) . '/include/config.php'; define('VV_ARRS_REFRESH_TIMEOUT', 120); // ── Manual remote refresh — runs remote_arr_cache_writer for one host ───────── -$_action = trim($_GET['action'] ?? ''); +// POST only. The refresh executes a script, and Unraid's CSRF prepend validates POSTs while +// ignoring GETs entirely — so reaching this over GET would mean running it with no token +// check. A GET naming the action is refused rather than falling through to the cached read, +// so a stale caller fails visibly instead of silently appearing to succeed. +if (trim($_GET['action'] ?? '') === 'refresh_remote') { + http_response_code(405); + echo json_encode(['ok' => false, 'error' => 'POST only']); exit; +} +$_action = ($_SERVER['REQUEST_METHOD'] === 'POST') ? trim($_POST['action'] ?? '') : ''; if ($_action === 'refresh_remote') { - $host = strtolower(trim($_GET['host'] ?? '')); + $host = strtolower(trim($_POST['host'] ?? '')); if (!preg_match('/^host\d+$/', $host)) { echo json_encode(['ok' => false, 'error' => 'Invalid host']); exit; } diff --git a/Plugin/unraid/api/auth.php b/Plugin/unraid/api/auth.php index 7895b3b..3aa040c 100644 --- a/Plugin/unraid/api/auth.php +++ b/Plugin/unraid/api/auth.php @@ -58,10 +58,12 @@ // authoring a fresh config — a config it created would carry no rules and a default // policy, which is an accidental open door. // -// Known gap: no CSRF token is validated on the POST actions. -// The endpoint is guarded by the Unraid WebGUI session alone, and this is the highest- -// value endpoint in the plugin to reach — it can create a user and open a proxy host. -// Shared with the rest of the api layer; see the CSRF note in README-unraid.md. +// The POST actions are CSRF-guarded by the platform, not by this file. +// Unraid's auto_prepend (webGui/local_prepend.php) validates a token on every POST and +// terminates the request before any code here runs. That guard is the reason every +// mutating action lives in the POST arm — this is the highest-value endpoint in the +// plugin to reach, since it can create a user and open a proxy host. The five GET +// actions are reads and are deliberately outside it. See README-unraid.md. // // REQUEST // GET ?action=npm_proxies | npm_certs | lldap_users | lldap_groups | authelia_rules diff --git a/Plugin/unraid/api/cert.php b/Plugin/unraid/api/cert.php index 9fdb4bf..a4ae61a 100644 --- a/Plugin/unraid/api/cert.php +++ b/Plugin/unraid/api/cert.php @@ -67,15 +67,17 @@ // response is treated as a certificate list — so an unreachable NPM is reported as such // instead of rendering as zero certificates. // -// Accepted: the run action is reachable over GET. -// It re-runs a read-only monitor and writes only its own cache, so repeating it is -// harmless. Guarded by the Unraid WebGUI session; see the CSRF note in README-unraid.md. +// The run action is POST only, which is what places it behind Unraid's CSRF guard. +// The platform prepend validates the token on every POST and inspects no GET at all, so +// an action that executes a script must not be reachable by GET. The three read actions +// stay GET-reachable because they change nothing. // // REQUEST // GET cached status, or the configured domains when no cache exists // GET|POST ?action=npm live certificate list from NPM's API // GET|POST ?action=domains configured domains and thresholds, no checks run -// GET|POST ?action=run re-run cert_monitor.sh, then return its fresh cache +// POST action=run re-run cert_monitor.sh, then return its fresh cache +// (POST, so Unraid's CSRF guard applies) // // RESPONSE // default {"ok":true,"checked_at","host","warn_days","crit_days","domains":[…]} @@ -167,6 +169,13 @@ if ($action === 'domains') { // ── Run cert_monitor.sh now ─────────────────────────────────────────────────── if ($action === 'run') { + // POST only. This executes a script, and Unraid's CSRF prepend validates POSTs while + // ignoring GETs entirely — over GET it would run with no token check at all. + if ($_SERVER['REQUEST_METHOD'] !== 'POST') { + http_response_code(405); + echo json_encode(['ok' => false, 'error' => 'POST only']); + exit; + } $script = SCRIPTS_DIR . '/Monitors/cert_monitor.sh'; if (!file_exists($script)) { echo json_encode(['ok' => false, 'error' => 'cert_monitor.sh not found']); diff --git a/Plugin/unraid/api/create_api_key.php b/Plugin/unraid/api/create_api_key.php index af5b47a..27fc59a 100644 --- a/Plugin/unraid/api/create_api_key.php +++ b/Plugin/unraid/api/create_api_key.php @@ -46,14 +46,14 @@ // Cache-Control: no-store, no-cache. A cached provisioning response would report an old // key preview after a genuine renewal — the one moment the preview matters. // -// Known gap: this is a state-changing action served over GET. -// Both callers (pages/setup.php, pages/partnership.php) fetch it, and POST bodies are -// unreliable on this nginx/PHP setup. It is guarded by the Unraid WebGUI session rather -// than by method or token. Documented rather than silently accepted — see the CSRF note -// in README-unraid.md. +// POST only, which is what places it behind Unraid's CSRF guard. +// The platform prepend validates the token on every POST and inspects no GET at all, so +// a state-changing action reachable by GET is a state-changing action with no CSRF +// protection. This was previously a GET; the callers now POST and the token travels as +// an X-CSRF-Token header set by the shim in Varaverk.page. See README-unraid.md. // // REQUEST -// GET, no parameters (the target host is the local host, by construction) +// POST, no parameters (the target host is the local host, by construction) // // RESPONSE // {"ok":true,"key_preview":"abcd1234...wxyz"} or "registered" when the key is not readable @@ -67,6 +67,14 @@ header('Content-Type: application/json'); header('Cache-Control: no-store, no-cache'); require_once dirname(__DIR__) . '/include/config.php'; +// POST is what puts this behind Unraid's CSRF guard — the platform prepend validates every +// POST and ignores every GET, so a state-changing action must not be reachable by GET. +if ($_SERVER['REQUEST_METHOD'] !== 'POST') { + http_response_code(405); + echo json_encode(['ok' => false, 'error' => 'POST only']); + exit; +} + $host = vv_detect_host(); if (!preg_match('/^host\d+$/', $host)) { echo json_encode(['ok' => false, 'error' => 'Cannot detect local host']); diff --git a/Plugin/unraid/api/import_script.php b/Plugin/unraid/api/import_script.php index ae75837..bf42f59 100644 --- a/Plugin/unraid/api/import_script.php +++ b/Plugin/unraid/api/import_script.php @@ -67,8 +67,9 @@ // // Accepted exposure: the browser can list any directory on the host. // It returns directory names and .sh filenames only — no file contents, and nothing -// outside those two types. That is the minimum a file picker rooted at / can do, and it -// is guarded by the Unraid WebGUI session; see the CSRF note in README-unraid.md. +// outside those two types. That is the minimum a file picker rooted at / can do. browse +// is a GET read, so the WebGUI session is its whole boundary; the import itself is POST +// and is CSRF-guarded by Unraid's auto_prepend. See README-unraid.md. // // REQUEST // GET ?action=browse&path=/absolute/dir list subdirectories and .sh files diff --git a/Plugin/unraid/api/manual_sync.php b/Plugin/unraid/api/manual_sync.php index eb965b8..b7fbcbf 100644 --- a/Plugin/unraid/api/manual_sync.php +++ b/Plugin/unraid/api/manual_sync.php @@ -93,8 +93,9 @@ // output in /tmp indefinitely. /tmp is tmpfs, so an abandoned one clears at reboot anyway. // // Accepted exposure: browse can list any directory on either host. -// Directory names only — no file contents, no file names. Guarded by the Unraid WebGUI -// session; see the CSRF note in README-unraid.md. +// Directory names only — no file contents, no file names. browse and poll are GET reads +// bounded by the WebGUI session; run and stop are POST and are CSRF-guarded by Unraid's +// auto_prepend before any code here runs. See README-unraid.md. // // REQUEST // GET ?action=hosts partners, Tailscale state, key presence diff --git a/Plugin/unraid/api/partnership_settings.php b/Plugin/unraid/api/partnership_settings.php index 29eb59f..ffa2123 100644 --- a/Plugin/unraid/api/partnership_settings.php +++ b/Plugin/unraid/api/partnership_settings.php @@ -43,7 +43,8 @@ // Credential fields are returned as they appear in the conf. // The partnership sections carry SSH key paths and API keys, and this endpoint returns // them for editing. That is the same exposure the raw conf editor has, over the same -// WebGUI session; see the CSRF note in README-unraid.md. +// WebGUI session. This is a GET read, so Unraid's POST-only CSRF guard does not apply — +// the session is its whole boundary. See README-unraid.md. // // REQUEST // GET, no parameters diff --git a/Plugin/unraid/api/script.php b/Plugin/unraid/api/script.php index 8f99904..c7f32a2 100644 --- a/Plugin/unraid/api/script.php +++ b/Plugin/unraid/api/script.php @@ -54,8 +54,9 @@ // // Accepted by design: this endpoint writes an executable root-run script from a browser. // That is the entire feature, and it is why it is confined to a directory outside the -// repo with a flat namespace and a strict name pattern. It is guarded by the Unraid -// WebGUI session; see the CSRF note in README-unraid.md. +// repo with a flat namespace and a strict name pattern. The save and delete paths are +// POST, so Unraid's auto_prepend validates a CSRF token before any of this runs; the +// WebGUI session is the outer boundary. See README-unraid.md. // // REQUEST // GET ?id=Custom/.sh read (empty content when absent) diff --git a/Plugin/unraid/api/setup.php b/Plugin/unraid/api/setup.php index fcd27ff..9bcebb9 100644 --- a/Plugin/unraid/api/setup.php +++ b/Plugin/unraid/api/setup.php @@ -82,13 +82,17 @@ // The SCP target is the local conf path, composed here — no part of the request names a // destination file. // -// Accepted: this endpoint writes credentials and runs setup scripts as root. -// It is the setup wizard; that is its function. It is guarded by the Unraid WebGUI -// session; see the CSRF note in README-unraid.md. +// Every action that changes anything is POST only, which is what places them behind +// Unraid's CSRF guard. +// The platform prepend validates the token on every POST and inspects no GET at all. +// Only `detect` remains GET-reachable, and it is a pure read. This endpoint writes +// credentials and runs setup scripts as root — that is its function, and it is why the +// method boundary is the one that matters here. // // REQUEST // GET ?action=detect hostname, Unraid version, boot transport, suggested mode -// GET|POST action=ssh_generate generate the local keypair, return the public key +// POST action=ssh_generate generate the local keypair, return the public key +// (POST, so Unraid's CSRF guard applies) // POST action=populate run conf_populate.sh --no-push // POST action=pull partner: SCP master.conf from HOST1, create host conf // [my_slot] [my_hostname] [host1_hostname] @@ -149,6 +153,13 @@ if ($action === 'detect') { // ── GET/POST: generate local SSH keypair ────────────────────────────────────────────────────── if ($action === 'ssh_generate') { + // POST only. This generates a keypair, and Unraid's CSRF prepend validates POSTs while + // ignoring GETs entirely — over GET it would run with no token check at all. + if ($_SERVER['REQUEST_METHOD'] !== 'POST') { + http_response_code(405); + echo json_encode(['ok' => false, 'error' => 'POST only']); + exit; + } $script = SCRIPTS_DIR . '/Partnership/ssh_setup.sh'; if (!file_exists($script)) { echo json_encode(['ok' => false, 'error' => 'ssh_setup.sh not found']); diff --git a/Plugin/unraid/api/webhook.php b/Plugin/unraid/api/webhook.php index fd642d7..0629c96 100644 --- a/Plugin/unraid/api/webhook.php +++ b/Plugin/unraid/api/webhook.php @@ -64,15 +64,22 @@ // Output is appended to a dedicated log with stdin detached, so a backgrounded handler // cannot hold the request's file descriptors open. // -// Known gap: this endpoint authenticates nothing. -// master.conf carries WEBHOOK_SECRET, and the standalone Node listener on WEBHOOK_PORT -// validates it — this WebGUI-hosted path does not. Anyone who can reach the URL can -// make it run the handler against any absolute path that passes validation. Injection -// is not the risk (the path is escaped); triggering work is. Left as-is deliberately -// rather than fixed in passing: adding a secret check here would break whichever arr -// instances are currently configured against this URL, and that is a change to make -// with the arr configs open, not as part of a documentation pass. See also the CSRF -// note in README-unraid.md. +// UNREACHABLE — this endpoint cannot currently be called by an arr at all. +// Two platform layers stand in front of it, and an arr satisfies neither. nginx applies +// `auth_request` to everything under /plugins/, so a request without a WebGUI session is +// redirected to the login page. And Unraid's auto_prepend terminates every POST that +// does not carry a valid CSRF token — which an arr has no way to obtain. +// +// The live path is the standalone Node listener, Arrs_Stack/webhook_listener.js, running +// on WEBHOOK_PORT outside nginx entirely and validating WEBHOOK_SECRET itself. That is +// what the arrs are pointed at, and it is why it exists. +// +// This file is therefore dead code kept for reference, not a second live entry point. +// Nothing here authenticates, because nothing here can be reached. Do not "fix" it by +// adding a secret check and re-pointing an arr at it — it would still be blocked by the +// two layers above. If it is ever wanted as a real entry point it needs to be served +// from outside /plugins/, at which point it needs WEBHOOK_SECRET validation first. +// Removing it outright is the other reasonable option. See README-unraid.md. // // REQUEST // POST diff --git a/Plugin/unraid/pages/arrs.php b/Plugin/unraid/pages/arrs.php index 00d8556..8acbba0 100644 --- a/Plugin/unraid/pages/arrs.php +++ b/Plugin/unraid/pages/arrs.php @@ -435,7 +435,10 @@ setInterval(vvArrsLoad, 60000); function vvArrsRefreshRemote(host) { const btn = document.getElementById('vv-arr-rfsh-' + host); if (btn) { btn.disabled = true; btn.textContent = '↻…'; } - fetch(`/plugins/varaverk/api/arrs.php?action=refresh_remote&host=${host}&_=` + Date.now()) + const fd = new FormData(); + fd.append('action', 'refresh_remote'); + fd.append('host', host); + fetch('/plugins/varaverk/api/arrs.php', { method: 'POST', body: fd }) .then(r => r.json()) .then(d => { if (btn) { btn.disabled = false; btn.textContent = d.ok ? '↻' : '✗'; } diff --git a/Plugin/unraid/pages/partnership.php b/Plugin/unraid/pages/partnership.php index fe64f5e..221a8e8 100644 --- a/Plugin/unraid/pages/partnership.php +++ b/Plugin/unraid/pages/partnership.php @@ -202,7 +202,7 @@ function vvApiKey(btn) { const origText = btn.textContent; btn.disabled = true; btn.textContent = '⟳ Working…'; - fetch('/plugins/varaverk/api/create_api_key.php?_=' + Date.now()) + fetch('/plugins/varaverk/api/create_api_key.php', { method: 'POST' }) .then(r => { if (!r.ok || r.status === 0) throw new Error('HTTP ' + r.status + ' ' + r.statusText); return r.text(); diff --git a/Plugin/unraid/pages/setup.php b/Plugin/unraid/pages/setup.php index 585c84a..94b0de0 100644 --- a/Plugin/unraid/pages/setup.php +++ b/Plugin/unraid/pages/setup.php @@ -397,7 +397,7 @@ function vvPullMaster(btn) { function vvCreateKey(btn) { const status = document.getElementById('vv-key-status'); btn.disabled = true; btn.textContent = '⟳ Creating…'; - fetch('/plugins/varaverk/api/create_api_key.php?_=' + Date.now()) + fetch('/plugins/varaverk/api/create_api_key.php', { method: 'POST' }) .then(r => r.json()).then(d => { if (d.ok) { status.textContent = '✓ Key created — ' + d.key_preview;