Harden the collector, cache refresh and WMI pages

.github/workflows/plugin-ci-workflow.yml

@@ -196,7 +196,19 @@ jobs:
     - name: Set expected Cacti version for unit tests
       run: echo -n "${{ env.CACTI }}" | sudo tee ${{ github.workspace }}/cacti/plugins/wmi/tests/.cacti-version > /dev/null
 
+    # Installing Cacti with sudo leaves log/cacti.log owned by root, and
+    # Cacti's CLI bootstrap refuses to start when it cannot write that file.
+    # The end-to-end collector test runs Cacti as the runner user.
+    - name: Make the Cacti log writable for the test user
+      run: |
+        sudo mkdir -p ${{ github.workspace }}/cacti/log
+        sudo chown -R www-data:runner ${{ github.workspace }}/cacti/log
+        sudo chmod 775 ${{ github.workspace }}/cacti/log
+        sudo find ${{ github.workspace }}/cacti/log -type f -exec chmod 664 {} \;
+
     - name: Run Pest Unit Tests
+      env:
+        WMI_E2E: '1'
       run: |
         cd ${{ github.workspace }}/cacti
         "$CACTI_PEST_BIN" --configuration=plugins/wmi/phpunit.xml \

CHANGELOG.md

@@ -2,6 +2,23 @@
 
 --- develop ---
 
+* issue: Record the WMI query start time from the database clock so devices are not polled again every cycle when PHP and the database use different time zones
+* security: Run the WMI Query Tool query action only for POST requests so credentials stay out of URLs, access logs and browser history
+* dev: Cover the Authentication, Queries and Query Tool pages and the Device Template hook with real-request end-to-end tests and replace the source-text checks
+* dev: Record which plugin lines the end-to-end run executes and report the changed lines no suite reaches (tests/bin/e2e-coverage-report.php)
+* issue: Fix the breadcrumb parents of the Edit WMI Account and WMI Query Tool pages so the account edit page no longer fails on Cacti develop
+* issue: Use the default of 10 concurrent WMI collectors when the setting is empty instead of waiting forever
+* dev: Restore cacti.log after the end-to-end collector test so the CI log check stays clean
+* dev: Add an end-to-end collector test that drives poller_wmi.php and the cache refresh through a fake wmic against the CI Cacti install and database
+* issue: Fix the form action of the remove WMI Query confirmation on the Device Template page
+* issue: Reject WMI account saves with a mismatched, missing or oversized password instead of saving a partial account
+* issue: Count all matching rows for the WMI Queries and Accounts pagers
+* security: Stop keeping the Query Tool password in the session and echoing it back into the form
+* issue: Pass the default WMI namespace to wmic with a single backslash
+* issue: Log the reason a WMI query failed during polling
+* issue: Index cached WMI rows by every column of a comma separated primary key so multi-row results are no longer overwritten
+* issue: Purge WMI cache rows the device stopped returning, and keep the existing rows when a query fails
+* issue: Expire stale wmi_processes lock rows and stop the master poller waiting indefinitely for collectors that died
 * security: Replace the confirmation pages' inline `onClick='cactiReturnTo()'` Cancel buttons with the CSP-safe `cactiReturnTo` class so the pages no longer trip Cacti's Content-Security-Policy script-src-attr directive
 * security: Move the WMI Accounts and Queries filter rows selects' inline onChange handlers into the ready block so the pages no longer trip Cacti's Content-Security-Policy script-src-attr directive
 * 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)

includes/functions.php

@@ -377,9 +377,11 @@ function plugin_wmi_create_resource_xml($id) {
  * Called from poller_wmi.php's polling loop for each host/query pairing
  * configured to use WMI-based data collection.
  *
- * @param int $host_id      The Cacti host id to query.
- * @param int $wmi_query_id The wmi_wql_queries.id to run against the
- *                          host.
+ * @param int                $host_id      The Cacti host id to query.
+ * @param int                $wmi_query_id The wmi_wql_queries.id to run against the
+ *                                         host.
+ * @param Wmi_Transport|null $transport    Transport override; the OS default
+ *                                         is used when null.
  *
  * @return bool|void True when the query succeeded and results were
  *                   cached; false when the host's WMI account/query
@@ -391,7 +393,7 @@ function plugin_wmi_create_resource_xml($id) {
  * @global array $config Cacti global configuration array; used to load
  *                        this plugin's linux_wmi.php client.
  */
-function run_store_wmi_query($host_id, $wmi_query_id) {
+function run_store_wmi_query($host_id, $wmi_query_id, $transport = null) {
 	global $config;
 
 	$host_info = db_fetch_row_prepared('SELECT *
@@ -401,13 +403,6 @@ function run_store_wmi_query($host_id, $wmi_query_id) {
 
 	$host_info = is_array($host_info) ? $host_info : [];
 
-	// Prepared old entries for removal
-	db_execute_prepared('UPDATE host_wmi_cache
-		SET present = 0
-		WHERE host_id = ?
-		AND wmi_query_id = ?',
-		[$host_id, $wmi_query_id]);
-
 	if (cacti_sizeof($host_info)) {
 		$auth_info = db_fetch_row_prepared('SELECT *
 			FROM wmi_user_accounts
@@ -447,7 +442,7 @@ function run_store_wmi_query($host_id, $wmi_query_id) {
 		// Cacti server and the wmic shell transport otherwise (see
 		// Linux_WMI::default_transport()), so the same client is used here
 		// regardless of $config['cacti_server_os'].
-		$wmi              = new Linux_WMI();
+		$wmi              = new Linux_WMI('', $transport);
 		$wmi->hostname    = $host;
 		$wmi->username    = $username;
 		$wmi->password    = $wmi->decode($password);
@@ -476,21 +471,9 @@ function run_store_wmi_query($host_id, $wmi_query_id) {
 		if (cacti_sizeof($data)) {
 			$sql = [];
 
-			$pk_index = -1;
-
-			if (cacti_sizeof($indexes)) {
-				foreach ($indexes as $index => $value) {
-					if ($value == $wmi_query['primary_key']) {
-						$pk_index = $index;
-
-						break;
-					}
-				}
-			}
-
 			if (cacti_sizeof($data)) {
 				foreach ($data as $row) {
-					$pk = isset($row[$pk_index]) ? $row[$pk_index] : 'N/A';
+					$pk = wmi_row_index($row, $indexes, $wmi_query['primary_key']);
 
 					foreach ($row as $index => $value) {
 						if (!isset($indexes[$index])) {
@@ -520,6 +503,13 @@ function run_store_wmi_query($host_id, $wmi_query_id) {
 				}
 			}
 
+			// Mark existing rows for removal only once fresh data is in hand
+			db_execute_prepared('UPDATE host_wmi_cache
+				SET present = 0
+				WHERE host_id = ?
+				AND wmi_query_id = ?',
+				[$host_id, $wmi_query_id]);
+
 			$parts = array_chunk($sql, 200);
 
 			foreach ($parts as $part) {
@@ -532,22 +522,130 @@ function run_store_wmi_query($host_id, $wmi_query_id) {
 						present=1');
 			}
 
+			// Remove entries the device no longer returns
+			db_execute_prepared('DELETE FROM host_wmi_cache
+				WHERE present = 0
+				AND host_id = ?
+				AND wmi_query_id = ?',
+				[$host_id, $wmi_query_id]);
+
 			return true;
 		} else {
+			$reason = is_string($wmi->error) ? $wmi->error : 'WMI returned no rows';
+
+			cacti_log('WARNING: WMI Query[' . $wmi_query_id . '] for Device[' . $host_id . '] failed: ' . $reason, false, 'WMI', POLLER_VERBOSITY_MEDIUM);
+
 			print $wmi->error;
 
 			return false;
 		}
 	}
 
-	// Remove old entries
+	// The device no longer exists, so drop whatever was cached for it
 	db_execute_prepared('DELETE FROM host_wmi_cache
-		WHERE present = 0
-		AND host_id = ?
+		WHERE host_id = ?
 		AND wmi_query_id = ?',
 		[$host_id, $wmi_query_id]);
 }
 
+/**
+ * Checks the fields of a submitted WMI account. A name and username are
+ * required, the password must match its confirmation, and a new account
+ * must have a password.
+ *
+ * @param string $name     The account name.
+ * @param string $username The Windows user name.
+ * @param string $password The new password, or '' to keep the stored one.
+ * @param string $confirm  The password confirmation.
+ * @param int    $id       The account id, 0 for a new account.
+ *
+ * @return bool True when the account can be saved.
+ */
+function wmi_account_input_valid($name, $username, $password, $confirm, $id) {
+	if ($name === '' || $username === '' || $password !== $confirm) {
+		return false;
+	}
+
+	return $password !== '' || $id > 0;
+}
+
+/**
+ * Checks that an encoded password fits the wmi_user_accounts.password
+ * column (varchar(256)) instead of being truncated or rejected by the
+ * database.
+ *
+ * @param string $encoded The value returned by Linux_WMI::encode().
+ *
+ * @return bool True when the value fits.
+ */
+function wmi_account_password_fits($encoded) {
+	return strlen($encoded) <= 256;
+}
+
+/**
+ * Builds the host_wmi_cache index value for one result row from the query's
+ * primary key. A comma separated key joins its columns' values with a comma.
+ * 'None', an empty key, or a key column missing from the result header yields
+ * 'N/A', which is the single shared index for queries that return one row.
+ *
+ * @param array<int, string> $row         One result row.
+ * @param array<int, string> $indexes     The result's column header.
+ * @param string             $primary_key The query's primary key setting.
+ *
+ * @return string The cache index for the row.
+ */
+function wmi_row_index($row, $indexes, $primary_key) {
+	$values = [];
+
+	foreach (explode(',', $primary_key) as $column) {
+		$position = array_search(trim($column), $indexes, true);
+
+		if ($position === false || !isset($row[$position])) {
+			return 'N/A';
+		}
+
+		$values[] = $row[$position];
+	}
+
+	return implode(',', $values);
+}
+
+/**
+ * Deletes wmi_processes lock rows older than $max_age seconds. A collector
+ * that was killed or never started leaves its row behind, and the master
+ * poller refuses to run while any row exists.
+ *
+ * @param int $max_age Age in seconds after which a lock row is stale.
+ *
+ * @return void
+ */
+function wmi_purge_stale_processes($max_age = 3600) {
+	db_execute_prepared('DELETE FROM wmi_processes WHERE started < DATE_SUB(NOW(), INTERVAL ? SECOND)', [(int) $max_age]);
+}
+
+/**
+ * Returns how many collectors launched under $seed are still running. Once
+ * $max_age seconds have passed since $began, the remaining lock rows are
+ * released and 0 is returned so the master poller stops waiting for them.
+ *
+ * @param int $seed    The master run's task identifier.
+ * @param int $began   Unix time the master started waiting.
+ * @param int $max_age Longest time in seconds to wait for the collectors.
+ *
+ * @return int The number of collectors still running.
+ */
+function wmi_collectors_running($seed, $began, $max_age = 3600) {
+	if (time() - $began > $max_age) {
+		db_execute_prepared('DELETE FROM wmi_processes WHERE taskid = ?', [$seed]);
+
+		cacti_log('WARNING: Gave up waiting for WMI collectors after ' . $max_age . ' seconds', false, 'WMI');
+
+		return 0;
+	}
+
+	return (int) db_fetch_cell_prepared('SELECT COUNT(*) FROM wmi_processes WHERE taskid = ?', [$seed]);
+}
+
 /**
  * Returns the current unique hash for a WMI query, or generates a new
  * one if the stored value doesn't already look like a valid 128-bit hex

includes/linux_wmi.php

@@ -106,7 +106,7 @@ public function error(): ?string {
 	 */
 	public function build_command(Wmi_Request $request): string {
 		$namespace = $request->namespace !== ''
-			? ' --namespace=' . cacti_escapeshellarg($this->strip_metachars($request->namespace))
+			? ' --namespace=' . cacti_escapeshellarg($this->single_backslashes($this->strip_metachars($request->namespace)))
 			: '';
 
 		return cacti_escapeshellarg($request->binary) .
@@ -118,6 +118,15 @@ public function build_command(Wmi_Request $request): string {
 			' ' . cacti_escapeshellarg($request->query);
 	}
 
+	/**
+	 * Namespaces are stored with doubled backslashes between segments, a form
+	 * that only collapsed to one backslash inside a double-quoted shell
+	 * string. The value is now single-quoted, so collapse it here.
+	 */
+	private function single_backslashes(string $value): string {
+		return preg_replace('/\\\\+/', '\\\\', $value);
+	}
+
 	/**
 	 * A hostname or namespace never legitimately contains shell or cmd.exe
 	 * metacharacters. cmd.exe ignores \" , toggles quoting on every " , and

phpunit.xml

@@ -17,6 +17,7 @@
             <directory>tests/Security</directory>
             <directory>tests/Unit</directory>
             <directory>tests/Integration</directory>
+            <directory>tests/E2E</directory>
             <file>tests/test_tab_wrapper.php</file>
             <file>tests/WmiCommandInjectionTest.php</file>
             <file>tests/WmiPowerShellTransportTest.php</file>

poller_wmi.php

@@ -175,6 +175,8 @@ function process_all_devices() {
 
 	print 'NOTE: Processing Hosts Begins' . PHP_EOL;
 
+	wmi_purge_stale_processes();
+
 	// Do not process collectors are still running
 	if (db_fetch_cell('SELECT COUNT(*) FROM wmi_processes') > 0) {
 		print 'WARNING: Another WMI Collector is still running!  Exiting' . PHP_EOL;
@@ -213,6 +215,8 @@ function process_all_devices() {
 	$concurrent_processes = read_config_option('wmi_processes');
 
 	if (empty($concurrent_processes)) {
+		$concurrent_processes = 10;
+
 		set_config_option('wmi_processes', '10');
 	}
 
@@ -247,8 +251,10 @@ function process_all_devices() {
 		print 'NOTE: All WMI Devices Launched, proceeding to wait for completion' . PHP_EOL;
 
 		// wait for all processes to end or max run time
+		$wait_began = time();
+
 		while (true) {
-			$processes_left = db_fetch_cell_prepared('SELECT COUNT(*) FROM wmi_processes WHERE taskid = ?', [$seed]);
+			$processes_left = wmi_collectors_running($seed, $wait_began);
 			$pl             = db_fetch_cell('SELECT COUNT(*) FROM wmi_processes');
 
 			if ($processes_left == 0) {
@@ -389,7 +395,9 @@ function process_device($host_id) {
 	db_execute_prepared('REPLACE INTO wmi_processes (pid, taskid) VALUES (?, ?)', [getmypid(), $seed]);
 	db_execute_prepared('DELETE FROM wmi_processes WHERE pid = ? AND taskid = ?', [$key, $seed]);
 
-	$qstart  = date('Y-m-d H:i:s');
+	// last_started is compared with NOW() in SQL, so take the time from the
+	// database clock: PHP and the database session may use different time zones.
+	$qstart = db_fetch_cell('SELECT NOW()');
 
 	if (cacti_sizeof($queries_to_run)) {
 		foreach ($queries_to_run as $q) {

setup.php

@@ -336,7 +336,7 @@ function wmi_draw_navigation_text($nav) {
 
 	$nav['wmi_accounts.php:edit'] = [
 		'title'   => __('(Edit)', 'wmi'),
-		'mapping' => 'index.php:wmi_accounts.php:',
+		'mapping' => 'index.php:,wmi_accounts.php:',
 		'url'     => 'wmi_accounts.php',
 		'level'   => '2'
 	];
@@ -378,7 +378,7 @@ function wmi_draw_navigation_text($nav) {
 
 	$nav['wmi_tools.php:query'] = [
 		'title'   => __('(Query)', 'wmi'),
-		'mapping' => 'index.php:wmi_tools.php:',
+		'mapping' => 'index.php:,wmi_tools.php:',
 		'url'     => 'wmi_tools.php',
 		'level'   => '2'
 	];
@@ -724,7 +724,7 @@ function wmi_device_template_top() {
 		get_filter_request_var('host_template_id');
 		// ====================================================
 
-		form_start('host_templates.php?action=edit&id' . get_request_var('host_template_id'));
+		form_start('host_templates.php?action=edit&id=' . get_request_var('host_template_id'));
 
 		html_start_box('', '100%', false, 3, 'center', '');