Harden the NPC controllers and fix review findings by somethingwithproof · Pull Request #54 · Cacti/plugin_npc

somethingwithproof · 2026-10-24T05:25:30Z

Description

Fixes the problems found in a review of the plugin: security hardening of the request handling and the host import, plus defects that stopped the pages from working at all. This PR contains security-relevant fixes. The security table below lists paths that any user with NPC access could reach.

Two further defects turned up while writing the end-to-end tests and are fixed here too: npc.php never loaded the base controller class (every page ended in a fatal error), and the listing queries used bound LIMIT values, which MariaDB rejects when Cacti binds them as strings.

The change is 41 commits. Each commit is one fix or one group of tests, with a CHANGELOG line where behaviour changed.

Related Issue

No open issue. Review findings are listed below as N-01 to N-22; N-23 and N-24 were found afterwards.

Motivation and Context

On a plain Cacti the NPC pages could not render: the front controller failed on a missing class, and the grids were empty because the JavaScript read a response shape the controllers do not send. The listing queries also failed on the database. Behind that, several actions were open to every NPC user and the host import built a shell command from request values.

Findings and fixes

Security-relevant:

ID	Fix
N-02	The host import quotes the description, address and template arguments of the `add_device.php` command and rejects a non-numeric template id.
N-04	The host import requires the Devices realm and creating a data input method requires the Data Input Methods realm.
N-05	Nagios commands, comment deletion, data input creation, host import, graph mapping and settings save are accepted only as POST, so Cacti's CSRF token check applies.
N-06	The frame accepts only known tab names; the raw value was written into an inline script block.
N-07	The host import address cache is a JSON file in the Cacti cache directory, written atomically with mode 0600. It was a PHP file in the system temp directory that was included on every call.
N-14	The command realm is checked before the command file settings, so a user without it no longer sees the configured path.
N-15	Only the listed request values are copied onto a controller, `p_config` is ignored, and `start` and `limit` are bounded (at most 1000 rows).
N-21	The front end escapes quotes in values placed inside HTML attributes.

Correctness and reliability:

ID	Fix
N-01, N-22	The front end reads both response shapes and shows the summary and process tables correctly.
N-03	Controller methods are called on instances (host import, selection lookup, comment deletion). Static calls raise an Error on PHP 8.
N-08	Host perfdata reads the newest check only. It returned the whole history and used the oldest row.
N-09	The command dialogs send the values Acknowledge, Add Comment, Schedule Downtime and Schedule Check need. Sticky, notify and persistent are options in the dialog; a downtime takes a duration in minutes and starts now. These defaults are a UI choice the maintainers may want to change.
N-10	A service is listed once; the hostgroup members are joined only when filtering by hostgroup.
N-11, N-12	The upgrade alters `npc_contacts` (it named `npc_contracts`) and the deleted-user settings cleanup reads the right column.
N-13	README describes the NPC Global Commands realm. The realm is checked by name, so no code change.
N-16	`searchFields` and `data` reach the controllers as JSON; the sanitizer removed their quotes.
N-17	The controller error response names the class and method.
N-18	Dispatcher actions that cannot work with one parameter array are removed: `getSetting`, `isMapped`, `mapHost`, `getSettings`, `checkHostExists`.
N-19	Creating a data input method looks up perfdata with the plain object id.
N-20	`perfdata.php` and `cli.php` skip entries such as `label=U` instead of printing warnings into the data input output.
N-23	`npc.php` loads the base controller class.
N-24	The listing queries write the `LIMIT` values as integers. Cacti passes bound values to PDO as strings and the server rejects a quoted `LIMIT`.

Also found while testing against a real Cacti and fixed here:

Change	Why
Unsaved date format, time format and Nagios URL settings are cast to strings in the frame.	`addslashes(null)` raised the PHP 8.1 deprecation on every page of a fresh install.
A missing `searchFields` value is treated as empty in the grid search.	`stripslashes(null)` raised the same deprecation.
An unknown Nagios command returns an empty definition.	It raised undefined index and foreach warnings before the "not a valid command" message.
A host import selection that is not a JSON object logs a fixed message.	The old message converted an array to a string.
Uninstall drops the device mapping column only when it exists.	A repeated uninstall logged a failed `ALTER TABLE`.
The hostgroup and unhandled request properties are declared on the base controller.	PHP 8.2 dynamic property deprecation.
`perfdata.php` and `cli.php` share one perfdata parser in the base controller.	Removes the duplicated loop.

The translation template is refreshed.

Risk and compatibility

Removed dispatcher actions and POST-only actions change the request surface. The bundled front end uses POST for commands, and none of the removed actions could run before. Out-of-tree scripts that call a state-changing action with GET will get a 405 with a JSON message.
limit is capped at 1000 rows per request.
The address cache moves to cache/npc_address_cache.json. The old /tmp/npc_address_cache.php is no longer read and can be deleted. If the cache directory is not writable the cache is rebuilt on each import call.
The host import and data input creation now need the Devices and Data Input Methods realms in addition to NPC. Users who relied on NPC access alone must be granted them.
No schema change, no INFO change, no new dependency, no new plugin requirement.

Rollback

Revert the merge commit. There is no data migration to undo. The only artefacts created at runtime are the JSON cache file, which can be deleted.

How Has This Been Tested?

Executed locally with PHP 8.4, Pest through a Docker image with Xdebug, MariaDB 11.8.9 in a throwaway container and Node 22 in Docker. The same CI-order simulation ran against two Cacti trees: a 1.2.x tree (1.2.32) and a develop tree (1.3.0). Each run installed the plugin with php cli/plugin_manage.php --plugin=npc --install --enable, linted every PHP file, ran Pest with coverage, ran the patch coverage gate, the translation template check, the manifest check, and the workflow's FATAL:|ERROR:|WARNING: grep over cacti.log.

Core ref	Install	`php -l`	Pest	Patch coverage	i18n and manifest	cacti.log grep
1.2.32 (1.2.x)	clean	clean	342 passed, 2 skipped, 989 assertions	100.00 percent (96 of 96)	pass	no matching line
develop (1.3.0)	clean	clean	342 passed, 2 skipped, 989 assertions	100.00 percent (96 of 96)	pass	none from the plugin; Cacti's own `--uninstall` step logs a `sess_user_id` warning, which the workflow does not run

The two skipped tests are a root-user permission check and the Node wrapper, because the PHP image has no Node. Node ran separately: 34 passed, js/npc.js at 100 percent line coverage and 87.6 percent branch coverage. Both cores used the Pest and PHPUnit versions of the develop tree's lock file; I could not run a fresh composer install for 1.2.x offline.

What the tests cover:

Unit and regression tests for every changed branch: success, rejection and boundary values. Datasets cover hostile values (quotes, separators, command substitution, line breaks) for the import, the Nagios command, the data input method and the tab name.
Real database: tests/Integration/NpcDatabaseTest.php runs the schema and the listing, perfdata, cleanup and upgrade queries against MariaDB through PDO with default settings, the way Cacti's database layer sends them. Against the old controllers it fails with the LIMIT syntax error.
Real web server and sessions: tests/E2E/RealServerTest.php starts PHP's built-in server on the Cacti tree and talks to it with real HTTP, real logins, Cacti's realm checks and CSRF check, and the Cacti database. Four users with different realms (NPC only, no NPC realm, NPC plus commands, NPC plus commands plus devices plus data input) cover: not logged in, wrong realm, wrong method (GET, PUT, DELETE), missing and invalid CSRF token, hostile values, every listing, page bounds, the host import (including the PHP 8 path that used to fatal), bulk comment deletion, data input creation, removed actions, and the command line scripts through Cacti's own bootstrap. The test cuts cacti.log back to its length from before the run and skips with a reason when the log is not writable.
Entry points as subprocesses: tests/E2E/FrontControllerTest.php and CliScriptsTest.php run the real npc.php, perfdata.php and cli.php with Cacti's own request helpers.
Front end: tests/js/npc.test.js runs js/npc.js under Node's test runner against a small jQuery and DOM stand-in, loaded with require (no vm or eval). tests/Unit/JsFrontEndTest.php runs it from Pest and skips when Node is missing; GitHub's Ubuntu runners have Node.
Each regression test was run against the pre-fix code and failed there.
Test helpers under tests/ do nothing when requested over the web or run without the test environment; a test runs each one and expects empty output. They use no eval, no http:// literal and no string-built SQL.
The Cacti function stubs in the test bootstrap use the real Cacti signatures.

Changed-line coverage, measured with Xdebug, merging the in-process Pest run with the line coverage that the subprocess and web server runs record (tests/E2E/support/coverage-prepend.php, active only when a test run sets NPC_COVERAGE_DIR):

File	Executable changed lines	Covered by in-process tests	Covered with subprocess and server runs
`controllers/cacti.php`	5	5	5
`controllers/comments.php`	6	6	6
`controllers/controller.php`	12	12	12
`controllers/downtime.php`	2	2	2
`controllers/hosts.php`	3	3	3
`controllers/layout.php`	8	8	8
`controllers/logentries.php`	2	2	2
`controllers/nagios.php`	3	3	3
`controllers/notifications.php`	2	2	2
`controllers/services.php`	7	7	7
`controllers/statehistory.php`	2	2	2
`controllers/sync.php`	37	37	37
`nagioscmd.php`	1	1	1
`setup.php`	6	6	6
`npc.php`	23	0	23
`perfdata.php`	2	0	2
`cli.php`	1	0	1
Total	122	96	122

npc.php, perfdata.php and cli.php run at the top level and cannot be loaded into the unit process, so they are allowlisted in the patch coverage gate and covered by the subprocess and server runs. The line-by-line list of changed lines and the tests that run each is in the traceability file kept with the review notes.

Not run locally, left to CI: PHP 8.2 and 8.3; a fresh composer install of the 1.2.x dependencies; the poller step; the PHPStan and coding standard steps (they run only for non-1.2.x Cacti).

Performance

Environment: MariaDB 11.8.9 and PHP 8.4 in Docker on one laptop, Xdebug loaded but coverage mode off. Fixture: 60 hosts, 300 services, 20 hostgroups with each host in 3 (180 memberships), and 100,000 stored checks for one host. Each figure is the median of 7 runs, each run in a fresh PHP process. "Before" is origin/develop. Before the change the listing queries fail under Cacti's default PDO settings (emulated prepares), so the "before" figures use native prepares, which accept the old LIMIT ?, ?. That is not how Cacti connects. After figures are given for both modes.

Path	Before	After
Service grid, first page (25 rows)	89 ms wall, 2 queries, row count 900, 9 distinct services on the page	16 ms native, 24 ms emulated, 2 queries, row count 300, 25 distinct services
Service grid, 1000-row page	164 ms wall, 19 ms CPU, 900 rows (300 distinct)	16 ms native, 4 ms emulated, 300 rows
Host perfdata, host with 100,000 checks	28 ms wall, 14 ms CPU, 58 MB peak, 100,000 rows returned	0.6 ms wall, 0.4 ms CPU, 2 MB peak, 1 row
Address cache hit, 5,000 devices	0.78 ms (include of PHP file)	0.40 ms (JSON decode)

The first-run wall times include a cold server cache, so read them as indicative; the row counts are exact. Query counts did not change.

Costs of the change:

The deleted-user cleanup now runs one extra SELECT per npc_settings row each time the NPC settings page loads (before, the loop never matched). With 1,000 users that is about 17 ms per page load in this setup.
The POST check, parameter allowlist and limit clamp add a few array lookups per request. I did not measure them separately.

Not measured: opcache effects on the cache include, the front controller as a whole, perfdata.php and cli.php throughput, and any browser rendering time. I did not use Xdebug cachegrind; none of the changed paths is complex enough to need it.

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)

The removal of dispatcher actions and the POST requirement narrow what callers can do; I list them under Risk rather than calling the PR breaking because none of those paths worked or was used by the bundled front end.