Merge pull request #785 from ArthurHoaro/hotfix/markdown-html

Add markdown_escape setting

6 files changed, 181 insertions, 27 deletions

diff --git a/application/Updater.php b/application/Updater.php
index 3f5d325b..f5ebf31a 100644
--- a/application/Updater.php
+++ b/application/Updater.php
@@ -336,6 +336,29 @@ class Updater
         }
         $this->conf->set('resource.theme', 'vintage');
         $this->conf->write($this->isLoggedIn);
+
+        return true;
+    }
+
+    /**
+     * * `markdown_escape` is a new setting, set to true as default.
+     *
+     * If the markdown plugin was already enabled, escaping is disabled to avoid
+     * breaking existing entries.
+     */
+    public function updateMethodEscapeMarkdown()
+    {
+        if ($this->conf->exists('security.markdown_escape')) {
+            return true;
+        }
+
+        if (in_array('markdown', $this->conf->get('general.enabled_plugins'))) {
+            $this->conf->set('security.markdown_escape', false);
+        } else {
+            $this->conf->set('security.markdown_escape', true);
+        }
+        $this->conf->write($this->isLoggedIn);
+
         return true;
     }
 }
diff --git a/plugins/markdown/README.md b/plugins/markdown/README.md
index aafcf066..bc9427e2 100644
--- a/plugins/markdown/README.md
+++ b/plugins/markdown/README.md
@@ -50,9 +50,20 @@ If the tag `nomarkdown` is set for a shaare, it won't be converted to Markdown s
  
 > Note: this is a special tag, so it won't be displayed in link list.
 
-### HTML rendering
+### HTML escape
 
-Markdown support HTML tags. For example:
+By default, HTML tags are escaped. You can enable HTML tags rendering
+by setting `security.markdwon_escape` to `false` in `data/config.json.php`:
+
+```json
+{
+  "security": {
+    "markdown_escape": false
+  }
+}
+```
+
+With this setting, Markdown support HTML tags. For example:
 
     > <strong>strong</strong><strike>strike</strike>
    
@@ -60,12 +71,14 @@ Will render as:
 
 > <strong>strong</strong><strike>strike</strike>
 
-If you want to shaare HTML code, it is necessary to use inline code or code blocks.
-  
-**If your shaared descriptions containing HTML tags before enabling the markdown plugin, 
-enabling it might break your page.**
 
-> Note: HTML tags such as script, iframe, etc. are disabled for security reasons.
+**Warning:**
+
+  * This setting might present **security risks** (XSS) on shared instances, even though tags 
+  such as script, iframe, etc should be disabled.
+  * If you want to shaare HTML code, it is necessary to use inline code or code blocks.
+  * If your shaared descriptions contained HTML tags before enabling the markdown plugin, 
+enabling it might break your page.
 
 ### Known issue
 
diff --git a/plugins/markdown/markdown.php b/plugins/markdown/markdown.php
index 0cf6e6e2..de7c823d 100644
--- a/plugins/markdown/markdown.php
+++ b/plugins/markdown/markdown.php
@@ -14,18 +14,19 @@ define('NO_MD_TAG', 'nomarkdown');
 /**
  * Parse linklist descriptions.
  *
- * @param array $data linklist data.
+ * @param array         $data linklist data.
+ * @param ConfigManager $conf instance.
  *
  * @return mixed linklist data parsed in markdown (and converted to HTML).
  */
-function hook_markdown_render_linklist($data)
+function hook_markdown_render_linklist($data, $conf)
 {
     foreach ($data['links'] as &$value) {
         if (!empty($value['tags']) && noMarkdownTag($value['tags'])) {
             $value = stripNoMarkdownTag($value);
             continue;
         }
-        $value['description'] = process_markdown($value['description']);
+        $value['description'] = process_markdown($value['description'], $conf->get('security.markdown_escape', true));
     }
     return $data;
 }
@@ -34,17 +35,18 @@ function hook_markdown_render_linklist($data)
  * Parse feed linklist descriptions.
  *
  * @param array $data linklist data.
+ * @param ConfigManager $conf instance.
  *
  * @return mixed linklist data parsed in markdown (and converted to HTML).
  */
-function hook_markdown_render_feed($data)
+function hook_markdown_render_feed($data, $conf)
 {
     foreach ($data['links'] as &$value) {
         if (!empty($value['tags']) && noMarkdownTag($value['tags'])) {
             $value = stripNoMarkdownTag($value);
             continue;
         }
-        $value['description'] = process_markdown($value['description']);
+        $value['description'] = process_markdown($value['description'], $conf->get('security.markdown_escape', true));
     }
 
     return $data;
@@ -53,11 +55,12 @@ function hook_markdown_render_feed($data)
 /**
  * Parse daily descriptions.
  *
- * @param array $data daily data.
+ * @param array         $data daily data.
+ * @param ConfigManager $conf instance.
  *
  * @return mixed daily data parsed in markdown (and converted to HTML).
  */
-function hook_markdown_render_daily($data)
+function hook_markdown_render_daily($data, $conf)
 {
     // Manipulate columns data
     foreach ($data['cols'] as &$value) {
@@ -66,7 +69,10 @@ function hook_markdown_render_daily($data)
                 $value2 = stripNoMarkdownTag($value2);
                 continue;
             }
-            $value2['formatedDescription'] = process_markdown($value2['formatedDescription']);
+            $value2['formatedDescription'] = process_markdown(
+                $value2['formatedDescription'],
+                $conf->get('security.markdown_escape', true)
+            );
         }
     }
 
@@ -250,7 +256,7 @@ function sanitize_html($description)
             $description);
     }
     $description = preg_replace(
-        '#(<[^>]+)on[a-z]*="[^"]*"#is',
+        '#(<[^>]+)on[a-z]*="?[^ "]*"?#is',
         '$1',
         $description);
     return $description;
@@ -265,10 +271,11 @@ function sanitize_html($description)
  *   5. Wrap description in 'markdown' CSS class.
  *
  * @param string $description input description text.
+ * @param bool   $escape      escape HTML entities
  *
  * @return string HTML processed $description.
  */
-function process_markdown($description)
+function process_markdown($description, $escape = true)
 {
     $parsedown = new Parsedown();
 
@@ -278,7 +285,7 @@ function process_markdown($description)
     $processedDescription = reverse_text2clickable($processedDescription);
     $processedDescription = unescape($processedDescription);
     $processedDescription = $parsedown
-        ->setMarkupEscaped(false)
+        ->setMarkupEscaped($escape)
         ->setBreaksEnabled(true)
         ->text($processedDescription);
     $processedDescription = sanitize_html($processedDescription);
diff --git a/tests/Updater/UpdaterTest.php b/tests/Updater/UpdaterTest.php
index de330ae2..39be88f9 100644
--- a/tests/Updater/UpdaterTest.php
+++ b/tests/Updater/UpdaterTest.php
@@ -506,4 +506,70 @@ $GLOBALS[\'privateLinkByDefault\'] = true;';
         $this->conf = new ConfigManager($sandboxConf);
         $this->assertEquals($theme, $this->conf->get('resource.theme'));
     }
+
+    /**
+     * Test updateMethodEscapeMarkdown with markdown plugin enabled
+     * => setting markdown_escape set to false.
+     */
+    public function testEscapeMarkdownSettingToFalse()
+    {
+        $sandboxConf = 'sandbox/config';
+        copy(self::$configFile . '.json.php', $sandboxConf . '.json.php');
+        $this->conf = new ConfigManager($sandboxConf);
+
+        $this->conf->set('general.enabled_plugins', ['markdown']);
+        $updater = new Updater([], [], $this->conf, true);
+        $this->assertTrue($updater->updateMethodEscapeMarkdown());
+        $this->assertFalse($this->conf->get('security.markdown_escape'));
+
+        // reload from file
+        $this->conf = new ConfigManager($sandboxConf);
+        $this->assertFalse($this->conf->get('security.markdown_escape'));
+    }
+
+
+    /**
+     * Test updateMethodEscapeMarkdown with markdown plugin disabled
+     * => setting markdown_escape set to true.
+     */
+    public function testEscapeMarkdownSettingToTrue()
+    {
+        $sandboxConf = 'sandbox/config';
+        copy(self::$configFile . '.json.php', $sandboxConf . '.json.php');
+        $this->conf = new ConfigManager($sandboxConf);
+
+        $this->conf->set('general.enabled_plugins', []);
+        $updater = new Updater([], [], $this->conf, true);
+        $this->assertTrue($updater->updateMethodEscapeMarkdown());
+        $this->assertTrue($this->conf->get('security.markdown_escape'));
+
+        // reload from file
+        $this->conf = new ConfigManager($sandboxConf);
+        $this->assertTrue($this->conf->get('security.markdown_escape'));
+    }
+
+    /**
+     * Test updateMethodEscapeMarkdown with nothing to do (setting already enabled)
+     */
+    public function testEscapeMarkdownSettingNothingToDoEnabled()
+    {
+        $sandboxConf = 'sandbox/config';
+        copy(self::$configFile . '.json.php', $sandboxConf . '.json.php');
+        $this->conf = new ConfigManager($sandboxConf);
+        $this->conf->set('security.markdown_escape', true);
+        $updater = new Updater([], [], $this->conf, true);
+        $this->assertTrue($updater->updateMethodEscapeMarkdown());
+        $this->assertTrue($this->conf->get('security.markdown_escape'));
+    }
+
+    /**
+     * Test updateMethodEscapeMarkdown with nothing to do (setting already disabled)
+     */
+    public function testEscapeMarkdownSettingNothingToDoDisabled()
+    {
+        $this->conf->set('security.markdown_escape', false);
+        $updater = new Updater([], [], $this->conf, true);
+        $this->assertTrue($updater->updateMethodEscapeMarkdown());
+        $this->assertFalse($this->conf->get('security.markdown_escape'));
+    }
 }
diff --git a/tests/plugins/PluginMarkdownTest.php b/tests/plugins/PluginMarkdownTest.php
index d359b2a1..d4cd1b97 100644
--- a/tests/plugins/PluginMarkdownTest.php
+++ b/tests/plugins/PluginMarkdownTest.php
@@ -14,11 +14,17 @@ require_once 'plugins/markdown/markdown.php';
 class PluginMarkdownTest extends PHPUnit_Framework_TestCase
 {
     /**
+     * @var ConfigManager instance.
+     */
+    protected $conf;
+
+    /**
      * Reset plugin path
      */
     public function setUp()
     {
         PluginManager::$PLUGINS_PATH = 'plugins';
+        $this->conf = new ConfigManager('tests/utils/config/configJson');
     }
 
     /**
@@ -36,7 +42,7 @@ class PluginMarkdownTest extends PHPUnit_Framework_TestCase
             ),
         );
 
-        $data = hook_markdown_render_linklist($data);
+        $data = hook_markdown_render_linklist($data, $this->conf);
         $this->assertNotFalse(strpos($data['links'][0]['description'], '<h1>'));
         $this->assertNotFalse(strpos($data['links'][0]['description'], '<p>'));
     }
@@ -61,7 +67,7 @@ class PluginMarkdownTest extends PHPUnit_Framework_TestCase
             ),
         );
 
-        $data = hook_markdown_render_daily($data);
+        $data = hook_markdown_render_daily($data, $this->conf);
         $this->assertNotFalse(strpos($data['cols'][0][0]['formatedDescription'], '<h1>'));
         $this->assertNotFalse(strpos($data['cols'][0][0]['formatedDescription'], '<p>'));
     }
@@ -110,6 +116,8 @@ class PluginMarkdownTest extends PHPUnit_Framework_TestCase
         $output = escape($input);
         $input .= '<a href="#" onmouseHover="alert(\'xss\');" attr="tt">link</a>';
         $output .= '<a href="#"  attr="tt">link</a>';
+        $input .= '<a href="#" onmouseHover=alert(\'xss\'); attr="tt">link</a>';
+        $output .= '<a href="#"  attr="tt">link</a>';
         $this->assertEquals($output, sanitize_html($input));
         // Do not touch escaped HTML.
         $input = escape($input);
@@ -130,10 +138,10 @@ class PluginMarkdownTest extends PHPUnit_Framework_TestCase
             ))
         );
 
-        $processed = hook_markdown_render_linklist($data);
+        $processed = hook_markdown_render_linklist($data, $this->conf);
         $this->assertEquals($str, $processed['links'][0]['description']);
 
-        $processed = hook_markdown_render_feed($data);
+        $processed = hook_markdown_render_feed($data, $this->conf);
         $this->assertEquals($str, $processed['links'][0]['description']);
 
         $data = array(
@@ -151,7 +159,7 @@ class PluginMarkdownTest extends PHPUnit_Framework_TestCase
             ),
         );
 
-        $data = hook_markdown_render_daily($data);
+        $data = hook_markdown_render_daily($data, $this->conf);
         $this->assertEquals($str, $data['cols'][0][0]['formatedDescription']);
     }
 
@@ -169,7 +177,7 @@ class PluginMarkdownTest extends PHPUnit_Framework_TestCase
             ))
         );
 
-        $data = hook_markdown_render_feed($data);
+        $data = hook_markdown_render_feed($data, $this->conf);
         $this->assertContains('<em>', $data['links'][0]['description']);
     }
 
@@ -185,4 +193,41 @@ class PluginMarkdownTest extends PHPUnit_Framework_TestCase
         $data = process_markdown($md);
         $this->assertEquals($html, $data);
     }
+
+    /**
+     * Make sure that the HTML tags are escaped.
+     */
+    public function testMarkdownWithHtmlEscape()
+    {
+        $md = '**strong** <strong>strong</strong>';
+        $html = '<div class="markdown"><p><strong>strong</strong> &lt;strong&gt;strong&lt;/strong&gt;</p></div>';
+        $data = array(
+            'links' => array(
+                0 => array(
+                    'description' => $md,
+                ),
+            ),
+        );
+        $data = hook_markdown_render_linklist($data, $this->conf);
+        $this->assertEquals($html, $data['links'][0]['description']);
+    }
+
+    /**
+     * Make sure that the HTML tags aren't escaped with the setting set to false.
+     */
+    public function testMarkdownWithHtmlNoEscape()
+    {
+        $this->conf->set('security.markdown_escape', false);
+        $md = '**strong** <strong>strong</strong>';
+        $html = '<div class="markdown"><p><strong>strong</strong> <strong>strong</strong></p></div>';
+        $data = array(
+            'links' => array(
+                0 => array(
+                    'description' => $md,
+                ),
+            ),
+        );
+        $data = hook_markdown_render_linklist($data, $this->conf);
+        $this->assertEquals($html, $data['links'][0]['description']);
+    }
 }
diff --git a/tests/plugins/resources/markdown.html b/tests/plugins/resources/markdown.html
index c0fbe7f4..07a5a32e 100644
--- a/tests/plugins/resources/markdown.html
+++ b/tests/plugins/resources/markdown.html
@@ -12,11 +12,11 @@
 <li><a href="http://link.tld">two</a></li>
 <li><a href="http://link.tld">three</a></li>
 <li><a href="http://link.tld">four</a></li>
-<li>foo <a href="?addtag=foobar" title="Hashtag foobar">#foobar</a></li>
+<li>foo &lt;a href=&quot;?addtag=foobar&quot; title=&quot;Hashtag foobar&quot;&gt;#foobar&lt;/a&gt;</li>
 </ol></li>
 </ol>
-<p><a href="?addtag=foobar" title="Hashtag foobar">#foobar</a> foo <code>lol #foo</code> <a href="?addtag=bar" title="Hashtag bar">#bar</a></p>
-<p>fsdfs <a href="http://link.tld">http://link.tld</a> <a href="?addtag=foobar" title="Hashtag foobar">#foobar</a> <code>http://link.tld</code></p>
+<p>&lt;a href=&quot;?addtag=foobar&quot; title=&quot;Hashtag foobar&quot;&gt;#foobar&lt;/a&gt; foo <code>lol #foo</code> &lt;a href=&quot;?addtag=bar&quot; title=&quot;Hashtag bar&quot;&gt;#bar&lt;/a&gt;</p>
+<p>fsdfs <a href="http://link.tld">http://link.tld</a> &lt;a href=&quot;?addtag=foobar&quot; title=&quot;Hashtag foobar&quot;&gt;#foobar&lt;/a&gt; <code>http://link.tld</code></p>
 <pre><code>http://link.tld #foobar
 next #foo</code></pre>
 <p>Block:</p>