From c749790d84efe21b0df1d5ddeaa19845c4c89741 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Tue, 13 Oct 2026 04:52:20 -0700 Subject: [PATCH] fix(security): [develop] require CSRF-protected POST for link and plugin reordering (#8278) * security: require CSRF-protected POST for link and plugin reordering (#7454) Signed-off-by: Thomas Vincent * fix(csrf): share POST and token guards for reorder actions Signed-off-by: Thomas Vincent --------- Signed-off-by: Thomas Vincent --- include/csrf.php | 9 ++- links.php | 5 +- plugins.php | 5 +- .../Csrf/ReorderPostAuthorizationTest.php | 59 +++++++++++++++++++ .../DataSourcePathContainmentTest.php | 2 - tests/js/csrf_post_actions.test.js | 19 ++++++ 6 files changed, 93 insertions(+), 6 deletions(-) create mode 100644 tests/Unit/Security/Csrf/ReorderPostAuthorizationTest.php diff --git a/include/csrf.php b/include/csrf.php index 19ffc8af4..5e6493099 100644 --- a/include/csrf.php +++ b/include/csrf.php @@ -445,11 +445,11 @@ function csrf_error_callback() : void { } /** - * csrf_require_post - reject a non-POST request for a state-changing action. + * csrf_require_post - require a CSRF-protected POST for a state-changing action. * * State-changing actions reached through a direct action= URL must arrive by * POST so they cannot be triggered cross-site with a GET (for example an - * tag). csrf-magic validates the token on the resulting POST. + * tag). Reject invalid POST tokens before the action can change state. * * @return void */ @@ -459,4 +459,9 @@ function csrf_require_post() : void { http_response_code(405); exit; } + + if (!csrf_check(false)) { + http_response_code(403); + exit; + } } diff --git a/links.php b/links.php index 860728c4c..7b18da5ac 100644 --- a/links.php +++ b/links.php @@ -68,6 +68,8 @@ switch (grv('action')) { break; case 'ajax_dnd': + csrf_require_post(); + $new_order = gnrv('dnd'); links_reorder($new_order); @@ -399,7 +401,8 @@ function pages() : void { $('#dnd').find('tr:first').addClass('nodrag').addClass('nodrop'); $('#dnd').tableDnD({ onDrop: function(table, row) { - loadUrl({url:'links.php?action=ajax_dnd&'+$.tableDnD.serialize()}) + const request = cactiPreparePostRequest('links.php', 'action=ajax_dnd&' + $.tableDnD.serialize()); + postUrl({url: request.url}, request.data); } }); }); diff --git a/plugins.php b/plugins.php index 2115163fc..fff2e97a8 100644 --- a/plugins.php +++ b/plugins.php @@ -408,6 +408,8 @@ switch($action) { break; case 'ajax_dnd': + csrf_require_post(); + $new_order = gnrv('dnd'); api_plugin_reorder($new_order); @@ -888,7 +890,8 @@ function update_show_current() : void { // console.log(row); }, onDrop: function(table, row) { - loadUrl({url:'plugins.php?action=ajax_dnd&'+$.tableDnD.serialize()}) + const request = cactiPreparePostRequest('plugins.php', 'action=ajax_dnd&' + $.tableDnD.serialize()); + postUrl({url: request.url}, request.data); } }); } diff --git a/tests/Unit/Security/Csrf/ReorderPostAuthorizationTest.php b/tests/Unit/Security/Csrf/ReorderPostAuthorizationTest.php new file mode 100644 index 000000000..d7c617469 --- /dev/null +++ b/tests/Unit/Security/Csrf/ReorderPostAuthorizationTest.php @@ -0,0 +1,59 @@ +createForNewestSupportedVersion(); + $finder = new PhpParser\NodeFinder(); + $printer = new PhpParser\PrettyPrinter\Standard(); + $helper = $finder->findFirst($parser->parse(file_get_contents($root . '/include/csrf.php')), + static fn ($node) => $node instanceof PhpParser\Node\Stmt\Function_ && $node->name->toString() === 'csrf_require_post'); + expect($helper)->not->toBeNull(); + $guard = $printer->prettyPrint([$helper]); + + foreach (['links.php', 'plugins.php'] as $file) { + $nodes = $parser->parse(file_get_contents($root . '/' . $file)); + $case = $finder->findFirst($nodes, static fn ($node) => $node instanceof PhpParser\Node\Stmt\Case_ + && $node->cond instanceof PhpParser\Node\Scalar\String_ && $node->cond->value === 'ajax_dnd'); + expect($case)->not->toBeNull(); + $body = $printer->prettyPrint($case->stmts); + + foreach ([['GET', null, 405], ['GET', 'valid', 405], ['HEAD', 'valid', 405], ['PUT', 'valid', 405], [null, 'valid', 405], ['POST', null, 403], ['POST', 'invalid', 403], ['POST', 'valid', 302]] as $scenario) { + [$method, $token, $status] = $scenario; + $seed = var_export(['method' => $method, 'token' => $token], true); + $script = ' http_response_code(), 'writes' => $GLOBALS['writes'])); }); +function csrf_check($fatal) { return $GLOBALS['seed']['token'] === 'valid'; } +function gnrv($name) { return array(2, 1); } +function links_reorder($order) { $GLOBALS['writes']++; } +function api_plugin_reorder($order) { $GLOBALS['writes']++; } +PHP; + $script .= "\n" . $guard . "\nswitch ('ajax_dnd') { case 'ajax_dnd':\n" . $body . "\n}"; + $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]); + expect(proc_close($process))->toBe(0); + expect($stderr)->toBe(''); + expect($result)->toBe(['status' => $status, 'writes' => $status === 302 ? 1 : 0]); + } + } +}); + +test('reorder clients submit their action and serialized order through the CSRF POST helper', function () { + $root = dirname(__DIR__, 4); + + foreach (['links.php', 'plugins.php'] as $file) { + $source = file_get_contents($root . '/' . $file); + expect($source)->toContain("cactiPreparePostRequest('" . $file . "', 'action=ajax_dnd&' + $.tableDnD.serialize())"); + expect($source)->toContain('postUrl({url: request.url}, request.data)'); + expect($source)->not->toContain("loadUrl({url:'" . $file . '?action=ajax_dnd'); + } +}); diff --git a/tests/Unit/Security/PathTraversal/DataSourcePathContainmentTest.php b/tests/Unit/Security/PathTraversal/DataSourcePathContainmentTest.php index d513191e1..5e1b200e7 100644 --- a/tests/Unit/Security/PathTraversal/DataSourcePathContainmentTest.php +++ b/tests/Unit/Security/PathTraversal/DataSourcePathContainmentTest.php @@ -199,5 +199,3 @@ test('a dangling symlink below the RRA directory is rejected even for a not-yet- unlink($link); } }); - - diff --git a/tests/js/csrf_post_actions.test.js b/tests/js/csrf_post_actions.test.js index babbfd7cf..82ce31ad3 100644 --- a/tests/js/csrf_post_actions.test.js +++ b/tests/js/csrf_post_actions.test.js @@ -47,6 +47,25 @@ const context = { vm.runInNewContext(`${extractFunction(source, 'cactiPreparePostRequest')}\n${extractFunction(source, 'cactiPreparePostRequestFromUrl')}`, context); +for (const page of ['links.php', 'plugins.php']) { + test(`${page} drag-and-drop sends the serialized order and trusted CSRF token in a POST body`, () => { + const pageSource = fs.readFileSync(path.join(__dirname, '..', '..', page), 'utf8'); + const callback = pageSource.match(/const request = cactiPreparePostRequest\([^\n]+\);\s*postUrl\(\{url: request\.url\}, request\.data\);/); + assert.ok(callback, 'drag-and-drop must use the POST helper'); + let sent; + const callbackContext = {...context, + $: {...context.$, tableDnD: {serialize: () => 'dnd%5B%5D=2&dnd%5B%5D=1'}}, + postUrl: (options, data) => { sent = {options, data}; }, + }; + vm.runInNewContext(callback[0], callbackContext); + assert.equal(sent.options.url, `/cacti/${page}`); + const fields = new URLSearchParams(sent.data); + assert.equal(fields.get('action'), 'ajax_dnd'); + assert.deepEqual(fields.getAll('dnd[]'), ['2', '1']); + assert.equal(fields.get('__csrf_magic'), 'trusted-token'); + }); +} + test('state-changing URL fields move into a same-origin POST body with the trusted token', () => { const request = context.cactiPreparePostRequestFromUrl('/cacti/cdef.php?action=item_remove&id=7&__csrf_magic=attacker'); const fields = new URLSearchParams(request.data);