Use relative URLs when linking to uploaded files38893-banzai-upload-filter-relative-urls

5 files changed, 122 insertions, 82 deletions

diff --git a/app/models/concerns/cache_markdown_field.rb b/app/models/concerns/cache_markdown_field.rb
index 90ad644ce34..4ae5dd8c677 100644
--- a/app/models/concerns/cache_markdown_field.rb
+++ b/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
diff --git a/lib/banzai/filter/relative_link_filter.rb b/lib/banzai/filter/relative_link_filter.rb
index 758f15c8a67..34952369ef8 100644
--- a/lib/banzai/filter/relative_link_filter.rb
+++ b/lib/banzai/filter/relative_link_filter.rb
@@ -2,18 +2,18 @@ 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
+    #   :only_path
     class RelativeLinkFilter < HTML::Pipeline::Filter
       def call
-        return doc unless linkable_files?
-
         @uri_types = {}
 
         doc.search('a:not(.gfm)').each do |el|
@@ -31,13 +31,40 @@ 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)
+        else
+          process_link_to_repository_attr(html_attr)
+        end
+      end
+
+      def process_link_to_upload_attr(html_attr)
+        base_path =
+          if context[:only_path]
+            Gitlab.config.gitlab.relative_url_root.to_s
+          else
+            Gitlab.config.gitlab.url.to_s
+          end
+
+        uri = html_attr.value
+
+        if context[:group]
+          html_attr.value = File.join(base_path, 'groups', context[:group].full_path, '-', uri)
+        elsif context[:project]
+          html_attr.value = File.join(base_path, context[:project].full_path, uri)
+        end
+      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
diff --git a/lib/banzai/filter/upload_link_filter.rb b/lib/banzai/filter/upload_link_filter.rb
deleted file mode 100644
index d64f9ac4eb6..00000000000
--- a/lib/banzai/filter/upload_link_filter.rb
+++ /dev/null
@@ -1,60 +0,0 @@
-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
diff --git a/lib/banzai/pipeline/gfm_pipeline.rb b/lib/banzai/pipeline/gfm_pipeline.rb
index 55874ad50a3..c746f6f64e9 100644
--- a/lib/banzai/pipeline/gfm_pipeline.rb
+++ b/lib/banzai/pipeline/gfm_pipeline.rb
@@ -15,7 +15,6 @@ module Banzai
 
           Filter::MathFilter,
           Filter::MermaidFilter,
-          Filter::UploadLinkFilter,
           Filter::VideoLinkFilter,
           Filter::ImageLazyLoadFilter,
           Filter::ImageLinkFilter,
diff --git a/spec/lib/banzai/filter/upload_link_filter_spec.rb b/spec/lib/banzai/filter/upload_link_filter_spec.rb
index 76bc0c36ab7..95c862f9551 100644
--- a/spec/lib/banzai/filter/upload_link_filter_spec.rb
+++ b/spec/lib/banzai/filter/upload_link_filter_spec.rb
@@ -1,6 +1,15 @@
 require 'spec_helper'
 
 describe Banzai::Filter::UploadLinkFilter do
+  def relative_filter(doc, contexts = {})
+    contexts.reverse_merge!({
+      project: project,
+      only_path: true
+    })
+
+    filter(doc, contexts)
+  end
+
   def filter(doc, contexts = {})
     contexts.reverse_merge!({
       project: project
@@ -50,27 +59,27 @@ describe Banzai::Filter::UploadLinkFilter do
 
   context 'with a valid repository' do
     it 'rebuilds relative URL for a link' do
-      doc = filter(link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
+      doc = relative_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"
+        .to eq "/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
 
-      doc = filter(nested_link('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
+      doc = relative_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"
+        .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'))
+      doc = relative_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"
+        .to eq "/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
 
-      doc = filter(nested_image('/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg'))
+      doc = relative_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"
+        .to eq "/#{project.full_path}/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
     end
 
     it 'does not modify absolute URL' do
-      doc = filter(link('http://example.com'))
+      doc = relative_filter(link('http://example.com'))
       expect(doc.at_css('a')['href']).to eq 'http://example.com'
     end
 
@@ -85,29 +94,29 @@ describe Banzai::Filter::UploadLinkFilter do
         .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"
+      expect(doc.at_css('img')['src']).to match "/#{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" }
+    let(:filter_context) { { project: nil, group: group, only_path: true } }
+    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}")
+      expect(doc.at_css('a')['href']).to eq(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"
+      relative_path = "/groups/#{subgroup.full_path}/-/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg"
 
-      doc = raw_filter(upload_link, { project: nil, group: subgroup })
+      doc = raw_filter(upload_link, { project: nil, group: subgroup, only_path: true })
 
-      expect(doc.at_css('a')['href']).to eq("#{Gitlab.config.gitlab.url}/#{relative_path}")
+      expect(doc.at_css('a')['href']).to eq(relative_path)
     end
 
     it 'does not modify absolute URL' do
@@ -130,4 +139,69 @@ describe Banzai::Filter::UploadLinkFilter do
       expect(doc.to_html).to eq upload_link
     end
   end
+
+  context 'GitLab relative URL root' do
+    let(:path) { '/uploads/e90decf88d8f96fe9e1389afc2e4a91f/test.jpg' }
+
+    where(:relative_url_root, :expected_prefix) do
+      [
+        [nil, '/'],
+        ['', '/'],
+        ['/', '/'],
+        ['/foo', '/foo/'],
+        ['/foo/', '/foo/'],
+        ['/foo/bar/', '/foo/bar/']
+      ]
+    end
+
+    with_them do
+      context 'relative' do
+        let(:expected_url) { [expected_prefix, project.full_path, path].join('') }
+
+        before do
+          stub_config_setting(relative_url_root: relative_url_root)
+        end
+
+        context 'image' do
+          let(:doc) { relative_filter(image(path)) }
+
+          subject { doc.at_css('img')['src'] }
+
+          it { is_expected.to eq(expected_url) }
+        end
+
+        context 'link' do
+          let(:doc) { relative_filter(link(path)) }
+
+          subject { doc.at_css('a')['href'] }
+
+          it { is_expected.to eq(expected_url) }
+        end
+      end
+
+      context 'absolute' do
+        let(:expected_url) { [Gitlab.config.gitlab.base_url, expected_prefix, project.full_path, path].join('') }
+
+        before do
+          stub_config_setting(url: Gitlab.config.gitlab.url + relative_url_root.to_s)
+        end
+
+        context 'image' do
+          let(:doc) { filter(image(path)) }
+
+          subject { doc.at_css('img')['src'] }
+
+          it { is_expected.to eq(expected_url) }
+        end
+
+        context 'link' do
+          let(:doc) { filter(link(path)) }
+
+          subject { doc.at_css('a')['href'] }
+
+          it { is_expected.to eq(expected_url) }
+        end
+      end
+    end
+  end
 end