diff --git a/CHANGELOG.md b/CHANGELOG.md index 8b20655..0e54bf0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ --- 3.0 --- +* security: Require POST for the save, remove, mute and card-order actions so csrf-magic covers them, and only let a dashboard's owner save over it * bug: Restore alert audio for down and triggered devices; getMonitorSound() looked for sounds in includes/sounds/ after the controller moved, so the audio element was never rendered * feature: Replace the single Status column/filter with a multi-select Events filter (additive) and an Events column that shows a short pill per active event (Down, Recovering, Triggered, Down/Triggered M/A, Warn/Alert Ping, Failed Svc), 'None' when clean; the Status scope dropdown now only selects All Monitored / All Devices / Not Monitored / Without Thresholds * bug: Centre the tile hostname under the glyph and keep it clickable in the Tiles & Time view diff --git a/includes/controller.php b/includes/controller.php index 1aa669b..345af6e 100644 --- a/includes/controller.php +++ b/includes/controller.php @@ -93,6 +93,29 @@ function monitorDashboardOwnedByUser(int $dashboard): bool { return $owner !== null && $owner !== '' && (int) $owner === (int) ($_SESSION['sess_user_id'] ?? -1); } +/** + * Requested action, or '' when a state-changing action arrives without POST. + * + * csrf-magic only checks POST requests, so these actions must not run from a + * GET or the token check never sees them. + * + * @return string + */ +function monitorRequestAction(): string { + $action = get_nfilter_request_var('action'); + + if (!is_string($action)) { + return ''; + } + + if (in_array($action, ['save', 'saveDb', 'remove', 'ajax_mute_all', 'ajax_unmute_all', 'ajax_saveorder'], true) + && ($_SERVER['REQUEST_METHOD'] ?? '') !== 'POST') { + return ''; + } + + return $action; +} + /** * Persist a grouping's card ordering after a drag/drop reorder. * @@ -975,12 +998,7 @@ function saveSettings(): void { 'site' => get_request_var('site'), ]; - if (!isset_request_var('user')) { - $user = $_SESSION['sess_user_id']; - } else { - $user = get_request_var('user'); - } - + $user = $_SESSION['sess_user_id']; $id = get_request_var('dashboard'); $name = get_nfilter_request_var('name'); @@ -998,13 +1016,21 @@ function saveSettings(): void { $save['user_id'] = $user; $save['properties'] = json_encode($props); - $id = sql_save($save, 'plugin_monitor_dashboards'); + $level = MESSAGE_LEVEL_INFO; + + // sql_save() updates an existing id in place, so only the owner may target one. + if ($id > 0 && !monitorDashboardOwnedByUser($id)) { + $id = 0; + $level = MESSAGE_LEVEL_ERROR; + } else { + $id = sql_save($save, 'plugin_monitor_dashboards'); + } if (!empty($id)) { raise_message('monitorsaved', __('Dashboard \'%s\' has been Saved!', $name, 'monitor'), MESSAGE_LEVEL_INFO); set_request_var('dashboard', $id); } else { - raise_message('monitornotsaved', __('Dashboard \'%s\' could not be Saved!', $name, 'monitor'), MESSAGE_LEVEL_INFO); + raise_message('monitornotsaved', __('Dashboard \'%s\' could not be Saved!', $name, 'monitor'), $level); set_request_var('dashboard', '0'); } } diff --git a/js/monitor.js b/js/monitor.js index 1e61c53..cfdd975 100644 --- a/js/monitor.js +++ b/js/monitor.js @@ -182,7 +182,11 @@ function applyFilter(action = '') { strURL += `&events=${events ? events.join(',') : ''}`; } - loadIt(strURL); + if (action === 'ajax_mute_all' || action === 'ajax_unmute_all') { + postIt(strURL, { __csrf_magic: csrfMagicToken }); + } else { + loadIt(strURL); + } } function saveFilter() { @@ -242,8 +246,13 @@ function saveNewDashboard(action) { } function removeDashboard() { - const url = `monitor.php?action=remove&header=false&dashboard=${$('#dashboard').val()}`; - loadIt(url); + const post = { + action: 'remove', + dashboard: $('#dashboard').val(), + __csrf_magic: csrfMagicToken + }; + + postIt('monitor.php?header=false', post); } function loadIt(url) { diff --git a/monitor.php b/monitor.php index a7b432e..7c2c3e5 100644 --- a/monitor.php +++ b/monitor.php @@ -172,7 +172,7 @@ $servcheck_counts = monitorServcheckCounts(); } -switch (get_nfilter_request_var('action')) { +switch (monitorRequestAction()) { case 'ajax_status': ajaxStatus(); diff --git a/tests/Unit/MonitorDashboardSaveTest.php b/tests/Unit/MonitorDashboardSaveTest.php new file mode 100644 index 0000000..14905e9 --- /dev/null +++ b/tests/Unit/MonitorDashboardSaveTest.php @@ -0,0 +1,85 @@ + $c['fn'] === 'sql_save')); +} + +it('refuses to save over a dashboard owned by another user', function () { + monitor_test_mock_db('db_fetch_cell_prepared', 'SELECT user_id', '2'); + test_set_request(['dashboard' => 7, 'name' => 'Mine']); + + saveSettings(); + + expect(monitor_test_dashboard_saves())->toBe([]) + ->and(get_request_var('dashboard'))->toBe('0') + ->and($GLOBALS['__test_messages'])->toBe([ + ['id' => 'monitornotsaved', 'text' => 'Dashboard \'%s\' could not be Saved!', 'level' => MESSAGE_LEVEL_ERROR], + ]); +}); + +it('refuses to save over a shared dashboard', function () { + monitor_test_mock_db('db_fetch_cell_prepared', 'SELECT user_id', '0'); + test_set_request(['dashboard' => 3, 'name' => 'Shared']); + + saveSettings(); + + expect(monitor_test_dashboard_saves())->toBe([]); +}); + +it('saves a dashboard the user owns', function () { + monitor_test_mock_db('db_fetch_cell_prepared', 'SELECT user_id', '5'); + test_set_request(['dashboard' => 7, 'name' => 'Mine']); + + saveSettings(); + + $saves = monitor_test_dashboard_saves(); + + expect($saves)->toHaveCount(1) + ->and($saves[0]['table'])->toBe('plugin_monitor_dashboards') + ->and($saves[0]['data']['id'])->toBe(7) + ->and($saves[0]['data']['user_id'])->toBe(5) + ->and(array_column($GLOBALS['__test_messages'], 'level'))->toBe([MESSAGE_LEVEL_INFO]); +}); + +it('ignores a forged user when creating a dashboard', function () { + test_set_request(['dashboard' => -1, 'name' => 'New', 'user' => 2]); + + saveSettings(); + + $saves = monitor_test_dashboard_saves(); + + expect($saves)->toHaveCount(1) + ->and($saves[0]['data']['user_id'])->toBe(5); +}); + +it('ignores a forged user when updating an owned dashboard', function () { + monitor_test_mock_db('db_fetch_cell_prepared', 'SELECT user_id', '5'); + test_set_request(['dashboard' => 7, 'name' => 'Mine', 'user' => 0]); + + saveSettings(); + + $saves = monitor_test_dashboard_saves(); + + expect($saves)->toHaveCount(1) + ->and($saves[0]['data']['user_id'])->toBe(5); +}); diff --git a/tests/Unit/MonitorRequestActionTest.php b/tests/Unit/MonitorRequestActionTest.php new file mode 100644 index 0000000..c1be123 --- /dev/null +++ b/tests/Unit/MonitorRequestActionTest.php @@ -0,0 +1,64 @@ + $action]); + + expect(monitorRequestAction())->toBe(''); +})->with('state_changing_actions'); + +it('drops a state-changing action when there is no request method', function (string $action) { + test_set_request(['action' => $action]); + + expect(monitorRequestAction())->toBe(''); +})->with('state_changing_actions'); + +it('dispatches a state-changing action sent by POST', function (string $action) { + $_SERVER['REQUEST_METHOD'] = 'POST'; + test_set_request(['action' => $action]); + + expect(monitorRequestAction())->toBe($action); +})->with('state_changing_actions'); + +it('still dispatches read-only actions sent by GET', function () { + $_SERVER['REQUEST_METHOD'] = 'GET'; + test_set_request(['action' => 'ajax_status']); + + expect(monitorRequestAction())->toBe('ajax_status'); +}); + +it('ignores an action that is not a string', function () { + $_SERVER['REQUEST_METHOD'] = 'POST'; + test_set_request(['action' => ['remove']]); + + expect(monitorRequestAction())->toBe(''); +}); + +it('routes the monitor.php dispatcher through the method check', function () { + $contents = file_get_contents(dirname(__DIR__, 2) . '/monitor.php'); + + expect($contents)->toContain('switch (monitorRequestAction()) {') + ->and($contents)->not->toContain("switch (get_nfilter_request_var('action'))"); +}); diff --git a/tests/bootstrap-unit.php b/tests/bootstrap-unit.php index da6c895..8e343fb 100644 --- a/tests/bootstrap-unit.php +++ b/tests/bootstrap-unit.php @@ -377,6 +377,7 @@ function is_realm_allowed($realm) { if (!function_exists('raise_message')) { function raise_message($id, $text = '', $level = 0) { + $GLOBALS['__test_messages'][] = array('id' => $id, 'text' => $text, 'level' => $level); } } @@ -386,6 +387,17 @@ function get_request_var($name) { } } +if (!function_exists('isset_request_var')) { + function isset_request_var($name) { + return isset($GLOBALS['__test_request'][$name]); + } +} + +if (!function_exists('validate_store_request_vars')) { + function validate_store_request_vars($filters, $sess_prefix = '') { + } +} + if (!function_exists('get_nfilter_request_var')) { function get_nfilter_request_var($name) { return get_request_var($name); @@ -433,6 +445,8 @@ function is_error_message() { if (!function_exists('sql_save')) { function sql_save($array, $table, $key = 'id') { + $GLOBALS['__test_db_calls'][] = array('fn' => 'sql_save', 'table' => $table, 'data' => $array); + return isset($array['id']) ? $array['id'] : 1; } } @@ -457,8 +471,16 @@ function sql_save($array, $table, $key = 'id') { define('POLLER_VERBOSITY_NONE', 6); } +if (!defined('FILTER_VALIDATE_IS_REGEX')) { + define('FILTER_VALIDATE_IS_REGEX', 99999); +} + +if (!defined('MESSAGE_LEVEL_INFO')) { + define('MESSAGE_LEVEL_INFO', 1); +} + if (!defined('MESSAGE_LEVEL_ERROR')) { - define('MESSAGE_LEVEL_ERROR', 1); + define('MESSAGE_LEVEL_ERROR', 3); } if (!function_exists('plugin_test_read_source')) {