Merge branch 'issue_23548_dev' into 'master'

disable markdown in comments when referencing disabled features

fixes https://gitlab.com/gitlab-org/gitlab-ce/issues/23548



This MR prevents the following references when tool is disabled:

- issues
- snippets
- commits - when repo is disabled
- commit range - when repo is disabled
- milestones

This MR does not prevent references to repository files, since they are just markdown links and don't leak
information.

See merge request !2011

Signed-off-by: Rémy Coutable <remy@rymai.me>

app/models/issue.rb

@@ -250,29 +250,9 @@ def can_be_worked_on?(current_user)
   # Returns `true` if the current issue can be viewed by either a logged in User
   # or an anonymous user.
   def visible_to_user?(user = nil)
-    user ? readable_by?(user) : publicly_visible?
-  end
-
-  # Returns `true` if the given User can read the current Issue.
-  def readable_by?(user)
-    if user.admin?
-      true
-    elsif project.owner == user
-      true
-    elsif confidential?
-      author == user ||
-        assignee == user ||
-        project.team.member?(user, Gitlab::Access::REPORTER)
-    else
-      project.public? ||
-        project.internal? && !user.external? ||
-        project.team.member?(user)
-    end
-  end
-  # Returns `true` if this Issue is visible to everybody.
-  def publicly_visible?
-    project.public? && !confidential?
+    return false unless project.feature_available?(:issues, user)
+    user ? readable_by?(user) : publicly_visible?
   end
   def overdue?
@@ -297,4 +277,32 @@ def as_json(options = {})
       end
     end
   end
+
+  private
+
+  # Returns `true` if the given User can read the current Issue.
+  #
+  # This method duplicates the same check of issue_policy.rb
+  # for performance reasons, check commit: 002ad215818450d2cbbc5fa065850a953dc7ada8
+  # Make sure to sync this method with issue_policy.rb
+  def readable_by?(user)
+    if user.admin?
+      true
+    elsif project.owner == user
+      true
+    elsif confidential?
+      author == user ||
+        assignee == user ||
+        project.team.member?(user, Gitlab::Access::REPORTER)
+    else
+      project.public? ||
+        project.internal? && !user.external? ||
+        project.team.member?(user)
+    end
+  end
+
+  # Returns `true` if this Issue is visible to everybody.
+  def publicly_visible?
+    project.public? && !confidential?
+  end
 end

app/policies/issue_policy.rb

 class IssuePolicy < IssuablePolicy
+  # This class duplicates the same check of Issue#readable_by? for performance reasons
+  # Make sure to sync this class checks with issue.rb to avoid security problems.
+  # Check commit 002ad215818450d2cbbc5fa065850a953dc7ada8 for more information.
+
   def issue
     @subject
   end

lib/banzai/reference_parser/base_parser.rb

@@ -63,12 +63,7 @@ def nodes_visible_to_user(user, nodes)
         nodes.select do |node|
           if node.has_attribute?(project_attr)
             node_id = node.attr(project_attr).to_i
-
-            if project && project.id == node_id
-              true
-            else
-              can?(user, :read_project, projects[node_id])
-            end
+            can_read_reference?(user, projects[node_id])
           else
             true
           end
@@ -226,6 +221,15 @@ def find_projects_for_hash_keys(hash)
       attr_reader :current_user, :project
+      # When a feature is disabled or visible only for
+      # team members we should not allow team members
+      # see reference comments.
+      # Override this method on subclasses
+      # to check if user can read resource
+      def can_read_reference?(user, ref_project)
+        raise NotImplementedError
+      end
+
       def lazy(&block)
         Gitlab::Lazy.new(&block)
       end

lib/banzai/reference_parser/commit_parser.rb

@@ -29,6 +29,12 @@ def find_commits(project, ids)
         commits
       end
+
+      private
+
+      def can_read_reference?(user, ref_project)
+        can?(user, :download_code, ref_project)
+      end
     end
   end
 end

lib/banzai/reference_parser/commit_range_parser.rb

@@ -33,6 +33,12 @@ def find_object(project, id)
         range.valid_commits? ? range : nil
       end
+
+      private
+
+      def can_read_reference?(user, ref_project)
+        can?(user, :download_code, ref_project)
+      end
     end
   end
 end

lib/banzai/reference_parser/external_issue_parser.rb

@@ -20,6 +20,12 @@ def referenced_by(nodes)
       def issue_ids_per_project(nodes)
         gather_attributes_per_project(nodes, self.class.data_attribute)
       end
+
+      private
+
+      def can_read_reference?(user, ref_project)
+        can?(user, :read_issue, ref_project)
+      end
     end
   end
 end

lib/banzai/reference_parser/label_parser.rb

@@ -6,6 +6,12 @@ class LabelParser < BaseParser
       def references_relation
         Label
       end
+
+      private
+
+      def can_read_reference?(user, ref_project)
+        can?(user, :read_label, ref_project)
+      end
     end
   end
 end

lib/banzai/reference_parser/merge_request_parser.rb

@@ -6,6 +6,12 @@ class MergeRequestParser < BaseParser
       def references_relation
         MergeRequest.includes(:author, :assignee, :target_project)
       end
+
+      private
+
+      def can_read_reference?(user, ref_project)
+        can?(user, :read_merge_request, ref_project)
+      end
     end
   end
 end

lib/banzai/reference_parser/milestone_parser.rb

@@ -6,6 +6,12 @@ class MilestoneParser < BaseParser
       def references_relation
         Milestone
       end
+
+      private
+
+      def can_read_reference?(user, ref_project)
+        can?(user, :read_milestone, ref_project)
+      end
     end
   end
 end

lib/banzai/reference_parser/snippet_parser.rb

@@ -6,6 +6,12 @@ class SnippetParser < BaseParser
       def references_relation
         Snippet
       end
+
+      private
+
+      def can_read_reference?(user, ref_project)
+        can?(user, :read_project_snippet, ref_project)
+      end
     end
   end
 end

lib/banzai/reference_parser/user_parser.rb

@@ -30,22 +30,36 @@ def nodes_visible_to_user(user, nodes)
         nodes.each do |node|
           if node.has_attribute?(group_attr)
-            node_group = groups[node.attr(group_attr).to_i]
-
-            if node_group &&
-              can?(user, :read_group, node_group)
-              visible << node
-            end
-          # Remaining nodes will be processed by the parent class'
-          # implementation of this method.
+            next unless can_read_group_reference?(node, user, groups)
+            visible << node
+          elsif can_read_project_reference?(node)
+            visible << node
           else
             remaining << node
           end
         end
+        # If project does not belong to a group
+        # and does not have the same project id as the current project
+        # base class will check if user can read the project that contains
+        # the user reference.
         visible + super(current_user, remaining)
       end
+      # Check if project belongs to a group which
+      # user can read.
+      def can_read_group_reference?(node, user, groups)
+        node_group = groups[node.attr('data-group').to_i]
+
+        node_group && can?(user, :read_group, node_group)
+      end
+
+      def can_read_project_reference?(node)
+        node_id = node.attr('data-project').to_i
+
+        project && project.id == node_id
+      end
+
       def nodes_user_can_reference(current_user, nodes)
         project_attr = 'data-project'
         author_attr = 'data-author'
@@ -88,6 +102,10 @@ def find_users_for_projects(ids)
         collection_objects_for_ids(Project, ids).
           flat_map { |p| p.team.members.to_a }
       end
+
+      def can_read_reference?(user, ref_project)
+        can?(user, :read_project, ref_project)
+      end
     end
   end
 end

spec/features/issues/new_branch_button_spec.rb

@@ -22,6 +22,7 @@
         create(:note, :on_issue, :system, project: project, noteable: issue,
                                           note: "Mentioned in !#{referenced_mr.iid}")
       end
+
       let(:referenced_mr) do
         create(:merge_request, :simple, source_project: project, target_project: project,
                                         description: "Fixes ##{issue.iid}", author: user)

spec/lib/banzai/filter/redactor_filter_spec.rb

@@ -28,31 +28,39 @@ def reference_link(data)
         and_return(parser_class)
     end
-    it 'removes unpermitted Project references' do
-      user = create(:user)
-      project = create(:empty_project)
-      link = reference_link(project: project.id, reference_type: 'test')
-      doc = filter(link, current_user: user)
-      expect(doc.css('a').length).to eq 0
+    context 'valid projects' do
+      before { allow_any_instance_of(Banzai::ReferenceParser::BaseParser).to receive(:can_read_reference?).and_return(true) }
+      it 'allows permitted Project references' do
+        user = create(:user)
+        project = create(:empty_project)
+        project.team << [user, :master]
+
+        link = reference_link(project: project.id, reference_type: 'test')
+        doc = filter(link, current_user: user)
+        expect(doc.css('a').length).to eq 1
+      end
     end
-    it 'allows permitted Project references' do
-      user = create(:user)
-      project = create(:empty_project)
-      project.team << [user, :master]
-      link = reference_link(project: project.id, reference_type: 'test')
-      doc = filter(link, current_user: user)
-      expect(doc.css('a').length).to eq 1
-    end
-    it 'handles invalid Project references' do
-      link = reference_link(project: 12345, reference_type: 'test')
-      expect { filter(link) }.not_to raise_error
+    context 'invalid projects' do
+      before { allow_any_instance_of(Banzai::ReferenceParser::BaseParser).to receive(:can_read_reference?).and_return(false) }
+      it 'removes unpermitted references' do
+        user = create(:user)
+        project = create(:empty_project)
+        link = reference_link(project: project.id, reference_type: 'test')
+        doc = filter(link, current_user: user)
+        expect(doc.css('a').length).to eq 0
+      end
+
+      it 'handles invalid references' do
+        link = reference_link(project: 12345, reference_type: 'test')
+        expect { filter(link) }.not_to raise_error
+      end
     end
   end

spec/lib/banzai/reference_parser/base_parser_spec.rb

@@ -27,41 +27,12 @@
     let(:link) { empty_html_link }
     context 'when the link has a data-project attribute' do
-      it 'returns the nodes if the attribute value equals the current project ID' do
+      it 'checks if user can read the resource' do
         link['data-project'] = project.id.to_s
-        expect(Ability).not_to receive(:allowed?)
-        expect(subject.nodes_visible_to_user(user, [link])).to eq([link])
-      end
-
-      it 'returns the nodes if the user can read the project' do
-        other_project = create(:empty_project, :public)
-
-        link['data-project'] = other_project.id.to_s
-
-        expect(Ability).to receive(:allowed?).
-          with(user, :read_project, other_project).
-          and_return(true)
-
-        expect(subject.nodes_visible_to_user(user, [link])).to eq([link])
-      end
-
-      it 'returns an empty Array when the attribute value is empty' do
-        link['data-project'] = ''
-
-        expect(subject.nodes_visible_to_user(user, [link])).to eq([])
-      end
-
-      it 'returns an empty Array when the user can not read the project' do
-        other_project = create(:empty_project, :public)
-
-        link['data-project'] = other_project.id.to_s
-
-        expect(Ability).to receive(:allowed?).
-          with(user, :read_project, other_project).
-          and_return(false)
-        expect(subject.nodes_visible_to_user(user, [link])).to eq([])
+        expect(subject).to receive(:can_read_reference?).with(user, project)
+        subject.nodes_visible_to_user(user, [link])
       end
     end

spec/lib/banzai/reference_parser/commit_parser_spec.rb

@@ -8,6 +8,14 @@
   subject { described_class.new(project, user) }
   let(:link) { empty_html_link }
+  describe '#nodes_visible_to_user' do
+    context 'when the link has a data-issue attribute' do
+      before { link['data-commit'] = 123 }
+
+      it_behaves_like "referenced feature visibility", "repository"
+    end
+  end
+
   describe '#referenced_by' do
     context 'when the link has a data-project attribute' do
       before do

spec/lib/banzai/reference_parser/commit_range_parser_spec.rb

@@ -8,6 +8,14 @@
   subject { described_class.new(project, user) }
   let(:link) { empty_html_link }
+  describe '#nodes_visible_to_user' do
+    context 'when the link has a data-issue attribute' do
+      before { link['data-commit-range'] = '123..456' }
+
+      it_behaves_like "referenced feature visibility", "repository"
+    end
+  end
+
   describe '#referenced_by' do
     context 'when the link has a data-project attribute' do
       before do

spec/lib/banzai/reference_parser/external_issue_parser_spec.rb

@@ -8,6 +8,14 @@
   subject { described_class.new(project, user) }
   let(:link) { empty_html_link }
+  describe '#nodes_visible_to_user' do
+    context 'when the link has a data-issue attribute' do
+      before { link['data-external-issue'] = 123 }
+
+      it_behaves_like "referenced feature visibility", "issues"
+    end
+  end
+
   describe '#referenced_by' do
     context 'when the link has a data-project attribute' do
       before do

spec/lib/banzai/reference_parser/issue_parser_spec.rb

@@ -4,10 +4,10 @@
   include ReferenceParserHelpers
   let(:project) { create(:empty_project, :public) }
-  let(:user) { create(:user) }
-  let(:issue) { create(:issue, project: project) }
-  subject { described_class.new(project, user) }
-  let(:link) { empty_html_link }
+  let(:user)    { create(:user) }
+  let(:issue)   { create(:issue, project: project) }
+  let(:link)    { empty_html_link }
+  subject       { described_class.new(project, user) }
   describe '#nodes_visible_to_user' do
     context 'when the link has a data-issue attribute' do
@@ -15,6 +15,8 @@
         link['data-issue'] = issue.id.to_s
       end
+      it_behaves_like "referenced feature visibility", "issues"
+
       it 'returns the nodes when the user can read the issue' do
         expect(Ability).to receive(:issues_readable_by_user).
           with([issue], user).

spec/lib/banzai/reference_parser/label_parser_spec.rb

@@ -9,6 +9,14 @@
   subject { described_class.new(project, user) }
   let(:link) { empty_html_link }
+  describe '#nodes_visible_to_user' do
+    context 'when the link has a data-issue attribute' do
+      before { link['data-label'] = label.id.to_s }
+
+      it_behaves_like "referenced feature visibility", "issues", "merge_requests"
+    end
+  end
+
   describe '#referenced_by' do
     describe 'when the link has a data-label attribute' do
       context 'using an existing label ID' do

spec/lib/banzai/reference_parser/merge_request_parser_spec.rb

@@ -8,6 +8,19 @@
   subject { described_class.new(merge_request.target_project, user) }
   let(:link) { empty_html_link }
+  describe '#nodes_visible_to_user' do
+    context 'when the link has a data-issue attribute' do
+      let(:project) { merge_request.target_project }
+
+      before do
+        project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC)
+        link['data-merge-request'] = merge_request.id.to_s
+      end
+
+      it_behaves_like "referenced feature visibility", "merge_requests"
+    end
+  end
+
   describe '#referenced_by' do
     describe 'when the link has a data-merge-request attribute' do
       context 'using an existing merge request ID' do