From 6798a6a8e224471b69018ad5cc4a526654ea5772 Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Fri, 10 Oct 2014 14:34:43 -0500 Subject: [PATCH 1/5] Allow HTML tags in user Markdown input Allow whitelisted tags to appear in rendered HTML output by disabling Redcarpet's `:filter_html` option. --- app/helpers/gitlab_markdown_helper.rb | 3 +-- lib/gitlab/markdown.rb | 5 ++++ spec/helpers/gitlab_markdown_helper_spec.rb | 30 +++++++++++++++++++++ 3 files changed, 36 insertions(+), 2 deletions(-) diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb index 0365681a12..3a0e8bcda4 100644 --- a/app/helpers/gitlab_markdown_helper.rb +++ b/app/helpers/gitlab_markdown_helper.rb @@ -33,7 +33,6 @@ module GitlabMarkdownHelper @options = options gitlab_renderer = Redcarpet::Render::GitlabHTML.new(self, { # 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)) @@ -48,7 +47,7 @@ module GitlabMarkdownHelper space_after_headers: true, superscript: true) end - @markdown.render(text).html_safe + @markdown.render(sanitize_html(text)).html_safe end def first_line_in_markdown(text) diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb index 17512a5165..464b88d07e 100644 --- a/lib/gitlab/markdown.rb +++ b/lib/gitlab/markdown.rb @@ -80,6 +80,11 @@ module Gitlab markdown_context) text = result[:output].to_html(save_with: 0) + sanitize_html(text) + end + + # Remove HTML tags and attributes that are not whitelisted + def sanitize_html(text) allowed_attributes = ActionView::Base.sanitized_allowed_attributes allowed_tags = ActionView::Base.sanitized_allowed_tags diff --git a/spec/helpers/gitlab_markdown_helper_spec.rb b/spec/helpers/gitlab_markdown_helper_spec.rb index 15033f0743..c75773ad32 100644 --- a/spec/helpers/gitlab_markdown_helper_spec.rb +++ b/spec/helpers/gitlab_markdown_helper_spec.rb @@ -614,6 +614,36 @@ describe GitlabMarkdownHelper do expected = "" markdown(actual).should match(expected) end + + it 'should allow whitelisted HTML tags from the user' do + actual = '
Term
Definition
' + expect(markdown(actual)).to match(actual) + end + + it 'should sanitize tags that are not whitelisted' do + actual = ' no blinks' + 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 = 'link text' + expect(markdown(actual)).to match(actual) + end + + it 'should sanitize tag attributes that are not whitelisted' do + actual = 'link text' + expected = 'link text' + expect(markdown(actual)).to match(expected) + end + + it 'should sanitize javascript in attributes' do + actual = %q(link text) + expected = 'link text' + expect(markdown(actual)).to match(expected) + end end describe 'markdown for empty repository' do From 1a9c2ddc55cf563ea42d67811a19b2693d7a44e9 Mon Sep 17 00:00:00 2001 From: Vinnie Okada Date: Fri, 10 Oct 2014 18:12:50 -0500 Subject: [PATCH 2/5] Document whitelisted HTML tags and attributes --- doc/markdown/markdown.md | 59 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/doc/markdown/markdown.md b/doc/markdown/markdown.md index 6d96da76ad..0f63eca1f6 100644 --- a/doc/markdown/markdown.md +++ b/doc/markdown/markdown.md @@ -438,6 +438,65 @@ You can also use raw HTML in your Markdown, and it'll mostly work pretty well.
Does *not* work **very** well. Use HTML tags.
+The following tags can be used: + +* `` +* `` +* `` +* `
` +* `` +* `` +* `
` +* `
` +* `` +* `` +* `
` +* `` +* `` +* `
` +* `
` +* `
` +* `` +* `

` +* `

` +* `

` +* `

` +* `

` +* `
` +* `
` +* `` +* `` +* `` +* `` +* `
  • ` +* `
      ` +* `

      ` +* `

      `
+* ``
+* ``
+* ``
+* ``
+* ``
+* ``
+* ``
+* `
        `
+* ``
+
+You can also use the following HTML attributes in your inline tags:
+
+* `abbr`
+* `alt`
+* `cite`
+* `class`
+* `datetime`
+* `height`
+* `href`
+* `name`
+* `src`
+* `title`
+* `width`
+* `xml:lang`
+
 ## Horizontal Rule
 
 ```

From 52bf95ae380dc06243d0c4e5c8eb80f8be15a4f3 Mon Sep 17 00:00:00 2001
From: Vinnie Okada 
Date: Tue, 17 Mar 2015 21:17:00 -0600
Subject: [PATCH 3/5] Change HTML sanitization

Use the `SanitizationFilter` class from the html-pipeline gem for inline
HTML instead of calling the Rails `sanitize` method.
---
 app/helpers/gitlab_markdown_helper.rb |  2 +-
 doc/markdown/markdown.md              | 59 +--------------------------
 lib/gitlab/markdown.rb                | 36 ++++++++++------
 3 files changed, 25 insertions(+), 72 deletions(-)

diff --git a/app/helpers/gitlab_markdown_helper.rb b/app/helpers/gitlab_markdown_helper.rb
index 7bafbbd5f3..6df506e835 100644
--- a/app/helpers/gitlab_markdown_helper.rb
+++ b/app/helpers/gitlab_markdown_helper.rb
@@ -49,7 +49,7 @@ module GitlabMarkdownHelper
                       space_after_headers: true,
                       superscript: true)
     end
-    @markdown.render(sanitize_html(text)).html_safe
+    @markdown.render(text).html_safe
   end
 
   # Return the first line of +text+, up to +max_chars+, after parsing the line
diff --git a/doc/markdown/markdown.md b/doc/markdown/markdown.md
index ddf1bbc6ee..4ab73df8af 100644
--- a/doc/markdown/markdown.md
+++ b/doc/markdown/markdown.md
@@ -440,64 +440,7 @@ Note that inline HTML is disabled in the default Gitlab configuration, although
   
        Does *not* work **very** well. Use HTML tags.
    • -The following tags can be used: - -* `    ` -* `` -* `` -* `
    ` -* `` -* `` -* `
    ` -* `
    ` -* `` -* `` -* `
    ` -* `` -* `` -* `
    ` -* `
    ` -* `
    ` -* `` -* `

    ` -* `

    ` -* `

    ` -* `

    ` -* `

    ` -* `
    ` -* `
    ` -* `` -* `` -* `` -* `` -* `
  • ` -* `
      ` -* `

      ` -* `

      `
-* ``
-* ``
-* ``
-* ``
-* ``
-* ``
-* ``
-* `
        `
-* ``
-
-You can also use the following HTML attributes in your inline tags:
-
-* `abbr`
-* `alt`
-* `cite`
-* `class`
-* `datetime`
-* `height`
-* `href`
-* `name`
-* `src`
-* `title`
-* `width`
-* `xml:lang`
+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
 
diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb
index 32f04c866e..cd70fd5e85 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -79,15 +79,34 @@ 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', 'style')
+
+      # 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,22 +116,13 @@ module Gitlab
       if options[:xhtml]
         saveoptions |= Nokogiri::XML::Node::SaveOptions::AS_XHTML
       end
+
       text = result[:output].to_html(save_with: saveoptions)
 
-      sanitize_html(text)
-    end
-
-    # Remove HTML tags and attributes that are not whitelisted
-    def sanitize_html(text)
-      allowed_attributes = ActionView::Base.sanitized_allowed_attributes
-      allowed_tags = ActionView::Base.sanitized_allowed_tags
-
-      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
     end
 

From cc29ce491786d631586c3b0d0da310b8b790a673 Mon Sep 17 00:00:00 2001
From: Vinnie Okada 
Date: Sat, 21 Mar 2015 08:39:54 -0600
Subject: [PATCH 4/5] Don't allow style attributes in inline HTML

---
 CHANGELOG              | 1 +
 lib/gitlab/markdown.rb | 2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/CHANGELOG b/CHANGELOG
index c4e47346fd..0046b73ba7 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,6 +1,7 @@
 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
 
 v 7.9.0 (unreleased)
   - Add HipChat integration documentation (Stan Hu)
diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb
index cd70fd5e85..65dce9291e 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -88,7 +88,7 @@ module Gitlab
       ]
 
       whitelist = HTML::Pipeline::SanitizationFilter::WHITELIST
-      whitelist[:attributes][:all].push('class', 'id', 'style')
+      whitelist[:attributes][:all].push('class', 'id')
 
       # Remove the rel attribute that the sanitize gem adds, and remove the
       # href attribute if it contains inline javascript

From a7afc0634240f5cddb6c6e1bf1f9fcf4374b852e Mon Sep 17 00:00:00 2001
From: Vinnie Okada 
Date: Sun, 22 Mar 2015 09:02:32 -0600
Subject: [PATCH 5/5] Fix SanitizationFilter bugs

Return a `SafeBuffer` instead of a `String` from the `#gfm_with_options`
method so that Rails doesn't escape our markup.

Also add `` to the sanitization whitelist to avoid breaking syntax
highlighting in code blocks.
---
 lib/gitlab/markdown.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/gitlab/markdown.rb b/lib/gitlab/markdown.rb
index 65dce9291e..11da4be402 100644
--- a/lib/gitlab/markdown.rb
+++ b/lib/gitlab/markdown.rb
@@ -89,6 +89,7 @@ module Gitlab
 
       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
@@ -123,7 +124,7 @@ module Gitlab
         text = parse_tasks(text)
       end
 
-      text
+      text.html_safe
     end
 
     private