Merge branch 'rs-redactor-filter' into 'master'

Separate rendering of Markdown reference links from redacting those the user doesn't have access to and extracting referenced objects

Related: !1014

See merge request !1090

app/helpers/gitlab_markdown_helper.rb

@@ -19,7 +19,8 @@ module GitlabMarkdownHelper
                      escape_once(body)
                    end
-    gfm_body = Gitlab::Markdown.gfm(escaped_body, project: @project, current_user: current_user)
+    user = current_user if defined?(current_user)
+    gfm_body = Gitlab::Markdown.gfm(escaped_body, project: @project, current_user: user)
     fragment = Nokogiri::HTML::DocumentFragment.parse(gfm_body)
     if fragment.children.size == 1 && fragment.children[0].name == 'a'
@@ -45,29 +46,39 @@ module GitlabMarkdownHelper
   end
   def markdown(text, context = {})
+    return "" unless text.present?
+
     context.reverse_merge!(
-      current_user: current_user,
       path:         @path,
+      pipeline:     :default,
       project:      @project,
       project_wiki: @project_wiki,
       ref:          @ref
     )
-    Gitlab::Markdown.render(text, context)
+    user = current_user if defined?(current_user)
+
+    html = Gitlab::Markdown.render(text, context)
+    Gitlab::Markdown.post_process(html, pipeline: context[:pipeline], project: @project, user: user)
   end
   # TODO (rspeicher): Remove all usages of this helper and just call `markdown`
   # with a custom pipeline depending on the content being rendered
   def gfm(text, options = {})
+    return "" unless text.present?
+
     options.reverse_merge!(
-      current_user: current_user,
       path:         @path,
+      pipeline:     :default,
       project:      @project,
       project_wiki: @project_wiki,
       ref:          @ref
     )
-    Gitlab::Markdown.gfm(text, options)
+    user = current_user if defined?(current_user)
+
+    html = Gitlab::Markdown.gfm(text, options)
+    Gitlab::Markdown.post_process(html, pipeline: options[:pipeline], project: @project, user: user)
   end
   def asciidoc(text)

app/models/commit.rb

@@ -2,13 +2,13 @@ class Commit
   extend ActiveModel::Naming
   include ActiveModel::Conversion
-  include Mentionable
   include Participable
+  include Mentionable
   include Referable
   include StaticModel
   attr_mentionable :safe_message
-  participant :author, :committer, :notes, :mentioned_users
+  participant :author, :committer, :notes
   attr_accessor :project

app/models/concerns/issuable.rb

@@ -6,8 +6,8 @@
 #
 module Issuable
   extend ActiveSupport::Concern
-  include Mentionable
   include Participable
+  include Mentionable
   included do
     belongs_to :author, class_name: "User"
@@ -47,8 +47,7 @@ module Issuable
              prefix: true
     attr_mentionable :title, :description
-
-    participant :author, :assignee, :notes_with_associations, :mentioned_users
+    participant :author, :assignee, :notes_with_associations
   end
   module ClassMethods

app/models/concerns/mentionable.rb

@@ -20,6 +20,12 @@ module Mentionable
     end
   end
+  included do
+    if self < Participable
+      participant ->(current_user) { mentioned_users(current_user, load_lazy_references: false) }
+    end
+  end
+
   # Returns the text used as the body of a Note when this object is referenced
   #
   # By default this will be the class name and the result of calling
@@ -41,22 +47,22 @@ module Mentionable
     self
   end
-  def all_references(current_user = self.author, text = self.mentionable_text)
-    ext = Gitlab::ReferenceExtractor.new(self.project, current_user)
+  def all_references(current_user = self.author, text = self.mentionable_text, load_lazy_references: true)
+    ext = Gitlab::ReferenceExtractor.new(self.project, current_user, load_lazy_references: load_lazy_references)
     ext.analyze(text)
     ext
   end
-  def mentioned_users(current_user = nil)
-    all_references(current_user).users.uniq
+  def mentioned_users(current_user = nil, load_lazy_references: true)
+    all_references(current_user, load_lazy_references: load_lazy_references).users
   end
   # Extract GFM references to other Mentionables from this Mentionable. Always excludes its #local_reference.
-  def referenced_mentionables(current_user = self.author, text = self.mentionable_text)
+  def referenced_mentionables(current_user = self.author, text = self.mentionable_text, load_lazy_references: true)
     return [] if text.blank?
-    refs = all_references(current_user, text)
-    (refs.issues + refs.merge_requests + refs.commits).uniq - [local_reference]
+    refs = all_references(current_user, text, load_lazy_references: load_lazy_references)
+    (refs.issues + refs.merge_requests + refs.commits) - [local_reference]
   end
   # Create a cross-reference Note for each GFM reference to another Mentionable found in +mentionable_text+.

app/models/concerns/participable.rb

@@ -12,7 +12,7 @@
 #
 #       # ...
 #
-#       participant :author, :assignee, :mentioned_users, :notes
+#       participant :author, :assignee, :notes, ->(current_user) { mentioned_users(current_user) }
 #     end
 #
 #     issue = Issue.last
@@ -27,7 +27,7 @@ module Participable
   module ClassMethods
     def participant(*attrs)
-      participant_attrs.concat(attrs.map(&:to_s))
+      participant_attrs.concat(attrs)
     end
     def participant_attrs
@@ -37,33 +37,39 @@ module Participable
   # Be aware that this method makes a lot of sql queries.
   # Save result into variable if you are going to reuse it inside same request
-  def participants(current_user = self.author)
-    self.class.participant_attrs.flat_map do |attr|
-      meth = method(attr)
-
+  def participants(current_user = self.author, load_lazy_references: true)
+    participants = self.class.participant_attrs.flat_map do |attr|
       value =
-        if meth.arity == 1 || meth.arity == -1
-          meth.call(current_user)
+        if attr.respond_to?(:call)
+          instance_exec(current_user, &attr)
         else
-          meth.call
+          send(attr)
         end
       participants_for(value, current_user)
-    end.compact.uniq.select do |user|
-      user.can?(:read_project, self.project)
+    end.compact.uniq
+
+    if load_lazy_references
+      participants = Gitlab::Markdown::ReferenceFilter::LazyReference.load(participants).uniq
+
+      participants.select! do |user|
+        user.can?(:read_project, project)
+      end
     end
+
+    participants
   end
   private
   def participants_for(value, current_user = nil)
     case value
-    when User
+    when User, Gitlab::Markdown::ReferenceFilter::LazyReference
       [value]
     when Enumerable, ActiveRecord::Relation
       value.flat_map { |v| participants_for(v, current_user) }
     when Participable
-      value.participants(current_user)
+      value.participants(current_user, load_lazy_references: false)
     end
   end
 end

app/models/note.rb

@@ -22,14 +22,14 @@ require 'carrierwave/orm/activerecord'
 require 'file_size_validator'
 class Note < ActiveRecord::Base
-  include Mentionable
   include Gitlab::CurrentSettings
   include Participable
+  include Mentionable
   default_value_for :system, false
   attr_mentionable :note
-  participant :author, :mentioned_users
+  participant :author
   belongs_to :project
   belongs_to :noteable, polymorphic: true

app/views/events/_event_issue.atom.haml

 %div{xmlns: "http://www.w3.org/1999/xhtml"}
-  - if issue.description.present?
-    = markdown(issue.description, xhtml: true, reference_only_path: false, project: issue.project)
+  = markdown(issue.description, pipeline: :atom, project: issue.project)

app/views/events/_event_merge_request.atom.haml

 %div{xmlns: "http://www.w3.org/1999/xhtml"}
-  - if merge_request.description.present?
-    = markdown(merge_request.description, xhtml: true, reference_only_path: false, project: merge_request.project)
+  = markdown(merge_request.description, pipeline: :atom, project: merge_request.project)

app/views/events/_event_note.atom.haml

 %div{xmlns: "http://www.w3.org/1999/xhtml"}
-  = markdown(note.note, xhtml: true, reference_only_path: false, project: note.project)
+  = markdown(note.note, pipeline: :atom, project: note.project)

app/views/events/_event_push.atom.haml

@@ -6,7 +6,7 @@
       %i
         at
         = commit[:timestamp].to_time.to_s(:short)
-    %blockquote= markdown(escape_once(commit[:message]), xhtml: true, reference_only_path: false, project: event.project)
+    %blockquote= markdown(escape_once(commit[:message]), pipeline: :atom, project: event.project)
   - if event.commits_count > 15
     %p
       %i

app/views/notify/_note_message.html.haml

 %div
-  = markdown(@note.note, reference_only_path: false)
+  = markdown(@note.note, pipeline: :email)

app/views/notify/new_issue_email.html.haml

 -if @issue.description
-  = markdown(@issue.description, reference_only_path: false)
+  = markdown(@issue.description, pipeline: :email)
 - if @issue.assignee_id.present?
   %p

app/views/notify/new_merge_request_email.html.haml

@@ -6,4 +6,4 @@
     Assignee: #{@merge_request.author_name} → #{@merge_request.assignee_name}
 -if @merge_request.description
-  = markdown(@merge_request.description, reference_only_path: false)
+  = markdown(@merge_request.description, pipeline: :email)

lib/gitlab/markdown.rb

@@ -7,6 +7,14 @@ module Gitlab
   module Markdown
     # Convert a Markdown String into an HTML-safe String of HTML
     #
+    # Note that while the returned HTML will have been sanitized of dangerous
+    # HTML, it may post a risk of information leakage if it's not also passed
+    # through `post_process`.
+    #
+    # Also note that the returned String is always HTML, not XHTML. Views
+    # requiring XHTML, such as Atom feeds, need to call `post_process` on the
+    # result, providing the appropriate `pipeline` option.
+    #
     # markdown - Markdown String
     # context  - Hash of context options passed to our HTML Pipeline
     #
@@ -31,6 +39,33 @@ module Gitlab
       renderer.render(markdown)
     end
+    # Perform post-processing on an HTML String
+    #
+    # This method is used to perform state-dependent changes to a String of
+    # HTML, such as removing references that the current user doesn't have
+    # permission to make (`RedactorFilter`).
+    #
+    # html     - String to process
+    # options  - Hash of options to customize output
+    #            :pipeline  - Symbol pipeline type
+    #            :project   - Project
+    #            :user      - User object
+    #
+    # Returns an HTML-safe String
+    def self.post_process(html, options)
+      context = {
+        project:      options[:project],
+        current_user: options[:user]
+      }
+      doc = post_processor.to_document(html, context)
+
+      if options[:pipeline] == :atom
+        doc.to_html(save_with: Nokogiri::XML::Node::SaveOptions::AS_XHTML)
+      else
+        doc.to_html
+      end.html_safe
+    end
+
     # Provide autoload paths for filters to prevent a circular dependency error
     autoload :AutolinkFilter,               'gitlab/markdown/autolink_filter'
     autoload :CommitRangeReferenceFilter,   'gitlab/markdown/commit_range_reference_filter'
@@ -41,6 +76,7 @@ module Gitlab
     autoload :IssueReferenceFilter,         'gitlab/markdown/issue_reference_filter'
     autoload :LabelReferenceFilter,         'gitlab/markdown/label_reference_filter'
     autoload :MergeRequestReferenceFilter,  'gitlab/markdown/merge_request_reference_filter'
+    autoload :RedactorFilter,               'gitlab/markdown/redactor_filter'
     autoload :RelativeLinkFilter,           'gitlab/markdown/relative_link_filter'
     autoload :SanitizationFilter,           'gitlab/markdown/sanitization_filter'
     autoload :SnippetReferenceFilter,       'gitlab/markdown/snippet_reference_filter'
@@ -50,26 +86,20 @@ module Gitlab
     autoload :UserReferenceFilter,          'gitlab/markdown/user_reference_filter'
     autoload :UploadLinkFilter,             'gitlab/markdown/upload_link_filter'
-    # Public: Parse the provided text with GitLab-Flavored Markdown
+    # Public: Parse the provided HTML with GitLab-Flavored Markdown
+    #
+    # html    - HTML String
+    # options - A Hash of options used to customize output (default: {})
+    #           :no_header_anchors - Disable header anchors in TableOfContentsFilter
+    #           :path              - Current path String
+    #           :pipeline          - Symbol pipeline type
+    #           :project           - Current Project object
+    #           :project_wiki      - Current ProjectWiki object
+    #           :ref               - Current ref String
     #
-    # text         - the source text
-    # options      - A Hash of options used to customize output (default: {}):
-    #                :xhtml               - output XHTML instead of HTML
-    #                :reference_only_path - Use relative path for reference links
-    def self.gfm(text, options = {})
-      return text if text.nil?
-
-      # Duplicate the string so we don't alter the original, then call to_str
-      # to cast it back to a String instead of a SafeBuffer. This is required
-      # for gsub calls to work as we need them to.
-      text = text.dup.to_str
-
-      options.reverse_merge!(
-        xhtml:                false,
-        reference_only_path:  true,
-        project:              options[:project],
-        current_user:         options[:current_user]
-      )
+    # Returns an HTML-safe String
+    def self.gfm(html, options = {})
+      return '' unless html.present?
       @pipeline ||= HTML::Pipeline.new(filters)
@@ -78,41 +108,36 @@ module Gitlab
         pipeline: options[:pipeline],
         # EmojiFilter
-        asset_root: Gitlab.config.gitlab.base_url,
         asset_host: Gitlab::Application.config.asset_host,
-
-        # TableOfContentsFilter
-        no_header_anchors: options[:no_header_anchors],
+        asset_root: Gitlab.config.gitlab.base_url,
         # ReferenceFilter
-        current_user:    options[:current_user],
-        only_path:       options[:reference_only_path],
-        project:         options[:project],
+        only_path: only_path_pipeline?(options[:pipeline]),
+        project:   options[:project],
         # RelativeLinkFilter
+        project_wiki:   options[:project_wiki],
         ref:            options[:ref],
         requested_path: options[:path],
-        project_wiki:   options[:project_wiki]
-      }
-
-      result = @pipeline.call(text, context)
-      save_options = 0
-      if options[:xhtml]
-        save_options |= Nokogiri::XML::Node::SaveOptions::AS_XHTML
-      end
-
-      text = result[:output].to_html(save_with: save_options)
-      text.html_safe
+        # TableOfContentsFilter
+        no_header_anchors: options[:no_header_anchors]
+      }
+      @pipeline.to_html(html, context).html_safe
     end
     private
-    def self.renderer
-      @markdown ||= begin
-        renderer = Redcarpet::Render::HTML.new
-        Redcarpet::Markdown.new(renderer, redcarpet_options)
+    # Check if a pipeline enables the `only_path` context option
+    #
+    # Returns Boolean
+    def self.only_path_pipeline?(pipeline)
+      case pipeline
+      when :atom, :email
+        false
+      else
+        true
       end
     end
@@ -130,6 +155,17 @@ module Gitlab
       }.freeze
     end
+    def self.renderer
+      @markdown ||= begin
+        renderer = Redcarpet::Render::HTML.new
+        Redcarpet::Markdown.new(renderer, redcarpet_options)
+      end
+    end
+
+    def self.post_processor
+      @post_processor ||= HTML::Pipeline.new([Gitlab::Markdown::RedactorFilter])
+    end
+
     # Filters used in our pipeline
     #
     # SanitizationFilter should come first so that all generated reference HTML

lib/gitlab/markdown/commit_range_reference_filter.rb

@@ -26,6 +26,18 @@ module Gitlab
         end
       end
+      def self.referenced_by(node)
+        project = Project.find(node.attr("data-project")) rescue nil
+        return unless project
+
+        id = node.attr("data-commit-range")
+        range = CommitRange.new(id, project)
+
+        return unless range.valid_commits?
+
+        { commit_range: range }
+      end
+
       def initialize(*args)
         super
@@ -53,13 +65,11 @@ module Gitlab
           range = CommitRange.new(id, project)
           if range.valid_commits?
-            push_result(:commit_range, range)
-
             url = url_for_commit_range(project, range)
             title = range.reference_title
             klass = reference_class(:commit_range)
-            data  = data_attribute(project.id)
+            data  = data_attribute(project: project.id, commit_range: id)
             project_ref += '@' if project_ref

lib/gitlab/markdown/commit_reference_filter.rb

@@ -26,6 +26,18 @@ module Gitlab
         end
       end
+      def self.referenced_by(node)
+        project = Project.find(node.attr("data-project")) rescue nil
+        return unless project
+
+        id = node.attr("data-commit")
+        commit = commit_from_ref(project, id)
+
+        return unless commit
+
+        { commit: commit }
+      end
+
       def call
         replace_text_nodes_matching(Commit.reference_pattern) do |content|
           commit_link_filter(content)
@@ -39,17 +51,15 @@ module Gitlab
       # Returns a String with commit references replaced with links. All links
       # have `gfm` and `gfm-commit` class names attached for styling.
       def commit_link_filter(text)
-        self.class.references_in(text) do |match, commit_ref, project_ref|
+        self.class.references_in(text) do |match, id, project_ref|
           project = self.project_from_ref(project_ref)
-          if commit = commit_from_ref(project, commit_ref)
-            push_result(:commit, commit)
-
+          if commit = self.class.commit_from_ref(project, id)
             url = url_for_commit(project, commit)
             title = escape_once(commit.link_title)
             klass = reference_class(:commit)
-            data  = data_attribute(project.id)
+            data  = data_attribute(project: project.id, commit: id)
             project_ref += '@' if project_ref
@@ -62,9 +72,9 @@ module Gitlab
         end
       end
-      def commit_from_ref(project, commit_ref)
+      def self.commit_from_ref(project, id)
         if project && project.valid_repo?
-          project.commit(commit_ref)
+          project.commit(id)
         end
       end

lib/gitlab/markdown/cross_project_reference.rb

@@ -13,18 +13,11 @@ module Gitlab
       #
       # ref - String reference.
       #
-      # Returns a Project, or nil if the reference can't be accessed
+      # Returns a Project, or nil if the reference can't be found
       def project_from_ref(ref)
         return context[:project] unless ref
-        other = Project.find_with_namespace(ref)
-        return nil unless other && user_can_reference_project?(other)
-
-        other
-      end
-
-      def user_can_reference_project?(project, user = context[:current_user])
-        Ability.abilities.allowed?(user, :read_project, project)
+        Project.find_with_namespace(ref)
       end
     end
   end

lib/gitlab/markdown/external_issue_reference_filter.rb

@@ -47,8 +47,9 @@ module Gitlab
           title = escape_once("Issue in #{project.external_issue_tracker.title}")
           klass = reference_class(:issue)
-          %(<a href="#{url}"
+          data  = data_attribute(project: project.id)
+          %(<a href="#{url}" #{data}
                title="#{title}"
                class="#{klass}">#{match}</a>)
         end

lib/gitlab/markdown/issue_reference_filter.rb

@@ -27,6 +27,10 @@ module Gitlab
         end
       end
+      def self.referenced_by(node)
+        { issue: LazyReference.new(Issue, node.attr("data-issue")) }
+      end
+
       def call
         replace_text_nodes_matching(Issue.reference_pattern) do |content|
           issue_link_filter(content)
@@ -45,13 +49,11 @@ module Gitlab
           project = self.project_from_ref(project_ref)
           if project && issue = project.get_issue(id)
-            push_result(:issue, issue)
-
             url = url_for_issue(id, project, only_path: context[:only_path])
             title = escape_once("Issue: #{issue.title}")
             klass = reference_class(:issue)
-            data  = data_attribute(project.id)
+            data  = data_attribute(project: project.id, issue: issue.id)
             %(<a href="#{url}" #{data}
                  title="#{title}"

lib/gitlab/markdown/label_reference_filter.rb

@@ -22,6 +22,10 @@ module Gitlab
         end
       end
+      def self.referenced_by(node)
+        { label: LazyReference.new(Label, node.attr("data-label")) }
+      end
+
       def call
         replace_text_nodes_matching(Label.reference_pattern) do |content|
           label_link_filter(content)
@@ -41,11 +45,9 @@ module Gitlab
           params = label_params(id, name)
           if label = project.labels.find_by(params)
-            push_result(:label, label)
-
             url = url_for_label(project, label)
             klass = reference_class(:label)
-            data = data_attribute(project.id)
+            data = data_attribute(project: project.id, label: label.id)
             %(<a href="#{url}" #{data}
                  class="#{klass}">#{render_colored_label(label)}</a>)