Merge pull request #255 from mnapoli/html-input-escaping

Add a new `html_input` option
thephpleague · Jul 2, 2016 · 1675311 · 1675311
2 parents 1c51ea0 + 2326168
commit 1675311
Show file tree

Hide file tree

Showing 20 changed files with 355 additions and 4 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -4,6 +4,11 @@ Updates should follow the [Keep a CHANGELOG](http://keepachangelog.com/) princip
 
 ## [Unreleased][unreleased]
 
+### Added
+ - The `safe` option is deprecated and replaced by 2 new options (#253, #255):
+   - `html_input` (`strip`, `allow` or `escape`): how to handle untrusted HTML input (the default is `strip` for BC reasons)
+   - `allow_unsafe_links` (`true` or `false`): whether to allow risky image URLs and links (the default is `true` for BC reasons)
+
 ## [0.13.4] - 2016-06-14
 
 ### Fixed

diff --git a/README.md b/README.md
@@ -62,7 +62,7 @@ $environment = Environment::createCommonMarkEnvironment();
 // For example:  $environment->addInlineParser(new TwitterHandleParser());
 
 // Define your configuration:
-$config = ['safe' => true];
+$config = ['html_input' => 'escape'];
 
 // Create the converter
 $converter = new CommonMarkConverter($config, $environment);

diff --git a/src/Block/Renderer/HtmlBlockRenderer.php b/src/Block/Renderer/HtmlBlockRenderer.php
@@ -17,6 +17,7 @@
 use League\CommonMark\Block\Element\AbstractBlock;
 use League\CommonMark\Block\Element\HtmlBlock;
 use League\CommonMark\ElementRendererInterface;
+use League\CommonMark\Environment;
 use League\CommonMark\Util\Configuration;
 use League\CommonMark\Util\ConfigurationAwareInterface;
 
@@ -40,10 +41,19 @@ public function render(AbstractBlock $block, ElementRendererInterface $htmlRende
             throw new \InvalidArgumentException('Incompatible block type: ' . get_class($block));
         }
 
+        // Kept for BC reasons
         if ($this->config->getConfig('safe') === true) {
             return '';
         }
 
+        if ($this->config->getConfig('html_input') === Environment::HTML_INPUT_STRIP) {
+            return '';
+        }
+
+        if ($this->config->getConfig('html_input') === Environment::HTML_INPUT_ESCAPE) {
+            return htmlspecialchars($block->getStringContent(), ENT_NOQUOTES);
+        }
+
         return $block->getStringContent();
     }
 

diff --git a/src/Environment.php b/src/Environment.php
@@ -27,6 +27,10 @@
 
 class Environment
 {
+    const HTML_INPUT_STRIP = 'strip';
+    const HTML_INPUT_ESCAPE = 'escape';
+    const HTML_INPUT_ALLOW = 'allow';
+
     /**
      * @var ExtensionInterface[]
      */
@@ -478,7 +482,9 @@ public static function createCommonMarkEnvironment()
                 'inner_separator' => "\n",
                 'soft_break'      => "\n",
             ],
-            'safe' => false,
+            'safe'               => false, // deprecated option
+            'html_input'         => self::HTML_INPUT_ALLOW,
+            'allow_unsafe_links' => true,
         ]);
 
         return $environment;

diff --git a/src/Inline/Renderer/HtmlInlineRenderer.php b/src/Inline/Renderer/HtmlInlineRenderer.php
@@ -15,6 +15,7 @@
 namespace League\CommonMark\Inline\Renderer;
 
 use League\CommonMark\ElementRendererInterface;
+use League\CommonMark\Environment;
 use League\CommonMark\Inline\Element\AbstractInline;
 use League\CommonMark\Inline\Element\HtmlInline;
 use League\CommonMark\Util\Configuration;
@@ -39,10 +40,19 @@ public function render(AbstractInline $inline, ElementRendererInterface $htmlRen
             throw new \InvalidArgumentException('Incompatible inline type: ' . get_class($inline));
         }
 
+        // Kept for BC reasons
         if ($this->config->getConfig('safe') === true) {
             return '';
         }
 
+        if ($this->config->getConfig('html_input') === Environment::HTML_INPUT_STRIP) {
+            return '';
+        }
+
+        if ($this->config->getConfig('html_input') === Environment::HTML_INPUT_ESCAPE) {
+            return htmlspecialchars($inline->getContent(), ENT_NOQUOTES);
+        }
+
         return $inline->getContent();
     }
 

diff --git a/src/Inline/Renderer/ImageRenderer.php b/src/Inline/Renderer/ImageRenderer.php
@@ -46,7 +46,8 @@ public function render(AbstractInline $inline, ElementRendererInterface $htmlRen
             $attrs[$key] = $htmlRenderer->escape($value, true);
         }
 
-        if ($this->config->getConfig('safe') && RegexHelper::isLinkPotentiallyUnsafe($inline->getUrl())) {
+        $forbidUnsafeLinks = $this->config->getConfig('safe') || !$this->config->getConfig('allow_unsafe_links');
+        if ($forbidUnsafeLinks && RegexHelper::isLinkPotentiallyUnsafe($inline->getUrl())) {
             $attrs['src'] = '';
         } else {
             $attrs['src'] = $htmlRenderer->escape($inline->getUrl(), true);

diff --git a/src/Inline/Renderer/LinkRenderer.php b/src/Inline/Renderer/LinkRenderer.php
@@ -46,7 +46,8 @@ public function render(AbstractInline $inline, ElementRendererInterface $htmlRen
             $attrs[$key] = $htmlRenderer->escape($value, true);
         }
 
-        if (!($this->config->getConfig('safe') && RegexHelper::isLinkPotentiallyUnsafe($inline->getUrl()))) {
+        $forbidUnsafeLinks = $this->config->getConfig('safe') || !$this->config->getConfig('allow_unsafe_links');
+        if (!($forbidUnsafeLinks && RegexHelper::isLinkPotentiallyUnsafe($inline->getUrl()))) {
             $attrs['href'] = $htmlRenderer->escape($inline->getUrl(), true);
         }
 

diff --git a/tests/functional/HtmlInputTest.php b/tests/functional/HtmlInputTest.php
@@ -0,0 +1,53 @@
+<?php
+
+namespace League\CommonMark\Tests\Functional;
+
+use League\CommonMark\CommonMarkConverter;
+use League\CommonMark\Environment;
+
+class HtmlInputTest extends \PHPUnit_Framework_TestCase
+{
+    public function testDefaultConfig()
+    {
+        $input = file_get_contents(__DIR__ . '/data/html_input/input.md');
+        $expectedOutput = trim(file_get_contents(__DIR__ . '/data/html_input/unsafe_output.html'));
+
+        $converter = new CommonMarkConverter();
+        $actualOutput = trim($converter->convertToHtml($input));
+
+        $this->assertEquals($expectedOutput, $actualOutput);
+    }
+
+    public function testAllowHtmlInputConfig()
+    {
+        $input = file_get_contents(__DIR__ . '/data/html_input/input.md');
+        $expectedOutput = trim(file_get_contents(__DIR__ . '/data/html_input/unsafe_output.html'));
+
+        $converter = new CommonMarkConverter(['html_input' => Environment::HTML_INPUT_ALLOW]);
+        $actualOutput = trim($converter->convertToHtml($input));
+
+        $this->assertEquals($expectedOutput, $actualOutput);
+    }
+
+    public function testEscapeHtmlInputConfig()
+    {
+        $input = file_get_contents(__DIR__ . '/data/html_input/input.md');
+        $expectedOutput = trim(file_get_contents(__DIR__ . '/data/html_input/escaped_output.html'));
+
+        $converter = new CommonMarkConverter(['html_input' => Environment::HTML_INPUT_ESCAPE]);
+        $actualOutput = trim($converter->convertToHtml($input));
+
+        $this->assertEquals($expectedOutput, $actualOutput);
+    }
+
+    public function testStripHtmlInputConfig()
+    {
+        $input = file_get_contents(__DIR__ . '/data/html_input/input.md');
+        $expectedOutput = trim(file_get_contents(__DIR__ . '/data/html_input/safe_output.html'));
+
+        $converter = new CommonMarkConverter(['html_input' => Environment::HTML_INPUT_STRIP]);
+        $actualOutput = trim($converter->convertToHtml($input));
+
+        $this->assertEquals($expectedOutput, $actualOutput);
+    }
+}
diff --git a/tests/functional/SafeLinksTest.php b/tests/functional/SafeLinksTest.php
@@ -0,0 +1,30 @@
+<?php
+
+namespace League\CommonMark\Tests\Functional;
+
+use League\CommonMark\CommonMarkConverter;
+
+class SafeLinksTest extends \PHPUnit_Framework_TestCase
+{
+    public function testDefaultConfig()
+    {
+        $input = file_get_contents(__DIR__ . '/data/safe_links/input.md');
+        $expectedOutput = trim(file_get_contents(__DIR__ . '/data/safe_links/unsafe_output.html'));
+
+        $converter = new CommonMarkConverter();
+        $actualOutput = trim($converter->convertToHtml($input));
+
+        $this->assertEquals($expectedOutput, $actualOutput);
+    }
+
+    public function testSafeConfig()
+    {
+        $input = file_get_contents(__DIR__ . '/data/safe_links/input.md');
+        $expectedOutput = trim(file_get_contents(__DIR__ . '/data/safe_links/safe_output.html'));
+
+        $converter = new CommonMarkConverter(['allow_unsafe_links' => false]);
+        $actualOutput = trim($converter->convertToHtml($input));
+
+        $this->assertEquals($expectedOutput, $actualOutput);
+    }
+}
diff --git a/tests/functional/data/html_input/escaped_output.html b/tests/functional/data/html_input/escaped_output.html
@@ -0,0 +1,5 @@
+&lt;div onclick="alert('XSS')"&gt;click me&lt;/div&gt;
+&lt;script&gt;alert("XSS")&lt;/script&gt;
+&lt;script&gt;
+    alert("XSS");
+&lt;/script&gt;
diff --git a/tests/functional/data/html_input/input.md b/tests/functional/data/html_input/input.md
@@ -0,0 +1,7 @@
+<div onclick="alert('XSS')">click me</div>
+
+<script>alert("XSS")</script>
+
+<script>
+    alert("XSS");
+</script>
diff --git a/tests/functional/data/html_input/safe_output.html b/tests/functional/data/html_input/safe_output.html
@@ -0,0 +1,2 @@
+
+
diff --git a/tests/functional/data/html_input/unsafe_output.html b/tests/functional/data/html_input/unsafe_output.html
@@ -0,0 +1,5 @@
+<div onclick="alert('XSS')">click me</div>
+<script>alert("XSS")</script>
+<script>
+    alert("XSS");
+</script>
diff --git a/tests/functional/data/safe_links/input.md b/tests/functional/data/safe_links/input.md
@@ -0,0 +1,5 @@
+[Click me](javascript:alert('XSS'))
+
+<javascript:alert("XSS")>
+
+![Inline image](data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7)
diff --git a/tests/functional/data/safe_links/safe_output.html b/tests/functional/data/safe_links/safe_output.html
@@ -0,0 +1,3 @@
+<p><a>Click me</a></p>
+<p><a>javascript:alert(&quot;XSS&quot;)</a></p>
+<p><img src="data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7" alt="Inline image" /></p>
diff --git a/tests/functional/data/safe_links/unsafe_output.html b/tests/functional/data/safe_links/unsafe_output.html
@@ -0,0 +1,3 @@
+<p><a href="javascript:alert('XSS')">Click me</a></p>
+<p><a href="javascript:alert(%22XSS%22)">javascript:alert(&quot;XSS&quot;)</a></p>
+<p><img src="data:image/gif;base64,R0lGODlhEAAQAMQAAORHHOVSKudfOulrSOp3WOyDZu6QdvCchPGolfO0o/XBs/fNwfjZ0frl3/zy7////wAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAACH5BAkAABAALAAAAAAQABAAAAVVICSOZGlCQAosJ6mu7fiyZeKqNKToQGDsM8hBADgUXoGAiqhSvp5QAnQKGIgUhwFUYLCVDFCrKUE1lBavAViFIDlTImbKC5Gm2hB0SlBCBMQiB0UjIQA7" alt="Inline image" /></p>
diff --git a/tests/unit/Block/Renderer/HtmlBlockRendererTest.php b/tests/unit/Block/Renderer/HtmlBlockRendererTest.php
@@ -16,6 +16,7 @@
 
 use League\CommonMark\Block\Element\HtmlBlock;
 use League\CommonMark\Block\Renderer\HtmlBlockRenderer;
+use League\CommonMark\Environment;
 use League\CommonMark\Tests\Unit\FakeHtmlRenderer;
 use League\CommonMark\Util\Configuration;
 
@@ -72,6 +73,72 @@ public function testRenderSafeMode()
         $this->assertEquals('', $result);
     }
 
+    public function testRenderAllowHtml()
+    {
+        $this->renderer->setConfiguration(new Configuration([
+            'html_input' => Environment::HTML_INPUT_ALLOW,
+        ]));
+
+        /** @var HtmlBlock|\PHPUnit_Framework_MockObject_MockObject $block */
+        $block = $this->getMockBuilder('League\CommonMark\Block\Element\HtmlBlock')
+            ->setConstructorArgs([HtmlBlock::TYPE_6_BLOCK_ELEMENT])
+            ->getMock();
+        $block->expects($this->any())
+            ->method('getStringContent')
+            ->will($this->returnValue('<button>Test</button>'));
+
+        $fakeRenderer = new FakeHtmlRenderer();
+
+        $result = $this->renderer->render($block, $fakeRenderer);
+
+        $this->assertInternalType('string', $result);
+        $this->assertContains('<button>Test</button>', $result);
+    }
+
+    public function testRenderEscapeHtml()
+    {
+        $this->renderer->setConfiguration(new Configuration([
+            'html_input' => Environment::HTML_INPUT_ESCAPE,
+        ]));
+
+        /** @var HtmlBlock|\PHPUnit_Framework_MockObject_MockObject $block */
+        $block = $this->getMockBuilder('League\CommonMark\Block\Element\HtmlBlock')
+            ->setConstructorArgs([HtmlBlock::TYPE_6_BLOCK_ELEMENT])
+            ->getMock();
+        $block->expects($this->any())
+            ->method('getStringContent')
+            ->will($this->returnValue('<button class="test">Test</button>'));
+
+        $fakeRenderer = new FakeHtmlRenderer();
+
+        $result = $this->renderer->render($block, $fakeRenderer);
+
+        $this->assertInternalType('string', $result);
+        $this->assertContains('&lt;button class="test"&gt;Test&lt;/button&gt;', $result);
+    }
+
+    public function testRenderStripHtml()
+    {
+        $this->renderer->setConfiguration(new Configuration([
+            'html_input' => Environment::HTML_INPUT_STRIP,
+        ]));
+
+        /** @var HtmlBlock|\PHPUnit_Framework_MockObject_MockObject $block */
+        $block = $this->getMockBuilder('League\CommonMark\Block\Element\HtmlBlock')
+            ->setConstructorArgs([HtmlBlock::TYPE_6_BLOCK_ELEMENT])
+            ->getMock();
+        $block->expects($this->any())
+            ->method('getStringContent')
+            ->will($this->returnValue('<button>Test</button>'));
+
+        $fakeRenderer = new FakeHtmlRenderer();
+
+        $result = $this->renderer->render($block, $fakeRenderer);
+
+        $this->assertInternalType('string', $result);
+        $this->assertEquals('', $result);
+    }
+
     /**
      * @expectedException \InvalidArgumentException
      */

diff --git a/tests/unit/Inline/Renderer/HtmlInlineRendererTest.php b/tests/unit/Inline/Renderer/HtmlInlineRendererTest.php
@@ -14,6 +14,7 @@
 
 namespace League\CommonMark\Tests\Unit\Inline\Renderer;
 
+use League\CommonMark\Environment;
 use League\CommonMark\Inline\Element\HtmlInline;
 use League\CommonMark\Inline\Renderer\HtmlInlineRenderer;
 use League\CommonMark\Tests\Unit\FakeHtmlRenderer;
@@ -58,6 +59,51 @@ public function testRenderSafeMode()
         $this->assertEquals('', $result);
     }
 
+    public function testRenderAllowHtml()
+    {
+        $this->renderer->setConfiguration(new Configuration([
+            'html_input' => Environment::HTML_INPUT_ALLOW,
+        ]));
+
+        $inline = new HtmlInline('<h1>Test</h1>');
+        $fakeRenderer = new FakeHtmlRenderer();
+
+        $result = $this->renderer->render($inline, $fakeRenderer);
+
+        $this->assertInternalType('string', $result);
+        $this->assertContains('<h1>Test</h1>', $result);
+    }
+
+    public function testRenderEscapeHtml()
+    {
+        $this->renderer->setConfiguration(new Configuration([
+            'html_input' => Environment::HTML_INPUT_ESCAPE,
+        ]));
+
+        $inline = new HtmlInline('<h1 class="test">Test</h1>');
+        $fakeRenderer = new FakeHtmlRenderer();
+
+        $result = $this->renderer->render($inline, $fakeRenderer);
+
+        $this->assertInternalType('string', $result);
+        $this->assertContains('&lt;h1 class="test"&gt;Test&lt;/h1&gt;', $result);
+    }
+
+    public function testRenderStripHtml()
+    {
+        $this->renderer->setConfiguration(new Configuration([
+            'html_input' => Environment::HTML_INPUT_STRIP,
+        ]));
+
+        $inline = new HtmlInline('<h1>Test</h1>');
+        $fakeRenderer = new FakeHtmlRenderer();
+
+        $result = $this->renderer->render($inline, $fakeRenderer);
+
+        $this->assertInternalType('string', $result);
+        $this->assertEquals('', $result);
+    }
+
     /**
      * @expectedException \InvalidArgumentException
      */