From 3fe17336dc4bc790ca60f5af4e47918fb7f58361 Mon Sep 17 00:00:00 2001 From: Johannes Meyer Date: Thu, 3 Aug 2023 14:40:46 +0200 Subject: [PATCH] Escape semicolons in node names fixes #312 --- application/forms/AddNodeForm.php | 6 +- application/forms/EditNodeForm.php | 7 +- library/Businessprocess/BpConfig.php | 75 ++++++++++++++++--- library/Businessprocess/BpNode.php | 3 +- library/Businessprocess/Common/EnumList.php | 15 ++-- library/Businessprocess/HostNode.php | 2 +- library/Businessprocess/ImportedNode.php | 8 +- .../Modification/NodeAddChildrenAction.php | 11 ++- .../ProvidedHook/Icingadb/HostActions.php | 3 +- .../ProvidedHook/Icingadb/ServiceActions.php | 5 +- .../ProvidedHook/Monitoring/HostActions.php | 3 +- .../Monitoring/ServiceActions.php | 5 +- library/Businessprocess/ServiceNode.php | 2 +- .../Businessprocess/State/IcingaDbState.php | 4 +- .../Businessprocess/State/MonitoringState.php | 12 +-- .../Storage/LegacyConfigParser.php | 16 ++-- .../processes/also-with-semicolons.conf | 8 ++ .../processes/with-semicolons.conf | 14 ++++ .../library/Businessprocess/BpConfigTest.php | 49 ++++++++++++ .../Storage/LegacyStorageTest.php | 37 +++++++++ 20 files changed, 223 insertions(+), 62 deletions(-) create mode 100644 test/config/modules/businessprocess/processes/also-with-semicolons.conf create mode 100644 test/config/modules/businessprocess/processes/with-semicolons.conf create mode 100644 test/php/library/Businessprocess/BpConfigTest.php diff --git a/application/forms/AddNodeForm.php b/application/forms/AddNodeForm.php index bab26bb..76880b6 100644 --- a/application/forms/AddNodeForm.php +++ b/application/forms/AddNodeForm.php @@ -3,6 +3,7 @@ namespace Icinga\Module\Businessprocess\Forms; use Exception; +use Icinga\Module\Businessprocess\BpConfig; use Icinga\Module\Businessprocess\BpNode; use Icinga\Module\Businessprocess\Common\EnumList; use Icinga\Module\Businessprocess\Common\Sort; @@ -498,10 +499,13 @@ class AddNodeForm extends BpConfigBaseForm case 'new-process': $properties = $this->getValues(); unset($properties['name']); + if (! $properties['alias']) { + unset($properties['alias']); + } if ($this->hasParentNode()) { $properties['parentName'] = $this->parent->getName(); } - $changes->createNode($this->getValue('name'), $properties); + $changes->createNode(BpConfig::escapeName($this->getValue('name')), $properties); break; } diff --git a/application/forms/EditNodeForm.php b/application/forms/EditNodeForm.php index aea064a..448f24f 100644 --- a/application/forms/EditNodeForm.php +++ b/application/forms/EditNodeForm.php @@ -2,6 +2,7 @@ namespace Icinga\Module\Businessprocess\Forms; +use Icinga\Module\Businessprocess\BpConfig; use Icinga\Module\Businessprocess\BpNode; use Icinga\Module\Businessprocess\Common\EnumList; use Icinga\Module\Businessprocess\Modification\ProcessChanges; @@ -29,9 +30,9 @@ class EditNodeForm extends BpConfigBaseForm public function setup() { - $this->host = substr($this->getNode()->getName(), 0, strpos($this->getNode()->getName(), ';')); - if ($this->isService()) { - $this->service = substr($this->getNode()->getName(), strpos($this->getNode()->getName(), ';') + 1); + [$this->host, $suffix] = BpConfig::splitNodeName($this->getNode()->getName()); + if ($suffix !== 'Hoststatus') { + $this->service = $suffix; } $view = $this->getView(); diff --git a/library/Businessprocess/BpConfig.php b/library/Businessprocess/BpConfig.php index 977186a..e99f81c 100644 --- a/library/Businessprocess/BpConfig.php +++ b/library/Businessprocess/BpConfig.php @@ -471,7 +471,7 @@ class BpConfig ) ); $node->setBpConfig($this); - $this->nodes[$host . ';' . $service] = $node; + $this->nodes[$node->getName()] = $node; $this->hosts[$host] = true; return $node; } @@ -480,7 +480,7 @@ class BpConfig { $node = new HostNode((object) array('hostname' => $host)); $node->setBpConfig($this); - $this->nodes[$host . ';Hoststatus'] = $node; + $this->nodes[$node->getName()] = $node; $this->hosts[$host] = true; return $node; } @@ -642,15 +642,13 @@ class BpConfig // Fallback: if it is a service, create an empty one: $this->warn(sprintf('The node "%s" doesn\'t exist', $name)); - $pos = strpos($name, ';'); - if ($pos !== false) { - $host = substr($name, 0, $pos); - $service = substr($name, $pos + 1); - // TODO: deactivated, this scares me, test it - if ($service === 'Hoststatus') { - return $this->createHost($host); + + [$name, $suffix] = self::splitNodeName($name); + if ($suffix !== null) { + if ($suffix === 'Hoststatus') { + return $this->createHost($name); } else { - return $this->createService($host, $service); + return $this->createService($name, $suffix); } } @@ -1015,4 +1013,61 @@ class BpConfig return $data; } + + /** + * Escape the given node name + * + * @param string $name + * + * @return string + */ + public static function escapeName(string $name): string + { + return preg_replace('/((?name = $object->name; + $this->name = BpConfig::escapeName($object->name); + $this->alias = BpConfig::unescapeName($object->name); $this->operator = $object->operator; $this->childNames = $object->child_names; } diff --git a/library/Businessprocess/Common/EnumList.php b/library/Businessprocess/Common/EnumList.php index 94747b2..3419505 100644 --- a/library/Businessprocess/Common/EnumList.php +++ b/library/Businessprocess/Common/EnumList.php @@ -4,6 +4,7 @@ namespace Icinga\Module\Businessprocess\Common; use Icinga\Application\Modules\Module; use Icinga\Data\Filter\Filter; +use Icinga\Module\Businessprocess\BpConfig; use Icinga\Module\Businessprocess\IcingaDbObject; use Icinga\Module\Businessprocess\MonitoringRestrictions; use Icinga\Module\Businessprocess\ProvidedHook\Icingadb\IcingadbSupport; @@ -51,9 +52,8 @@ trait EnumList // fetchPairs doesn't seem to work when using the same column with // different aliases twice $res = array(); - $suffix = ';Hoststatus'; foreach ($names as $name) { - $res[$name . $suffix] = $name; + $res[BpConfig::joinNodeName($name, 'Hoststatus')] = $name; } return $res; @@ -76,7 +76,7 @@ trait EnumList $services = array(); foreach ($names as $name) { - $services[$host . ';' . $name] = $name; + $services[BpConfig::joinNodeName($host, $name)] = $name; } return $services; @@ -100,9 +100,8 @@ trait EnumList // fetchPairs doesn't seem to work when using the same column with // different aliases twice $res = array(); - $suffix = ';Hoststatus'; foreach ($names as $name) { - $res[$name . $suffix] = $name; + $res[BpConfig::joinNodeName($name, 'Hoststatus')] = $name; } return $res; @@ -115,7 +114,8 @@ trait EnumList if ($this->useIcingaDbBackend()) { $objects = (new IcingaDbObject())->fetchServices($filter); foreach ($objects as $object) { - $services[$object->host->name . ';' . $object->name] = $object->host->name . ':' . $object->name; + $services[BpConfig::joinNodeName($object->host->name, $object->name)] + = $object->host->name . ':' . $object->name; } } else { $objects = $this->backend @@ -127,7 +127,8 @@ trait EnumList ->getQuery() ->fetchAll(); foreach ($objects as $object) { - $services[$object->host . ';' . $object->service] = $object->host . ':' . $object->service; + $services[BpConfig::joinNodeName($object->host, $object->service)] + = $object->host . ':' . $object->service; } } diff --git a/library/Businessprocess/HostNode.php b/library/Businessprocess/HostNode.php index b66f66f..ca3f796 100644 --- a/library/Businessprocess/HostNode.php +++ b/library/Businessprocess/HostNode.php @@ -35,7 +35,7 @@ class HostNode extends MonitoredNode public function __construct($object) { - $this->name = $object->hostname . ';Hoststatus'; + $this->name = BpConfig::joinNodeName($object->hostname, 'Hoststatus'); $this->hostname = $object->hostname; if (isset($object->state)) { $this->setState($object->state); diff --git a/library/Businessprocess/ImportedNode.php b/library/Businessprocess/ImportedNode.php index 3f0b460..e20b0a7 100644 --- a/library/Businessprocess/ImportedNode.php +++ b/library/Businessprocess/ImportedNode.php @@ -28,7 +28,7 @@ class ImportedNode extends BpNode { $this->parentBp = $parentBp; $this->configName = $object->configName; - $this->nodeName = $object->node; + $this->nodeName = BpConfig::escapeName($object->node); parent::__construct((object) [ 'name' => '@' . $this->configName . ':' . $this->nodeName, @@ -69,11 +69,7 @@ class ImportedNode extends BpNode public function getAlias() { - if ($this->alias === null) { - $this->alias = $this->importedNode()->getAlias(); - } - - return $this->alias; + return $this->importedNode()->getAlias(); } public function getOperator() diff --git a/library/Businessprocess/Modification/NodeAddChildrenAction.php b/library/Businessprocess/Modification/NodeAddChildrenAction.php index 5d5ab29..162c380 100644 --- a/library/Businessprocess/Modification/NodeAddChildrenAction.php +++ b/library/Businessprocess/Modification/NodeAddChildrenAction.php @@ -33,13 +33,12 @@ class NodeAddChildrenAction extends NodeAction foreach ($this->children as $name) { if (! $config->hasNode($name) || $config->getNode($name)->getBpConfig()->getName() !== $config->getName()) { - if (strpos($name, ';') !== false) { - list($host, $service) = preg_split('/;/', $name, 2); - - if ($service === 'Hoststatus') { - $config->createHost($host); + [$prefix, $suffix] = BpConfig::splitNodeName($name); + if ($suffix !== null) { + if ($suffix === 'Hoststatus') { + $config->createHost($prefix); } else { - $config->createService($host, $service); + $config->createService($prefix, $suffix); } } elseif ($name[0] === '@' && strpos($name, ':') !== false) { list($configName, $nodeName) = preg_split('~:\s*~', substr($name, 1), 2); diff --git a/library/Businessprocess/ProvidedHook/Icingadb/HostActions.php b/library/Businessprocess/ProvidedHook/Icingadb/HostActions.php index 27f4551..ac18959 100644 --- a/library/Businessprocess/ProvidedHook/Icingadb/HostActions.php +++ b/library/Businessprocess/ProvidedHook/Icingadb/HostActions.php @@ -2,6 +2,7 @@ namespace Icinga\Module\Businessprocess\ProvidedHook\Icingadb; +use Icinga\Module\Businessprocess\BpConfig; use Icinga\Module\Icingadb\Hook\HostActionsHook; use Icinga\Module\Icingadb\Model\Host; use ipl\Web\Widget\Link; @@ -15,7 +16,7 @@ class HostActions extends HostActionsHook new Link( $label, 'businessprocess/node/impact?name=' - . rawurlencode($host->name . ';Hoststatus') + . rawurlencode(BpConfig::joinNodeName($host->name, 'Hoststatus')) ) ); } diff --git a/library/Businessprocess/ProvidedHook/Icingadb/ServiceActions.php b/library/Businessprocess/ProvidedHook/Icingadb/ServiceActions.php index 24e6829..d416d90 100644 --- a/library/Businessprocess/ProvidedHook/Icingadb/ServiceActions.php +++ b/library/Businessprocess/ProvidedHook/Icingadb/ServiceActions.php @@ -2,6 +2,7 @@ namespace Icinga\Module\Businessprocess\ProvidedHook\Icingadb; +use Icinga\Module\Businessprocess\BpConfig; use Icinga\Module\Icingadb\Hook\ServiceActionsHook; use Icinga\Module\Icingadb\Model\Service; use ipl\Web\Widget\Link; @@ -16,9 +17,7 @@ class ServiceActions extends ServiceActionsHook $label, sprintf( 'businessprocess/node/impact?name=%s', - rawurlencode( - sprintf('%s;%s', $service->host->name, $service->name) - ) + rawurlencode(BpConfig::joinNodeName($service->host->name, $service->name)) ) ) ); diff --git a/library/Businessprocess/ProvidedHook/Monitoring/HostActions.php b/library/Businessprocess/ProvidedHook/Monitoring/HostActions.php index 57ce8f5..e2b9c59 100644 --- a/library/Businessprocess/ProvidedHook/Monitoring/HostActions.php +++ b/library/Businessprocess/ProvidedHook/Monitoring/HostActions.php @@ -2,6 +2,7 @@ namespace Icinga\Module\Businessprocess\ProvidedHook\Monitoring; +use Icinga\Module\Businessprocess\BpConfig; use Icinga\Module\Monitoring\Hook\HostActionsHook; use Icinga\Module\Monitoring\Object\Host; @@ -12,7 +13,7 @@ class HostActions extends HostActionsHook $label = mt('businessprocess', 'Business Impact'); return array( $label => 'businessprocess/node/impact?name=' - . rawurlencode($host->getName() . ';Hoststatus') + . rawurlencode(BpConfig::joinNodeName($host->getName(), 'Hoststatus')) ); } } diff --git a/library/Businessprocess/ProvidedHook/Monitoring/ServiceActions.php b/library/Businessprocess/ProvidedHook/Monitoring/ServiceActions.php index 69a93ae..ce9fabf 100644 --- a/library/Businessprocess/ProvidedHook/Monitoring/ServiceActions.php +++ b/library/Businessprocess/ProvidedHook/Monitoring/ServiceActions.php @@ -4,6 +4,7 @@ namespace Icinga\Module\Businessprocess\ProvidedHook\Monitoring; use Exception; use Icinga\Application\Config; +use Icinga\Module\Businessprocess\BpConfig; use Icinga\Module\Monitoring\Hook\ServiceActionsHook; use Icinga\Module\Monitoring\Object\Service; use Icinga\Web\Url; @@ -16,9 +17,7 @@ class ServiceActions extends ServiceActionsHook return array( $label => sprintf( 'businessprocess/node/impact?name=%s', - rawurlencode( - sprintf('%s;%s', $service->getHost()->getName(), $service->getName()) - ) + rawurlencode(BpConfig::joinNodeName($service->getHost()->getName(), $service->getName())) ) ); } diff --git a/library/Businessprocess/ServiceNode.php b/library/Businessprocess/ServiceNode.php index 53cef21..09df484 100644 --- a/library/Businessprocess/ServiceNode.php +++ b/library/Businessprocess/ServiceNode.php @@ -19,7 +19,7 @@ class ServiceNode extends MonitoredNode public function __construct($object) { - $this->name = $object->hostname . ';' . $object->service; + $this->name = BpConfig::joinNodeName($object->hostname, $object->service); $this->hostname = $object->hostname; $this->service = $object->service; if (isset($object->state)) { diff --git a/library/Businessprocess/State/IcingaDbState.php b/library/Businessprocess/State/IcingaDbState.php index 0eb0927..f145395 100644 --- a/library/Businessprocess/State/IcingaDbState.php +++ b/library/Businessprocess/State/IcingaDbState.php @@ -99,9 +99,9 @@ class IcingaDbState protected function handleDbRow($row, BpConfig $config, $objectName) { if ($objectName === 'service') { - $key = $row->host->name . ';' . $row->name; + $key = BpConfig::joinNodeName($row->host->name, $row->name); } else { - $key = $row->name . ';Hoststatus'; + $key = BpConfig::joinNodeName($row->name, 'Hoststatus'); } // We fetch more states than we need, so skip unknown ones diff --git a/library/Businessprocess/State/MonitoringState.php b/library/Businessprocess/State/MonitoringState.php index d317528..b6a2391 100644 --- a/library/Businessprocess/State/MonitoringState.php +++ b/library/Businessprocess/State/MonitoringState.php @@ -115,12 +115,12 @@ class MonitoringState protected function handleDbRow($row, BpConfig $config) { - $key = $row->hostname; - if (property_exists($row, 'service')) { - $key .= ';' . $row->service; - } else { - $key .= ';Hoststatus'; - } + $key = BpConfig::joinNodeName( + $row->hostname, + property_exists($row, 'service') + ? $row->service + : 'Hoststatus' + ); // We fetch more states than we need, so skip unknown ones if (! $config->hasNode($key)) { diff --git a/library/Businessprocess/Storage/LegacyConfigParser.php b/library/Businessprocess/Storage/LegacyConfigParser.php index 68cc1be..83c9240 100644 --- a/library/Businessprocess/Storage/LegacyConfigParser.php +++ b/library/Businessprocess/Storage/LegacyConfigParser.php @@ -230,7 +230,7 @@ class LegacyConfigParser */ protected function parseDisplay(&$line, BpConfig $bp) { - list($display, $name, $desc) = preg_split('~\s*;\s*~', substr($line, 8), 3); + list($display, $name, $desc) = preg_split('~\s*(?getBpNode($name)->setAlias($desc)->setDisplay($display); if ($display > 0) { $bp->addRootNode($name); @@ -239,7 +239,7 @@ class LegacyConfigParser protected function parseInfoUrl(&$line, BpConfig $bp) { - list($name, $url) = preg_split('~\s*;\s*~', substr($line, 9), 2); + list($name, $url) = preg_split('~\s*(?getBpNode($name)->setInfoUrl($url); } @@ -324,10 +324,6 @@ class LegacyConfigParser list($name, $value) = preg_split('~\s*=\s*~', $line, 2); - if (strpos($name, ';') !== false) { - $this->parseError('No semicolon allowed in varname'); - } - $op = '&'; if (preg_match_all('~(?hasNode($val)) { $node->addChild($bp->getNode($val)); } else { - list($host, $service) = preg_split('~;~', $val, 2); + list($host, $service) = preg_split('~(?addChild($bp->createHost($host)); + $node->addChild($bp->createHost(str_replace('\\;', ';', $host))); } else { - $node->addChild($bp->createService($host, $service)); + $node->addChild($bp->createService(str_replace('\\;', ';', $host), $service)); } } } elseif ($val[0] === '@') { diff --git a/test/config/modules/businessprocess/processes/also-with-semicolons.conf b/test/config/modules/businessprocess/processes/also-with-semicolons.conf new file mode 100644 index 0000000..a023aaf --- /dev/null +++ b/test/config/modules/businessprocess/processes/also-with-semicolons.conf @@ -0,0 +1,8 @@ +############################################ +# +# Title: Also With Semicolons +# +############################################ + +b\;ar = +display 1;b\;ar;Bar diff --git a/test/config/modules/businessprocess/processes/with-semicolons.conf b/test/config/modules/businessprocess/processes/with-semicolons.conf new file mode 100644 index 0000000..310d473 --- /dev/null +++ b/test/config/modules/businessprocess/processes/with-semicolons.conf @@ -0,0 +1,14 @@ +############################################ +# +# Title: With Semicolons +# +############################################ + +hostsAnd = host\;1;Hoststatus & host2;Hoststatus +servicesOr = host\;1;pi;ng | host2;ping | host3;ping +singleHost = host\;1;Hoststatus & to\;p & @also-with-semicolons:b\;ar +to\;p = hostsAnd & servicesOr & singleHost +display 1;to\;p;Top Node +info_url to\;p;https://top.example.com/ +no\;alias = +display 1;no\;alias;no;alias diff --git a/test/php/library/Businessprocess/BpConfigTest.php b/test/php/library/Businessprocess/BpConfigTest.php new file mode 100644 index 0000000..f42c58c --- /dev/null +++ b/test/php/library/Businessprocess/BpConfigTest.php @@ -0,0 +1,49 @@ +assertSame( + 'foo;bar', + BpConfig::joinNodeName('foo', 'bar') + ); + $this->assertSame( + 'foo\;bar', + BpConfig::joinNodeName('foo;bar') + ); + $this->assertSame( + 'foo\;bar;baroof', + BpConfig::joinNodeName('foo;bar', 'baroof') + ); + $this->assertSame( + 'foo\;bar;bar;oof', + BpConfig::joinNodeName('foo;bar', 'bar;oof') + ); + } + + public function testSplitNodeName() + { + $this->assertSame( + ['foo', 'bar'], + BpConfig::splitNodeName('foo;bar') + ); + $this->assertSame( + ['foo;bar', null], + BpConfig::splitNodeName('foo\;bar') + ); + $this->assertSame( + ['foo;bar', 'baroof'], + BpConfig::splitNodeName('foo\;bar;baroof') + ); + $this->assertSame( + ['foo;bar', 'bar;oof'], + BpConfig::splitNodeName('foo\;bar;bar;oof') + ); + } +} diff --git a/test/php/library/Businessprocess/Storage/LegacyStorageTest.php b/test/php/library/Businessprocess/Storage/LegacyStorageTest.php index b3be257..75bfcd5 100644 --- a/test/php/library/Businessprocess/Storage/LegacyStorageTest.php +++ b/test/php/library/Businessprocess/Storage/LegacyStorageTest.php @@ -2,6 +2,8 @@ namespace Tests\Icinga\Module\Businessprocess\Storage; +use Icinga\Module\Businessprocess\BpNode; +use Icinga\Module\Businessprocess\ImportedNode; use Icinga\Module\Businessprocess\Test\BaseTestCase; use Icinga\Module\Businessprocess\Storage\LegacyStorage; @@ -31,10 +33,12 @@ class LegacyStorageTest extends BaseTestCase $keys = array_keys($this->makeInstance()->listProcesses()); $this->assertEquals( array( + 'also-with-semicolons', 'broken_wrong-operator', 'combined', 'simple_with-header', 'simple_without-header', + 'with-semicolons' ), $keys ); @@ -45,10 +49,12 @@ class LegacyStorageTest extends BaseTestCase $keys = array_values($this->makeInstance()->listProcesses()); $this->assertEquals( array( + 'Also With Semicolons (also-with-semicolons)', 'broken_wrong-operator', 'combined', 'Simple with header (simple_with-header)', 'simple_without-header', + 'With Semicolons (with-semicolons)' ), $keys ); @@ -135,4 +141,35 @@ class LegacyStorageTest extends BaseTestCase $this->makeInstance()->loadProcess('combined') ); } + + public function testConfigsWithNodesThatHaveSemicolonsInTheirNameCanBeParsed() + { + $bp = $this->makeInstance()->loadProcess('with-semicolons'); + + $this->assertInstanceOf($this->processClass, $bp); + + $this->assertTrue($bp->hasNode('to\\;p')); + $this->assertSame( + 'https://top.example.com/', + $bp->getNode('to\\;p')->getInfoUrl() + ); + + $this->assertTrue($bp->hasNode('host\;1;Hoststatus')); + $this->assertSame('host;1', $bp->getNode('host\;1;Hoststatus')->getHostname()); + + $this->assertTrue($bp->hasNode('host\;1;pi;ng')); + $this->assertSame('host;1', $bp->getNode('host\;1;pi;ng')->getHostname()); + $this->assertSame('pi;ng', $bp->getNode('host\;1;pi;ng')->getServiceDescription()); + + $this->assertTrue($bp->hasNode('singleHost')); + $this->assertTrue($bp->getNode('singleHost')->hasChild('to\\;p')); + $this->assertInstanceOf(BpNode::class, $bp->getNode('to\\;p')); + + $this->assertInstanceOf(BpNode::class, $bp->getNode('no\\;alias')); + $this->assertSame('no;alias', $bp->getNode('no\\;alias')->getAlias()); + + $this->assertTrue($bp->hasNode('@also-with-semicolons:b\;ar')); + $this->assertTrue($bp->getNode('singleHost')->hasChild('@also-with-semicolons:b\;ar')); + $this->assertInstanceOf(ImportedNode::class, $bp->getNode('@also-with-semicolons:b\;ar')); + } }