[somethingwithproof]

Title

Fix permission, CSRF and chart-label handling and several correctness bugs found in a code review

Description

This PR contains the fixes from a read-through of develop at 13159fc, plus end-to-end tests that exercise them against a running Cacti. Some of the fixes are security-relevant and are called out below. Each fix is its own commit with its own regression test.

Security-relevant fixes:

• Panel permissions were only applied when the dashboard rendered. The reload, detail and add-panel requests did not check them, so a user with the Intropage Viewer realm could reload or open the details of a system panel an administrator had not granted, and could add one to a dashboard. All three now check the permission.
• Requests that change data (add, remove and resize a panel, save settings, share a dashboard, change the login option, favorite a graph) were accepted as GET, which Cacti's CSRF token does not cover. They are now only honoured as POST. The dashboard script and the graph page button post instead of navigating.
• Pie and treemap chart labels (device template names) were pasted into an inline script. A template name containing a quote could break out of the string. The columns are now encoded as JSON.

Correctness fixes:

• The poller threw a TypeError, and Cacti then disabled the plugin, when a system panel definition had no data row yet (for example after Reload Panel Definitions added a new panel). Rows without an owner now run as user 0, which also stops a system panel inheriting the user of the row processed before it.
• Deleting a user group ran DELETE ... WHERE id = ? on a table keyed by user_group_id.
• The upgrade from 3.0.0 or older failed on PHP 8 while converting per-panel permissions.
• Saving the dashboard settings form used REPLACE, which reset the shared flag, and could create dashboards the user did not have. It now updates the name of an existing dashboard.
• The console_after hook only checked the viewer realm on the main poller because of missing parentheses.
• When a panel was withdrawn from a user, the cleanup compared the row id with the panel name column and never deleted the user's data row.
• intropage_get_allowed_devices() clears the user's hide_disabled setting for the lookup and now restores it in a finally block.
• The threshold event queries joined user_auth_perms without a user condition, so each log row was repeated once per user holding a matching permission.
• The details request called get_panel() before checking anything; for an unknown id that logged a PHP warning into the response and tried to create a row. The row is now looked up first.

Not changed: intropage_add_panel() and intropage_remove_panel() in setup.php cannot work against the current schema. intropage_add_panel() writes file, has_detail and refresh_interval, which plugin_intropage_panel_definition no longer has, and intropage_remove_panel() uses DROP ?, which cannot bind an identifier. Nothing in the repository calls them and the panel library README describes register_<name>() as the way to add panels. Removing a public function is a decision for the maintainers. Options: leave as is, deprecate with a log message, or remove in a later release.

Found while writing the end-to-end tests and left alone, because they are outside the lines this PR changes:

• favgraph sets the priority of a new favourite to MAX(priority) + 1 for the user. For a user with no panel rows that is NULL, the insert fails, and the dashboard link is then written with panel id 0. The tests seed a panel first.
• When the poller processes a panel row whose user no longer exists, Cacti logs AUTH ERROR: Invalid Cacti User ID ... is being used in a permission that does not exist on every cycle. Rows of deleted users are only removed by the user_remove hook.
• On Cacti develop, sql_save() logs DBCALL ERROR: Column: height contains an invalid value: normal for this plugin's panel rows. It also happens without this PR.

Related Issue

No tracking issue. These come from a code review of develop. Issue #366 is a broader automated audit and is not closed by this PR.

Motivation and Context

The permission check and the POST requirement close gaps that were reachable by any authenticated user with the viewer realm. The poller fix matters because a TypeError in the poller makes Cacti disable the plugin; this was reproduced on a live Cacti. The end-to-end tests exist because most of these paths (display.php, poller_intropage.php, the hooks, the upgrade) cannot be loaded by the stub-based unit suite.

How Has This Been Tested?

Unit and regression tests (Pest, the repository's stub bootstrap): Tests: 128 passed, 103 skipped, 1 warning without the end-to-end environment (the skips are the end-to-end tests and one test that was already skipped; the warning is an rmdir notice that the baseline run also has). tests/bin/patch-coverage.php against origin/develop reports 100.00 percent (89/89 changed lines); display.php and poller_intropage.php are on its existing allowlist, which is why they also have end-to-end tests. check-i18n-pot.php, validate-manifest.php and php -l on every PHP file pass.

End-to-end tests (tests/Integration/IntropageHttpTest.php, 102 tests): real login, real CSRF tokens, real requests to intropage.php and user_group_admin.php, the real poller script, the real graph_buttons hook through Cacti's plugin API, the real upgrade code against the Cacti database, and the real database. They run when INTROPAGE_E2E_HOST and INTROPAGE_E2E_DB_NAME are set (plus the other INTROPAGE_E2E_* variables described at the top of the file) and are skipped otherwise. The workflow now sets them, makes cacti/log writable for the runner, and serves the installed Cacti with PHP's built-in server as the web user before the Pest step, so they run in CI on every job. Paths covered:

• reload, details and add: a user with the panel, a user without it, a panel the user lost, another user's row, an unknown row, a non-numeric id, the exempt admin alert panel, an unauthenticated client, a missing CSRF token, a GET instead of POST
• 13 data-changing actions, each with GET (no change), POST (changes), POST without a token, and an unauthenticated POST
• dashboard names with quotes, markup, unicode and 30 characters (stored and escaped on output), invalid ids, a list instead of a string, another user's dashboard, and add, share, copy and remove
• withdrawn panels: link and row removed, still-permitted panels and system rows kept
• device template names that try to break out of the chart script, including a name cut inside a multibyte character
• the dashboard in the console for users with and without the viewer realm, and a user with restricted devices and hide_disabled
• the poller: system panels without data rows, a user panel, a deleted user, and a panel that is not due
• group removal by a user management user and by a user without that realm
• favourite graphs stored and rendered, and the graph button hook for a granted user, an existing favourite, a custom timespan and a user without the permission
• threshold events with three users holding permissions, run on the real database (it creates and removes the two Thold tables only when they do not exist)
• the upgrade from a 3.0.0 schema, run from a throwaway copy of the plugin tree so the real tree is never pruned

Results per Cacti core, PHP 8.3, MariaDB 11.8.9 in Docker, full Pest suite with the end-to-end environment (the order the workflow uses: install the plugin with plugin_manage, serve Cacti, run Pest, run poller.php --force, scan cacti.log):

Core	Full suite	Patch coverage	Poller run	FATAL:|ERROR:|WARNING: in cacti.log
1.2.x (1.2.32, as the workflow clones it)	230 passed, 1 skipped, 1 warning	100.00 percent	exit 0, SYSTEM STATS and INTROPAGE STATS logged	none
develop (1.3.0 dev)	230 passed, 1 skipped, 1 warning	100.00 percent	exit 0, SYSTEM STATS and INTROPAGE STATS logged	none after the tests; the tests restore cacti.log and fail on unexpected lines

Limits of that check: the Docker image has PHP 8.3 only (the workflow matrix is 8.2 to 8.4), the installer step was replaced by loading cacti.sql and setting the version because rrdtool and fping are not in the image, and the tests ran as root instead of the runner user (the log permission step is untested). Pest on 1.2.x used develop's include/vendor copied over the 1.2.x vendor directory instead of the workflow's composer install.

The same end-to-end file run against the unfixed origin/develop code: 53 failed, 49 passed on both cores. The 49 that pass are the Cacti core guarantees (CSRF, login) and the paths that must keep working. A few of the 53 are knock-on failures after the poller TypeError makes Cacti disable the plugin.

Traceability: tests/bin/e2e-coverage-report.php takes Xdebug dumps written by tests/Integration/coverage-prepend.php while the real entry points run (one directory per test) and the PHPUnit XML coverage of the unit tests, and lists for every added or modified executable line the tests that run it. For this branch every changed line is run by at least one unit or end-to-end test. Whole-file coverage of the changed files, line and Xdebug branch coverage, end-to-end run on develop:

File	Unit before	Unit after	End-to-end lines	End-to-end branches hit
display.php	not measured	not measured	240/271 (88.6%)	192/276 (69.6%)
includes/database.php	122/223 (54.7%)	210/223 (94.2%)	90/207 (43.5%)	56/77 (72.7%)
includes/functions.php	108/1184 (9.1%)	427/1243 (34.4%)	711/1137 (62.5%)	493/767 (64.3%)
includes/settings.php	0/282 (0.0%)	5/282 (1.8%)	27/266 (10.2%)	15/132 (11.4%)
panellib/thold.php	98/342 (28.7%)	154/344 (44.8%)	31/312 (9.9%)	2/174 (1.1%)
poller_intropage.php	not measured	not measured	82/143 (57.3%)	44/85 (51.8%)
setup.php	180/193 (93.3%)	180/193 (93.3%)	89/209 (42.6%)	68/151 (45.0%)

Changed-line gaps in the end-to-end run, covered by unit tests instead: the Thold event queries (the Thold plugin is not installed; the real-database test runs the panel's own SQL in the Pest process, not through the web server), the non-numeric branch of intropage_chart_column(), and the remote poller branches of the console_after condition (the stack runs as poller 1). Not tested: the JavaScript in includes/intropage.js (no JavaScript harness in the repository; the end-to-end tests cover the server side of the same requests).

Rules for the test helpers: nothing uses eval, a weak hash or an http:// literal, and no test builds SQL from strings (passwords use password_hash, template hashes use random bytes, the address comes from INTROPAGE_E2E_SCHEME and INTROPAGE_E2E_HOST). tests/ ships inside the plugin directory on a git clone, so every runnable helper there (the end-to-end file, coverage-prepend.php, upgrade-runner.php, hook-runner.php, the report script) exits with 404, or silently under the CLI, unless started by the test process with its environment, takes every path from environment variables, writes only below the directory named there under a validated name, and never includes a path from a request. tests/Security/TestFilesInertTest.php runs each of them without the environment and expects no output and no files. The upgrade runner refuses any tree that is not a throwaway copy below the system temp directory.

Performance

Fixture: empty Cacti develop install, no devices, one user, one dashboard with one panel. Cacti and MariaDB on one Docker network; PHP's built-in server. Times are noisy at this size and are not a claim of speedup or slowdown.

Statements per request, counted with the MariaDB general log for one panel reload (origin/develop, then this branch): 131, then 133 (131 plus a group-membership query and a permission query). Com_select/insert/update/delete/replace deltas over 25 requests, three alternating rounds:

Request	Statements before	Statements after
Panel reload (allowed panel)	117	119
Dashboard page, one panel	315	315
Add panel (POST)	194	194
Save settings (POST)	248	248

The two extra statements on reload come from the new permission check, and the same cost applies to the detail request (which now also reads the row once before get_panel()). Add panel has the same count because the permission result is cached for the rest of the request and the dashboard render that follows reuses it.

Median wall time per request over the same rounds stayed within run-to-run noise (reload 6.9 to 9.0 ms before, 6.9 to 7.2 ms after; dashboard page 22.1 to 27.9 ms before, 22.6 to 24.6 ms after). Peak memory and server CPU per request were not measured.

Chart columns: building 20 columns with json_encode costs 3.2 microseconds instead of 1.6 (median of 7 runs of 20000, PHP 8.4, CPU time), with the same peak memory. This runs once per chart refresh.

Not measured: poller duration with many users and panels (the change is one integer cast per row), the threshold event query plan (fewer rows are joined, but no Thold data was available), the end-to-end suite's time in CI (20 seconds locally), and Xdebug cachegrind.

Rollback

Revert the merge commit. No schema change and no data migration. The permission and POST checks change behaviour for anyone relying on GET links to dashboard actions or on reloading panels they were not granted; both were unintended. The favorite-graph button on Cacti's graph pages now posts from a small inline script (CSP nonce applied when Cacti provides one). The workflow change can be reverted on its own: it adds environment variables, a step that makes cacti/log writable and a step that serves Cacti on 127.0.0.1:8080.

Types of changes

• Bug fix (non-breaking change which fixes an issue)
• New feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change): GET requests that change data are no longer honoured

Checklist

• My code follows the code style of this project.
• My change requires a change to the documentation.
• I have updated the documentation repository accordingly.

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.1% Duplication on New Code

See analysis details on SonarQube Cloud