From 2fefc755523783ae3face92e356df1002ae4b1a1 Mon Sep 17 00:00:00 2001 From: Sukhwinder Dhillon Date: Mon, 17 Jun 2024 13:07:02 +0200 Subject: [PATCH 1/5] `PluginOutput::render()`: Shorten the output by characterLimit before processing it If the shorten output contained (not properly closed) HTML element, it merged the next list-item into the same html tag, because the closing tag was missing. So we therefor shorten the output by characterLimit before proccessing it --- library/Icingadb/Util/PluginOutput.php | 6 +- .../Icingadb/Util/PluginOutputTest.php | 128 ++++++++++++++++++ 2 files changed, 130 insertions(+), 4 deletions(-) create mode 100644 test/php/library/Icingadb/Util/PluginOutputTest.php diff --git a/library/Icingadb/Util/PluginOutput.php b/library/Icingadb/Util/PluginOutput.php index 03055a9ef..14d9cbf45 100644 --- a/library/Icingadb/Util/PluginOutput.php +++ b/library/Icingadb/Util/PluginOutput.php @@ -174,6 +174,8 @@ public function render() $output = PluginOutputHook::processOutput($output, $this->commandName, $this->enrichOutput); } + $output = substr($output, 0, $this->characterLimit); + if (preg_match('~<\w+(?>\s\w+=[^>]*)?>~', $output)) { // HTML $output = HtmlPurifier::process(preg_replace( @@ -202,10 +204,6 @@ public function render() $output = $this->processHtml($output); } - if ($this->characterLimit) { - $output = substr($output, 0, $this->characterLimit); - } - $this->renderedOutput = $output; return $output; diff --git a/test/php/library/Icingadb/Util/PluginOutputTest.php b/test/php/library/Icingadb/Util/PluginOutputTest.php new file mode 100644 index 000000000..821be98ae --- /dev/null +++ b/test/php/library/Icingadb/Util/PluginOutputTest.php @@ -0,0 +1,128 @@ +assertSame( + $expectedOutput, + (new PluginOutput($input))->render(), + 'PluginOutput::render does not return expected values' + ); + } + + public function testRenderTextWithStates() + { + $input = <<<'INPUT' +[OK] Dummy state + \_ [OK] Fake "state" + \_ [WARNING] Fake state again +INPUT; + + $expectedOutput = <<<'EXPECTED_OUTPUT' + Dummy state + \_ Fake "state" + \_ Fake state again +EXPECTED_OUTPUT; + + $this->assertSame( + $expectedOutput, + (new PluginOutput($input))->render(), + 'PluginOutput::render does not return expected values' + ); + } + + public function testRenderTextWithStatesAndCharacterLimit() + { + $input = <<<'INPUT' +[OK] Dummy state + \_ [OK] Fake "state" + \_ [WARNING] Fake state again +INPUT; + + $expectedOutput = <<<'EXPECTED_OUTPUT' + Dummy +EXPECTED_OUTPUT; + + $this->assertSame( + $expectedOutput, + (new PluginOutput($input))->setCharacterLimit(10)->render(), + 'PluginOutput::render does not return expected values' + ); + } + + public function testRenderTextWithHtml() + { + $input = <<<'INPUT' +Hello

World

, this "is" 'a test. +INPUT; + + $expectedOutput = <<<'EXPECTED_OUTPUT' +Hello

World

, this "is" 'a test. +EXPECTED_OUTPUT; + + $this->assertSame( + $expectedOutput, + (new PluginOutput($input))->render(), + 'PluginOutput::render does not return expected values' + ); + } + + public function testRenderTextWithHtmlAndStates() + { + $input = <<<'INPUT' +Hello

World

, this "is" a test. +[OK] Dummy state + \_ [OK] Fake "state" + \_ [WARNING] Fake state again +text ends here +INPUT; + + $expectedOutput = <<<'EXPECTED_OUTPUT' +Hello

World

, this "is" a test. + Dummy state + \_ Fake "state" + \_ Fake state again +text ends here +EXPECTED_OUTPUT; + + $this->assertSame( + $expectedOutput, + (new PluginOutput($input))->render(), + 'PluginOutput::render does not return expected values' + ); + } + + public function testRenderTextWithHtmlIncludingStatesAndSpecialChars() + { + $input = <<<'INPUT' +Hello

World

, this "is" a test. +[OK] Dummy state + special chars: !@#$%^&*()_+{}|:"<>?`-=[]\;',./ +text ends here +INPUT; + + $expectedOutput = <<<'EXPECTED_OUTPUT' +Hello

World

, this "is" a test. + Dummy state + special chars: !@#$%^&*()_+{}|:"<>?`-=[]\;',​./ +text ends here +EXPECTED_OUTPUT; + + $this->assertSame( + $expectedOutput, + (new PluginOutput($input))->render(), + 'PluginOutput::render does not return expected values' + ); + } +} From 91b3f81880525887efd51e128b98f8e1525ad119 Mon Sep 17 00:00:00 2001 From: Sukhwinder Dhillon Date: Tue, 23 Jul 2024 15:18:06 +0200 Subject: [PATCH 2/5] PluginOutput: Remove `processHtml()` method `preg_replace` is sufficient. --- library/Icingadb/Util/PluginOutput.php | 97 +++----------------------- 1 file changed, 10 insertions(+), 87 deletions(-) diff --git a/library/Icingadb/Util/PluginOutput.php b/library/Icingadb/Util/PluginOutput.php index 14d9cbf45..235258cdb 100644 --- a/library/Icingadb/Util/PluginOutput.php +++ b/library/Icingadb/Util/PluginOutput.php @@ -4,19 +4,14 @@ namespace Icinga\Module\Icingadb\Util; -use DOMDocument; -use DOMNode; -use DOMText; use Icinga\Module\Icingadb\Hook\PluginOutputHook; use Icinga\Module\Icingadb\Model\Host; use Icinga\Module\Icingadb\Model\Service; -use Icinga\Web\Dom\DomNodeIterator; use Icinga\Web\Helper\HtmlPurifier; use InvalidArgumentException; use ipl\Html\HtmlString; use ipl\Orm\Model; use LogicException; -use RecursiveIteratorIterator; class PluginOutput extends HtmlString { @@ -176,22 +171,22 @@ public function render() $output = substr($output, 0, $this->characterLimit); - if (preg_match('~<\w+(?>\s\w+=[^>]*)?>~', $output)) { - // HTML - $output = HtmlPurifier::process(preg_replace( - self::HTML_PATTERNS, - self::HTML_REPLACEMENTS, - $output - )); - $this->isHtml = true; + $this->isHtml = (bool) preg_match('~<\w+(?>\s\w+=[^>]*)?>~', $output); + + if ($this->isHtml) { + if ($this->enrichOutput) { + $output = preg_replace(self::TEXT_PATTERNS, self::TEXT_REPLACEMENTS, $output); + } else { + $output = preg_replace(self::HTML_PATTERNS, self::HTML_REPLACEMENTS, $output); + } + + $output = HtmlPurifier::process($output); } else { - // Plaintext $output = preg_replace( self::TEXT_PATTERNS, self::TEXT_REPLACEMENTS, htmlspecialchars($output, ENT_COMPAT | ENT_SUBSTITUTE | ENT_HTML5, null, false) ); - $this->isHtml = false; } $output = trim($output); @@ -200,80 +195,8 @@ public function render() // in oder to help browsers to break words in plugin output $output = preg_replace('/,(?=[^\s])/', ',​', $output); - if ($this->enrichOutput && $this->isHtml) { - $output = $this->processHtml($output); - } - $this->renderedOutput = $output; return $output; } - - /** - * Replace color state information, if any - * - * @param string $html - * - * @todo Do we really need to create a DOM here? Or is a preg_replace like we do it for text also feasible? - * @return string - */ - protected function processHtml(string $html): string - { - $pattern = '/[([](OK|WARNING|CRITICAL|UNKNOWN|UP|DOWN)[)\]]/'; - $doc = new DOMDocument(); - $doc->loadXML('
' . $html . '
', LIBXML_NOERROR | LIBXML_NOWARNING); - $dom = new RecursiveIteratorIterator(new DomNodeIterator($doc), RecursiveIteratorIterator::SELF_FIRST); - - $nodesToRemove = []; - foreach ($dom as $node) { - /** @var DOMNode $node */ - if ($node->nodeType !== XML_TEXT_NODE) { - continue; - } - - $start = 0; - while (preg_match($pattern, $node->nodeValue, $match, PREG_OFFSET_CAPTURE, $start)) { - $offsetLeft = $match[0][1]; - $matchLength = strlen($match[0][0]); - $leftLength = $offsetLeft - $start; - - // if there is text before the match - if ($leftLength) { - // create node for leading text - $text = new DOMText(substr($node->nodeValue, $start, $leftLength)); - $node->parentNode->insertBefore($text, $node); - } - - // create the state ball for the match - $span = $doc->createElement('span'); - $span->setAttribute( - 'class', - 'state-ball ball-size-m state-' . strtolower($match[1][0]) - ); - $node->parentNode->insertBefore($span, $node); - - // start for next match - $start = $offsetLeft + $matchLength; - } - - if ($start) { - // is there text left? - if (strlen($node->nodeValue) > $start) { - // create node for trailing text - $text = new DOMText(substr($node->nodeValue, $start)); - $node->parentNode->insertBefore($text, $node); - } - - // delete the old node later - $nodesToRemove[] = $node; - } - } - - foreach ($nodesToRemove as $node) { - /** @var DOMNode $node */ - $node->parentNode->removeChild($node); - } - - return substr($doc->saveHTML(), 5, -7); - } } From ef96037ec5645f043cb0647f04851d4532417b8c Mon Sep 17 00:00:00 2001 From: Sukhwinder Dhillon Date: Wed, 24 Jul 2024 10:40:08 +0200 Subject: [PATCH 3/5] PluginOutput: Add method return type and visibility to constants --- library/Icingadb/Util/PluginOutput.php | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/library/Icingadb/Util/PluginOutput.php b/library/Icingadb/Util/PluginOutput.php index 235258cdb..463ea36db 100644 --- a/library/Icingadb/Util/PluginOutput.php +++ b/library/Icingadb/Util/PluginOutput.php @@ -16,7 +16,7 @@ class PluginOutput extends HtmlString { /** @var string[] Patterns to be replaced in plain text plugin output */ - const TEXT_PATTERNS = [ + protected const TEXT_PATTERNS = [ '~\\\t~', '~\\\n~', '~(\[|\()OK(\]|\))~', @@ -29,7 +29,7 @@ class PluginOutput extends HtmlString ]; /** @var string[] Replacements for {@see PluginOutput::TEXT_PATTERNS} */ - const TEXT_REPLACEMENTS = [ + protected const TEXT_REPLACEMENTS = [ "\t", "\n", '', @@ -42,13 +42,13 @@ class PluginOutput extends HtmlString ]; /** @var string[] Patterns to be replaced in html plugin output */ - const HTML_PATTERNS = [ + protected const HTML_PATTERNS = [ '~\\\t~', '~\\\n~' ]; /** @var string[] Replacements for {@see PluginOutput::HTML_PATTERNS} */ - const HTML_REPLACEMENTS = [ + protected const HTML_REPLACEMENTS = [ "\t", "\n" ]; @@ -154,7 +154,7 @@ public static function fromObject(Model $object): self ->setCommandName($object->checkcommand_name); } - public function render() + public function render(): string { if ($this->renderedOutput !== null) { return $this->renderedOutput; From 929258c30f26e6a1bee87716c9eaafedfc661cab Mon Sep 17 00:00:00 2001 From: Sukhwinder Dhillon Date: Tue, 23 Jul 2024 15:21:28 +0200 Subject: [PATCH 4/5] PluginOutputTest: Migrate tests from monitoring/PluginOutputTest --- .../Icingadb/Util/PluginOutputTest.php | 147 +++++++++++++----- 1 file changed, 109 insertions(+), 38 deletions(-) diff --git a/test/php/library/Icingadb/Util/PluginOutputTest.php b/test/php/library/Icingadb/Util/PluginOutputTest.php index 821be98ae..80d9f8d76 100644 --- a/test/php/library/Icingadb/Util/PluginOutputTest.php +++ b/test/php/library/Icingadb/Util/PluginOutputTest.php @@ -9,19 +9,26 @@ class PluginOutputTest extends TestCase { - public function testRenderPlainText() + public function checkOutput(string $expected, string $input, int $characterLimit = 0): void + { + $p = new PluginOutput($input); + + if ($characterLimit) { + $p->setCharacterLimit($characterLimit); + } + + $this->assertSame($expected, $p->render(), 'PluginOutput::render does not return expected values'); + } + + + public function testRenderPlainText(): void { $input = 'This is a plain text'; - $expectedOutput = $input; - $this->assertSame( - $expectedOutput, - (new PluginOutput($input))->render(), - 'PluginOutput::render does not return expected values' - ); + $this->checkOutput($input, $input); } - public function testRenderTextWithStates() + public function testRenderTextWithStates(): void { $input = <<<'INPUT' [OK] Dummy state @@ -35,14 +42,10 @@ public function testRenderTextWithStates() \_ Fake state again EXPECTED_OUTPUT; - $this->assertSame( - $expectedOutput, - (new PluginOutput($input))->render(), - 'PluginOutput::render does not return expected values' - ); + $this->checkOutput($expectedOutput, $input); } - public function testRenderTextWithStatesAndCharacterLimit() + public function testRenderTextWithStatesAndCharacterLimit(): void { $input = <<<'INPUT' [OK] Dummy state @@ -54,14 +57,10 @@ public function testRenderTextWithStatesAndCharacterLimit() Dummy EXPECTED_OUTPUT; - $this->assertSame( - $expectedOutput, - (new PluginOutput($input))->setCharacterLimit(10)->render(), - 'PluginOutput::render does not return expected values' - ); + $this->checkOutput($expectedOutput, $input, 10); } - public function testRenderTextWithHtml() + public function testRenderTextWithHtml(): void { $input = <<<'INPUT' Hello

World

, this "is" 'a test. @@ -71,14 +70,10 @@ public function testRenderTextWithHtml() Hello

World

, this "is" 'a test. EXPECTED_OUTPUT; - $this->assertSame( - $expectedOutput, - (new PluginOutput($input))->render(), - 'PluginOutput::render does not return expected values' - ); + $this->checkOutput($expectedOutput, $input); } - public function testRenderTextWithHtmlAndStates() + public function testRenderTextWithHtmlAndStates(): void { $input = <<<'INPUT' Hello

World

, this "is" a test. @@ -96,14 +91,10 @@ public function testRenderTextWithHtmlAndStates() text ends here EXPECTED_OUTPUT; - $this->assertSame( - $expectedOutput, - (new PluginOutput($input))->render(), - 'PluginOutput::render does not return expected values' - ); + $this->checkOutput($expectedOutput, $input); } - public function testRenderTextWithHtmlIncludingStatesAndSpecialChars() + public function testRenderTextWithHtmlIncludingStatesAndSpecialChars(): void { $input = <<<'INPUT' Hello

World

, this "is" a test. @@ -115,14 +106,94 @@ public function testRenderTextWithHtmlIncludingStatesAndSpecialChars() $expectedOutput = <<<'EXPECTED_OUTPUT' Hello

World

, this "is" a test. Dummy state - special chars: !@#$%^&*()_+{}|:"<>?`-=[]\;',​./ + special chars: !@#$%^&*()_+{}|:"<>?`-=[]\;',​./ text ends here EXPECTED_OUTPUT; - $this->assertSame( - $expectedOutput, - (new PluginOutput($input))->render(), - 'PluginOutput::render does not return expected values' - ); + $this->checkOutput($expectedOutput, $input); + } + + public function testOutputWithNewlines(): void + { + $input = 'foo\nbar\n\nraboof'; + $expectedOutput = "foo\nbar\n\nraboof"; + + $this->checkOutput($expectedOutput, $input); + } + + public function testOutputWithHtmlEntities(): void + { + $input = 'foo & bar'; + $expectedOutput = $input; + + $this->checkOutput($expectedOutput, $input); + } + + public function testSimpleHtmlOutput(): void + { + $input = <<<'INPUT' +OK - Teststatus Info +INPUT; + + $expectedOutput = <<<'EXPECTED_OUTPUT' +OK - Teststatus Info +EXPECTED_OUTPUT; + + $this->checkOutput($expectedOutput, $input); + } + + public function testTextStatusTags(): void + { + foreach (['OK', 'WARNING', 'CRITICAL', 'UNKNOWN', 'UP', 'DOWN'] as $s) { + $l = strtolower($s); + + $input = sprintf('[%s] Test', $s); + $expectedOutput = sprintf(' Test', $l); + + $this->checkOutput($expectedOutput, $input); + + $input = sprintf('(%s) Test', $s); + + $this->checkOutput($expectedOutput, $input); + } + } + + public function testHtmlStatusTags(): void + { + $dummyHtml = ''; + + foreach (['OK', 'WARNING', 'CRITICAL', 'UNKNOWN', 'UP', 'DOWN'] as $s) { + $l = strtolower($s); + + $input = sprintf('%s [%s] Test', $dummyHtml, $s); + $expectedOutput = sprintf('%s Test', $dummyHtml, $l); + + $this->checkOutput($expectedOutput, $input); + + $input = sprintf('%s (%s) Test', $dummyHtml, $s); + + $this->checkOutput($expectedOutput, $input); + } + } + + public function testNewlineProcessingInHtmlOutput(): void + { + $input = 'This is plugin output\n\n
    \n
  • with a HTML list
    • \n
\n\n' + . 'and more text that\nis split onto multiple\n\nlines'; + + $expectedOutput = <<<'EXPECTED_OUTPUT' +This is plugin output + +
    +
  • with a HTML list
    • +
+ +and more text that +is split onto multiple + +lines +EXPECTED_OUTPUT; + + $this->checkOutput($expectedOutput, $input); } } From a40df7f0c20d4b5637c6e5c4d44cd3413dfb736e Mon Sep 17 00:00:00 2001 From: Sukhwinder Dhillon Date: Wed, 24 Jul 2024 11:12:26 +0200 Subject: [PATCH 5/5] Phpstan: Update baseline files --- phpstan-baseline-7x.neon | 10 ---------- phpstan-baseline-8x.neon | 10 ---------- phpstan-baseline-standard.neon | 34 +++++++--------------------------- 3 files changed, 7 insertions(+), 47 deletions(-) diff --git a/phpstan-baseline-7x.neon b/phpstan-baseline-7x.neon index 6da352556..95668ced4 100644 --- a/phpstan-baseline-7x.neon +++ b/phpstan-baseline-7x.neon @@ -80,11 +80,6 @@ parameters: count: 1 path: library/Icingadb/Util/PerfData.php - - - message: "#^Method Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PluginOutput\\:\\:render\\(\\) should return string but returns string\\|false\\|null\\.$#" - count: 1 - path: library/Icingadb/Util/PluginOutput.php - - message: "#^Parameter \\#1 \\$str of function trim expects string, string\\|null given\\.$#" count: 1 @@ -95,11 +90,6 @@ parameters: count: 1 path: library/Icingadb/Util/PluginOutput.php - - - message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PluginOutput\\:\\:\\$renderedOutput \\(string\\) does not accept string\\|false\\|null\\.$#" - count: 1 - path: library/Icingadb/Util/PluginOutput.php - - message: "#^Parameter \\#1 \\$str of function trim expects string, mixed given\\.$#" count: 1 diff --git a/phpstan-baseline-8x.neon b/phpstan-baseline-8x.neon index b5b85f7f9..d27ed7950 100644 --- a/phpstan-baseline-8x.neon +++ b/phpstan-baseline-8x.neon @@ -80,21 +80,11 @@ parameters: count: 1 path: library/Icingadb/Util/PerfData.php - - - message: "#^Method Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PluginOutput\\:\\:render\\(\\) should return string but returns string\\|null\\.$#" - count: 1 - path: library/Icingadb/Util/PluginOutput.php - - message: "#^Parameter \\#1 \\$string of function trim expects string, string\\|null given\\.$#" count: 1 path: library/Icingadb/Util/PluginOutput.php - - - message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PluginOutput\\:\\:\\$renderedOutput \\(string\\) does not accept string\\|null\\.$#" - count: 1 - path: library/Icingadb/Util/PluginOutput.php - - message: "#^Parameter \\#1 \\$string of function trim expects string, mixed given\\.$#" count: 1 diff --git a/phpstan-baseline-standard.neon b/phpstan-baseline-standard.neon index c974c5e11..cc744dfed 100644 --- a/phpstan-baseline-standard.neon +++ b/phpstan-baseline-standard.neon @@ -4936,17 +4936,7 @@ parameters: path: library/Icingadb/Util/PluginOutput.php - - message: "#^Cannot call method insertBefore\\(\\) on DOMNode\\|null\\.$#" - count: 3 - path: library/Icingadb/Util/PluginOutput.php - - - - message: "#^Cannot call method removeChild\\(\\) on DOMNode\\|null\\.$#" - count: 1 - path: library/Icingadb/Util/PluginOutput.php - - - - message: "#^Parameter \\#1 \\$html of method Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PluginOutput\\:\\:processHtml\\(\\) expects string, string\\|null given\\.$#" + message: "#^Method Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PluginOutput\\:\\:render\\(\\) should return string but returns string\\|null\\.$#" count: 1 path: library/Icingadb/Util/PluginOutput.php @@ -4956,22 +4946,7 @@ parameters: path: library/Icingadb/Util/PluginOutput.php - - message: "#^Parameter \\#1 \\$string of function strlen expects string, string\\|null given\\.$#" - count: 1 - path: library/Icingadb/Util/PluginOutput.php - - - - message: "#^Parameter \\#1 \\$string of function substr expects string, string\\|false given\\.$#" - count: 1 - path: library/Icingadb/Util/PluginOutput.php - - - - message: "#^Parameter \\#1 \\$string of function substr expects string, string\\|null given\\.$#" - count: 3 - path: library/Icingadb/Util/PluginOutput.php - - - - message: "#^Parameter \\#2 \\$subject of function preg_match expects string, string\\|null given\\.$#" + message: "#^Property Icinga\\\\Module\\\\Icingadb\\\\Util\\\\PluginOutput\\:\\:\\$renderedOutput \\(string\\) does not accept string\\|null\\.$#" count: 1 path: library/Icingadb/Util/PluginOutput.php @@ -6635,6 +6610,11 @@ parameters: count: 1 path: library/Icingadb/Widget/Detail/UserDetail.php + - + message: "#^Cannot access property \\$display_name on mixed\\.$#" + count: 1 + path: library/Icingadb/Widget/Detail/UserDetail.php + - message: "#^Cannot call method getModel\\(\\) on mixed\\.$#" count: 1