Merge branch '66803-fix-uploads-relative-link-filter' into 'master'

Fix permissions check in `RelativeLinkFilter`

See merge request gitlab-org/gitlab-ce!32448

changelogs/unreleased/66803-fix-uploads-relative-link-filter.yml

+---
+title: Fix upload URLs in Markdown for users without access to project repository
+merge_request: 32448
+author:
+type: fixed

lib/banzai/filter/relative_link_filter.rb

@@ -19,7 +19,6 @@ module Banzai
 
       def call
         return doc if context[:system_note]
-        return doc unless visible_to_user?
 
         @uri_types = {}
         clear_memoization(:linkable_files)
@@ -50,7 +49,7 @@ module Banzai
 
         if html_attr.value.start_with?('/uploads/')
           process_link_to_upload_attr(html_attr)
-        elsif linkable_files?
+        elsif linkable_files? && repo_visible_to_user?
           process_link_to_repository_attr(html_attr)
         end
       end
@@ -168,14 +167,8 @@ 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
+      def repo_visible_to_user?
+        project && Ability.allowed?(current_user, :download_code, project)
       end
 
       def ref

spec/helpers/markup_helper_spec.rb

@@ -65,9 +65,6 @@ 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
@@ -81,9 +78,6 @@ 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

@@ -289,121 +289,72 @@ 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
-
+      shared_examples 'rewrite project uploads' do
         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
+          it 'rewrites the link correctly' do
             doc = filter(link(upload_path))
 
-            expect(doc.at_css('a')['href']).to eq(upload_path)
+            expect(doc.at_css('a')['href']).to eq(absolute_path)
           end
         end
-      end
 
-      context 'with an absolute URL' do
-        let(:absolute_path) { Gitlab.config.gitlab.url + relative_path }
-        let(:only_path) { false }
-
-        it 'rewrites the link correctly' do
+        it 'rebuilds relative URL for a link' do
           doc = filter(link(upload_path))
+          expect(doc.at_css('a')['href']).to eq(relative_path)
 
-          expect(doc.at_css('a')['href']).to eq(absolute_path)
+          doc = filter(nested(link(upload_path)))
+          expect(doc.at_css('a')['href']).to eq(relative_path)
         end
-      end
 
-      it 'rebuilds relative URL for a link' do
-        doc = filter(link(upload_path))
-        expect(doc.at_css('a')['href']).to eq(relative_path)
+        it 'rebuilds relative URL for an image' do
+          doc = filter(image(upload_path))
+          expect(doc.at_css('img')['src']).to eq(relative_path)
 
-        doc = filter(nested(link(upload_path)))
-        expect(doc.at_css('a')['href']).to eq(relative_path)
-      end
+          doc = filter(nested(image(upload_path)))
+          expect(doc.at_css('img')['src']).to eq(relative_path)
+        end
 
-      it 'rebuilds relative URL for an image' do
-        doc = filter(image(upload_path))
-        expect(doc.at_css('img')['src']).to eq(relative_path)
+        it 'does not modify absolute URL' do
+          doc = filter(link('http://example.com'))
+          expect(doc.at_css('a')['href']).to eq 'http://example.com'
+        end
 
-        doc = filter(nested(image(upload_path)))
-        expect(doc.at_css('img')['src']).to eq(relative_path)
-      end
+        it 'supports unescaped Unicode filenames' do
+          path = '/uploads/한글.png'
+          doc = filter(link(path))
 
-      it 'does not modify absolute URL' do
-        doc = filter(link('http://example.com'))
-        expect(doc.at_css('a')['href']).to eq 'http://example.com'
-      end
+          expect(doc.at_css('a')['href']).to eq("/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png")
+        end
 
-      it 'supports unescaped Unicode filenames' do
-        path = '/uploads/한글.png'
-        doc = filter(link(path))
+        it 'supports escaped Unicode filenames' do
+          path = '/uploads/한글.png'
+          escaped = Addressable::URI.escape(path)
+          doc = filter(image(escaped))
 
-        expect(doc.at_css('a')['href']).to eq("/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png")
+          expect(doc.at_css('img')['src']).to eq("/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png")
+        end
       end
 
-      it 'supports escaped Unicode filenames' do
-        path = '/uploads/한글.png'
-        escaped = Addressable::URI.escape(path)
-        doc = filter(image(escaped))
+      context 'without project repository access' do
+        let(:project) { create(:project, :repository, repository_access_level: ProjectFeature::PRIVATE) }
+
+        it_behaves_like 'rewrite project uploads'
+      end
 
-        expect(doc.at_css('img')['src']).to eq("/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png")
+      context 'with project repository access' do
+        it_behaves_like 'rewrite project uploads'
       end
     end
 
     context 'to a group upload' do
-      let(:upload_path) { '/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg' }
-      let(:upload_link) { link(upload_path) }
+      let(:upload_link) { link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg') }
       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 }