Merge branch 'rs-more-nofollow' into 'master'

Render Group and Project descriptions with our Markdown pipeline

Continuation of !727, this ensures external links in these fields also get `rel="nofollow"` added.

Bonus: Emoji now works in them! ✨

See merge request !735

Gemfile

@@ -10,9 +10,6 @@ end
 
 gem "rails", "~> 4.1.0"
 
-# Make links from text
-gem 'rails_autolink', '~> 1.1'
-
 # Default values for AR models
 gem "default_value_for", "~> 3.0.0"
 

Gemfile.lock

@@ -448,8 +448,6 @@ GEM
       sprockets-rails (~> 2.0)
     rails-observers (0.1.2)
       activemodel (~> 4.0)
-    rails_autolink (1.1.6)
-      rails (> 3.1)
     railties (4.1.9)
       actionpack (= 4.1.9)
       activesupport (= 4.1.9)
@@ -779,7 +777,6 @@ DEPENDENCIES
   rack-mini-profiler
   rack-oauth2 (~> 1.0.5)
   rails (~> 4.1.0)
-  rails_autolink (~> 1.1)
   raphael-rails (~> 2.1.2)
   rb-fsevent
   rb-inotify

app/assets/stylesheets/pages/projects.scss

@@ -48,14 +48,16 @@
     }
 
     .project-home-desc {
+      color: $gray;
+      float: left;
       font-size: 16px;
       line-height: 1.3;
       margin-right: 250px;
-    }
 
-    .project-home-desc {
-      float: left;
-      color: $gray;
+      // Render Markdown-generated HTML inline for this block
+      p {
+        display: inline;
+      }
     }
   }
 

app/helpers/application_helper.rb

@@ -279,10 +279,6 @@ module ApplicationHelper
     html_options
   end
 
-  def escaped_autolink(text)
-    auto_link ERB::Util.html_escape(text), link: :urls
-  end
-
   def promo_host
     'about.gitlab.com'
   end

app/views/groups/show.html.haml

@@ -11,7 +11,7 @@
       @#{@group.path}
     - if @group.description.present?
       .description
-        = escaped_autolink(@group.description)
+        = markdown(@group.description, pipeline: :description)
   %hr
 
   = render 'shared/show_aside'

app/views/projects/_home_panel.html.haml

@@ -5,7 +5,7 @@
   .project-home-row.project-home-row-top
     .project-home-desc
       - if @project.description.present?
-        = escaped_autolink(@project.description)
+        = markdown(@project.description, pipeline: :description)
       - if can?(current_user, :admin_project, @project)
         –
         = link_to 'Edit', edit_namespace_project_path

lib/gitlab/markdown.rb

@@ -57,6 +57,9 @@ module Gitlab
       pipeline = HTML::Pipeline.new(filters)
 
       context = {
+        # SanitizationFilter
+        pipeline: options[:pipeline],
+
         # EmojiFilter
         asset_root: Gitlab.config.gitlab.url,
         asset_host: Gitlab::Application.config.asset_host,

lib/gitlab/markdown/sanitization_filter.rb

@@ -8,33 +8,54 @@ module Gitlab
     # Extends HTML::Pipeline::SanitizationFilter with a custom whitelist.
     class SanitizationFilter < HTML::Pipeline::SanitizationFilter
       def whitelist
-        whitelist = super
+        # Descriptions are more heavily sanitized, allowing only a few elements.
+        # See http://git.io/vkuAN
+        if pipeline == :description
+          whitelist = LIMITED
+          whitelist[:elements] -= %w(pre code img ol ul li)
+        else
+          whitelist = super
+        end
+
+        customize_whitelist(whitelist)
+
+        whitelist
+      end
 
+      private
+
+      def pipeline
+        context[:pipeline] || :default
+      end
+
+      def customized?(transformers)
+        transformers.last.source_location[0] == __FILE__
+      end
+
+      def customize_whitelist(whitelist)
         # Only push these customizations once
-        unless customized?(whitelist[:transformers])
-          # Allow code highlighting
-          whitelist[:attributes]['pre'] = %w(class)
-          whitelist[:attributes]['span'] = %w(class)
+        return if customized?(whitelist[:transformers])
 
-          # Allow table alignment
-          whitelist[:attributes]['th'] = %w(style)
-          whitelist[:attributes]['td'] = %w(style)
+        # Allow code highlighting
+        whitelist[:attributes]['pre'] = %w(class)
+        whitelist[:attributes]['span'] = %w(class)
 
-          # Allow span elements
-          whitelist[:elements].push('span')
+        # Allow table alignment
+        whitelist[:attributes]['th'] = %w(style)
+        whitelist[:attributes]['td'] = %w(style)
 
-          # Remove `rel` attribute from `a` elements
-          whitelist[:transformers].push(remove_rel)
+        # Allow span elements
+        whitelist[:elements].push('span')
 
-          # Remove `class` attribute from non-highlight spans
-          whitelist[:transformers].push(clean_spans)
-        end
+        # Remove `rel` attribute from `a` elements
+        whitelist[:transformers].push(remove_rel)
+
+        # Remove `class` attribute from non-highlight spans
+        whitelist[:transformers].push(clean_spans)
 
         whitelist
       end
 
-      private
-
       def remove_rel
         lambda do |env|
           if env[:node_name] == 'a'
@@ -53,10 +74,6 @@ module Gitlab
           end
         end
       end
-
-      def customized?(transformers)
-        transformers.last.source_location[0] == __FILE__
-      end
     end
   end
 end

spec/features/groups_spec.rb

+require 'spec_helper'
+
+feature 'Group' do
+  describe 'description' do
+    let(:group) { create(:group) }
+    let(:path)  { group_path(group) }
+
+    before do
+      login_as(:admin)
+    end
+
+    it 'parses Markdown' do
+      group.update_attribute(:description, 'This is **my** group')
+      visit path
+      expect(page).to have_css('.description > p > strong')
+    end
+
+    it 'passes through html-pipeline' do
+      group.update_attribute(:description, 'This group is the :poop:')
+      visit path
+      expect(page).to have_css('.description > p > img')
+    end
+
+    it 'sanitizes unwanted tags' do
+      group.update_attribute(:description, '# Group Description')
+      visit path
+      expect(page).not_to have_css('.description h1')
+    end
+
+    it 'permits `rel` attribute on links' do
+      group.update_attribute(:description, 'https://google.com/')
+      visit path
+      expect(page).to have_css('.description a[rel]')
+    end
+  end
+end

spec/features/markdown_spec.rb

@@ -18,11 +18,13 @@ require 'erb'
 #         -> `gfm_with_options` helper
 #           -> HTML::Pipeline
 #             -> Sanitize
+#             -> RelativeLink
 #             -> Emoji
 #             -> Table of Contents
 #             -> Autolinks
 #               -> Rinku (http, https, ftp)
 #               -> Other schemes
+#             -> ExternalLink
 #             -> References
 #             -> TaskList
 #           -> `html_safe`

spec/features/projects_spec.rb

 require 'spec_helper'
 
-describe "Projects", feature: true, js: true do
-  before { login_as :user }
+feature 'Project' do
+  describe 'description' do
+    let(:project) { create(:project) }
+    let(:path)    { namespace_project_path(project.namespace, project) }
 
-  describe "DELETE /projects/:id" do
     before do
-      @project = create(:project, namespace: @user.namespace)
-      @project.team << [@user, :master]
-      visit edit_namespace_project_path(@project.namespace, @project)
+      login_as(:admin)
     end
 
-    it "should remove project" do
+    it 'parses Markdown' do
+      project.update_attribute(:description, 'This is **my** project')
+      visit path
+      expect(page).to have_css('.project-home-desc > p > strong')
+    end
+
+    it 'passes through html-pipeline' do
+      project.update_attribute(:description, 'This project is the :poop:')
+      visit path
+      expect(page).to have_css('.project-home-desc > p > img')
+    end
+
+    it 'sanitizes unwanted tags' do
+      project.update_attribute(:description, '# Project Description')
+      visit path
+      expect(page).not_to have_css('.project-home-desc h1')
+    end
+
+    it 'permits `rel` attribute on links' do
+      project.update_attribute(:description, 'https://google.com/')
+      visit path
+      expect(page).to have_css('.project-home-desc a[rel]')
+    end
+  end
+
+  describe 'removal', js: true do
+    let(:user)    { create(:user) }
+    let(:project) { create(:project, namespace: user.namespace) }
+
+    before do
+      login_with(user)
+      project.team << [user, :master]
+      visit edit_namespace_project_path(project.namespace, project)
+    end
+
+    it 'should remove project' do
       expect { remove_project }.to change {Project.count}.by(-1)
     end
 
     it 'should delete the project from disk' do
-      expect(GitlabShellWorker).to(
-        receive(:perform_async).with(:remove_repository,
-                                     /#{@project.path_with_namespace}/)
-      ).twice
+      expect(GitlabShellWorker).to receive(:perform_async).
+        with(:remove_repository, /#{project.path_with_namespace}/).twice
 
       remove_project
     end
@@ -26,7 +58,7 @@ describe "Projects", feature: true, js: true do
 
   def remove_project
     click_link "Remove project"
-    fill_in 'confirm_name_input', with: @project.path
+    fill_in 'confirm_name_input', with: project.path
     click_button 'Confirm'
   end
 end

spec/lib/gitlab/markdown/autolink_filter_spec.rb

@@ -2,11 +2,9 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe AutolinkFilter do
-    let(:link) { 'http://about.gitlab.com/' }
+    include FilterSpecHelper
 
-    def filter(html, options = {})
-      described_class.call(html, options)
-    end
+    let(:link) { 'http://about.gitlab.com/' }
 
     it 'does nothing when :autolink is false' do
       exp = act = link

spec/lib/gitlab/markdown/commit_range_reference_filter_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe CommitRangeReferenceFilter do
-    include ReferenceFilterSpecHelper
+    include FilterSpecHelper
 
     let(:project) { create(:project) }
     let(:commit1) { project.commit }

spec/lib/gitlab/markdown/commit_reference_filter_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe CommitReferenceFilter do
-    include ReferenceFilterSpecHelper
+    include FilterSpecHelper
 
     let(:project) { create(:project) }
     let(:commit)  { project.commit }

spec/lib/gitlab/markdown/emoji_filter_spec.rb

@@ -2,9 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe EmojiFilter do
-    def filter(html, contexts = {})
-      described_class.call(html, contexts)
-    end
+    include FilterSpecHelper
 
     before do
       ActionController::Base.asset_host = 'https://foo.com'

spec/lib/gitlab/markdown/external_issue_reference_filter_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe ExternalIssueReferenceFilter do
-    include ReferenceFilterSpecHelper
+    include FilterSpecHelper
 
     def helper
       IssuesHelper

spec/lib/gitlab/markdown/external_link_filter_spec.rb

@@ -2,9 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe ExternalLinkFilter do
-    def filter(html, options = {})
-      described_class.call(html, options)
-    end
+    include FilterSpecHelper
 
     it 'ignores elements without an href attribute' do
       exp = act = %q(<a id="ignored">Ignore Me</a>)

spec/lib/gitlab/markdown/issue_reference_filter_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe IssueReferenceFilter do
-    include ReferenceFilterSpecHelper
+    include FilterSpecHelper
 
     def helper
       IssuesHelper

spec/lib/gitlab/markdown/label_reference_filter_spec.rb

@@ -3,7 +3,7 @@ require 'html/pipeline'
 
 module Gitlab::Markdown
   describe LabelReferenceFilter do
-    include ReferenceFilterSpecHelper
+    include FilterSpecHelper
 
     let(:project)   { create(:empty_project) }
     let(:label)     { create(:label, project: project) }

spec/lib/gitlab/markdown/merge_request_reference_filter_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe MergeRequestReferenceFilter do
-    include ReferenceFilterSpecHelper
+    include FilterSpecHelper
 
     let(:project) { create(:project) }
     let(:merge)   { create(:merge_request, source_project: project) }

spec/lib/gitlab/markdown/sanitization_filter_spec.rb

@@ -2,9 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe SanitizationFilter do
-    def filter(html, options = {})
-      described_class.call(html, options)
-    end
+    include FilterSpecHelper
 
     describe 'default whitelist' do
       it 'sanitizes tags that are not whitelisted' do
@@ -42,6 +40,13 @@ module Gitlab::Markdown
     end
 
     describe 'custom whitelist' do
+      it 'customizes the whitelist only once' do
+        instance = described_class.new('Foo')
+        3.times { instance.whitelist }
+
+        expect(instance.whitelist[:transformers].size).to eq 4
+      end
+
       it 'allows syntax highlighting' do
         exp = act = %q{<pre class="code highlight white c"><code><span class="k">def</span></code></pre>}
         expect(filter(act).to_html).to eq exp
@@ -87,5 +92,27 @@ module Gitlab::Markdown
         expect(doc.at_css('a')['href']).to be_nil
       end
     end
+
+    context 'when pipeline is :description' do
+      it 'uses a stricter whitelist' do
+        doc = filter('<h1>Description</h1>', pipeline: :description)
+        expect(doc.to_html.strip).to eq 'Description'
+      end
+
+      %w(pre code img ol ul li).each do |elem|
+        it "removes '#{elem}' elements" do
+          act = "<#{elem}>Description</#{elem}>"
+          expect(filter(act, pipeline: :description).to_html.strip).
+            to eq 'Description'
+        end
+      end
+
+      %w(b i strong em a ins del sup sub p).each do |elem|
+        it "still allows '#{elem}' elements" do
+          exp = act = "<#{elem}>Description</#{elem}>"
+          expect(filter(act, pipeline: :description).to_html).to eq exp
+        end
+      end
+    end
   end
 end

spec/lib/gitlab/markdown/snippet_reference_filter_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe SnippetReferenceFilter do
-    include ReferenceFilterSpecHelper
+    include FilterSpecHelper
 
     let(:project)   { create(:empty_project) }
     let(:snippet)   { create(:project_snippet, project: project) }

spec/lib/gitlab/markdown/table_of_contents_filter_spec.rb

@@ -4,9 +4,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe TableOfContentsFilter do
-    def filter(html, options = {})
-      described_class.call(html, options)
-    end
+    include FilterSpecHelper
 
     def header(level, text)
       "<h#{level}>#{text}</h#{level}>\n"

spec/lib/gitlab/markdown/task_list_filter_spec.rb

@@ -2,9 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe TaskListFilter do
-    def filter(html, options = {})
-      described_class.call(html, options)
-    end
+    include FilterSpecHelper
 
     it 'does not apply `task-list` class to non-task lists' do
       exp = act = %(<ul><li>Item</li></ul>)

spec/lib/gitlab/markdown/user_reference_filter_spec.rb

@@ -2,7 +2,7 @@ require 'spec_helper'
 
 module Gitlab::Markdown
   describe UserReferenceFilter do
-    include ReferenceFilterSpecHelper
+    include FilterSpecHelper
 
     let(:project)   { create(:empty_project) }
     let(:user)      { create(:user) }

spec/support/reference_filter_spec_helper.rb → spec/support/filter_spec_helper.rb

-# Common methods and setup for Gitlab::Markdown reference filter specs
+# Helper methods for Gitlab::Markdown filter specs
 #
 # Must be included into specs manually
-module ReferenceFilterSpecHelper
+module FilterSpecHelper
   extend ActiveSupport::Concern
 
-  # Shortcut to Rails' auto-generated routes helpers, to avoid including the
-  # module
-  def urls
-    Rails.application.routes.url_helpers
-  end
-
-  # Modify a String reference to make it invalid
-  #
-  # Commit SHAs get reversed, IDs get incremented by 1, all other Strings get
-  # their word characters reversed.
-  #
-  # reference - String reference to modify
-  #
-  # Returns a String
-  def invalidate_reference(reference)
-    if reference =~ /\A(.+)?.\d+\z/
-      # Integer-based reference with optional project prefix
-      reference.gsub(/\d+\z/) { |i| i.to_i + 1 }
-    elsif reference =~ /\A(.+@)?(\h{6,40}\z)/
-      # SHA-based reference with optional prefix
-      reference.gsub(/\h{6,40}\z/) { |v| v.reverse }
-    else
-      reference.gsub(/\w+\z/) { |v| v.reverse }
-    end
-  end
-
   # Perform `call` on the described class
   #
-  # Automatically passes the current `project` value to the context if none is
-  # provided.
+  # Automatically passes the current `project` value, if defined, to the context
+  # if none is provided.
   #
-  # html     - String text to pass to the filter's `call` method.
+  # html     - HTML String to pass to the filter's `call` method.
   # contexts - Hash context for the filter. (default: {project: project})
   #
-  # Returns the String text returned by the filter's `call` method.
+  # Returns a Nokogiri::XML::DocumentFragment
   def filter(html, contexts = {})
-    contexts.reverse_merge!(project: project)
+    if defined?(project)
+      contexts.reverse_merge!(project: project)
+    end
+
     described_class.call(html, contexts)
   end
 
@@ -50,7 +27,7 @@ module ReferenceFilterSpecHelper
   # body     - String text to run through the pipeline
   # contexts - Hash context for the filter. (default: {project: project})
   #
-  # Returns the Hash of the pipeline result
+  # Returns the Hash
   def pipeline_result(body, contexts = {})
     contexts.reverse_merge!(project: project)
 
@@ -58,13 +35,43 @@ module ReferenceFilterSpecHelper
     pipeline.call(body)
   end
 
+  # Modify a String reference to make it invalid
+  #
+  # Commit SHAs get reversed, IDs get incremented by 1, all other Strings get
+  # their word characters reversed.
+  #
+  # reference - String reference to modify
+  #
+  # Returns a String
+  def invalidate_reference(reference)
+    if reference =~ /\A(.+)?.\d+\z/
+      # Integer-based reference with optional project prefix
+      reference.gsub(/\d+\z/) { |i| i.to_i + 1 }
+    elsif reference =~ /\A(.+@)?(\h{6,40}\z)/
+      # SHA-based reference with optional prefix
+      reference.gsub(/\h{6,40}\z/) { |v| v.reverse }
+    else
+      reference.gsub(/\w+\z/) { |v| v.reverse }
+    end
+  end
+
+  # Stub CrossProjectReference#user_can_reference_project? to return true for
+  # the current test
   def allow_cross_reference!
     allow_any_instance_of(described_class).
       to receive(:user_can_reference_project?).and_return(true)
   end
 
+  # Stub CrossProjectReference#user_can_reference_project? to return false for
+  # the current test
   def disallow_cross_reference!
     allow_any_instance_of(described_class).
       to receive(:user_can_reference_project?).and_return(false)
   end
+
+  # Shortcut to Rails' auto-generated routes helpers, to avoid including the
+  # module
+  def urls
+    Rails.application.routes.url_helpers
+  end
 end