Merge branch '38893-banzai-upload-filter-relative-urls' into 'master'

Use relative URLs when linking to uploaded files

Closes #38893

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

app/models/concerns/cache_markdown_field.rb

@@ -11,7 +11,7 @@ module CacheMarkdownField
   extend ActiveSupport::Concern
 
   # Increment this number every time the renderer changes its output
-  CACHE_VERSION = 2
+  CACHE_VERSION = 3
 
   # changes to these attributes cause the cache to be invalidates
   INVALIDATED_BY = %w[author project].freeze

changelogs/unreleased/38893-banzai-upload-filter-relative-urls.yml

+---
+title: Use relative URLs when linking to uploaded files
+merge_request: 15751
+author:
+type: other

lib/banzai/filter/relative_link_filter.rb

@@ -2,19 +2,21 @@ require 'uri'
 
 module Banzai
   module Filter
-    # HTML filter that "fixes" relative links to files in a repository.
+    # HTML filter that "fixes" relative links to uploads or files in a repository.
     #
     # Context options:
     #   :commit
+    #   :group
     #   :project
     #   :project_wiki
     #   :ref
     #   :requested_path
     class RelativeLinkFilter < HTML::Pipeline::Filter
-      def call
-        return doc unless linkable_files?
+      include Gitlab::Utils::StrongMemoize
 
+      def call
         @uri_types = {}
+        clear_memoization(:linkable_files)
 
         doc.search('a:not(.gfm)').each do |el|
           process_link_attr el.attribute('href')
@@ -31,13 +33,35 @@ module Banzai
       protected
 
       def linkable_files?
-        context[:project_wiki].nil? && repository.try(:exists?) && !repository.empty?
+        strong_memoize(:linkable_files) do
+          context[:project_wiki].nil? && repository.try(:exists?) && !repository.empty?
+        end
       end
 
       def process_link_attr(html_attr)
         return if html_attr.blank?
         return if html_attr.value.start_with?('//')
 
+        if html_attr.value.start_with?('/uploads/')
+          process_link_to_upload_attr(html_attr)
+        elsif linkable_files?
+          process_link_to_repository_attr(html_attr)
+        end
+      end
+
+      def process_link_to_upload_attr(html_attr)
+        uri_parts = [html_attr.value]
+
+        if group
+          uri_parts.unshift(relative_url_root, 'groups', group.full_path, '-')
+        elsif project
+          uri_parts.unshift(relative_url_root, project.full_path)
+        end
+
+        html_attr.value = File.join(*uri_parts)
+      end
+
+      def process_link_to_repository_attr(html_attr)
         uri = URI(html_attr.value)
         if uri.relative? && uri.path.present?
           html_attr.value = rebuild_relative_uri(uri).to_s
@@ -51,7 +75,7 @@ module Banzai
 
         uri.path = [
           relative_url_root,
-          context[:project].full_path,
+          project.full_path,
           uri_type(file_path),
           Addressable::URI.escape(ref),
           Addressable::URI.escape(file_path)
@@ -123,11 +147,19 @@ module Banzai
       end
 
       def ref
-        context[:ref] || context[:project].default_branch
+        context[:ref] || project.default_branch
+      end
+
+      def group
+        context[:group]
+      end
+
+      def project
+        context[:project]
       end
 
       def repository
-        @repository ||= context[:project].try(:repository)
+        @repository ||= project&.repository
       end
     end
   end

lib/banzai/filter/upload_link_filter.rb

-require 'uri'
-
-module Banzai
-  module Filter
-    # HTML filter that "fixes" relative upload links to files.
-    # Context options:
-    #   :project (required) - Current project
-    #
-    class UploadLinkFilter < HTML::Pipeline::Filter
-      def call
-        return doc unless project || group
-
-        doc.xpath('descendant-or-self::a[starts-with(@href, "/uploads/")]').each do |el|
-          process_link_attr el.attribute('href')
-        end
-
-        doc.xpath('descendant-or-self::img[starts-with(@src, "/uploads/")]').each do |el|
-          process_link_attr el.attribute('src')
-        end
-
-        doc
-      end
-
-      protected
-
-      def process_link_attr(html_attr)
-        html_attr.value = build_url(html_attr.value).to_s
-      end
-
-      def build_url(uri)
-        base_path = Gitlab.config.gitlab.url
-
-        if group
-          urls = Gitlab::Routing.url_helpers
-          # we need to get last 2 parts of the uri which are secret and filename
-          uri_parts = uri.split(File::SEPARATOR)
-          file_path = urls.show_group_uploads_path(group, uri_parts[-2], uri_parts[-1])
-          File.join(base_path, file_path)
-        else
-          File.join(base_path, project.full_path, uri)
-        end
-      end
-
-      def project
-        context[:project]
-      end
-
-      def group
-        context[:group]
-      end
-
-      # Ensure that a :project key exists in context
-      #
-      # Note that while the key might exist, its value could be nil!
-      def validate
-        needs :project
-      end
-    end
-  end
-end

lib/banzai/pipeline/gfm_pipeline.rb

@@ -15,7 +15,6 @@ module Banzai
 
           Filter::MathFilter,
           Filter::MermaidFilter,
-          Filter::UploadLinkFilter,
           Filter::VideoLinkFilter,
           Filter::ImageLazyLoadFilter,
           Filter::ImageLinkFilter,

spec/lib/banzai/filter/relative_link_filter_spec.rb

@@ -5,6 +5,7 @@ describe Banzai::Filter::RelativeLinkFilter do
     contexts.reverse_merge!({
       commit:         commit,
       project:        project,
+      group:          group,
       project_wiki:   project_wiki,
       ref:            ref,
       requested_path: requested_path
@@ -25,7 +26,12 @@ describe Banzai::Filter::RelativeLinkFilter do
     %(<a href="#{path}">#{path}</a>)
   end
 
+  def nested(element)
+    %(<div>#{element}</div>)
+  end
+
   let(:project)        { create(:project, :repository) }
+  let(:group)          { nil }
   let(:project_path)   { project.full_path }
   let(:ref)            { 'markdown' }
   let(:commit)         { project.commit(ref) }
@@ -223,4 +229,79 @@ describe Banzai::Filter::RelativeLinkFilter do
     let(:commit) { nil } # force filter to use ref instead of commit
     include_examples :valid_repository
   end
+
+  context 'with a /upload/ URL' do
+    # not needed
+    let(:commit) { nil }
+    let(:ref) { nil }
+    let(:requested_path) { nil }
+
+    context 'to a project upload' do
+      it 'rebuilds relative URL for a link' do
+        doc = filter(link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
+        expect(doc.at_css('a')['href'])
+          .to eq "/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
+
+        doc = filter(nested(link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg')))
+        expect(doc.at_css('a')['href'])
+          .to eq "/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
+      end
+
+      it 'rebuilds relative URL for an image' do
+        doc = filter(image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
+        expect(doc.at_css('img')['src'])
+          .to eq "/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
+
+        doc = filter(nested(image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg')))
+        expect(doc.at_css('img')['src'])
+          .to eq "/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
+      end
+
+      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
+
+      it 'supports Unicode filenames' do
+        path = '/uploads/한글.png'
+        escaped = Addressable::URI.escape(path)
+
+        # Stub these methods so the file doesn't actually need to be in the repo
+        allow_any_instance_of(described_class)
+          .to receive(:file_exists?).and_return(true)
+        allow_any_instance_of(described_class)
+          .to receive(:image?).with(path).and_return(true)
+
+        doc = filter(image(escaped))
+        expect(doc.at_css('img')['src']).to match "/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png"
+      end
+    end
+
+    context 'to a group upload' do
+      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" }
+
+      it 'rewrites the link correctly' do
+        doc = filter(upload_link)
+
+        expect(doc.at_css('a')['href']).to eq(relative_path)
+      end
+
+      it 'rewrites the link correctly for subgroup' do
+        group.update!(parent: create(:group))
+
+        doc = filter(upload_link)
+
+        expect(doc.at_css('a')['href']).to eq(relative_path)
+      end
+
+      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
+    end
+  end
 end

spec/lib/banzai/filter/upload_link_filter_spec.rb

-require 'spec_helper'
-
-describe Banzai::Filter::UploadLinkFilter do
-  def filter(doc, contexts = {})
-    contexts.reverse_merge!({
-      project: project
-    })
-
-    raw_filter(doc, contexts)
-  end
-
-  def raw_filter(doc, contexts = {})
-    described_class.call(doc, contexts)
-  end
-
-  def image(path)
-    %(<img src="#{path}" />)
-  end
-
-  def link(path)
-    %(<a href="#{path}">#{path}</a>)
-  end
-
-  def nested_image(path)
-    %(<div><img src="#{path}" /></div>)
-  end
-
-  def nested_link(path)
-    %(<div><a href="#{path}">#{path}</a></div>)
-  end
-
-  let(:project) { create(:project) }
-
-  shared_examples :preserve_unchanged do
-    it 'does not modify any relative URL in anchor' do
-      doc = filter(link('README.md'))
-      expect(doc.at_css('a')['href']).to eq 'README.md'
-    end
-
-    it 'does not modify any relative URL in image' do
-      doc = filter(image('files/images/logo-black.png'))
-      expect(doc.at_css('img')['src']).to eq 'files/images/logo-black.png'
-    end
-  end
-
-  it 'does not raise an exception on invalid URIs' do
-    act = link("://foo")
-    expect { filter(act) }.not_to raise_error
-  end
-
-  context 'with a valid repository' do
-    it 'rebuilds relative URL for a link' do
-      doc = filter(link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
-      expect(doc.at_css('a')['href'])
-        .to eq "#{Gitlab.config.gitlab.url}/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
-
-      doc = filter(nested_link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
-      expect(doc.at_css('a')['href'])
-        .to eq "#{Gitlab.config.gitlab.url}/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
-    end
-
-    it 'rebuilds relative URL for an image' do
-      doc = filter(image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
-      expect(doc.at_css('img')['src'])
-        .to eq "#{Gitlab.config.gitlab.url}/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
-
-      doc = filter(nested_image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
-      expect(doc.at_css('img')['src'])
-        .to eq "#{Gitlab.config.gitlab.url}/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
-    end
-
-    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
-
-    it 'supports Unicode filenames' do
-      path = '/uploads/한글.png'
-      escaped = Addressable::URI.escape(path)
-
-      # Stub these methods so the file doesn't actually need to be in the repo
-      allow_any_instance_of(described_class)
-        .to receive(:file_exists?).and_return(true)
-      allow_any_instance_of(described_class)
-        .to receive(:image?).with(path).and_return(true)
-
-      doc = filter(image(escaped))
-      expect(doc.at_css('img')['src']).to match "#{Gitlab.config.gitlab.url}/#{project.full_path}/uploads/%ED%95%9C%EA%B8%80.png"
-    end
-  end
-
-  context 'in group context' do
-    let(:upload_link) { link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg') }
-    let(:group) { create(:group) }
-    let(:filter_context) { { project: nil, group: group } }
-    let(:relative_path) { "groups/#{group.full_path}/-/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg" }
-
-    it 'rewrites the link correctly' do
-      doc = raw_filter(upload_link, filter_context)
-
-      expect(doc.at_css('a')['href']).to eq("#{Gitlab.config.gitlab.url}/#{relative_path}")
-    end
-
-    it 'rewrites the link correctly for subgroup' do
-      subgroup = create(:group, parent: group)
-      relative_path = "groups/#{subgroup.full_path}/-/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
-
-      doc = raw_filter(upload_link, { project: nil, group: subgroup })
-
-      expect(doc.at_css('a')['href']).to eq("#{Gitlab.config.gitlab.url}/#{relative_path}")
-    end
-
-    it 'does not modify absolute URL' do
-      doc = filter(link('http://example.com'), filter_context)
-
-      expect(doc.at_css('a')['href']).to eq 'http://example.com'
-    end
-  end
-
-  context 'when project or group context does not exist' do
-    let(:upload_link) { link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg') }
-
-    it 'does not raise error' do
-      expect { raw_filter(upload_link, project: nil) }.not_to raise_error
-    end
-
-    it 'does not rewrite link' do
-      doc = raw_filter(upload_link, project: nil)
-
-      expect(doc.to_html).to eq upload_link
-    end
-  end
-end