From 180790c2a5b6a108abcecfb36923338d48bb33b5 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 23 Oct 2026 21:09:31 -0600 Subject: [PATCH 01/41] Call Cacti controller methods on an instance in the host import Signed-off-by: Thomas Vincent --- controllers/sync.php | 10 +-- phpunit.xml | 1 + tests/Unit/SyncImportTest.php | 116 ++++++++++++++++++++++++++++++++++ tests/bootstrap-unit.php | 72 +++++++++++++++++++-- 4 files changed, 189 insertions(+), 10 deletions(-) create mode 100644 tests/Unit/SyncImportTest.php diff --git a/controllers/sync.php b/controllers/sync.php index c24b839..a30c194 100644 --- a/controllers/sync.php +++ b/controllers/sync.php @@ -48,11 +48,12 @@ function import($params) { $config = $params['config']; $path = $config["base_path"] . "/cli/add_device.php"; + $cacti = new NpcCactiController(); $return = $params['description'] . '|'; // If the host exists and is already mapped return with notice - if (NpcCactiController::isMapped($params['host_object_id'])) { + if ($cacti->isMapped($params['host_object_id'])) { $this->logger('info', get_class($this), __FUNCTION__ , $params['description'] . " already exists and is mapped."); $return .= '0|0|The host already exists and is mapped'; return($return); @@ -60,7 +61,7 @@ function import($params) { // If the host already exists, map it and return. if ($cactiId = $this->checkHostExists($params['ip'], $params['cache_id'])) { - NpcCactiController::mapHost($params['host_object_id'], $cactiId); + $cacti->mapHost($params['host_object_id'], $cactiId); $this->logger('info', get_class($this), __FUNCTION__ , $params['description'] . " already existed and was mapped."); $return .= '0|1|The host already existed in Cacti with device ID: ' . $cactiId; return($return); @@ -83,7 +84,7 @@ function import($params) { if (isset($matches[1])) { // Import was successful. Now map the hosts - NpcCactiController::mapHost($params['host_object_id'], $matches[1]); + $cacti->mapHost($params['host_object_id'], $matches[1]); $this->logger('info', get_class($this), __FUNCTION__ , "Successfully imported and mapped " . $params['description'] . ": $output"); $return .= '1|1|' . $output; } elseif (isset($matches[0])) { @@ -204,7 +205,8 @@ function checkHostExists($ip, $cache_id) { $string .= "\$cacheKey = '" . $cache_id . "';\n"; $string .= "\$hosts = array(\n"; - $results = NpcCactiController::getHostnames(); + $cacti = new NpcCactiController(); + $results = $cacti->getHostnames(); for ($i = 0; $i < count($results); $i++) { $address = gethostbyname($results[$i]['hostname']); diff --git a/phpunit.xml b/phpunit.xml index bace346..493d1f2 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -29,6 +29,7 @@ setup.php + controllers diff --git a/tests/Unit/SyncImportTest.php b/tests/Unit/SyncImportTest.php new file mode 100644 index 0000000..625474a --- /dev/null +++ b/tests/Unit/SyncImportTest.php @@ -0,0 +1,116 @@ + 3)); + + $out = (new NpcSyncController())->import(array( + 'config' => array('base_path' => '/nonexistent'), + 'description' => 'web1', + 'host_object_id' => 10, + 'ip' => '192.0.2.10', + 'cache_id' => 'k', + 'template_id' => 1, + )); + + expect($out)->toBe('web1|0|0|The host already exists and is mapped'); +}); + +it('maps a host whose address already exists in Cacti', function () { + $sync = new class extends NpcSyncController { + function checkHostExists($ip, $cache_id) { + return 5; + } + }; + + $out = $sync->import(array( + 'config' => array('base_path' => '/nonexistent'), + 'description' => 'web1', + 'host_object_id' => 10, + 'ip' => '192.0.2.10', + 'cache_id' => 'k', + 'template_id' => 1, + )); + + expect($out)->toBe('web1|0|1|The host already existed in Cacti with device ID: 5'); + + $update = end($GLOBALS['__test_db_calls']); + expect($update['sql'])->toContain('UPDATE host'); + expect($update['params'])->toBe(array(10, 5)); +}); + +it('imports a new host and maps the returned device id', function () { + $base = npc_sync_sandbox(); + + $out = npc_sync_unmapped_controller()->import(array( + 'config' => array('base_path' => $base), + 'description' => 'web1', + 'host_object_id' => 10, + 'ip' => '192.0.2.10', + 'cache_id' => 'k', + 'template_id' => 7, + )); + + expect($out)->toBe('web1|1|1|Success - new device-id: (12)'); + + $update = end($GLOBALS['__test_db_calls']); + expect($update['params'])->toBe(array(10, '12')); +})->skip(trim((string) shell_exec('command -v php')) === '', 'php binary is not on PATH'); + +it('builds the address cache from Cacti devices when none exists', function () { + $cache = '/tmp/npc_address_cache.php'; + if (file_exists($cache)) { + unlink($cache); + } + + $GLOBALS['__test_db_results']['db_fetch_assoc']['FROM host'] = array(array('id' => 4, 'hostname' => '127.0.0.1')); + + try { + expect((new NpcSyncController())->checkHostExists('127.0.0.1', 'k1'))->toBe(4); + } finally { + if (file_exists($cache)) { + unlink($cache); + } + } +}); diff --git a/tests/bootstrap-unit.php b/tests/bootstrap-unit.php index 87a17ef..3977c50 100644 --- a/tests/bootstrap-unit.php +++ b/tests/bootstrap-unit.php @@ -80,6 +80,42 @@ ); $GLOBALS['__test_db_calls'] = array(); +$GLOBALS['__test_db_results'] = array(); +$GLOBALS['__test_db_fetch_log'] = array(); + +/** + * Result for a db_fetch_* stub. + * + * A test maps a substring of the SQL to the rows it wants back, per stub: + * $GLOBALS['__test_db_results']['db_fetch_assoc']['FROM user_auth'] = array(...). + * Every fetch is logged so a test can assert the bound parameters. + */ +function npc_test_db_result($fn, $sql, $params, $default) { + $GLOBALS['__test_db_fetch_log'][] = array('fn' => $fn, 'sql' => $sql, 'params' => $params); + + foreach (($GLOBALS['__test_db_results'][$fn] ?? array()) as $match => $result) { + if (strpos($sql, (string) $match) !== false) { + return $result; + } + } + + return $default; +} + +/** + * Make require_once('include/auth.php') resolve to an empty stub, ahead of + * the real Cacti file that the working directory would otherwise supply. + */ +function npc_test_stub_cacti_includes() { + static $dir = null; + + if ($dir === null) { + $dir = sys_get_temp_dir() . '/npc-inc-' . uniqid(); + mkdir($dir . '/include', 0777, true); + file_put_contents($dir . '/include/auth.php', " Date: Fri, 23 Oct 2026 21:10:37 -0600 Subject: [PATCH 02/41] Quote the arguments of the host import command Signed-off-by: Thomas Vincent --- CHANGELOG.md | 2 ++ controllers/sync.php | 14 ++++++++---- tests/Unit/SyncImportTest.php | 40 +++++++++++++++++++++++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e4afea3..0258604 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ --- develop --- +* security: Quote the description, address and template arguments of the Nagios host + import command and reject non-numeric template IDs * dev: Relocate the schema provisioning function `npc_setup_tables()` from `setup.php` into `includes/database.php` (loaded via `require_once`) to align with the fleet schema/includes convention; behavior is unchanged diff --git a/controllers/sync.php b/controllers/sync.php index a30c194..46997b4 100644 --- a/controllers/sync.php +++ b/controllers/sync.php @@ -67,10 +67,16 @@ function import($params) { return($return); } - $importCmd = 'php ' . $path - . ' --description=' . $params['description'] - . ' --ip=' . $params['ip'] - . ' --template=' . $params['template_id'] + if ($params['description'] === '' || $params['ip'] === '' || !ctype_digit((string) $params['template_id'])) { + $this->logger('warn', get_class($this), __FUNCTION__ , "Import rejected, invalid parameters for " . $params['description']); + $return .= '0|0|Invalid import parameters'; + return($return); + } + + $importCmd = 'php ' . cacti_escapeshellarg($path) + . ' --description=' . cacti_escapeshellarg($params['description']) + . ' --ip=' . cacti_escapeshellarg($params['ip']) + . ' --template=' . cacti_escapeshellarg($params['template_id']) . ' 2> /dev/null'; $this->logger('debug', get_class($this), __FUNCTION__ , "Import command: $importCmd"); diff --git a/tests/Unit/SyncImportTest.php b/tests/Unit/SyncImportTest.php index 625474a..750f08e 100644 --- a/tests/Unit/SyncImportTest.php +++ b/tests/Unit/SyncImportTest.php @@ -114,3 +114,43 @@ function checkHostExists($ip, $cache_id) { } } }); + +it('passes the import arguments to add_device.php as single values', function () { + $base = npc_sync_sandbox(); + + $out = npc_sync_unmapped_controller()->import(array( + 'config' => array('base_path' => $base), + 'description' => 'web 1 & echo extra', + 'host_object_id' => 10, + 'ip' => '192.0.2.10', + 'cache_id' => 'k', + 'template_id' => 7, + )); + + expect($out)->toBe('web 1 & echo extra|1|1|Success - new device-id: (12)'); + expect(json_decode(file_get_contents($base . '/cli/argv.json'), true))->toBe(array( + '--description=web 1 & echo extra', + '--ip=192.0.2.10', + '--template=7', + )); +})->skip(trim((string) shell_exec('command -v php')) === '', 'php binary is not on PATH'); + +it('rejects an import with a non-numeric template id or an empty address', function (array $override) { + $base = npc_sync_sandbox(); + + $out = npc_sync_unmapped_controller()->import(array_merge(array( + 'config' => array('base_path' => $base), + 'description' => 'web1', + 'host_object_id' => 10, + 'ip' => '192.0.2.10', + 'cache_id' => 'k', + 'template_id' => 7, + ), $override)); + + expect($out)->toBe(($override['description'] ?? 'web1') . '|0|0|Invalid import parameters'); + expect(file_exists($base . '/cli/argv.json'))->toBeFalse(); +})->with(array( + 'template id with a suffix' => array(array('template_id' => '7; id')), + 'empty address' => array(array('ip' => '')), + 'empty description' => array(array('description' => '')), +)); From 65cb48b685bb2480103c594fae95197f756a9c8b Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 23 Oct 2026 21:13:55 -0600 Subject: [PATCH 03/41] Store the host import address cache as JSON in the Cacti cache directory Signed-off-by: Thomas Vincent --- CHANGELOG.md | 2 + controllers/sync.php | 62 +++++++++++-------- tests/Unit/SyncImportTest.php | 109 +++++++++++++++++++++++++++++++--- 3 files changed, 140 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0258604..598e732 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ --- develop --- +* security: Keep the host import address cache as JSON in the Cacti cache directory + instead of a PHP file in the system temp directory that was included on each call * security: Quote the description, address and template arguments of the Nagios host import command and reject non-numeric template IDs * dev: Relocate the schema provisioning function `npc_setup_tables()` from diff --git a/controllers/sync.php b/controllers/sync.php index 46997b4..0422991 100644 --- a/controllers/sync.php +++ b/controllers/sync.php @@ -187,44 +187,30 @@ function listHostgroups() { * @return int The Cacti host ID */ function checkHostExists($ip, $cache_id) { + global $config; - $cacheFile = '/tmp/npc_address_cache.php'; + $cacheFile = $config['base_path'] . '/cache/npc_address_cache.json'; + $hosts = null; - $buildCache = 0; + if (is_file($cacheFile) && is_readable($cacheFile)) { + $cache = json_decode((string) file_get_contents($cacheFile), true); - if (file_exists($cacheFile)) { - include($cacheFile); - if ($cacheKey != $cache_id) { - $buildCache = 1; + if (is_array($cache) && isset($cache['key']) && $cache['key'] === (string) $cache_id && isset($cache['hosts']) && is_array($cache['hosts'])) { + $hosts = $cache['hosts']; } - } else { - $buildCache = 1; } - if ($buildCache) { - - $fh = fopen($cacheFile, 'w') or die("can't open file"); - + if ($hosts === null) { $hosts = array(); - $string = "getHostnames(); for ($i = 0; $i < count($results); $i++) { - $address = gethostbyname($results[$i]['hostname']); - $string .= "'" . $address . "' => '" . $results[$i]['id'] . "',\n"; - $hosts[$address] = $results[$i]['id']; + $hosts[gethostbyname($results[$i]['hostname'])] = $results[$i]['id']; } - $string .= ");"; - - fwrite($fh, $string); - - fclose($fh); + $this->writeAddressCache($cacheFile, $cache_id, $hosts); } if (isset($hosts[$ip])) { @@ -234,4 +220,30 @@ function checkHostExists($ip, $cache_id) { return(null); } + /** + * writeAddressCache + * + * Stores the address map as JSON in the Cacti cache directory. The file is + * written under a temporary name and renamed so a concurrent import never + * reads a partial file. A cache that cannot be written only costs a rebuild + * on the next call. + * + * @return void + */ + private function writeAddressCache($cacheFile, $cache_id, $hosts) { + if (!is_dir(dirname($cacheFile)) || !is_writable(dirname($cacheFile))) { + $this->logger('warn', get_class($this), __FUNCTION__, 'Unable to create the address cache in ' . dirname($cacheFile)); + return; + } + + $tmp = tempnam(dirname($cacheFile), 'npc'); + + $bytes = file_put_contents($tmp, json_encode(array('key' => (string) $cache_id, 'hosts' => $hosts))); + + if ($bytes === false || is_dir($cacheFile) || !rename($tmp, $cacheFile)) { + $this->logger('warn', get_class($this), __FUNCTION__, 'Unable to write the address cache ' . $cacheFile); + unlink($tmp); + } + } + } diff --git a/tests/Unit/SyncImportTest.php b/tests/Unit/SyncImportTest.php index 750f08e..760a414 100644 --- a/tests/Unit/SyncImportTest.php +++ b/tests/Unit/SyncImportTest.php @@ -21,6 +21,7 @@ beforeEach(function () { $GLOBALS['__test_db_calls'] = array(); $GLOBALS['__test_db_results'] = array(); + $GLOBALS['__test_db_fetch_log'] = array(); }); /** @@ -98,21 +99,113 @@ function checkHostExists($ip, $cache_id) { expect($update['params'])->toBe(array(10, '12')); })->skip(trim((string) shell_exec('command -v php')) === '', 'php binary is not on PATH'); -it('builds the address cache from Cacti devices when none exists', function () { - $cache = '/tmp/npc_address_cache.php'; - if (file_exists($cache)) { - unlink($cache); +/** + * Run checkHostExists() against a temporary base_path and return the result + * together with that path, so each test can inspect the cache directory. + */ +function npc_sync_cache_run($ip, $cache_id, $prepare = null) { + $base = sys_get_temp_dir() . '/npc-cache-' . uniqid(); + mkdir($base . '/cache', 0777, true); + + if ($prepare !== null) { + $prepare($base . '/cache/npc_address_cache.json'); + } + + $restore = $GLOBALS['config']['base_path']; + $GLOBALS['config']['base_path'] = $base; + + try { + $found = (new NpcSyncController())->checkHostExists($ip, $cache_id); + } finally { + $GLOBALS['config']['base_path'] = $restore; + } + + return array($found, $base); +} + +it('builds the address cache as JSON from Cacti devices', function () { + $GLOBALS['__test_db_results']['db_fetch_assoc']['FROM host'] = array(array('id' => 4, 'hostname' => '127.0.0.1')); + + list($found, $base) = npc_sync_cache_run('127.0.0.1', 'k1'); + $file = $base . '/cache/npc_address_cache.json'; + + expect($found)->toBe(4); + expect(json_decode(file_get_contents($file), true))->toBe(array('key' => 'k1', 'hosts' => array('127.0.0.1' => 4))); + expect(substr(sprintf('%o', fileperms($file)), -4))->toBe('0600'); + expect(glob($base . '/cache/*'))->toBe(array($file)); +}); + +it('answers from a current cache without reading the device list', function () { + list($found) = npc_sync_cache_run('10.0.0.1', 'k1', function ($file) { + file_put_contents($file, json_encode(array('key' => 'k1', 'hosts' => array('10.0.0.1' => 7)))); + }); + + expect($found)->toBe(7); + expect($GLOBALS['__test_db_fetch_log'])->toBe(array()); +}); + +it('rebuilds the cache when the key differs or the file is damaged', function (string $content) { + $GLOBALS['__test_db_results']['db_fetch_assoc']['FROM host'] = array(array('id' => 4, 'hostname' => '127.0.0.1')); + $GLOBALS['__test_db_fetch_log'] = array(); + + list($found) = npc_sync_cache_run('127.0.0.1', 'k2', function ($file) use ($content) { + file_put_contents($file, $content); + }); + + expect($found)->toBe(4); + expect($GLOBALS['__test_db_fetch_log'])->not->toBe(array()); +})->with(array( + 'stale key' => array('{"key":"k1","hosts":{"127.0.0.1":9}}'), + 'invalid JSON' => array('not json'), + 'wrong shape' => array('{"key":"k2"}'), +)); + +it('does not execute a PHP cache file left in the system temp directory', function () { + $legacy = '/tmp/npc_address_cache.php'; + + if (file_exists($legacy)) { + $this->markTestSkipped($legacy . ' already exists'); + } + + file_put_contents($legacy, " '9');\n"); + unset($GLOBALS['npc_legacy_cache_ran']); + + try { + list($found) = npc_sync_cache_run('10.0.0.1', 'k1'); + } finally { + unlink($legacy); } + expect($found)->toBeNull(); + expect(isset($GLOBALS['npc_legacy_cache_ran']))->toBeFalse(); +}); + +it('still answers when the cache directory is missing', function () { $GLOBALS['__test_db_results']['db_fetch_assoc']['FROM host'] = array(array('id' => 4, 'hostname' => '127.0.0.1')); + $base = sys_get_temp_dir() . '/npc-nocache-' . uniqid(); + $restore = $GLOBALS['config']['base_path']; + $GLOBALS['config']['base_path'] = $base; + try { - expect((new NpcSyncController())->checkHostExists('127.0.0.1', 'k1'))->toBe(4); + $found = (new NpcSyncController())->checkHostExists('127.0.0.1', 'k1'); } finally { - if (file_exists($cache)) { - unlink($cache); - } + $GLOBALS['config']['base_path'] = $restore; } + + expect($found)->toBe(4); + expect(file_exists($base))->toBeFalse(); +}); + +it('removes its temporary file when the cache cannot be moved into place', function () { + $GLOBALS['__test_db_results']['db_fetch_assoc']['FROM host'] = array(array('id' => 4, 'hostname' => '127.0.0.1')); + + list($found, $base) = npc_sync_cache_run('127.0.0.1', 'k1', function ($file) { + mkdir($file); + }); + + expect($found)->toBe(4); + expect(glob($base . '/cache/*'))->toBe(array($base . '/cache/npc_address_cache.json')); }); it('passes the import arguments to add_device.php as single values', function () { From e6797ef9298b52819e28fc275d5886ddc497cf1f Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 23 Oct 2026 21:16:07 -0600 Subject: [PATCH 04/41] Require device and data input realms for the host import and data input creation Signed-off-by: Thomas Vincent --- CHANGELOG.md | 3 +++ controllers/cacti.php | 4 ++++ controllers/sync.php | 4 ++++ tests/Unit/CactiControllerTest.php | 33 ++++++++++++++++++++++++++++++ tests/Unit/SyncImportTest.php | 20 ++++++++++++++++++ 5 files changed, 64 insertions(+) create mode 100644 tests/Unit/CactiControllerTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 598e732..6fd6f63 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ --- develop --- +* security: Require the Devices realm for the Nagios host import and the Data Input + Methods realm for creating a perfdata data input method; both were open to any + user with NPC access * security: Keep the host import address cache as JSON in the Cacti cache directory instead of a PHP file in the system temp directory that was included on each call * security: Quote the description, address and template arguments of the Nagios host diff --git a/controllers/cacti.php b/controllers/cacti.php index 3bdb2a0..3b57609 100644 --- a/controllers/cacti.php +++ b/controllers/cacti.php @@ -76,6 +76,10 @@ function getHostnames() { * @return string json encoded results */ function addDataInputMethod($params) { + if (!api_user_realm_auth('data_input.php')) { + return(json_encode(array('success' => false, 'msg' => __('You do not have permission to create data input methods.', 'npc')))); + } + $name = 'NPC - Perfdata - ' . $params['host']; if (isset($params['service'])) { diff --git a/controllers/sync.php b/controllers/sync.php index 0422991..7355adf 100644 --- a/controllers/sync.php +++ b/controllers/sync.php @@ -44,6 +44,10 @@ class NpcSyncController extends Controller { */ function import($params) { + if (!api_user_realm_auth('host.php')) { + return($params['description'] . '|0|0|You do not have permission to add devices'); + } + $this->logger('info', get_class($this), __FUNCTION__ , "Starting import of " . $params['description']); $config = $params['config']; diff --git a/tests/Unit/CactiControllerTest.php b/tests/Unit/CactiControllerTest.php new file mode 100644 index 0000000..09d6f02 --- /dev/null +++ b/tests/Unit/CactiControllerTest.php @@ -0,0 +1,33 @@ +addDataInputMethod(array('host' => 'web1', 'object_id' => '12')), true); + + expect($out['success'])->toBeFalse(); + expect($out['msg'])->toBe('You do not have permission to create data input methods.'); + expect($GLOBALS['__test_db_calls'])->toBe(array()); +}); diff --git a/tests/Unit/SyncImportTest.php b/tests/Unit/SyncImportTest.php index 760a414..cef43a7 100644 --- a/tests/Unit/SyncImportTest.php +++ b/tests/Unit/SyncImportTest.php @@ -22,6 +22,7 @@ $GLOBALS['__test_db_calls'] = array(); $GLOBALS['__test_db_results'] = array(); $GLOBALS['__test_db_fetch_log'] = array(); + $GLOBALS['__test_denied_realms'] = array(); }); /** @@ -247,3 +248,22 @@ function npc_sync_cache_run($ip, $cache_id, $prepare = null) { 'empty address' => array(array('ip' => '')), 'empty description' => array(array('description' => '')), )); + +it('refuses the import to a user without device management rights', function () { + $GLOBALS['__test_denied_realms'] = array('host.php'); + + $base = npc_sync_sandbox(); + + $out = npc_sync_unmapped_controller()->import(array( + 'config' => array('base_path' => $base), + 'description' => 'web1', + 'host_object_id' => 10, + 'ip' => '192.0.2.10', + 'cache_id' => 'k', + 'template_id' => 7, + )); + + expect($out)->toBe('web1|0|0|You do not have permission to add devices'); + expect($GLOBALS['__test_db_calls'])->toBe(array()); + expect(file_exists($base . '/cli/argv.json'))->toBeFalse(); +}); From 3f462f7364f3421ce43793a2b6cea04ffdc6b817 Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 23 Oct 2026 21:17:14 -0600 Subject: [PATCH 05/41] Use the plain object id when reading perfdata for a new data input method Signed-off-by: Thomas Vincent --- CHANGELOG.md | 3 +++ controllers/cacti.php | 6 +++--- tests/Unit/CactiControllerTest.php | 31 ++++++++++++++++++++++++++++++ tests/bootstrap-unit.php | 2 ++ 4 files changed, 39 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6fd6f63..5e75637 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,9 @@ --- develop --- +* issue: Look up the stored perfdata with the plain object ID when creating a + perfdata data input method; the shell-quoted value never matched, so no output + fields were created * security: Require the Devices realm for the Nagios host import and the Data Input Methods realm for creating a perfdata data input method; both were open to any user with NPC access diff --git a/controllers/cacti.php b/controllers/cacti.php index 3b57609..3e3dcd4 100644 --- a/controllers/cacti.php +++ b/controllers/cacti.php @@ -91,9 +91,9 @@ function addDataInputMethod($params) { $class = 'NpcHostsController'; } - $object_id = cacti_escapeshellarg($params['object_id']); + $object_id = $params['object_id']; - $input = "php /plugins/npc/perfdata.php --type=$type --id=$object_id 2> /dev/null"; + $input = "php /plugins/npc/perfdata.php --type=$type --id=" . cacti_escapeshellarg($object_id) . " 2> /dev/null"; $save = array(); $save['hash'] = $this->generateHash(); @@ -106,7 +106,7 @@ function addDataInputMethod($params) { $obj = new $class; $results = $obj->getPerfData($object_id); - $perfParts = explode(';', $results[0]['perfdata']); + $perfParts = explode(';', isset($results[0]['perfdata']) ? $results[0]['perfdata'] : ''); foreach ($perfParts as $perf) { if (preg_match('/=/', $perf)) { diff --git a/tests/Unit/CactiControllerTest.php b/tests/Unit/CactiControllerTest.php index 09d6f02..dd6b1fd 100644 --- a/tests/Unit/CactiControllerTest.php +++ b/tests/Unit/CactiControllerTest.php @@ -20,6 +20,7 @@ $GLOBALS['__test_db_results'] = array(); $GLOBALS['__test_db_fetch_log'] = array(); $GLOBALS['__test_denied_realms'] = array(); + $GLOBALS['__test_sql_save'] = array(); }); it('refuses to create a data input method without the data input realm', function () { @@ -31,3 +32,33 @@ expect($out['msg'])->toBe('You do not have permission to create data input methods.'); expect($GLOBALS['__test_db_calls'])->toBe(array()); }); + +it('reads the stored perfdata for the plain object id and adds one output field per label', function () { + $GLOBALS['__test_db_results']['db_fetch_assoc_prepared']['FROM npc_hostchecks'] = array(array('perfdata' => 'rta=0.5ms;100;200;0;10;pl=0%')); + + $out = json_decode((new NpcCactiController())->addDataInputMethod(array('host' => 'web1', 'object_id' => '12')), true); + + expect($out['success'])->toBeTrue(); + + $save = $GLOBALS['__test_sql_save'][0]; + expect($save['table'])->toBe('data_input'); + expect($save['row']['name'])->toBe('NPC - Perfdata - web1'); + expect($save['row']['input_string'])->toBe("php /plugins/npc/perfdata.php --type=host --id='12' 2> /dev/null"); + + expect($GLOBALS['__test_db_fetch_log'][0]['params'])->toBe(array('12')); + + $fields = array_values(array_filter($GLOBALS['__test_db_calls'], function ($call) { + return strpos($call['sql'], 'INSERT INTO data_input_fields') !== false; + })); + + expect(array_column(array_column($fields, 'params'), 2))->toBe(array('rta', 'pl')); +}); + +it('creates the data input method without fields when no check has been stored', function () { + $out = json_decode((new NpcCactiController())->addDataInputMethod(array('host' => 'web1', 'service' => 'Load', 'object_id' => '31')), true); + + expect($out['success'])->toBeTrue(); + expect($GLOBALS['__test_sql_save'][0]['row']['name'])->toBe('NPC - Perfdata - web1: Load'); + expect($GLOBALS['__test_sql_save'][0]['row']['input_string'])->toContain('--type=service --id='); + expect($GLOBALS['__test_db_calls'])->toBe(array()); +}); diff --git a/tests/bootstrap-unit.php b/tests/bootstrap-unit.php index 3977c50..fd86bca 100644 --- a/tests/bootstrap-unit.php +++ b/tests/bootstrap-unit.php @@ -389,6 +389,8 @@ function is_error_message() { if (!function_exists('sql_save')) { function sql_save($array, $table, $key = 'id') { + $GLOBALS['__test_sql_save'][] = array('table' => $table, 'row' => $array); + return isset($array['id']) ? $array['id'] : 1; } } From 12a6cff3a48c19bad3e38d46bb7e7100c43f3fac Mon Sep 17 00:00:00 2001 From: Thomas Vincent Date: Fri, 23 Oct 2026 21:18:21 -0600 Subject: [PATCH 06/41] fix: accept only known tab names in the NPC frame script Signed-off-by: Thomas Vincent --- CHANGELOG.md | 2 + controllers/layout.php | 15 ++++-- tests/Unit/LayoutControllerTest.php | 74 +++++++++++++++++++++++++++++ 3 files changed, 86 insertions(+), 5 deletions(-) create mode 100644 tests/Unit/LayoutControllerTest.php diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e75637..519c4e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,8 @@ --- develop --- +* security: Accept only known tab names for the `tab` request variable of the NPC + frame; the raw value was written into an inline script block * issue: Look up the stored perfdata with the plain object ID when creating a perfdata data input method; the shell-quoted value never matched, so no output fields were created diff --git a/controllers/layout.php b/controllers/layout.php index 3618c31..6533751 100644 --- a/controllers/layout.php +++ b/controllers/layout.php @@ -10,11 +10,6 @@ function drawFrame($params) { $npc_base = $config['url_path'] . 'plugins/npc/'; - $current_tab = 'dashboard'; - if (isset_request_var('tab')) { - $current_tab = get_request_var('tab'); - } - $tabs = array( 'dashboard' => __('Dashboard', 'npc'), 'hosts' => __('Hosts', 'npc'), @@ -25,6 +20,16 @@ function drawFrame($params) { 'process' => __('Process Info', 'npc'), ); + /* only a known tab name is ever written into the page */ + $current_tab = 'dashboard'; + if (isset_request_var('tab')) { + $requested_tab = get_request_var('tab'); + + if (is_string($requested_tab) && isset($tabs[$requested_tab])) { + $current_tab = $requested_tab; + } + } + /* draw the tabs */ print "