Merge commit from fork

Fix XSS vulnerability of `HtmlGenerator`
jgniecki · Oct 3, 2024 · b0ab9d6 · b0ab9d6
2 parents 0412f68 + 3f93a53
commit b0ab9d6
Show file tree

Hide file tree

Showing 2 changed files with 71 additions and 3 deletions.
diff --git a/src/Generator/HtmlGenerator.php b/src/Generator/HtmlGenerator.php
@@ -46,7 +46,12 @@ public function generate(MotdItemCollection $collection): string
 
             if ($motdItem->getColor()) {
                 if (str_contains($motdItem->getColor(), '#')) {
-                    $tags['span'][] = sprintf('color: %s;', $motdItem->getColor());
+                    // Only allow valid hex color codes (without alpha channel), such as #FFF and #000000.
+                    if(!preg_match('/^#(([0-9A-Fa-f]{2}){3}|[0-9A-Fa-f]{3})$/i', $motdItem->getColor())) {
+                        continue;
+                    }
+
+                    $tags['span'][] = sprintf('color: %s;', $this->escape($motdItem->getColor()));
                 } else {
                     $color = $this->colorCollection->get($motdItem->getColor());
                     if (!$color) {
@@ -77,7 +82,7 @@ public function generate(MotdItemCollection $collection): string
             foreach ($tags as $tag => $styles) {
                 $value = sprintf('<%s style="%s">%s</%s>', $tag, implode(' ', $styles), $value, $tag);
             }
-            $value = sprintf($value, $motdItem->getText());
+            $value = sprintf($value, $this->escape($motdItem->getText()));
             $result .= $value;
         }
 
@@ -88,4 +93,9 @@ public function setFormatNewLine(string $format): void
     {
         $this->formatNewLine = $format;
     }
+
+    private function escape(string $text): string
+    {
+        return htmlentities($text, ENT_QUOTES | ENT_HTML5, 'UTF-8');
+    }
 }
diff --git a/tests/Generator/HtmlGeneratorTest.php b/tests/Generator/HtmlGeneratorTest.php
@@ -275,4 +275,62 @@ public function testGenerateWithEmptyCollection()
 
         $this->assertEquals('', $result);
     }
-}
+
+    public function testFilterInvalidHexColor()
+    {
+        $collection = new MotdItemCollection();
+
+        $item1 = new MotdItem();
+        $item1->setText('Hello');
+        $item1->setColor('#FF555');
+        $collection->add($item1);
+
+        $item2 = new MotdItem();
+        $item2->setText("\n");
+        $collection->add($item2);
+
+        $item3 = new MotdItem();
+        $item3->setText('Beautiful');
+        $item3->setColor('#800080');
+        $collection->add($item3);
+
+        $item4 = new MotdItem();
+        $item4->setText("\n");
+        $collection->add($item4);
+
+        $item5 = new MotdItem();
+        $item5->setText('World');
+        $item5->setColor('#42');
+        $collection->add($item5);
+
+        $item6 = new MotdItem();
+        $item6->setText('!');
+        $item6->setColor('#42a');
+        $collection->add($item6);
+
+        $generator = new HtmlGenerator();
+        $result = $generator->generate($collection);
+
+        $this->assertEquals('<br /><span style="color: &num;800080;">Beautiful</span><br /><span style="color: &num;42a;">&excl;</span>', $result);
+    }
+
+    public function testEscapeInput()
+    {
+        $collection = new MotdItemCollection();
+
+        $item1 = new MotdItem();
+        $item1->setText('Hover me');
+        $item1->setColor('#000000" onmouseover="javascript:alert(\'XSS when mouse pointer enters the span element\')"');
+        $collection->add($item1);
+
+        $item2 = new MotdItem();
+        $item2->setText('<script>alert("XSS on page load")</script>');
+        $item2->setColor('#800080');
+        $collection->add($item2);
+
+        $generator = new HtmlGenerator();
+        $result = $generator->generate($collection);
+
+        $this->assertEquals('<span style="color: &num;800080;">&lt;script&gt;alert&lpar;&quot;XSS on page load&quot;&rpar;&lt;&sol;script&gt;</span>', $result);
+    }
+}