make sure that markdown uses safe mode (#2961)

kimai · Nov 19, 2021 · 76e0944 · 76e0944
1 parent 9470699
commit 76e0944
Show file tree

Hide file tree

Showing 4 changed files with 39 additions and 4 deletions.
diff --git a/src/Twig/Runtime/MarkdownExtension.php b/src/Twig/Runtime/MarkdownExtension.php
@@ -61,7 +61,7 @@ public function commentContent(?string $content, bool $fullLength = false): stri
         }
 
         if ($this->isMarkdownEnabled()) {
-            $content = $this->markdown->toHtml($content, false);
+            $content = $this->markdown->toHtml($content);
         } elseif ($fullLength) {
             $content = '<p>' . nl2br($content) . '</p>';
         }
@@ -112,7 +112,7 @@ public function timesheetContent(?string $content): string
         }
 
         if ($this->isMarkdownEnabled()) {
-            return $this->markdown->toHtml($content, false);
+            return $this->markdown->toHtml($content);
         }
 
         return nl2br($content);
@@ -126,6 +126,6 @@ public function timesheetContent(?string $content): string
      */
     public function markdownToHtml(string $content): string
     {
-        return $this->markdown->toHtml($content, false);
+        return $this->markdown->toHtml($content);
     }
 }
diff --git a/src/Utils/Markdown.php b/src/Utils/Markdown.php
@@ -33,7 +33,12 @@ public function __construct()
      */
     public function toHtml(string $text, bool $safe = true): string
     {
-        $this->parser->setSafeMode($safe);
+        if ($safe !== true) {
+            @trigger_error('Only safe mode is supported in Markdown since 1.16.3 to prevent XSS attacks. Parameter $safe will be removed with 2.0', E_USER_DEPRECATED);
+        }
+
+        $this->parser->setSafeMode(true);
+        $this->parser->setMarkupEscaped(true);
 
         return $this->parser->text($text);
     }

diff --git a/tests/Twig/Runtime/MarkdownExtensionTest.php b/tests/Twig/Runtime/MarkdownExtensionTest.php
@@ -27,6 +27,10 @@ public function testMarkdownToHtml()
         $sut = new MarkdownExtension(new Markdown(), $config);
         $this->assertEquals('<p><em>test</em></p>', $sut->markdownToHtml('*test*'));
         $this->assertEquals('<p># foobar</p>', $sut->markdownToHtml('# foobar'));
+        $this->assertEquals(
+            '<p><a href="javascript%3Aalert(`XSS`)">XSS</a></p>',
+            $sut->markdownToHtml('[XSS](javascript:alert(`XSS`))')
+        );
     }
 
     public function testTimesheetContent()
@@ -47,6 +51,10 @@ public function testTimesheetContent()
             "<ul>\n<li>test</li>\n<li>foo</li>\n</ul>\n<p>foo <strong>bar</strong></p>",
             $sut->timesheetContent("- test\n- foo\n\nfoo __bar__")
         );
+        $this->assertEquals(
+            '<p><a href="javascript%3Aalert(`XSS`)">XSS</a></p>',
+            $sut->timesheetContent('[XSS](javascript:alert(`XSS`))')
+        );
     }
 
     public function testCommentContent()
@@ -76,6 +84,10 @@ public function testCommentContent()
             "<ul>\n<li>test</li>\n<li>foo</li>\n</ul>\n<p>foo <strong>bar</strong></p>",
             $sut->commentContent("- test\n- foo\n\nfoo __bar__")
         );
+        $this->assertEquals(
+            '<p><a href="javascript%3Aalert(`XSS`)">XSS</a></p>',
+            $sut->commentContent('[XSS](javascript:alert(`XSS`))')
+        );
     }
 
     public function testCommentOneLiner()

diff --git a/tests/Utils/MarkdownTest.php b/tests/Utils/MarkdownTest.php
@@ -81,6 +81,24 @@ public function testDuplicateIds()
 ## test
 ### test
 # test
+EOT;
+        $this->assertEquals($html, $sut->toHtml($markdown));
+    }
+
+    public function testLinksAreSanitized()
+    {
+        $sut = new Markdown();
+
+        $html = <<<'EOT'
+<p><a href="javascript%3Aalert(`XSS`)">XSS</a><br />
+<a href="javascript%3Aalert(&quot;XSS&quot;)">XSS</a><br />
+<a href="javascript%3Aalert(&#039;XSS&#039;)">XSS</a></p>
+EOT;
+
+        $markdown = <<<EOT
+[XSS](javascript:alert(`XSS`))
+[XSS](javascript:alert("XSS"))
+[XSS](javascript:alert('XSS'))
 EOT;
         $this->assertEquals($html, $sut->toHtml($markdown));
     }