diff --git a/CHANGELOG.md b/CHANGELOG.md index c544929..ba6cdb3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ * feature: Shorten the graph 'Add to QuickTree' action icon's title to "Add to Quicktree" so it reads cleanly as a label in the Cacti graph drilldown action menu * bug: Pass a boolean (not the integer 1) to api_plugin_register_hook()'s $enable argument so the hook registration stays type-correct under Cacti 1.3's strict bool type-hint (the int only coerces while the file lacks declare(strict_types=1)) * security: Move the graph 'Add to QuickTree' icon's inline `onClick='addQuickTree()'` handler into a delegated jQuery binding in `js/quicktree.js` (via a `quicktreeAddLink` class and `data-` attributes) and replace the confirmation pages' inline `onClick='cactiReturnTo()'` Cancel buttons with the CSP-safe `cactiReturnTo` class, so the pages no longer trip Cacti's Content-Security-Policy script-src-attr directive +* security: Require a POST request with a valid CSRF token for the add, clear and save actions using Cacti's shared guard with a fallback for older hosts, and send the add request from the graph button as a POST * dev: Replace the translation-template CI check that regenerated locales/po/cacti.pot and compared it with a diff-based gate (tests/bin/check-i18n-pot.php) requiring cacti.pot to be updated only when a pull request adds, removes, or modifies an i18n function call * dev: Measure CI coverage with xdebug instead of pcov so the plugin's own sources are instrumented (pcov auto-scopes to the Composer root and skipped cacti/plugins/, leaving the patch-coverage gate with nothing to measure) * security: Normalize the requested location before building the breadcrumb URL so a crafted location value is not reflected into the navigation diff --git a/js/quicktree.js b/js/quicktree.js index 70dafd0..083340e 100644 --- a/js/quicktree.js +++ b/js/quicktree.js @@ -23,13 +23,11 @@ */ function addQuickTree(local_graph_id, rra_id) { - var strURL = urlPath + - 'plugins/quicktree/quicktree.php' + - '?action=add' + - '&rra_id=' + rra_id + - '&graph_id=' + local_graph_id; + const strURL = urlPath + 'plugins/quicktree/quicktree.php'; - $.getJSON(strURL, function(message) { + $.ajax({type: 'POST', url: strURL, dataType: 'json', + data: {action: 'add', rra_id: rra_id, graph_id: local_graph_id}, + success: function(message) { if (message.title) { sessionMessageTitle = message.title; } @@ -39,7 +37,7 @@ function addQuickTree(local_graph_id, rra_id) { } displayMessages(); - }); + }}); } $(function() { diff --git a/quicktree.php b/quicktree.php index 806d0d2..242b25d 100644 --- a/quicktree.php +++ b/quicktree.php @@ -59,6 +59,23 @@ $action = $code_actions[$drp_action]; } +// remove stays GET until its row links become POST forms. +if (in_array($action, ['add', 'clear', 'save'], true)) { + if (function_exists('csrf_require_post')) { + csrf_require_post(); + } elseif (($_SERVER['REQUEST_METHOD'] ?? 'GET') !== 'POST') { + header('Allow: POST'); + http_response_code(405); + exit; + } + + // The 1.2.x helper checks the method only; validate the token on every host. + if (!function_exists('csrf_check') || !csrf_check(false)) { + http_response_code(403); + exit; + } +} + switch ($action) { case 'add': $graph = 0; diff --git a/tests/Unit/MutationStatusTest.php b/tests/Unit/MutationStatusTest.php new file mode 100644 index 0000000..68046bf --- /dev/null +++ b/tests/Unit/MutationStatusTest.php @@ -0,0 +1,20 @@ + ['pipe', 'w'], 2 => ['pipe', 'w']], $pipes); + $stdout = stream_get_contents($pipes[1]); + $stderr = stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + + expect(proc_close($process))->toBe(0) + ->and($stderr)->toBe('') + ->and(trim($stdout))->toBe('69 mutation guard cases passed'); +}); diff --git a/tests/client-post.test.mjs b/tests/client-post.test.mjs new file mode 100644 index 0000000..63888d9 --- /dev/null +++ b/tests/client-post.test.mjs @@ -0,0 +1,25 @@ +/* Copyright (C) 2026 The Cacti Group. GPL version 2 or later. */ +import assert from 'node:assert/strict'; +import { readFileSync } from 'node:fs'; +import { runInNewContext } from 'node:vm'; +import test from 'node:test'; + +test('adding a graph sends a POST body and preserves the success notification', () => { + let request; + let notifications = 0; + const context = { + urlPath: '/cacti/', + $: Object.assign(() => ({}), { ajax: (options) => { request = options; } }), + displayMessages: () => { notifications++; }, + }; + runInNewContext(readFileSync(new URL('../js/quicktree.js', import.meta.url), 'utf8'), context); + context.addQuickTree(42, 7); + assert.equal(request.type, 'POST'); + assert.equal(request.url, '/cacti/plugins/quicktree/quicktree.php'); + assert.equal(request.dataType, 'json'); + assert.deepEqual(JSON.parse(JSON.stringify(request.data)), {action: 'add', rra_id: 7, graph_id: 42}); + request.success({title: 'Added', message: 'Queued'}); + assert.equal(context.sessionMessageTitle, 'Added'); + assert.equal(context.sessionMessage.message, 'Queued'); + assert.equal(notifications, 1); +}); diff --git a/tests/csrf-actions.php b/tests/csrf-actions.php new file mode 100644 index 0000000..5e0dbbb --- /dev/null +++ b/tests/csrf-actions.php @@ -0,0 +1,97 @@ + $method, 'token' => $token, 'action' => $route[0], 'drp_action' => $route[1]], true); + $script = ' 'add_tree', 2 => 'add_branch', 3 => 'clear'); +$mutated = false; +$core_guard_calls = 0; +function csrf_check($fatal) { return $GLOBALS['seed']['token'] === 'valid'; } +register_shutdown_function(function () { echo json_encode(array('status' => http_response_code(), 'mutated' => $GLOBALS['mutated'], 'core_guard_calls' => $GLOBALS['core_guard_calls'])); }); +PHP; + $script .= "\nrequire " . var_export(__DIR__ . '/../includes/security.php', true) . ';'; + + if ($host !== 'legacy') { + $script .= <<<'PHP' + +function csrf_require_post() { + $GLOBALS['core_guard_calls']++; + if ($_SERVER['REQUEST_METHOD'] !== 'POST') { + header('Allow: POST'); + http_response_code(405); + exit; + } +PHP; + + if ($host === 'method-and-token') { + $script .= 'if (!csrf_check(false)) { http_response_code(403); exit; }'; + } + $script .= "\n}\n"; + } + $script .= "\n" . $dispatch_guard . "\n\$mutated = true;"; + $process = proc_open([PHP_BINARY], [0 => ['pipe', 'r'], 1 => ['pipe', 'w'], 2 => ['pipe', 'w']], $pipes); + fwrite($pipes[0], $script); + fclose($pipes[0]); + $result = json_decode(stream_get_contents($pipes[1]), true); + $stderr = stream_get_contents($pipes[2]); + fclose($pipes[1]); + fclose($pipes[2]); + + $uses_core_guard = $host !== 'legacy' && (in_array($route[0], ['add', 'clear', 'save'], true) || $route[1] === 3); + + if (proc_close($process) !== 0 || $stderr !== '' || $result['status'] !== $expected || $result['mutated'] !== ($expected === 200) || $result['core_guard_calls'] !== (int) $uses_core_guard) { + throw new RuntimeException('QuickTree mutation guard regression: ' . $stderr); + } + + $count++; + } + } +} +print "$count mutation guard cases passed\n";