Merge branch 'security-exposed-default-branch' into 'master'

Avoid exposing unaccessible repo data upon GFM post processing

See merge request gitlab/gitlabhq!3344

changelogs/unreleased/security-exposed-default-branch.yml

+---
+title: Avoid exposing unaccessible repo data upon GFM post processing
+merge_request:
+author:
+type: security

lib/banzai/filter/relative_link_filter.rb

@@ -9,6 +9,7 @@ module Banzai
     # Context options:
     #   :commit
     #   :group
+    #   :current_user
     #   :project
     #   :project_wiki
     #   :ref
@@ -18,6 +19,7 @@ module Banzai
 
       def call
         return doc if context[:system_note]
+        return doc unless visible_to_user?
 
         @uri_types = {}
         clear_memoization(:linkable_files)
@@ -166,6 +168,16 @@ module Banzai
         Gitlab.config.gitlab.relative_url_root.presence || '/'
       end
 
+      def visible_to_user?
+        if project
+          Ability.allowed?(current_user, :download_code, project)
+        elsif group
+          Ability.allowed?(current_user, :read_group, group)
+        else # Objects detached from projects or groups, e.g. Personal Snippets.
+          true
+        end
+      end
+
       def ref
         context[:ref] || project.default_branch
       end
@@ -178,6 +190,10 @@ module Banzai
         context[:project]
       end
 
+      def current_user
+        context[:current_user]
+      end
+
       def repository
         @repository ||= project&.repository
       end

spec/helpers/markup_helper_spec.rb

@@ -65,6 +65,9 @@ describe MarkupHelper do
 
       describe 'inside a group' do
         before do
+          # Ensure the generated reference links aren't redacted
+          group.add_maintainer(user)
+
           helper.instance_variable_set(:@group, group)
           helper.instance_variable_set(:@project, nil)
         end
@@ -78,6 +81,9 @@ describe MarkupHelper do
         let(:project_in_group) { create(:project, group: group) }
 
         before do
+          # Ensure the generated reference links aren't redacted
+          project_in_group.add_maintainer(user)
+
           helper.instance_variable_set(:@group, group)
           helper.instance_variable_set(:@project, project_in_group)
         end

spec/lib/banzai/filter/relative_link_filter_spec.rb

@@ -7,6 +7,7 @@ describe Banzai::Filter::RelativeLinkFilter do
     contexts.reverse_merge!({
       commit:         commit,
       project:        project,
+      current_user:   user,
       group:          group,
       project_wiki:   project_wiki,
       ref:            ref,
@@ -33,7 +34,8 @@ describe Banzai::Filter::RelativeLinkFilter do
     %(<div>#{element}</div>)
   end
 
-  let(:project)        { create(:project, :repository) }
+  let(:project)        { create(:project, :repository, :public) }
+  let(:user)           { create(:user) }
   let(:group)          { nil }
   let(:project_path)   { project.full_path }
   let(:ref)            { 'markdown' }
@@ -75,6 +77,11 @@ describe Banzai::Filter::RelativeLinkFilter do
     include_examples :preserve_unchanged
   end
 
+  context 'without project repository access' do
+    let(:project) { create(:project, :repository, repository_access_level: ProjectFeature::PRIVATE) }
+    include_examples :preserve_unchanged
+  end
+
   it 'does not raise an exception on invalid URIs' do
     act = link("://foo")
     expect { filter(act) }.not_to raise_error
@@ -282,6 +289,37 @@ describe Banzai::Filter::RelativeLinkFilter do
     let(:relative_path) { "/#{project.full_path}#{upload_path}" }
 
     context 'to a project upload' do
+      context 'without project repository access' do
+        let(:project) { create(:project, :repository, repository_access_level: ProjectFeature::PRIVATE) }
+
+        it 'does not rebuild relative URL for a link' do
+          doc = filter(link(upload_path))
+          expect(doc.at_css('a')['href']).to eq(upload_path)
+
+          doc = filter(nested(link(upload_path)))
+          expect(doc.at_css('a')['href']).to eq(upload_path)
+        end
+
+        it 'does not rebuild relative URL for an image' do
+          doc = filter(image(upload_path))
+          expect(doc.at_css('img')['src']).to eq(upload_path)
+
+          doc = filter(nested(image(upload_path)))
+          expect(doc.at_css('img')['src']).to eq(upload_path)
+        end
+
+        context 'with an absolute URL' do
+          let(:absolute_path) { Gitlab.config.gitlab.url + relative_path }
+          let(:only_path) { false }
+
+          it 'does not rewrite the link' do
+            doc = filter(link(upload_path))
+
+            expect(doc.at_css('a')['href']).to eq(upload_path)
+          end
+        end
+      end
+
       context 'with an absolute URL' do
         let(:absolute_path) { Gitlab.config.gitlab.url + relative_path }
         let(:only_path) { false }
@@ -331,11 +369,41 @@ describe Banzai::Filter::RelativeLinkFilter do
     end
 
     context 'to a group upload' do
-      let(:upload_link) { link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg') }
+      let(:upload_path) { '/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg' }
+      let(:upload_link) { link(upload_path) }
       let(:group) { create(:group) }
       let(:project) { nil }
       let(:relative_path) { "/groups/#{group.full_path}/-/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg" }
 
+      context 'without group read access' do
+        let(:group) { create(:group, :private) }
+
+        it 'does not rewrite the link' do
+          doc = filter(upload_link)
+
+          expect(doc.at_css('a')['href']).to eq(upload_path)
+        end
+
+        it 'does not rewrite the link for subgroup' do
+          group.update!(parent: create(:group))
+
+          doc = filter(upload_link)
+
+          expect(doc.at_css('a')['href']).to eq(upload_path)
+        end
+
+        context 'with an absolute URL' do
+          let(:absolute_path) { Gitlab.config.gitlab.url + relative_path }
+          let(:only_path) { false }
+
+          it 'does not rewrite the link' do
+            doc = filter(upload_link)
+
+            expect(doc.at_css('a')['href']).to eq(upload_path)
+          end
+        end
+      end
+
       context 'with an absolute URL' do
         let(:absolute_path) { Gitlab.config.gitlab.url + relative_path }
         let(:only_path) { false }