[somethingwithproof]

Description

Fixes thirteen defects found in a read-through of the plugin and by the new end-to-end tests, several of them security relevant: two SQL injection paths, one stored cross-site scripting path, a request-forgery path, and an unvalidated ORDER BY. Each fix is its own commit with a regression test. The remaining changes make the test suite able to run the plugin's SQL against MariaDB.

This pull request contains security-relevant fixes. Details are in the table below; none of them is exploitable without either a Host MIB login or control of a monitored device's SNMP replies.

Related Issue

Relates to #41 (defense-in-depth audit) and #39 (SQL hardening). It does not close either; #41 lists items that this change does not touch.

Motivation and Context

ID	Area	Problem	Fix
P-1	hmib.php, hmib_get_graph_url()	The Summary Processes view passes a stored process name (taken from SNMP) as the SNMP index and it was placed in the SQL text. A name containing a quote broke the query.	Bind the index.
P-2, F-5	hmib_types.php, rescan_types()	The stored sysObjectID and sysDescr patterns (editable, and seeded from device-reported sysDescr) were concatenated into an UPDATE. The OR branch was also not limited to unclassified devices, so typed devices were reassigned, and the RLIKE branch contradicted the form text that says regular expressions were removed.	Bind the patterns, group the condition under host_type = 0, drop RLIKE.
F-11	hmib_types.php, hmib.php	Type name, version, sysObjectID and sysDescrMatch were printed unescaped in the OS type list and the Summary tab. These fields are editable by the Host MIB Admin realm and are also filled from device data by the rescan.	html_escape() each value.
F-3	hmib.php, Summary	Sort column and direction from the session were concatenated into ORDER BY. The request filter strips some characters only.	Allowlist of sortable columns per table, direction limited to ASC or DESC.
F-6	hmib_types.php	?scan=true ran the rescan from a GET link, which Cacti's CSRF protection does not cover.	POST only, sent from the page with the CSRF token.
F-1	hmib.php	$host_id != '' with an int: on PHP 8, 0 != '' is true, so "no device" became gl.host_id=0 and the data-query graph links on the Summary tab disappeared.	Compare with != 0 (the existing -1 sentinel still restricts).
F-2	hmib.php	The Summary Processes page count query reused the filter fragment, which starts with AND (, but had no WHERE clause, so any filter gave a SQL syntax error.	Give the count query the same base WHERE as the row query.
F-9	hmib.php	The Devices tab processes link was action=running%action=running, which decodes to an unknown action and shows an empty page.	Correct the link.
F-4	hmib_types.php	The rescan's INSERT ... SELECT had no DISTINCT, no truncation and no empty filter. Two unclassified devices with the same sysDescr and sysObjectID violate the table key; long sysDescr exceeds the 100 character column under strict mode; empty values created a type that matches every device.	INSERT IGNORE ... SELECT DISTINCT, LEFT(..., 100), skip empty values, bound names.
F-13	hmib_types.php	The CSV export did not double embedded quotes and wrote formula-like values as-is.	Quote fields with a helper; prefix formula-like values with a single quote.
F-8	poller_graphs.php	The processes graph was guarded by the users template check.	Check the processes template.
F-12	poller_graphs.php	After creating the summary device the host id was not read again, so its graphs waited for the next automation run (days apart by default).	Read the id after add_device.php.
F-14	hmib.php	A Summary request that named only the device type table or only the process table printed PHP warnings, because the other table's stored sort was read from the session without a default. Found by the end-to-end tests.	Read the stored sort with the same defaults the table uses.
F-7	poller_hmib.php	The hrSWRun_last_seen.total_time upsert added the performance interval on every poller cycle, not once per performance collection.	Add the interval only when the performance collector ran.
H-1	poller_hmib.php, collectHostIndexedOid()	Each table collection marked the device's rows present=0 before walking and deleted the unmarked ones after. Core returns an empty array for a timed out walk, so one timeout deleted the device's processes, storage, hardware, software or processor rows.	Walk first and keep the stored rows when a walk fails. Core reports neither a failed nor a timed out walk (it returns an empty array and leaves $snmp_error empty), so an empty table is trusted only when the device then answers a sysUpTime GET, which core does report as failed; then the stale rows are cleared. Mark and delete with bound values.
H-2	poller_hmib.php, main run	The gate was a count of plugin_hmib_processes rows, purged after 600 s, so a run longer than that let the next one start beside it. The wait loop had no deadline, and the launch placeholder key was rand(), which can equal a real process id.	Hold GET_LOCK() for the main run (the server drops it with the connection; released again at shutdown). Stop launching and waiting 600 s after the run began. Placeholder keys start at 2^31. Seed and key are bound.
H-3	poller_hmib.php, autoDiscoverHosts()	The autodiscovery lock row was deleted only on the normal path, so a scan that died blocked later runs until the purge. The scan had no time limit, and $add = true; if ($add) was dead code.	Remove the row from a shutdown function; SIGTERM and SIGINT now exit through it. Stop the scan 600 s after the run began. Drop the dead branch.
H-4	hmib.php, poller_hmib.php	The Summary tab looked up the summary device once per device type row, each OS type graph link ran a separate host lookup, and the processor collector asked whether the device runs Linux once per processor row.	One lookup before the loop, a join in the graph query, one Linux check per device. The statistics insert casts its ids and values.
H-5	poller_hmib.php, auto purge	Seven DELETEs per removed device, and process history was never aged out.	One DELETE per table for all removed devices. A new Process History Retention setting (default Never Purge) drops Use History rows not seen for the chosen number of days; Automatically Purge Devices no longer touches history.
H-6	hmib.php, hmib_types.php	The sort column of the Devices, Storage, Hardware, Processes, Use History and Inventory tabs and of the OS type list reached ORDER BY unchecked; an unknown column broke the query, and on the OS type list it ended in a fatal error that disabled the plugin. The edit form and the CSV import built SQL from request and file values.	Allowlist per table with the existing ORDER BY helper, bound edit and import queries, numeric option values cast.
H-7	hmib.php	The OS Type, Device and Template filters were copied into six tabs, with the OS type concatenated into the Device query.	One shared function with the OS type bound.
H-8	hmib.php	The Search and Type fields on the Storage tab and the Process field on the Devices tab had no label (reported by SonarCloud once the shared filter moved them).	Wrap the existing captions in <label for>.
H-9	hmib.php, hmib_summary_order_by()	Moving the tabs off core's get_order_string() dropped shift-click multi-column sorting, which 1.2.x core keeps in $_SESSION['sort_data'].	Use the stored multi-column sort again, keeping each column and direction only when the table can sort on it.

Not changed: the union return types in setup.php and hmib.php (array|string, string|int). The repository's CI matrix and PhpCompatibilityTest set the floor at PHP 8.2, so they are valid. Raising or lowering the floor is a separate decision.

Behavior changes to be aware of:

• The rescan no longer uses RLIKE and no longer creates a type from a device with an empty sysDescr or sysObjectID.
• The rescan is POST only. Anything that called hmib_types.php?scan=true directly must POST.
• CSV export values that start with =, +, - or @ gain a leading single quote.
• The Summary tab data-query links now cover all devices when no device is given. On large installs that link carries every matching graph id (see Performance).

How Has This Been Tested?

Every fix has a test that fails on the previous code. With the four production files restored to origin/develop, the suite reports 97 failures; with this branch it reports none.

The tests are of four kinds:

• Unit and security tests (tests/Unit, tests/Security). The page scripts and the pollers chdir, include Cacti's bootstrap or exit at the top level, so a test cannot include them. These tests cut one function out of the source text and run it (hmib_test_define_function() in tests/bootstrap-unit.php, which writes the function to a private temporary file, requires it and deletes it).
• SQL tests (tests/Integration/HmibSqlDbTest.php). The plugin's own statements run against a real Cacti schema in MariaDB, each in three SQL modes: strict, not strict, and strict with ONLY_FULL_GROUP_BY.
• End-to-end tests (tests/E2E). They start PHP's built-in web server on the Cacti install the plugin sits in, log in as three users created for the test (viewer and admin realms, viewer only, and no plugin realm), and send real requests that are answered from the install's database. The pollers and associate_os_type.php run as subprocesses. 140 of the 235 tests are end-to-end. Paths covered: the Summary tab with and without a filter, for every sortable column in both directions, with hostile sort values, with 1,200 processes, with quotes in process names and an empty database; the Devices, Processes and Graphs tabs and the links on each device row, followed through to the page they open; the OS type page with markup in every stored field, the rescan by POST and its rejection by GET, by missing token, by lacking realm and when anonymous, CSV export with quote and formula characters, import with and without update, with quotes, with markup, with a bad header and with no file, and the edit form; graph automation with only the users template, only the processes template, both, neither, and the summary device created from its template; the per-device worker with and without the performance collector due; the main run; and OS type association.
• Helper tests. TestsDirectoryInertTest shows that every PHP helper under tests/ answers 404 with no body when a web server executes it, and that the coverage helper does nothing unless the test process asks for it, writes only inside the directory it is given, and sanitises the test name it uses in a file name.

The end-to-end tests skip when the install, its database or the plugin is missing. The CI workflow starts MariaDB, installs Cacti and installs the plugin before the Pest step, so they run there; the connection settings come from the install's include/config.php.

To see which test runs a changed line, tests/support/coverage.php and tests/bin/coverage-prepend.php record Xdebug coverage test by test (in the test process and in the server and subprocesses it starts), and tests/bin/merge-coverage.php merges the dumps and lists, for each changed executable line, the tests that ran it. All 52 changed executable lines of the four production files are run (26 in hmib.php, 19 in hmib_types.php, 3 in poller_graphs.php, 4 in poller_hmib.php), by between 2 and 19 tests each. The last commit makes CI run that second, per-test pass and feed the merged result to tests/bin/patch-coverage.php, so the four files no longer need the script's allowlist: it is now empty, and tests/bin/patch-coverage.php reports 100.00% (52/52 lines) against origin/develop.

Results, all run locally in Docker (PHP 8.4.27 with Xdebug 3, MariaDB 11.8.9), in the CI order: database loaded from the core's cacti.sql, php cli/plugin_manage.php --plugin=hmib --install --enable, Pest with Clover, Pest per test, merge, patch coverage:

Cacti core	Pest	Patch coverage
develop	235 tests: 233 passed, 2 skipped, 894 assertions	100.00% (52/52)
1.2.x (1.2.32)	235 tests: 232 passed, 3 skipped, 891 assertions	100.00% (52/52)

The skips are a permissions test that cannot fail as root, the SNMP-agent test (no agent answers in the container; with snmpd on the CI runner it checks that a second worker run leaves the time seen total alone), and, on 1.2.x only, the Graphs tab test, because that core defines graph_view_has_untemplated_graphs() in graph_view.php only and so its graph filter fatals on a plugin page. That is a defect of the Graphs tab on 1.2.x that exists before this change; it is not fixed here.

Also run: php -l on every PHP file, tests/bin/validate-manifest.php, tests/bin/check-i18n-pot.php origin/develop. The 1.2.x run used the core's own tree with the Pest from the develop tree, because Composer could not install the core's dev dependencies offline.

Not run locally: PHP 8.2 and 8.3 (the CI matrix), the SNMP-agent test, the CI workflow itself (the added step was run by hand in the same order), the Cacti installer (the databases were loaded from cacti.sql), and any browser. Xdebug's branch data is reported by the merge tool but not relied on: it marks three branches of changed functions as not taken although the lines inside them ran.

Rules the new test code follows: no eval, no weak hashes, no URL or IP literals apart from the loopback address of a silent UDP socket in one poller test, no SQL built from variables (the table copy-and-restore statements are spelled out), the test users' passwords are random and hashed with password_hash(), and nothing deletes outside a directory the test created.

tests/bootstrap-unit.php and the other helpers under tests/ exit with a 404 when requested over the web, because tests/ can sit in the web root on git-clone installs.

The H rows were added after the review. Their tests fail on the code before each fix. Run locally in Docker in the CI order (Cacti 1.2.x, MariaDB 11.8.9, the Cacti log made writable as the workflow now does) on PHP 8.2, 8.3 and 8.4: PHP 8.2.35, 8.3.36 and 8.4.27 each ran 301 tests, 300 passed and 1 skipped, in both Pest passes; patch coverage 100.00% (256/256 lines); the translation template check, the poller smoke step and the log check pass. The new workflow step runs poller_hmib.php -M --force as www-data after the Cacti poller, checks that it finishes and that the plugin tables exist; it first waits for the main run the Cacti poller starts in the background, which the run lock would otherwise refuse.

Performance

Measured with a throwaway harness (not part of the pull request) that runs the functions cut from origin/develop and from this branch against the same MariaDB 11.8.9 container. The harness uses PDO directly, not Cacti's database layer. Median of 9 runs (5 for the rescan); wall time from hrtime, CPU from getrusage, queries from SHOW SESSION STATUS LIKE 'Questions'. Fixture: graph_local 20,000 rows over 2,000 devices, plugin_hmib_hrSWRun 50,000 rows (500 distinct names), plugin_hmib_hrSystem 200 unclassified devices with distinct sysDescr, 20 stored types.

Case	develop	this branch
graph link, no device, no index	0.03 ms, 1 query, 0 graphs found (the F-1 bug)	4.89 ms, 1 query, 20,000 graph ids in the link, peak memory 0.7 MB to 8.7 MB
graph link, one device	0.04 ms, 1 query	0.04 ms, 1 query
graph link, one index	0.03 ms, 1 query	0.03 ms, 1 query
50 graph links (one summary page loop)	1.87 ms, 50 queries	1.35 ms, 50 queries
Summary processes page count (50,000 rows, unfiltered)	3.72 ms, 1 query	3.58 ms, 1 query
rescan_types(), 200 unclassified devices, 20 types	2,613 ms, 4,006 queries	2,560 ms, 4,006 queries

Reading the numbers: the only cost is in F-1. Restoring the intended "all devices" link makes the link carry every matching graph id (about 5 ms and 8 MB here for 20,000 graphs). The 50-link loop differences are within run-to-run noise. The rescan keeps its existing shape of one UPDATE per unclassified device type and stored type pair (4,006 queries here), so a large unclassified fleet with many stored types is slow before and after; this change does not alter that. The count query, the sort allowlist, the CSV helper, the escaping and the total_time change add no queries. F-12 adds one query, once, when the summary device is created.

The Summary sort defaults (F-14) add two array lookups with defaults per request and no query. CI cost, measured locally: the full suite takes about 36 s in the first Pest step (with Clover) and 110 to 185 s in the new per-test pass, because every request the server answers is instrumented in that pass; that is the price of the second pass on pull requests.

Not measured: Cacti's own db_* wrappers, rendered page time, the poller with real SNMP devices, memory growth in the poller, and Xdebug cachegrind.

Screenshots (if appropriate):

None.

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 rescan now requires POST, which breaks a script that called the old GET link.

Rollback

Each commit is independent apart from the shared test helpers added in tests/bootstrap-unit.php. Reverting one commit restores that behavior only. The last commit changes the CI workflow and empties the allowlist in tests/bin/patch-coverage.php; revert it alone to measure only the files the unit run loads again. No schema, INFO or install changes are involved, so nothing needs a data migration. Rows added by the rescan are ordinary plugin_hmib_hrSystemTypes rows and remain valid after a revert.

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.

[TheWitness]

ecell

[somethingwithproof]

I could not tell what change these three comments ask for. The cells already go through html_escape(). Could you say what should change here?

[TheWitness]

Excll

[TheWitness]

ecell

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

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

See analysis details on SonarQube Cloud