Merge pull request #8007 from mr-vinn/markdown-tags

Allow HTML tags in user Markdown input

CHANGELOG

 Please view this file on the master branch, on stable branches it's out of date.
 
 v 7.10.0 (unreleased)
+  - Allow HTML tags in Markdown input
   - Include missing events and fix save functionality in admin service template settings form (Stan Hu)
   - Fix "Import projects from" button to show the correct instructions (Stan Hu)
   - Fix dots in Wiki slugs causing errors (Stan Hu)

app/helpers/gitlab_markdown_helper.rb

@@ -35,7 +35,6 @@ module GitlabMarkdownHelper
                                                           user_color_scheme_class,
                                                           {
                             # see https://github.com/vmg/redcarpet#darling-i-packed-you-a-couple-renderers-for-lunch-
-                            filter_html: true,
                             with_toc_data: true,
                             safe_links_only: true
                           }.merge(options))

doc/markdown/markdown.md

@@ -441,6 +441,8 @@ Note that inline HTML is disabled in the default Gitlab configuration, although
   <dd>Does *not* work **very** well. Use HTML <em>tags</em>.</dd>
 </dl>
 
+See the documentation for HTML::Pipeline's [SanitizationFilter](http://www.rubydoc.info/gems/html-pipeline/HTML/Pipeline/SanitizationFilter#WHITELIST-constant) class for the list of allowed HTML tags and attributes.  In addition to the default `SanitizationFilter` whitelist, GitLab allows the `class`, `id`, and `style` attributes.
+
 ## Horizontal Rule
 
 ```

lib/gitlab/markdown.rb

@@ -79,15 +79,35 @@ module Gitlab
 
       # Used markdown pipelines in GitLab:
       # GitlabEmojiFilter - performs emoji replacement.
+      # SanitizationFilter - remove unsafe HTML tags and attributes
       #
       # see https://gitlab.com/gitlab-org/html-pipeline-gitlab for more filters
       filters = [
-        HTML::Pipeline::Gitlab::GitlabEmojiFilter
+        HTML::Pipeline::Gitlab::GitlabEmojiFilter,
+        HTML::Pipeline::SanitizationFilter
       ]
 
+      whitelist = HTML::Pipeline::SanitizationFilter::WHITELIST
+      whitelist[:attributes][:all].push('class', 'id')
+      whitelist[:elements].push('span')
+
+      # Remove the rel attribute that the sanitize gem adds, and remove the
+      # href attribute if it contains inline javascript
+      fix_anchors = lambda do |env|
+        name, node = env[:node_name], env[:node]
+        if name == 'a'
+          node.remove_attribute('rel')
+          if node['href'] && node['href'].match('javascript:')
+            node.remove_attribute('href')
+          end
+        end
+      end
+      whitelist[:transformers].push(fix_anchors)
+
       markdown_context = {
               asset_root: Gitlab.config.gitlab.url,
-              asset_host: Gitlab::Application.config.asset_host
+              asset_host: Gitlab::Application.config.asset_host,
+              whitelist: whitelist
       }
 
       markdown_pipeline = HTML::Pipeline::Gitlab.new(filters).pipeline
@@ -97,18 +117,14 @@ module Gitlab
       if options[:xhtml]
         saveoptions |= Nokogiri::XML::Node::SaveOptions::AS_XHTML
       end
-      text = result[:output].to_html(save_with: saveoptions)
 
-      allowed_attributes = ActionView::Base.sanitized_allowed_attributes
-      allowed_tags = ActionView::Base.sanitized_allowed_tags
+      text = result[:output].to_html(save_with: saveoptions)
 
-      text = sanitize text.html_safe,
-                      attributes: allowed_attributes + %w(id class style),
-                      tags: allowed_tags + %w(table tr td th)
       if options[:parse_tasks]
         text = parse_tasks(text)
       end
-      text
+
+      text.html_safe
     end
 
     private

spec/helpers/gitlab_markdown_helper_spec.rb

@@ -727,6 +727,36 @@ describe GitlabMarkdownHelper do
       expected = ""
       expect(markdown(actual)).to match(expected)
     end
+
+    it 'should allow whitelisted HTML tags from the user' do
+      actual = '<dl><dt>Term</dt><dd>Definition</dd></dl>'
+      expect(markdown(actual)).to match(actual)
+    end
+
+    it 'should sanitize tags that are not whitelisted' do
+      actual = '<textarea>no inputs allowed</textarea> <blink>no blinks</blink>'
+      expected = 'no inputs allowed no blinks'
+      expect(markdown(actual)).to match(expected)
+      expect(markdown(actual)).not_to match('<.textarea>')
+      expect(markdown(actual)).not_to match('<.blink>')
+    end
+
+    it 'should allow whitelisted tag attributes from the user' do
+      actual = '<a class="custom">link text</a>'
+      expect(markdown(actual)).to match(actual)
+    end
+
+    it 'should sanitize tag attributes that are not whitelisted' do
+      actual = '<a href="http://example.com/bar.html" foo="bar">link text</a>'
+      expected = '<a href="http://example.com/bar.html">link text</a>'
+      expect(markdown(actual)).to match(expected)
+    end
+
+    it 'should sanitize javascript in attributes' do
+      actual = %q(<a href="javascript:alert('foo')">link text</a>)
+      expected = '<a>link text</a>'
+      expect(markdown(actual)).to match(expected)
+    end
   end
 
   describe 'markdown for empty repository' do