From 137b60b6ad0b4dc6856b41f7d430fe5bec17d686 Mon Sep 17 00:00:00 2001
From: Thomas Vincent <thomasvincent@gmail.com>
Date: Sat, 10 Oct 2026 02:29:04 -0700
Subject: [PATCH 1/2] security: Authorize existing target trees before
 QuickTree writes (#41)

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
---
 CHANGELOG.md                                  |  1 +
 quicktree.php                                 | 16 ++++
 setup.php                                     | 28 ++++++
 tests/Unit/QuicktreeTreeAuthorizationTest.php | 91 +++++++++++++++++++
 tests/save-authorization.php                  | 54 +++++++++++
 5 files changed, 190 insertions(+)
 create mode 100644 tests/Unit/QuicktreeTreeAuthorizationTest.php
 create mode 100644 tests/save-authorization.php

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c65bf20..01997ad 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,7 @@
 * 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)
 * dev: Enforce patch coverage of changed lines in CI and remove the inert COMPOSER_ROOT_VERSION env from the Pest step
 * security: Add a version-safe CSP nonce (`plugin_quicktree_csp_nonce()`) to every inline `<script>` tag so pages stay compatible with Cacti's Content-Security-Policy nonce enforcement, while falling back cleanly on older Cacti releases that lack the `CactiSecureHeaders` class
+* security: Authorize the target tree before QuickTree adds items to an existing tree, and hide trees the user cannot edit from the tree selector
 - issue: Quicktree continually registers a realm
 - issue: Fix quicktree_show_tab() building an invalid `?location=tabtab`/`?location=tabconsole` URL, an undefined-variable text-domain typo in the QuickTree page's "no graph specified" message, and a misspelled `qucktree` text domain
 - security: Migrate quicktree SQL helpers to prepared statements, normalize location handling, and ensure redirects terminate execution
diff --git a/quicktree.php b/quicktree.php
index e9b40a0..ad4de17 100644
--- a/quicktree.php
+++ b/quicktree.php
@@ -29,6 +29,7 @@
 include_once('include/auth.php');
 include_once('plugins/quicktree/includes/helpers.php');
 include_once('plugins/quicktree/includes/security.php');
+include_once('plugins/quicktree/setup.php');
 
 /** @var array<string,mixed> $config */
 define('QUICKTREE_BASE_URI', $config['url_path'] . 'plugins/quicktree/');
@@ -162,6 +163,10 @@
 
 		if (cacti_sizeof($queryrows)) {
 			foreach ($queryrows as $tr) {
+				if (!quicktree_can_edit_tree((int) $tr['id'])) {
+					continue;
+				}
+
 				printf('<option value="%d">%s</option>', $tr['id'], html_escape($tr['name']));
 			}
 		}
@@ -219,6 +224,17 @@
 			$tree_name = get_nfilter_request_var('tree');
 		}
 
+		// Recheck submitted IDs AND names; filtering the dropdown is not authorization.
+		if ($new_tree_id > 0 && !quicktree_can_edit_tree((int) $new_tree_id)) {
+			http_response_code(403);
+			exit;
+		}
+
+		if (isset_request_var('tree_id') && ($new_tree_id <= 0 || empty($tree_name))) {
+			http_response_code(403);
+			exit;
+		}
+
 		if (empty($tree_name)) {
 			$tree_name = 'New Tree';
 		}
diff --git a/setup.php b/setup.php
index 9d0049f..bd92939 100644
--- a/setup.php
+++ b/setup.php
@@ -39,6 +39,34 @@ function plugin_quicktree_csp_nonce(): string {
 	return '';
 }
 
+/**
+ * Authorize an existing tree before QuickTree writes any items to it.
+ *
+ * @param int $tree_id Existing tree ID.
+ *
+ * @return bool Whether the current user may edit this tree.
+ */
+function quicktree_can_edit_tree(int $tree_id): bool {
+	$user_id = isset($_SESSION['sess_user_id']) ? (int) $_SESSION['sess_user_id'] : 0;
+
+	if ($user_id <= 0 || $tree_id <= 0 || !is_tree_allowed($tree_id)) {
+		return false;
+	}
+
+	if (function_exists('cacti_authorize_resource')) {
+		return cacti_authorize_resource($user_id, $tree_id, 'graph_tree');
+	}
+
+	// Older hosts lack the ownership helper. Preserve its admin/owner policy.
+	if (api_user_realm_auth('user_admin.php')) {
+		return true;
+	}
+
+	$owner = db_fetch_cell_prepared('SELECT user_id FROM graph_tree WHERE id = ?', array($tree_id));
+
+	return $owner !== false && $owner !== null && (int) $owner === $user_id;
+}
+
 /**
  * Installs the quicktree plugin: registers its Cacti hooks (top_header_tabs,
  * top_graph_header_tabs, config_arrays, config_settings,
diff --git a/tests/Unit/QuicktreeTreeAuthorizationTest.php b/tests/Unit/QuicktreeTreeAuthorizationTest.php
new file mode 100644
index 0000000..b1e798b
--- /dev/null
+++ b/tests/Unit/QuicktreeTreeAuthorizationTest.php
@@ -0,0 +1,91 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group                                 |
+ +-------------------------------------------------------------------------+
+*/
+
+/*
+ * quicktree_can_edit_tree() gates every write into an existing graph tree.
+ * The legacy cases run first because the host ownership helper, once
+ * defined, cannot be removed from the process.
+ */
+
+beforeAll(function () {
+	require_once __DIR__ . '/../../setup.php';
+
+	if (!function_exists('is_tree_allowed')) {
+		function is_tree_allowed($tree_id) {
+			return $GLOBALS['__test_tree_allowed'];
+		}
+	}
+});
+
+beforeEach(function () {
+	$GLOBALS['__test_tree_allowed'] = true;
+	$GLOBALS['__test_realm_auth']   = false;
+	quicktree_test_set_db_fetch_cell_prepared_return(1);
+	$_SESSION['sess_user_id'] = 1;
+});
+
+it('allows the tree owner', function () {
+	expect(quicktree_can_edit_tree(5))->toBeTrue();
+});
+
+it('denies a user who does not own the tree', function () {
+	quicktree_test_set_db_fetch_cell_prepared_return(2);
+
+	expect(quicktree_can_edit_tree(5))->toBeFalse();
+});
+
+it('allows an administrator on a tree owned by someone else', function () {
+	quicktree_test_set_db_fetch_cell_prepared_return(2);
+	$GLOBALS['__test_realm_auth'] = true;
+
+	expect(quicktree_can_edit_tree(5))->toBeTrue();
+});
+
+it('denies a tree the user cannot view even when they are an administrator', function () {
+	$GLOBALS['__test_tree_allowed'] = false;
+	$GLOBALS['__test_realm_auth']   = true;
+
+	expect(quicktree_can_edit_tree(5))->toBeFalse();
+});
+
+it('denies an unauthenticated session', function () {
+	$_SESSION['sess_user_id'] = 0;
+	$GLOBALS['__test_realm_auth'] = true;
+
+	expect(quicktree_can_edit_tree(5))->toBeFalse();
+});
+
+it('denies a missing tree id', function () {
+	$GLOBALS['__test_realm_auth'] = true;
+
+	expect(quicktree_can_edit_tree(0))->toBeFalse();
+});
+
+it('denies a tree with no owner row', function () {
+	quicktree_test_set_db_fetch_cell_prepared_return(false);
+
+	expect(quicktree_can_edit_tree(5))->toBeFalse();
+});
+
+it('defers to the host ownership helper when it exists', function () {
+	if (!function_exists('cacti_authorize_resource')) {
+		function cacti_authorize_resource($user_id, $tree_id, $type) {
+			return $GLOBALS['__test_resource_allowed'];
+		}
+	}
+
+	$GLOBALS['__test_resource_allowed'] = false;
+	expect(quicktree_can_edit_tree(5))->toBeFalse();
+
+	$GLOBALS['__test_resource_allowed'] = true;
+	expect(quicktree_can_edit_tree(5))->toBeTrue();
+});
+
+afterEach(function () {
+	quicktree_test_set_db_fetch_cell_prepared_return('');
+	$GLOBALS['__test_realm_auth'] = true;
+});
diff --git a/tests/save-authorization.php b/tests/save-authorization.php
new file mode 100644
index 0000000..c5e736e
--- /dev/null
+++ b/tests/save-authorization.php
@@ -0,0 +1,54 @@
+<?php
+/*
+ +-------------------------------------------------------------------------+
+ | Copyright (C) 2004-2026 The Cacti Group                                 |
+ +-------------------------------------------------------------------------+
+*/
+
+/*
+ * Runs the 'save' branch up to the first tree write with the tree lookup
+ * stubbed, for both the tree_id and the tree-name request paths.
+ */
+
+$source = file_get_contents(__DIR__ . '/../quicktree.php');
+$start  = strpos($source, "case 'save':");
+$end    = strpos($source, "if (empty(\$tree_name)) {", $start);
+$save   = substr($source, $start + strlen("case 'save':"), $end - $start - strlen("case 'save':"));
+
+$cases = array(
+	array('tree_id', true, 403),
+	array('tree_id', false, 200),
+	array('tree', true, 403),
+	array('tree', false, 200),
+);
+
+foreach ($cases as $case) {
+	list($path, $denied, $expected) = $case;
+	$seed   = var_export(array('path' => $path, 'denied' => $denied), true);
+	$script = '<?php $seed = ' . $seed . ';' . <<<'PHP'
+
+http_response_code(200);
+$user = 1;
+$mutated = false;
+function isset_request_var($name) { return $name === $GLOBALS['seed']['path']; }
+function get_filter_request_var($name) { return 7; }
+function get_nfilter_request_var($name) { return 'Shared'; }
+function db_fetch_cell_prepared($sql, $params = array()) {
+	return stripos($sql, 'SELECT id') !== false ? 7 : 'Shared';
+}
+function quicktree_can_edit_tree($id) { return !$GLOBALS['seed']['denied'] && $id === 7; }
+register_shutdown_function(function () { echo json_encode(array('status' => http_response_code(), 'mutated' => $GLOBALS['mutated'])); });
+PHP;
+	$script .= "\n" . $save . "\n\$mutated = true;";
+	$process = proc_open(array(PHP_BINARY), array(0 => array('pipe', 'r'), 1 => array('pipe', 'w'), 2 => array('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]);
+	if (proc_close($process) !== 0 || $stderr !== '' || $result['status'] !== $expected || $result['mutated'] !== ($expected === 200)) {
+		throw new RuntimeException('QuickTree save authorization regression: ' . $path . ' ' . $stderr);
+	}
+}
+echo "4 save authorization cases passed\n";

From 8b1878cfd85019acce271e68c4df3e25dee18d14 Mon Sep 17 00:00:00 2001
From: Thomas Vincent <thomasvincent@gmail.com>
Date: Sun, 18 Oct 2026 14:26:03 -0700
Subject: [PATCH 2/2] refactor: move quicktree_can_edit_tree into
 includes/security.php

Signed-off-by: Thomas Vincent <thomasvincent@gmail.com>
---
 includes/security.php                         | 28 +++++++++++++++++++
 quicktree.php                                 |  1 -
 setup.php                                     | 28 -------------------
 tests/Unit/QuicktreeTreeAuthorizationTest.php |  2 +-
 4 files changed, 29 insertions(+), 30 deletions(-)

diff --git a/includes/security.php b/includes/security.php
index 011aa36..97c51ae 100644
--- a/includes/security.php
+++ b/includes/security.php
@@ -40,3 +40,31 @@ function quicktree_normalize_location($value): string {
 
 	return 'tab';
 }
+
+/**
+ * Authorize an existing tree before QuickTree writes any items to it.
+ *
+ * @param int $tree_id Existing tree ID.
+ *
+ * @return bool Whether the current user may edit this tree.
+ */
+function quicktree_can_edit_tree(int $tree_id): bool {
+	$user_id = isset($_SESSION['sess_user_id']) ? (int) $_SESSION['sess_user_id'] : 0;
+
+	if ($user_id <= 0 || $tree_id <= 0 || !is_tree_allowed($tree_id)) {
+		return false;
+	}
+
+	if (function_exists('cacti_authorize_resource')) {
+		return cacti_authorize_resource($user_id, $tree_id, 'graph_tree');
+	}
+
+	// Older hosts lack the ownership helper. Preserve its admin/owner policy.
+	if (api_user_realm_auth('user_admin.php')) {
+		return true;
+	}
+
+	$owner = db_fetch_cell_prepared('SELECT user_id FROM graph_tree WHERE id = ?', array($tree_id));
+
+	return $owner !== false && $owner !== null && (int) $owner === $user_id;
+}
diff --git a/quicktree.php b/quicktree.php
index ad4de17..806d0d2 100644
--- a/quicktree.php
+++ b/quicktree.php
@@ -29,7 +29,6 @@
 include_once('include/auth.php');
 include_once('plugins/quicktree/includes/helpers.php');
 include_once('plugins/quicktree/includes/security.php');
-include_once('plugins/quicktree/setup.php');
 
 /** @var array<string,mixed> $config */
 define('QUICKTREE_BASE_URI', $config['url_path'] . 'plugins/quicktree/');
diff --git a/setup.php b/setup.php
index bd92939..9d0049f 100644
--- a/setup.php
+++ b/setup.php
@@ -39,34 +39,6 @@ function plugin_quicktree_csp_nonce(): string {
 	return '';
 }
 
-/**
- * Authorize an existing tree before QuickTree writes any items to it.
- *
- * @param int $tree_id Existing tree ID.
- *
- * @return bool Whether the current user may edit this tree.
- */
-function quicktree_can_edit_tree(int $tree_id): bool {
-	$user_id = isset($_SESSION['sess_user_id']) ? (int) $_SESSION['sess_user_id'] : 0;
-
-	if ($user_id <= 0 || $tree_id <= 0 || !is_tree_allowed($tree_id)) {
-		return false;
-	}
-
-	if (function_exists('cacti_authorize_resource')) {
-		return cacti_authorize_resource($user_id, $tree_id, 'graph_tree');
-	}
-
-	// Older hosts lack the ownership helper. Preserve its admin/owner policy.
-	if (api_user_realm_auth('user_admin.php')) {
-		return true;
-	}
-
-	$owner = db_fetch_cell_prepared('SELECT user_id FROM graph_tree WHERE id = ?', array($tree_id));
-
-	return $owner !== false && $owner !== null && (int) $owner === $user_id;
-}
-
 /**
  * Installs the quicktree plugin: registers its Cacti hooks (top_header_tabs,
  * top_graph_header_tabs, config_arrays, config_settings,
diff --git a/tests/Unit/QuicktreeTreeAuthorizationTest.php b/tests/Unit/QuicktreeTreeAuthorizationTest.php
index b1e798b..3c6d816 100644
--- a/tests/Unit/QuicktreeTreeAuthorizationTest.php
+++ b/tests/Unit/QuicktreeTreeAuthorizationTest.php
@@ -12,7 +12,7 @@
  */
 
 beforeAll(function () {
-	require_once __DIR__ . '/../../setup.php';
+	require_once __DIR__ . '/../../includes/security.php';
 
 	if (!function_exists('is_tree_allowed')) {
 		function is_tree_allowed($tree_id) {