From e03761011521929a375ebb56f21adacb226a3a8d Mon Sep 17 00:00:00 2001 From: ArthurHoaro Date: Mon, 27 Feb 2017 19:45:55 +0100 Subject: [PATCH] Add markdown_escape setting This setting allows to escape HTML in markdown rendering or not. The goal behind it is to avoid XSS issue in shared instances. More info: * the setting is set to true by default * it is set to false for anyone who already have the plugin enabled (avoid breaking existing entries) * improve the HTML sanitization when the setting is set to false - but don't consider it XSS proof * mention the setting in the plugin README --- application/Updater.php | 23 ++++++++++ plugins/markdown/README.md | 27 ++++++++--- plugins/markdown/markdown.php | 29 +++++++----- tests/Updater/UpdaterTest.php | 66 +++++++++++++++++++++++++++ tests/plugins/PluginMarkdownTest.php | 57 ++++++++++++++++++++--- tests/plugins/resources/markdown.html | 6 +-- 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 @@ public function updateMethodDefaultThemeVintage() } $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: > strongstrike @@ -60,12 +71,14 @@ Will render as: > strongstrike -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 @@ /** * 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 ($match) { return escape($match[0]); }, $description); } $description = preg_replace( - '#(<[^>]+)on[a-z]*="[^"]*"#is', + '#(<[^>]+)on[a-z]*="?[^ "]*"?#is', '$1', $description); return $description; @@ -265,10 +271,11 @@ function ($match) { return escape($match[0]); }, * 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 @@ public function testSetDefaultThemeNothingToDo() $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 @@ -13,12 +13,18 @@ */ 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 @@ public function testMarkdownLinklist() ), ); - $data = hook_markdown_render_linklist($data); + $data = hook_markdown_render_linklist($data, $this->conf); $this->assertNotFalse(strpos($data['links'][0]['description'], '

')); $this->assertNotFalse(strpos($data['links'][0]['description'], '

')); } @@ -61,7 +67,7 @@ public function testMarkdownDaily() ), ); - $data = hook_markdown_render_daily($data); + $data = hook_markdown_render_daily($data, $this->conf); $this->assertNotFalse(strpos($data['cols'][0][0]['formatedDescription'], '

')); $this->assertNotFalse(strpos($data['cols'][0][0]['formatedDescription'], '

')); } @@ -110,6 +116,8 @@ public function testSanitizeHtml() $output = escape($input); $input .= 'link'; $output .= 'link'; + $input .= 'link'; + $output .= 'link'; $this->assertEquals($output, sanitize_html($input)); // Do not touch escaped HTML. $input = escape($input); @@ -130,10 +138,10 @@ public function testNoMarkdownTag() )) ); - $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 @@ public function testNoMarkdownTag() ), ); - $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 @@ public function testNoMarkdownNotExcactlyMatching() )) ); - $data = hook_markdown_render_feed($data); + $data = hook_markdown_render_feed($data, $this->conf); $this->assertContains('', $data['links'][0]['description']); } @@ -185,4 +193,41 @@ public function testMarkdownHashtagLinks() $data = process_markdown($md); $this->assertEquals($html, $data); } + + /** + * Make sure that the HTML tags are escaped. + */ + public function testMarkdownWithHtmlEscape() + { + $md = '**strong** strong'; + $html = '

strong <strong>strong</strong>

'; + $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'; + $html = '

strong strong

'; + $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 @@
  • two
  • three
  • four
    • -
  • foo #foobar
    • +
  • foo <a href="?addtag=foobar" title="Hashtag foobar">#foobar</a>
    • -

    #foobar foo lol #foo #bar

    -

    fsdfs http://link.tld #foobar http://link.tld

    +

    <a href="?addtag=foobar" title="Hashtag foobar">#foobar</a> foo lol #foo <a href="?addtag=bar" title="Hashtag bar">#bar</a>

    +

    fsdfs http://link.tld <a href="?addtag=foobar" title="Hashtag foobar">#foobar</a> http://link.tld

    http://link.tld #foobar
 next #foo

    Block: